-
-
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 #22787, another bounds circularity in type intersection #38245
Conversation
86609ec
to
2ff075e
Compare
(The single tester_macos64 failure is in Distributed and appears unrelated.) |
(tester_linuxaarch64 timed out with no preceding failures.) |
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 do not fully understand the context of the changes to intersect
and so can't comment meaningfully on those changes, but comparto_var
in isolation seems sensical to me and the tests look good. Thanks Jeff! :)
If no one beats me to it, and absent objections or requests for time, I plan to merge these changes on November 2 late in the day PT. Thanks again Jeff! |
test/subtype.jl
Outdated
# for now check that these don't stack overflow | ||
@test_broken typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q<:Ref, | ||
Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R where S) != Union{} | ||
@test_broken typeintersect(Tuple{Type{T}, T, Ref{T}} where T, | ||
Tuple{Type{S}, Ref{S}, S} where S) != Union{} |
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.
If I'm not mistaken, @test_broken
passes, even if they do stack overflow:
Here's an example of these tests passing on master, despite the stack overflow:
julia> # for now check that these don't stack overflow
@test_broken typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q<:Ref,
Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R where S) != Union{}
Test Broken
Expression: typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q <: Ref, (Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R) where S) != Union{}
julia> @test_broken typeintersect(Tuple{Type{T}, T, Ref{T}} where T,
Tuple{Type{S}, Ref{S}, S} where S) != Union{}
Test Broken
Expression: typeintersect(Tuple{Type{T}, T, Ref{T}} where T, Tuple{Type{S}, Ref{S}, S} where S) != Union{}
julia> # You can see that it's stack overflowing when we remove the `@test_broken`:
@test typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q<:Ref,
Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R where S) != Union{}
Error During Test at REPL[4]:2
Test threw exception
Expression: typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q <: Ref, (Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R) where S) != Union{}
StackOverflowError:
Stacktrace:
[1] typeintersect(a::Any, b::Any)
@ Base ./reflection.jl:563
[2] top-level scope
@ REPL[4]:2
[3] eval(m::Module, e::Any)
@ Core ./boot.jl:360
...
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.
Maybe you can change to something like this, which would fail if they stack overflow, but still allow the broken intersection test? Thanks!
# for now check that these don't stack overflow | |
@test_broken typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q<:Ref, | |
Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R where S) != Union{} | |
@test_broken typeintersect(Tuple{Type{T}, T, Ref{T}} where T, | |
Tuple{Type{S}, Ref{S}, S} where S) != Union{} | |
@testset begin | |
# These tests don't pass, but for now check that at least they don't stack overflow | |
t = typeintersect(Tuple{Type{Q}, Q, Ref{Q}} where Q<:Ref, | |
Tuple{Type{S}, Union{Ref{S}, Ref{R}}, R} where R where S) | |
@test_broken t != Union{} | |
t = typeintersect(Tuple{Type{T}, T, Ref{T}} where T, | |
Tuple{Type{S}, Ref{S}, S} where S) | |
@test_broken t != Union{} | |
end |
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.
The above suggestion passes for me on this branch, and fails on master.
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.
To stop my head spinning... what's the correct answer to these intersection problems?
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.
Here's some:
julia> Tuple{Type{Ref}, Ref{Ref}, Ref{Ref}} <: Tuple{Type{T}, Ref{T}, T} where T
true
julia> Tuple{Type{Ref}, Ref{Ref}, Ref{Ref}} <: Tuple{Type{T}, T, Ref{T}} where T
true
julia> Tuple{Type{Ref{<:Ref}}, Ref{Ref{<:Ref}}, Ref{Ref{<:Ref}}} <: Tuple{Type{T}, T, Ref{T}} where T
true
julia> Tuple{Type{Ref{<:Ref}}, Ref{Ref{<:Ref}}, Ref{Ref{<:Ref}}} <: Tuple{Type{T}, T, Ref{T}} where T
true
julia> Tuple{Type{Ref{<:Ref{<:Ref}}}, Ref{Ref{<:Ref{<:Ref}}}, Ref{Ref{<:Ref{<:Ref}}}} <: Tuple{Type{T}, T, Ref{T}} where T
true
julia> Tuple{Type{Ref{<:Ref{<:Ref}}}, Ref{Ref{<:Ref{<:Ref}}}, Ref{Ref{<:Ref{<:Ref}}}} <: Tuple{Type{T}, Ref{T}, T} where T
true
I don't know if there's a general closed-form expression of this, since it seems to be the Union of this out to infinity?
2ff075e
to
addb20c
Compare
Note the added tests are broken, but at least they don't stack overflow. The first test is reduced from the issue. The second test is a simpler version that did not overflow before, but did incorrectly give
Union{}
before. That makes me hopeful that this will not make anything worse.