Branches that restrict a type

[tecosaur]

Applying JET to my code base, there seem to be many instances where the following pattern:

a = ... # ::Union{X, Nothing}
if !isnothing(a)
  # do things with a
else
  ...

produces a JET "possible error" where within the if !isnothing(a) branch it complains that a could be nothing.

Is JET likely to gain the capability to infer the restriction of type unions in if statements?

JET v0.7.15, Julia 1.9

[aviatesk]

It should work. Can you provide a concrete example where it does not work?

[tecosaur]

Sure, here's one example.

In https://github.com/tecosaur/DataToolkitBase.jl/blob/ca3de2496eb8517b3e8c78b5ab86ae67121a3971/src/model/errors.jl#L42-L49

if !isnothing(err.collection)
    ident = @advise err.collection parse(Identifier, err.identifier)
    if !isnothing(ident.type)
        identnotype = Identifier(ident.collection, ident.dataset,
                                 nothing, ident.parameters)
        notypematches = refine( # Line 47, that JET complains about
            err.collection, err.collection.datasets, identnotype)
    end

JET reports

┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/errors.jl:47 err.collection.datasets
│┌ @ Base.jl:37 Base.getfield(x, f)
││ type Nothing has no field datasets
│└──────────────
┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/errors.jl:47 notypematches = refine(err.collection, err.collection.datasets, identnotype)
│ no matching method found `refine(::Nothing, ::Vector{DataToolkitBase.DataSet}, ::Any)` (1/2 union split): notypematches = refine((err::DataToolkitBase.UnresolveableIdentifier{DataToolkitBase.DataSet, String}).collection::Union{Nothing, DataToolkitBase.DataCollection}, ((err::DataToolkitBase.UnresolveableIdentifier{DataToolkitBase.DataSet, String}).collection::Union{Nothing, DataToolkitBase.DataCollection}).datasets::Vector{DataToolkitBase.DataSet}, identnotype)
└───────────────────────────────────────────────────────────────

Since JET is complaining that refine(::Nothing, ...) might be called, it must be thinking that err.collection could be nothing as it is a Union{DataCollection, Nothing}, however a few lines earlier we enter an if !isnothing(err.collection) branch.

---

A similar false-positive occurs here https://github.com/tecosaur/DataToolkitBase.jl/blob/ca3de2496eb8517b3e8c78b5ab86ae67121a3971/src/model/writer.jl#L10

Base.iswritable(dc::DataCollection) =
    !isnothing(dc.path) &&
    get(dc, "locked", false) !== true &&
    try # why is this such a hassle?
        open(io -> iswritable(io), dc.path, "a")

On that last line JET reports that open(::Nothing, ::String) could be called, presumably because dc.path is initially of the type Union{String, Nothing}, but we have the !isnothing(dc.path) check earlier.

┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/errors.jl:126 DataToolkitBase.show(crep, DataToolkitBase.MIME("text/plain"), collection)
│┌ @ multimedia.jl:47 Base.Multimedia.show(io, x)
││┌ @ /home/tec/.julia/dev/DataToolkitBase/src/interaction/display.jl:177 DataToolkitBase.iswritable(datacollection)
│││┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/writer.jl:10 DataToolkitBase.open(#68, dc.path, "a")
││││┌ @ io.jl:392 Base.:(var"#open#409")(tuple(pairs(NamedTuple()), #self#, f), args...)
│││││┌ @ io.jl:393  = open(args...)
││││││ no matching method found `open(::Nothing, ::String)`:  = open(args::Tuple{Nothing, String}...)
│││││└─────────────

[aviatesk]

Thanks, I will take a look later.

[tecosaur]

Thanks for getting back to me so promptly! I love the idea of JET and can find it very useful at times, but the main barrier to me is the number of false positives I need to wade through.

If that could be improved, I'd be very happy indeed! :heart_eyes:

On a similar note, do you know if there's any hope for examples like this (https://github.com/tecosaur/DataToolkitBase.jl/blob/ca3de2496eb8517b3e8c78b5ab86ae67121a3971/src/model/errors.jl#L85-L87):

if isnothing(err.collection) && !isempty(STACK)
    for collection in last(Iterators.peel(STACK))

where I think JET is complaining that if STACK is empty that last could be called on nothing, but above I've just ensured that STACK is non-empty.

┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/errors.jl:86 DataToolkitBase.last(DataToolkitBase.Iterators.peel(STACK))
│┌ @ abstractarray.jl:520 lastindex(a)
││ no matching method found `lastindex(::Nothing)`: lastindex(a::Nothing)
│└────────────────────────

[aviatesk]

Okay, to avoid these issues, you can extract the variable into a local variable. In the first case, for example, the false positive error will not be reported if you extract err.collection into a local variable before you perform the type branching on it, like this:

    collection = err.collection
    if !isnothing(collection)
        ident = @advise collection parse(Identifier, err.identifier)
        if !isnothing(ident.type)
            identnotype = Identifier(ident.collection, ident.dataset,
                                     nothing, ident.parameters)
            notypematches = refine(
                collection, err.collection.datasets, identnotype)
        end
    else

This is because the compiler cannot prove that the multiple field accesses to err.collection are all pointing to the same memory location.

However, the good news is that starting from Julia version 1.10, the compiler will be able to analyze such aliased field accesses, so you won't need the "local variable extraction" to achieve the best inferrability. One caveat is that it only works if the struct is immutable or if the field being accessed is declared as const.

[aviatesk]

FWIW, here is a documentation that may help you fix the issue: https://aviatesk.github.io/JET.jl/dev/jetanalysis/#How-to-fix-2

[aviatesk]

On a similar note, do you know if there's any hope for examples like this (https://github.com/tecosaur/DataToolkitBase.jl/blob/ca3de2496eb8517b3e8c78b5ab86ae67121a3971/src/model/errors.jl#L85-L87):

if isnothing(err.collection) && !isempty(STACK)
    for collection in last(Iterators.peel(STACK))

where I think JET is complaining that if STACK is empty that last could be called on nothing, but above I've just ensured that STACK is non-empty.

┌ @ /home/tec/.julia/dev/DataToolkitBase/src/model/errors.jl:86 DataToolkitBase.last(DataToolkitBase.Iterators.peel(STACK))
│┌ @ abstractarray.jl:520 lastindex(a)
││ no matching method found `lastindex(::Nothing)`: lastindex(a::Nothing)
│└────────────────────────

This example is interesting. JET should not report the false positive here. For my own note, here is a reduced example:

julia> report_call((Vector{Int},)) do STACK
           a = Iterators.peel(STACK)
           last(a)
       end
═════ 1 possible error found ═════
┌ @ REPL[5]:3 last(a)
│┌ @ abstractarray.jl:520 lastindex(a)
││ no matching method found `lastindex(::Nothing)`: lastindex(a::Nothing)
│└────────────────────────

Thanks for getting back to me so promptly! I love the idea of JET and can find it very useful at times, but the main barrier to me is the number of false positives I need to wade through.

Yeah, glad to here that! And yes, I'm very interested in reducing the false positives and this sort of issue is really helpful to improve JET.

[aviatesk]

This example is interesting. JET should not report the false positive here. For my own note, here is a reduced example:

julia> report_call((Vector{Int},)) do STACK
           a = Iterators.peel(STACK)
           last(a)
       end
═════ 1 possible error found ═════
┌ @ REPL[5]:3 last(a)
│┌ @ abstractarray.jl:520 lastindex(a)
││ no matching method found `lastindex(::Nothing)`: lastindex(a::Nothing)
│└────────────────────────

Ah, this is a known problem. Ideally I would like to fix it by allowing the compiler to reason about array shapes, but it is one of the hard compiler problems.

As a short-term fix, I think we can add an additional lattice layer to JET's custom abstract interpretation, adjointing a lattice element that specially represents non-empty array objects.

For now, I recommend you add type annotation like:

    if isnothing(err.collection) && !isempty(STACK)
            for collection in last(Iterators.peel(STACK)::Tuple)

[tecosaur]

Thanks for the comment @aviatesk! I'll look forward to fewer false-positives with 1.10.

One idea that might be helpful, would it be possible to split the errors/warnings produced by JET into different confidence levels? From "we're completely certain this is a bug" to "it's possible this might not have been handled properly", and then either split the reporting by confidence or allow filtering to only the confidence >= X issues.

I'm thinking this could be helpful when I use JET for the first time in a project and there are 10s-100s of issues. I know I'd like to be able to start with the worst/definitive issues before looking at speculative/potential problems.

[tecosaur]

@JuliaRegistrator registor

[JuliaRegistrator]

Error while trying to register: Action not recognized: registor

[tecosaur]

@JuliaRegistrator register

[JuliaRegistrator]

Error while trying to register: Register Failed @tecosaur, it looks like you don't have collaborator status on this repository.

[tecosaur]

Oh dammit, I switched back into the wrong tab :facepalm:

please ignore this.... (and feel free to delete these spurious messages)