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

Union causes incorrect type parameter extraction? #41841

Closed
githubtomtom opened this issue Aug 9, 2021 · 10 comments
Closed

Union causes incorrect type parameter extraction? #41841

githubtomtom opened this issue Aug 9, 2021 · 10 comments
Labels
types and dispatch Types, subtyping and method dispatch

Comments

@githubtomtom
Copy link
Contributor

githubtomtom commented Aug 9, 2021

in a fresh Mac v1.6.2 session:

julia> function f1(x::Union{AbstractArray{T}, AbstractArray{Union{T, Missing}}},
                    y::Union{AbstractArray{T}, AbstractArray{Union{T, Missing}}};
                    α::T = T(3.0) ) where {T<:Real}
           println(typeof(x), " ", typeof(y))
       end;

julia> f1([1.0, missing], [2.0, missing])
ERROR: MethodError: no method matching Union{Missing, Float64}(::Float64)
Stacktrace:
 [1] f1(x::Vector{Union{Missing, Float64}}, y::Vector{Union{Missing, Float64}})
   @ Main ./REPL[1]:4
 [2] top-level scope
   @ REPL[2]:1

julia> function f2(x::Union{AbstractArray{T}, AbstractArray{<:Union{T, Missing}}},
                   y::Union{AbstractArray{T}, AbstractArray{<:Union{T, Missing}}};
                   α::T = T(3.0) ) where {T<:Real}
           println(typeof(x), " ", typeof(y))
       end;

julia> f2([1.0, missing], [2.0, missing])
Vector{Union{Missing, Float64}} Vector{Union{Missing, Float64}}

error seems coming from mistaking Union{Missing, Float64} as T, which should be Float64, in the default keyword argument assignment.

noted that if I supply the keyboard argument such that the default value assignment is not called, no error:

julia> f1([1.0, missing], [2.0, missing]; α = 2.2)
Vector{Union{Missing, Float64}} Vector{Union{Missing, Float64}}

it seems to be a bug in extracting the T (when Union is involved?).

the original discussion is from here. Thanks.

@vtjnash
Copy link
Member

vtjnash commented Aug 9, 2021

This should give an UndefVarError, but that can be hard to determine, so we are currently somewhat conservative and return a result, if it none should be returned.

Closing as related to / duplicate of #41728

@vtjnash vtjnash added the types and dispatch Types, subtyping and method dispatch label Aug 9, 2021
@githubtomtom
Copy link
Contributor Author

why should it give an UndefVarError ?

in the error case:

julia> f1([1.0, missing], [2.0, missing])
ERROR: MethodError: no method matching Union{Missing, Float64}(::Float64)

T should be clearly dispatched as Float64. Of course, that "clearly" refers to a human mind...

@vtjnash
Copy link
Member

vtjnash commented Aug 9, 2021

We merely have Float64 <: T <: Union{Float64, Nothing} as the constraint on that. There's no reason to expect it must pick any particular value from that set.

@vtjnash vtjnash closed this as completed Aug 9, 2021
@githubtomtom
Copy link
Contributor Author

githubtomtom commented Aug 9, 2021

to clarify, the specific error occurs only when there are more than one Union inputs. For example, the following is fine:

julia> function f3(x::Union{AbstractArray{T}, AbstractArray{Union{T, Missing}}};
                   α::T = T(3.0) ) where {T<:Real}
           println(typeof(x))
       end
f3 (generic function with 1 method)

julia> f3([2.0, missing])
Vector{Union{Missing, Float64}}

julia> f3([2.0, missing]; α = 2.0)
Vector{Union{Missing, Float64}}

that said, when more than one Unions, a human could "match" both inputs as the same AbstractArray{Union{T, Missing}}, and then infer that T is Float64. It's difficult for the compiler to do a two steps inference???

@vtjnash
Copy link
Member

vtjnash commented Aug 9, 2021

oh, I see now there is an assertion that T<:Real in the original, so it seems that the result got widen and is not imposing that constraint when it extracted the output

@vtjnash vtjnash reopened this Aug 9, 2021
@JeffBezanson
Copy link
Member

It's difficult for the compiler to do a two steps inference?

I see the problem here, but yes, this is difficult!! Us humans have a lovely way of being able to "just see" certain things 😄

@githubtomtom
Copy link
Contributor Author

githubtomtom commented Aug 10, 2021

it's difficult ... requiring some "AI inference" !!!

my suggestion is: as long as the type inference is beyond the (current) ability of julia, it should throw an "InferenceError" to force the user to adapt other type specifications

right now, it's doing some wrong inference instead of detecting a difficult inference problem...


also worth noting is that Windows has no error?! It means that the "wrong inference" luckily make the correct inference? This inconsistency between Windows and Mac is worrisome...

@fingolfin
Copy link
Member

Issue #42710 was closed as a duplicate of this. The refined example that was used there is the following (issues reproduced with 1.6.3, 1.7.0-rc1 and master; note that no stacktrace is printed):

julia> f(::Type{Union{A,B}}) where {A,B<:AbstractFloat} = f(A)
f (generic function with 1 method)

julia> f(Int)
ERROR: StackOverflowError:

julia>

The confusion here is why it would match Int to Type{Union{A,B}) with B<:AbstractFloat? Note that this changes if one moves the type restriction:

julia> f(::Type{Union{A,B}}) where {A<:AbstractFloat,B} = f(A)
f (generic function with 1 method)

julia> f(Int)
ERROR: UndefVarError: A not defined
Stacktrace:
 [1] f(#unused#::Type{Int64})
   @ Main ./REPL[1]:1
 [2] top-level scope
   @ REPL[2]:1

alas...

julia> g(::Type{Union{A,B}}) where {A<:AbstractFloat,B} = g(B)
g (generic function with 1 method)

julia> g(Int)
ERROR: StackOverflowError:

@martinholters
Copy link
Member

The confusion here is why it would match Int to Type{Union{A,B}) with B<:AbstractFloat?

Because Union{} <: AbstractFloat, so A==Int, B==Union{} is a valid choice. So the only thing that remains to be desired is to have B bound to Union{} inside the function as it is AFAICT the only valid choice.

@vtjnash
Copy link
Member

vtjnash commented Oct 31, 2023

This is fixed now

@vtjnash vtjnash closed this as completed Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

5 participants