Skip to content

Fix a JET warning #59111

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix a JET warning #59111

wants to merge 1 commit into from

Conversation

fingolfin
Copy link
Member

Encountered while looking through JET report for GAP.jl

invalid builtin function call: getfield(x::RawFD, f::Symbol)

Clearly the code itself really is fine, but the annotation silences this warning.

Full context:

┌ _spawn_primitive(file::String, cmd::Cmd, stdio::Memory{Union{RawFD, Base.SyncCloseFD, IO}}) @ Base ./process.jl:113
│┌ collect(::Type{Task}, itr::Base.Generator{Base.Iterators.Filter{…}, Base.var"#_spawn_primitive##0#_spawn_primitive##1"}) @ Base ./array.jl:641
││┌ _collect(::Type{…}, itr::Base.Generator{…}, isz::Base.SizeUnknown) @ Base ./array.jl:647
│││┌ iterate(::Base.Generator{Base.Iterators.Filter{…}, Base.var"#_spawn_primitive##0#_spawn_primitive##1"}) @ Base ./generator.jl:48
││││┌ (::Base.var"#_spawn_primitive##0#_spawn_primitive##1")(io::Union{RawFD, Base.SyncCloseFD, IO}) @ Base ./none:0
│││││┌ getproperty(x::RawFD, f::Symbol) @ Base ./Base_compiler.jl:54
││││││ invalid builtin function call: getfield(x::RawFD, f::Symbol)
│││││└────────────────────

@fingolfin
Fix a JET warning
401bc31 
invalid builtin function call: getfield(x::RawFD, f::Symbol)
@fingolfin fingolfin added the backport 1.12 Change should be backported to release-1.12 label Jul 27, 2025
@Keno
Copy link
Member

Keno commented Jul 29, 2025

I'm ok with it, but also does point to missing inference quality in guarded generators.

@fingolfin
Copy link
Member Author

Yes I agree this is kinda papering over a problem (or opportunity for improvement).

I am seeing more (all against the Julia release-1.12 branch right now):

│││││┌ complete_path_string(path::String, hint::Bool; shell_escape::Bool, cmd_escape::Bool, string_escape::Bool, dirsep::Char, kws::@Kwargs{…}) @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:1353
││││││┌ kwcall(::@NamedTuple{dirsep::Char, use_envpath::Bool}, ::typeof(REPL.REPLCompletions.complete_path), path::String) @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:426
│││││││┌  @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:468
││││││││┌ maybe_spawn_cache_PATH() @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:338
│││││││││ no matching method found `istaskdone(::Nothing)` (1/2 union split): REPL.REPLCompletions.istaskdone(REPL.REPLCompletions.PATH_cache_task)
││││││││└────────────────────
││││││││┌ maybe_spawn_cache_PATH() @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:338
│││││││││┌ (::REPL.REPLCompletions.var"#maybe_spawn_cache_PATH##0#maybe_spawn_cache_PATH##1")() @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:343
││││││││││ no matching method found `notify(::Nothing)` (1/2 union split): @_6 = REPL.REPLCompletions.notify(REPL.REPLCompletions.PATH_cache_condition)
│││││││││└────────────────────
││││││││┌ maybe_spawn_cache_PATH() @ REPL.REPLCompletions /Users/mhorn/Projekte/Julia/julia.release-1.12/usr/share/julia/stdlib/v1.12/REPL/src/REPLCompletions.jl:338
│││││││││ no matching method found `errormonitor(::Nothing)` (1/2 union split): (REPL.REPLCompletions.Base).errormonitor::typeof(errormonitor)(REPL.REPLCompletions.PATH_cache_task)
││││││││└────────────────────

Yet the code in question seems like it should require no union splitting at all, at least in two of the tree places, since either a concrete type is checked, or a !== nothing check is there. Alas I am not sure if this is an issue in Julia, in JET, both, neither... (CC @aviatesk)

function maybe_spawn_cache_PATH()
    global PATH_cache_task, next_cache_update
    @lock PATH_cache_lock begin
        PATH_cache_task isa Task && !istaskdone(PATH_cache_task) && return  # <- JET warning 1
        time() < next_cache_update && return
        PATH_cache_task = Threads.@spawn begin
            REPLCompletions.cache_PATH()
            @lock PATH_cache_lock begin
                next_cache_update = time() + 10 # earliest next update can run is 10s after
                PATH_cache_task = nothing # release memory when done
                PATH_cache_condition !== nothing && notify(PATH_cache_condition)  # <- JET warning 2
            end
        end
        Base.errormonitor(PATH_cache_task)  # <- JET warning 3
    end
end

aviatesk added a commit that referenced this pull request Jul 29, 2025
@aviatesk
iterator: improve type inference for Filter
3c99fd0 
This addresses type inference issues with filtered list comprehensions
reported in #59111. The root cause is the compiler's
inability to perform type refinement in cases like:

```julia let t::Tuple{Any,Any} x, y = t if x isa Int return t # still
`::Tuple{Any,Any}` end nothing end ```

Fixing this fundamentally would require extensive improvements to
abstract interpretation. As a workaround, I improved the iterators.jl
implementation to avoid this pattern.

One implementation consideration: if `iterate(f.itr, state...)` always
returns `Union{Nothing,Tuple{Any,Any}}`, the `if y isa Tuple{Any,Any}`
branch should be unnecessary. However, it's unclear whether we can
safely assume this for the iterate interface, so I kept the branch for
now.
@aviatesk aviatesk mentioned this pull request Jul 29, 2025
Merged
@aviatesk
Copy link
Member

I'm ok with it, but also does point to missing inference quality in guarded generators.

Will be fixed by #59142.

aviatesk added a commit that referenced this pull request Jul 29, 2025
@aviatesk
iterator: improve type inference for Filter
664a71d 
This addresses type inference issues with filtered list comprehensions
reported in #59111. The root cause is the compiler's
inability to perform type refinement in cases like:

```julia
let t::Tuple{Any,Any}
    x, y = t
    if x isa Int
        return t # still `::Tuple{Any,Any}`
    end
    nothing
end
```

Fixing this fundamentally would require extensive improvements to
abstract interpretation. As a workaround, I improved the iterators.jl
implementation to avoid this pattern.

One implementation consideration: if `iterate(f.itr, state...)` always
returns `Union{Nothing,Tuple{Any,Any}}`, the `if y isa Tuple{Any,Any}`
branch should be unnecessary. However, it's unclear whether we can
safely assume this for the iterate interface, so I kept the branch for
now.
@aviatesk
Copy link
Member

Yet the code in question seems like it should require no union splitting at all, at least in two of the tree places, since either a concrete type is checked, or a !== nothing check is there. Alas I am not sure if this is an issue in Julia, in JET, both, neither... (CC @aviatesk)

#59144

aviatesk added a commit that referenced this pull request Jul 29, 2025
@aviatesk
iterator: improve type inference for Filter
d06896d 
This addresses type inference issues with filtered list comprehensions
reported in #59111. The root cause is the compiler's
inability to perform type refinement in cases like:

```julia
let t::Tuple{Any,Any}
    x, y = t
    if x isa Int
        return t # still `::Tuple{Any,Any}`
    end
    nothing
end
```

Fixing this fundamentally would require extensive improvements to
abstract interpretation. As a workaround, I improved the iterators.jl
implementation to avoid this pattern.

One implementation consideration: if `iterate(f.itr, state...)` always
returns `Union{Nothing,Tuple{Any,Any}}`, the `if y isa Tuple{Any,Any}`
branch should be unnecessary. However, it's unclear whether we can
safely assume this for the iterate interface, so I kept the branch for
now.
@aviatesk
Copy link
Member

With these linked PRs merged, now I think this PR can be closed.

@lgoettgens
Copy link
Contributor

With these linked PRs merged, now I think this PR can be closed.

Since the issues brought up here were observed in julia 1.12, could we backport #59142 and #59144?

@aviatesk
Copy link
Member

Backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timholy timholy timholy approved these changes

Assignees
No one assigned
Labels
backport 1.12 Change should be backported to release-1.12
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fingolfin @Keno @aviatesk @lgoettgens @timholy