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

inference: fix missing LimitedAccuracy handling in conditional_changes #40744

Merged
merged 3 commits into from
May 13, 2021

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented May 7, 2021

The following JET analysis told me that we missed to handle
LimitedAccuracy in conditional_changes and limited conditional
constraints weren't propagated:

julia> using JET, Test

julia> report_call() do
           @testset "foo" begin; true; end
       end
┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1262 Test.finish(ts)
│┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1007 Test.print_test_results(ts)
││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:941 #self#(ts, 0)
│││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:961 Test.get_alignment(ts, 0)
││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.map(#36, Base.getproperty(ts, :results))
│││││┌ @ abstractarray.jl:2362 Base.collect_similar(A, Base.Generator(f, A))
││││││┌ @ array.jl:638 Base._collect(cont, itr, Base.IteratorEltype(itr), Base.IteratorSize(itr))
│││││││┌ @ array.jl:723 Base.iterate(itr)
││││││││┌ @ generator.jl:47 Base.getproperty(g, :f)(Base.getindex(y, 1))
│││││││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.get_alignment(t, Test.+(Core.getfield(#self#, :depth), 1))
│││││││││││││┌ @ array.jl:728 Base.collect_to_with_first!(Base._similar_for(c, Base.typeof(v1), itr, isz), v1, itr, st)
││││││││││││││┌ @ array.jl:734 Base.collect_to!(dest, itr, Base.+(i1, 1), st)
│││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:764 Base.collect_to!(new, itr, Base.+(i, 1), st)
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 2, _8)
│││││││││││││││││┌ @ tuple.jl:94 Base.iterate(I, state)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
│││││││││││││││││└───────────────
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 1)
│││││││││││││││││┌ @ tuple.jl:89 Base.iterate(I)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
│││││││││││││││││└───────────────
Union{Test.FallbackTestSet, Test.DefaultTestSet}

After this PR:

julia> report_call() do
           @testset "foo" begin; true; end
       end
No errors !
Union{Test.FallbackTestSet, Test.DefaultTestSet}

Well, it's hard to test this kind of intermediate state of inference, so
I didn't include test cases this PR fixes.
(maybe can we use JET as an inference test ?)

…nges`

The following JET analysis told me that we missed to handle
`LimitedAccuracy` in `conditional_changes` and limited conditional
constraints weren't propagated:
```julia
julia> using JET, Test

julia> report_call() do
           @testset "foo" begin; true; end
       end
┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1262 Test.finish(ts)
│┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1007 Test.print_test_results(ts)
││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:941 #self#(ts, 0)
│││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:961 Test.get_alignment(ts, 0)
││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.map(JuliaLang#36, Base.getproperty(ts, :results))
│││││┌ @ abstractarray.jl:2362 Base.collect_similar(A, Base.Generator(f, A))
││││││┌ @ array.jl:638 Base._collect(cont, itr, Base.IteratorEltype(itr), Base.IteratorSize(itr))
│││││││┌ @ array.jl:723 Base.iterate(itr)
││││││││┌ @ generator.jl:47 Base.getproperty(g, :f)(Base.getindex(y, 1))
│││││││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.get_alignment(t, Test.+(Core.getfield(#self#, :depth), 1))
│││││││││││││┌ @ array.jl:728 Base.collect_to_with_first!(Base._similar_for(c, Base.typeof(v1), itr, isz), v1, itr, st)
││││││││││││││┌ @ array.jl:734 Base.collect_to!(dest, itr, Base.+(i1, 1), st)
│││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:764 Base.collect_to!(new, itr, Base.+(i, 1), st)
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 2, _8)
│││││││││││││││││┌ @ tuple.jl:94 Base.iterate(I, state)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
│││││││││││││││││└───────────────
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 1)
│││││││││││││││││┌ @ tuple.jl:89 Base.iterate(I)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
│││││││││││││││││└───────────────
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```

After this PR:
```julia
julia> report_call() do
           @testset "foo" begin; true; end
       end
No errors !
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```

Well, it's hard to test this kind of intermediate state of inference, so
I didn't include test cases this PR fixes.
(maybe can we use JET as an inference test ?)
@aviatesk aviatesk requested a review from vtjnash May 7, 2021 14:42
@aviatesk aviatesk added the compiler:inference Type inference label May 7, 2021
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Might want to leave behind a comment about why this is legal though? The was answering this query correctly, since the value newtyp is indeed lower in the lattice than typ, but we're using as an approximation to taking the intersection (tmeet).

@aviatesk aviatesk closed this May 8, 2021
@aviatesk aviatesk reopened this May 8, 2021
@aviatesk
Copy link
Sponsor Member Author

@vtjnash bump

@vtjnash vtjnash merged commit 853a936 into JuliaLang:master May 13, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented May 13, 2021

oh, oops, I assumed you would be able to merge

@aviatesk
Copy link
Sponsor Member Author

hehe, I don't have the write access yet.
I'd appreciate if you merge #40745 as well.

@aviatesk aviatesk deleted the missinglimited branch May 13, 2021 16:22
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
…nges` (JuliaLang#40744)

The following JET analysis told me that we missed to handle
`LimitedAccuracy` in `conditional_changes` and limited conditional
constraints weren't propagated:
```julia
julia> using JET, Test

julia> report_call() do
           @testset "foo" begin; true; end
       end
┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1262 Test.finish(ts)
│┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1007 Test.print_test_results(ts)
││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:941 #self#(ts, 0)
│││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:961 Test.get_alignment(ts, 0)
││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.map(JuliaLang#36, Base.getproperty(ts, :results))
│││││┌ @ abstractarray.jl:2362 Base.collect_similar(A, Base.Generator(f, A))
││││││┌ @ array.jl:638 Base._collect(cont, itr, Base.IteratorEltype(itr), Base.IteratorSize(itr))
│││││││┌ @ array.jl:723 Base.iterate(itr)
││││││││┌ @ generator.jl:47 Base.getproperty(g, :f)(Base.getindex(y, 1))
│││││││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.get_alignment(t, Test.+(Core.getfield(#self#, :depth), 1))
│││││││││││││┌ @ array.jl:728 Base.collect_to_with_first!(Base._similar_for(c, Base.typeof(v1), itr, isz), v1, itr, st)
││││││││││││││┌ @ array.jl:734 Base.collect_to!(dest, itr, Base.+(i1, 1), st)
│││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:764 Base.collect_to!(new, itr, Base.+(i, 1), st)
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 2, _8)
│││││││││││││││││┌ @ tuple.jl:94 Base.iterate(I, state)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
│││││││││││││││││└───────────────
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 1)
│││││││││││││││││┌ @ tuple.jl:89 Base.iterate(I)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
│││││││││││││││││└───────────────
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```

After this PR:
```julia
julia> report_call() do
           @testset "foo" begin; true; end
       end
No errors !
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…nges` (JuliaLang#40744)

The following JET analysis told me that we missed to handle
`LimitedAccuracy` in `conditional_changes` and limited conditional
constraints weren't propagated:
```julia
julia> using JET, Test

julia> report_call() do
           @testset "foo" begin; true; end
       end
┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1262 Test.finish(ts)
│┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1007 Test.print_test_results(ts)
││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:941 #self#(ts, 0)
│││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:961 Test.get_alignment(ts, 0)
││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.map(JuliaLang#36, Base.getproperty(ts, :results))
│││││┌ @ abstractarray.jl:2362 Base.collect_similar(A, Base.Generator(f, A))
││││││┌ @ array.jl:638 Base._collect(cont, itr, Base.IteratorEltype(itr), Base.IteratorSize(itr))
│││││││┌ @ array.jl:723 Base.iterate(itr)
││││││││┌ @ generator.jl:47 Base.getproperty(g, :f)(Base.getindex(y, 1))
│││││││││┌ @ /Users/aviatesk/julia/julia/usr/share/julia/stdlib/v1.7/Test/src/Test.jl:1034 Test.get_alignment(t, Test.+(Core.getfield(#self#, :depth), 1))
│││││││││││││┌ @ array.jl:728 Base.collect_to_with_first!(Base._similar_for(c, Base.typeof(v1), itr, isz), v1, itr, st)
││││││││││││││┌ @ array.jl:734 Base.collect_to!(dest, itr, Base.+(i1, 1), st)
│││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:764 Base.collect_to!(new, itr, Base.+(i, 1), st)
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 2, _8)
│││││││││││││││││┌ @ tuple.jl:94 Base.iterate(I, state)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing, Int64}): Base.iterate(I::Nothing, state::Int64)
│││││││││││││││││└───────────────
││││││││││││││││┌ @ /Users/aviatesk/julia/julia/base/array.jl:758 Base.indexed_iterate(y, 1)
│││││││││││││││││┌ @ tuple.jl:89 Base.iterate(I)
││││││││││││││││││ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Base.iterate(I::Nothing)
│││││││││││││││││└───────────────
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```

After this PR:
```julia
julia> report_call() do
           @testset "foo" begin; true; end
       end
No errors !
Union{Test.FallbackTestSet, Test.DefaultTestSet}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants