Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make constructors for basic number types inferable #37213

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

martinholters
Copy link
Member

Starting from the assumption that a constructor should return an instance of the correct type, i.e. T(...)::T where T isa Type, I wondered: Does that hold for all types in julia proper? (Answer: no, e.g. ref. #37186 for an accidental violation I stumbled upon.) And can inference figure it out?

This PR makes the answer to the second question "yes" for the basic number types, see the test:

julia/test/numbers.jl

Lines 2696 to 2700 in dca67c9

@testset "constructor inferability for $T" for T in [AbstractFloat, #=BigFloat,=# Float16,
Float32, Float64, Integer, Bool, Signed, BigInt, Int128, Int16, Int32, Int64, Int8,
Unsigned, UInt128, UInt16, UInt32, UInt64, UInt8]
@test all(R -> R<:T, Base.return_types(T))
end

(There are two deprecated BigFloat methods which I haven't touched that would fail this test.)

However, while that holds for the individual constructor methods, for unknown argument types, usually too many methods match, so that e.g. Float64(x::Any) (26 possible methods) will still only be inferred as Any. So while I don't expect these changes to be wrong, I am not really sure how useful they are. Maybe @timholy has some intuition about it from all his work on invalidations where he looked at a lot code with incomplete type information?

@timholy
Copy link
Sponsor Member

timholy commented Aug 26, 2020

I think these are worth having even if they don't solve the problem for cases of abstract inference. But yes, I think we'll still have to annotate the return type in a few cases.

Some day it might be helpful to have a tool that goes through the source code and suggests type-annotations to remove, because all inference problems have been fixed. It would be much nicer to write such a tool if we had #31162 and similar links so that we could transition seamlessly from the type-inferred code to the source code, but there are other ways of achieving similar ends.

@test all(R -> R<:T, Base.return_types(T))
end
@testset "constructor inferability for BigFloat" begin
@test_broken all(R -> R<:T, Base.return_types(T))
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think T is not defined here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'Oh. I certainly did not intend the test to be that broken. Fix incoming.

@JeffBezanson JeffBezanson merged commit c10c451 into master Aug 28, 2020
@JeffBezanson JeffBezanson deleted the mh/number-ctor-inferability branch August 28, 2020 00:10
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants