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

Possible wrong error detection #184

Closed
jakobnissen opened this issue May 2, 2021 · 3 comments · Fixed by #252
Closed

Possible wrong error detection #184

jakobnissen opened this issue May 2, 2021 · 3 comments · Fixed by #252
Labels
abstract interpretation Improvements to abstract interpretation

Comments

@jakobnissen
Copy link
Contributor

jakobnissen commented May 2, 2021

MWE:

julia> function g()
           y = Ref{Any}(0)
           y[] != 0
       end
g (generic function with 1 method)

julia> @report_call g()
═════ 2 possible errors found ═════
┌ @ REPL[22]:3 Main.!=(Base.getindex(y), 0)
│┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:399 Base.!=(x, Core.apply_type(SIMD.Vec, _, _)(y))
││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:399 Core.apply_type(SIMD.Vec, _, _)(y)
│││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:20 SIMD.constantvector(v, Core.apply_type(SIMD.Vec, _, _))
││││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:15 SIMD._unsafe_convert(_, v)
│││││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:13 SIMD.%(v, _)
││││││ no matching method found for call signature: SIMD.%(v::Float32, _::Type{T} where T<:Union{Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8})
│││││└───────────────────────────────────────────────────────────────────
│││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:20 SIMD.constantvector(v, Core.apply_type(SIMD.Vec, _, _))
││││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:15 SIMD._unsafe_convert(_, v)
│││││┌ @ /Users/jakobnissen/.julia/packages/SIMD/x4UOS/src/simdvec.jl:13 SIMD.%(v, _)
││││││ no matching method found for call signature: SIMD.%(v::Float64, _::Type{T} where T<:Union{Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8})
│││││└───────────────────────────────────────────────────────────────────
Any

The function it "detects" is:

Base.:(!=)(x::Vec{N, Float32}, y::T) where {T <: Union{Float32, Float64, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}}
    x != Vec{N, T}(y)
end

My question is: Isn't it a bug that something that is inferred as x::Any != y::Int64 hits this method?

@aviatesk
Copy link
Owner

aviatesk commented May 6, 2021

Guess SIMD is required to be loaded to reproduce this ?
EDIT: yea I could repro this.

@aviatesk
Copy link
Owner

aviatesk commented May 6, 2021

My question is: Isn't it a bug that something that is inferred as x::Any != y::Int64 hits this method?

Unfortunately, this is expected -- ≠(::Any, ::Int) entails the possibility to be dispatched to those defined in SIMD.jl:

julia> methods(, (Any, Int,))
# 3 methods for generic function "!=":
[1] !=(x::Vec{N, T}, y::T2) where {N, T2<:Union{Float32, Float64, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}, T<:Union{Float32, Float64}} in SIMD at /Users/aviatesk/.julia/packages/SIMD/x4UOS/src/simdvec.jl:398
[2] !=(x::Vec{N, T}, y::T2) where {N, T2<:Union{Float32, Float64, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}, T<:Union{Bool, Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}} in SIMD at /Users/aviatesk/.julia/packages/SIMD/x4UOS/src/simdvec.jl:398
[3] !=(x, y) in Base at operators.jl:278

In order to improve the inference accuracy, Julia's abstract interpretation tries to infer all the matching methods when the # of the matches are not so many (heuristically this is set to 3 by default, but we can configure it with max_methods configuration).
In this case there are certainly 3 matching methods, and so this error report is expected.

julia> report_call((Any,); max_methods=2) do a
           a  0
       end
No errors !
Any

Well, as explained above, inference will give up when there are too many of them in order to obtain analysis performance, and otherwise inference will never terminate in practical time, e.g. consider inferring all the matching methods for show(::IO, ::Any).
So in other word JET isn't a sound analyzer at all by design, and I feel it might be okay even if we set MAX_METHODS to 1 or 2, by default. Then JET analysis will be faster and produce less false positives like this, while it will be less accurate when the target code is not well typed.

@jakobnissen
Copy link
Contributor Author

Perhaps it makes sense to special case Any (or perhaps even other, abstract types?), such that they don't match any methods in JET's analysis?

The issue is that it's not very useful output to get. Sure, !=(::Vec{N, T}, ::Float64) is one possible method that could be called, and that would error, but so will !=(::String, ::Float64), and that is also a possibility when the arguments are inferred to be !=(::Any, ::Float64).

I guess the real issue is that complete type instability (e.g. inference to Any) is a different beast than inference to e.g. a union. Perhaps anything that infers to Any should not show up in further analysis and instead go into a "can't be inferred bin"?

@aviatesk aviatesk pinned this issue May 8, 2021
@aviatesk aviatesk added the abstract interpretation Improvements to abstract interpretation label May 8, 2021
aviatesk added a commit that referenced this issue Sep 13, 2021
For `BasicPass`, we skip errors reported from abstract frames.

- fixes #184
- fixes #154
aviatesk added a commit that referenced this issue Sep 13, 2021
For `BasicPass`, we skip errors reported from abstract frames.

- fixes #184
- fixes #154
aviatesk added a commit that referenced this issue Sep 13, 2021
For `BasicPass`, we skip errors reported from abstract frames.

- fixes #184
- fixes #154
aviatesk added a commit that referenced this issue Sep 13, 2021
For `BasicPass`, we skip errors reported from abstract frames.

- fixes #184
- fixes #154
@aviatesk aviatesk unpinned this issue Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstract interpretation Improvements to abstract interpretation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants