Work around type intersection bug in _isequal, _isless, __inc, and __dec
#42686
Conversation
|
Nice! Does an issue tracking the underlying intersection bug exist? :) |
|
Possibly #39088 (comment)? |
|
Yes, that would be it. I meant to link to it but forgot, thanks for bringing it up. |
test/tuple.jl
Outdated
| f42457(a::Tuple{Int,Int,Int}, b::Tuple) = Base.isequal(a, Base.inferencebarrier(b)::Tuple) | ||
| @test f42457((1, 1, 1), (1, 1, 1)) |
There was a problem hiding this comment.
This test, as written, passes for me on master. This fails, but still seems indirect as a test:
| f42457(a::Tuple{Int,Int,Int}, b::Tuple) = Base.isequal(a, Base.inferencebarrier(b)::Tuple) | |
| @test f42457((1, 1, 1), (1, 1, 1)) | |
| f42457(a::Tuple{Int,Int,Int}, b::Tuple) = Base.isequal(a, Base.inferencebarrier(b)::NTuple) | |
| @test f42457((1, 1, 1), (1, 1, 1)) |
More direct tests might be:
@test !isempty(methods(Base._isequal, (NTuple{3, Int}, Tuple)))and
g42457(a, b) = Base.isequal(a, b) ? 1 : 2.0
@test only(return_types(f42457, (NTuple{3, Int}, Tuple))) === Union{Float64, Int}
@test only(return_types(f42457, (NTuple{3, Int}, NTuple))) === Union{Float64, Int}
There was a problem hiding this comment.
You need the ::Bool return type decl from the original issue to make it fail, and also yes more tests is good.
There was a problem hiding this comment.
Good catch. Added the ::Bool to the original test to have a test case closely derived from the issue and also added the tests proposed by @vtjnash. (Although I don't understand the intention of the !isempty(methods(...)) one, which also happens to pass on master.)
…and `__dec` The underlying intersection bug (ref. #39088) looks like ``` julia> typeintersect(Tuple{Tuple{Any, Vararg{Any, N}}, Tuple{Any, Vararg{Any, N}}} where N, Tuple{Tuple{Int,Int,Int}, Tuple}) Tuple{Tuple{Int64, Int64, Int64}, NTuple{4, Any}} ``` It leads to the argument types of the functions with signatures of this type to be inferred wrongly, not actually matching the signature, which easily leads to problems down the road. For the four methods touched here, the fact that the two tuple arguments must have the same length is not actually relevant for dispatch as they are internal functions which are only called if this actually holds. Also, this change will just replace the method error with another error if, for some reason, these fuctions would get called for tuples of unequal length.
martinholters
force-pushed
the
mh/fix-42457
branch
from
debe0ae to
e1afeda
Compare
|
Motivated by the following (false positive) error reports of JET, I wanted to introduce # without `N` parameters
julia> using JET
julia> report_call((Any, Tuple{Any})) do t1, t2
isequal(t1, t2) # or `Base._isequal(t1, t2)`
end
═════ 1 possible error found ═════
┌ @ REPL[9]:2 Main.isequal(t1, t2)
│┌ @ tuple.jl:383 Base._isequal(t1, t2)
││┌ @ tuple.jl:389 Core.typeassert(t2, Core.apply_type(Base.Tuple, Base.Any, Core.apply_type(Base.Vararg, Base.Any)))
│││ invalid builtin function call: Core.typeassert(t2::Tuple{}, Core.apply_type(Base.Tuple, Base.Any, Core.apply_type(Base.Vararg, Base.Any)::Core.TypeofVararg)::Type{Tuple{Any, Vararg{Any}}})
││└────────────────(so the idea is to use dispatch signature to narrow
Yes, the "improvement" from #40594 rarely benefits runtime, it's only useful from the static analysis point of view. Having said that, I think the wrong behaviors reported at #42457 are more serious than the benefit for JET's static analysis. |
|
I'm not quite clear, does this PR have any advantage (besides the added tests) over reverting #40594? If it's mainly about JET reporting false positives, then just getting rid of the type-asserts seems to cut it, i.e. just --- a/base/tuple.jl
+++ b/base/tuple.jl
@@ -386,7 +386,7 @@ function _isequal(t1::Tuple{Any,Vararg{Any}}, t2::Tuple{Any,Vararg{Any}})
isequal(t1[1], t2[1]) || return false
t1, t2 = tail(t1), tail(t2)
# avoid dynamic dispatch by telling the compiler relational invariants
- return isa(t1, Tuple{}) ? _isequal((), t2::Tuple{}) : _isequal(t1, t2::Tuple{Any,Vararg{Any}})
+ return isa(t1, Tuple{}) ? _isequal((), t2) : _isequal(t1, t2)
endmakes that |
|
I'd like to keep a part of #40594, since the current dispatch space of And yeah, I think we can just eliminate the type assertion. We can even do: diff --git a/base/tuple.jl b/base/tuple.jl
index 52c8ae1e3f..9ee5df3ccf 100644
--- a/base/tuple.jl
+++ b/base/tuple.jl
@@ -384,9 +384,7 @@ isequal(t1::Tuple, t2::Tuple) = length(t1) == length(t2) && _isequal(t1, t2)
_isequal(::Tuple{}, ::Tuple{}) = true
function _isequal(t1::Tuple{Any,Vararg{Any}}, t2::Tuple{Any,Vararg{Any}})
isequal(t1[1], t2[1]) || return false
- t1, t2 = tail(t1), tail(t2)
- # avoid dynamic dispatch by telling the compiler relational invariants
- return isa(t1, Tuple{}) ? _isequal((), t2::Tuple{}) : _isequal(t1, t2::Tuple{Any,Vararg{Any}})
+ return _isequal(tail(t1), tail(t2))
end
function _isequal(t1::Any32, t2::Any32)
for i = 1:length(t1)(I just confirmed JET is happy with the diff above) julia> using JET
julia> report_call((Any, Tuple{Any})) do t1, t2
Base._isequal(t1, t2)
end
═════ 1 possible error found ═════
┌ @ REPL[2]:2 Base._isequal(t1, t2)
│┌ @ tuple.jl:389 Core.typeassert(t2, Core.apply_type(Base.Tuple, Base.Any, Core.apply_type(Base.Vararg, Base.Any)))
││ invalid builtin function call: Core.typeassert(t2::Tuple{}, Core.apply_type(Base.Tuple, Base.Any, Core.apply_type(Base.Vararg, Base.Any)::Core.TypeofVararg)::Type{Tuple{Any, Vararg{Any}}})
│└────────────────
julia> @eval Base begin
delete_method.(methods(_isequal, (Tuple, Tuple)));
_isequal(::Tuple{}, ::Tuple{}) = true
function _isequal(t1::Tuple{Any,Vararg{Any}}, t2::Tuple{Any,Vararg{Any}})
isequal(t1[1], t2[1]) || return false
return _isequal(tail(t1), tail(t2))
end
end
_isequal (generic function with 2 methods)
julia> report_call((Any, Tuple{Any})) do t1, t2
Base._isequal(t1, t2)
end
No errors !
julia> report_call((Any, Tuple{Any,Any})) do t1, t2
Base._isequal(t1, t2)
end
No errors ! |
|
Ah, that looks quite nice. I'll do that and also take another look at the other functions whether similar simplifications are possible. |
|
Indeed, looks like we can just revert to the function bodies from before #40594. I now get for T1 in [Any, Tuple{Any}, Tuple{Any,Any}, Tuple{Any,Any,Any}],
T2 in (T1 == Any ? [Tuple{Any}, Tuple{Any,Any}, Tuple{Any,Any,Any}] : [Any])
display(report_call((T1, T2)) do t1, t2
Base._isequal(t1, t2)
Base.IteratorsMD._isless(0, t1, t2)
Base.IteratorsMD.__inc(t1, t2)
Base.IteratorsMD.__dec(t1, t2)
end)
endto produce "No errors !" six times. For reference, this would be the diff of the relevant parts compared to before #40594: -isequal(t1::Tuple, t2::Tuple) = (length(t1) == length(t2)) && _isequal(t1, t2)
-_isequal(t1::Tuple{}, t2::Tuple{}) = true
-_isequal(t1::Tuple{Any}, t2::Tuple{Any}) = isequal(t1[1], t2[1])
-_isequal(t1::Tuple, t2::Tuple) = isequal(t1[1], t2[1]) && _isequal(tail(t1), tail(t2))
+isequal(t1::Tuple, t2::Tuple) = length(t1) == length(t2) && _isequal(t1, t2)
+_isequal(::Tuple{}, ::Tuple{}) = true
+function _isequal(t1::Tuple{Any,Vararg{Any}}, t2::Tuple{Any,Vararg{Any}})
+ return isequal(t1[1], t2[1]) && _isequal(tail(t1), tail(t2))
+end- @inline function _isless(ret, I1::NTuple{N,Int}, I2::NTuple{N,Int}) where N
- newret = ifelse(ret==0, icmp(I1[N], I2[N]), ret)
- _isless(newret, Base.front(I1), Base.front(I2))
+ @inline function _isless(ret, I1::Tuple{Int,Vararg{Int}}, I2::Tuple{Int,Vararg{Int}})
+ newret = ifelse(ret==0, icmp(last(I1), last(I2)), ret)
+ return _isless(newret, Base.front(I1), Base.front(I2))
end __inc(::Tuple{}, ::Tuple{}) = false, ()
- @inline function __inc(state::Tuple{Int}, indices::Tuple{<:OrdinalRange})
+ @inline function __inc(state::Tuple{Int}, indices::Tuple{OrdinalRangeInt})
rng = indices[1]
I = state[1] + step(rng)
valid = __is_valid_range(I, rng) && state[1] != last(rng)
return valid, (I, )
end
- @inline function __inc(state, indices)
+ @inline function __inc(state::Tuple{Int,Int,Vararg{Int}}, indices::Tuple{OrdinalRangeInt,OrdinalRangeInt,Vararg{OrdinalRangeInt}})
rng = indices[1]
I = state[1] + step(rng)
if __is_valid_range(I, rng) && state[1] != last(rng) @inline __dec(::Tuple{}, ::Tuple{}) = false, ()
- @inline function __dec(state::Tuple{Int}, indices::Tuple{<:OrdinalRange})
+ @inline function __dec(state::Tuple{Int}, indices::Tuple{OrdinalRangeInt})
rng = indices[1]
I = state[1] - step(rng)
valid = __is_valid_range(I, rng) && state[1] != first(rng)
return valid, (I,)
end
-
- @inline function __dec(state, indices)
+ @inline function __dec(state::Tuple{Int,Int,Vararg{Int}}, indices::Tuple{OrdinalRangeInt,OrdinalRangeInt,Vararg{OrdinalRangeInt}})
rng = indices[1]
I = state[1] - step(rng)
if __is_valid_range(I, rng) && state[1] != first(rng) |
The underlying intersection bug looks like
It leads to the argument types of the functions with signatures of this type to be inferred wrongly, not actually matching the signature, which easily leads to problems down the road.
For the four methods touched here, the fact that the two tuple arguments must have the same length is not actually relevant for dispatch as they are internal functions which are only called if this actually holds. Also, this change will just replace the method error with another error if, for some reason, these functions would get called for tuples of unequal length. (For
_isequaland_islessI've added type-assertions to ensure this, for__incand__decI'm not sure about the early returns, maybe someone who knows this code might take a look. Although it probably doesn't matter anyway.)@aviatesk Can you confirm this preserves the benefits from #40594?
Fixes #42457. Closes #42682.