-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix type intersection for invalidation during method insertion to handle typevars properly #27351
Conversation
@@ -509,9 +509,19 @@ int jl_typemap_intersection_visitor(union jl_typemap_t map, int offs, | |||
ty = jl_tparam(ttypes, offs); | |||
} | |||
if (ty) { | |||
while (jl_is_typevar(ty)) | |||
ty = ((jl_tvar_t*)ty)->ub; | |||
// approxify the tparam until we have a valid type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approxify is not a word unless you live in silicon valley.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write that comment, it's a good ol' fashioned jamesonism
EDIT: just to clarify, I stand by approxify
in all of its linguistic glory
Looks like the majority of test failures are from method ambiguities such as the following: julia> using SparseArrays: SparseVector
julia> x = SparseVector(rand(3));
julia> map(min, x, x)
ERROR: MethodError: map(::typeof(min), ::SparseVector{Float64,Int64}, ::SparseVector{Float64,Int64}) is ambiguous. Candidates:
map(f::Tf, A::Union{SparseArrays.SparseMatrixCSC, SparseVector}, Bs::Vararg{Union{SparseArrays.SparseMatrixCSC, SparseVector},N}) where {Tf, N} in SparseArrays.HigherOrderFns at /Users/jarrettrevels/data/repos/julia/usr/share/julia/stdlib/v0.7/SparseArrays/src/higherorderfns.jl:141
map(::typeof(min), x::SparseArrays.AbstractSparseArray{Tv,Ti,1} where Ti where Tv, y::SparseArrays.AbstractSparseArray{Tv,Ti,1} where Ti where Tv) in SparseArrays at /Users/jarrettrevels/data/repos/julia/usr/share/julia/stdlib/v0.7/SparseArrays/src/sparsevector.jl:1285
Possible fix, define
map(::typeof(min), ::SparseVector, ::SparseVector) whereas on master julia> using SparseArrays: SparseVector
julia> x = SparseVector(rand(3));
julia> map(min, x, x)
3-element SparseVector{Float64,Int64} with 3 stored entries:
[1] = 0.272931
[2] = 0.982437
[3] = 0.420046
julia> @which map(min, x, x)
map(::typeof(min), x::SparseArrays.AbstractSparseArray{Tv,Ti,1} where Ti where Tv, y::SparseArrays.AbstractSparseArray{Tv,Ti,1} where Ti where Tv) in SparseArrays at /Users/jarrettrevels/data/repos/juliadev/usr/share/julia/stdlib/v0.7/SparseArrays/src/sparsevector.jl:1285 This seems like an actual method ambiguity that was exposed by this PR's fix. A gist of the FreeBSD logs containing these kinds of failures: https://gist.github.com/jrevels/b9eaae8208fcfa041dd2097c8878eb83 I made a smaller MWE to construct a test case, but lo and behold, master is getting the right answer in my MWE: # tried to be as close as possible to the SparseArrays
# implementation that triggers the bug
julia> module Foos
abstract type AbstractFoo{A,B,C} end
const AbstractFoo1{A,B} = AbstractFoo{A,B,1}
struct Foo1{A,B<:Integer} <: AbstractFoo{A,B,1} end
struct Foo2{A,B<:Integer} <: AbstractFoo{A,B,2} end
g(::typeof(min), x::AbstractFoo1, y::AbstractFoo1) = "oops"
module Funcs
import ..Foos: g, Foo1, Foo2
FooEither = Union{Foo1,Foo2}
g(f::F, A::FooEither, B::Vararg{FooEither,N}) where {F,N} = "boo"
end
end # module Foo
Main.Foos
julia> x = Foos.Foo1{Float64,Int}()
Main.Foos.Foo1{Float64,Int64}()
julia> Foos.g(min, x, x)
ERROR: MethodError: Main.Foos.g(::typeof(min), ::Main.Foos.Foo1{Float64,Int64}, ::Main.Foos.Foo1{Float64,Int64}) is ambiguous. Candidates:
g(f::F, A::Union{Main.Foos.Foo1, Main.Foos.Foo2}, B::Vararg{Union{Main.Foos.Foo1, Main.Foos.Foo2},N}) where {F, N} in Main.Foos.Funcs at REPL[1]:10
g(::typeof(min), x::Main.Foos.AbstractFoo{A,B,1} where B where A, y::Main.Foos.AbstractFoo{A,B,1} where B where A) in Main.Foos at REPL[1]:6
Possible fix, define
g(::typeof(min), ::Main.Foos.Foo1, ::Main.Foos.Foo1) It seems like master is getting the right answer here, even though it doesn't for the analogous |
The optimization here only triggers once you have a dozen or so methods for the function. I think you can even add a bunch of dummy methods with the same signature ( |
Thanks, @vtjnash, with that I can now trigger it reliably on master: julia> module Foos
abstract type AbstractFoo{A,B,C} end
const AbstractFoo1{A,B} = AbstractFoo{A,B,1}
struct Foo1{A,B<:Integer} <: AbstractFoo{A,B,1} end
struct Foo2{A,B<:Integer} <: AbstractFoo{A,B,2} end
g(::typeof(min), x::AbstractFoo1, y::AbstractFoo1) = "oops"
for i in 1:15
@eval g(::Val{$i}, x::AbstractFoo1, y::AbstractFoo1) = $i
end
module Funcs
import ..Foos: g, Foo1, Foo2
FooEither = Union{Foo1,Foo2}
g(f::F, A::FooEither, B::Vararg{FooEither,N}) where {F,N} = "boo"
end
end
Main.Foos
julia> x = Foos.Foo1{Float64,Int}()
Main.Foos.Foo1{Float64,Int64}()
julia> Foos.g(min, x, x)
"oops" I'll work on making this more minimal and adding it as a test. EDIT: Here's a more minimal MWE: abstract type AbstractFoo end
struct Foo <: AbstractFoo end
for i in 1:15
@eval f(::Val{$i}, ::AbstractFoo, ::AbstractFoo) = $i
end
f(::T, ::Foo, ::Foo) where {T} = 16
f(Val(1), Foo(), Foo()) |
…dle typevars properly
a3a75f8
to
a849ded
Compare
Woohoo, things are passing here! Just waiting on CircleCI now. Assuming it passes, I'll merge this in the morning unless there are any objections. |
Okay, the one build that failed here did so for an unrelated reason:
In case anyone is interested, here's a gist of the failing test's logs: https://gist.github.com/jrevels/00e596cd7fb27d297b5e8e63b4599c0b |
I still need to add a test, but fixes JuliaLabs/Cassette.jl#44 and thus resolves a bunch of the world age problems that I thought we'd need #27073 to fix. With this fix, we might not need #27073 to block an initial Cassette release (though eventually we should still probably refactor the world age mechanism such that #27073 is correctly implementable).
We should probably backport this once it's ready.
All credit to @vtjnash as usual.