Fix kwarg pass-through

[timholy]

No description provided.

[johnnychen94]

Looking at the git blame, nobody noticed this until we actually hit it 😄

[timholy]

We should use JET on some of our packages...I can add that to my TODO list.

[johnnychen94]

Do you mean @test_nodispatch?

[timholy]

No, more the report_file("runtests.jl"). For looking for inference problems, I think SnoopCompile is currently a better approach (it gets all the runtime dispatches, whereas JET does not).

As awesome as JET is, it still sometimes throws its own errors (it seems to on this package's tests). So those might need fixing too.

[timholy]

Also see timholy/SnoopCompile.jl#253

[timholy]

That one worker seems stuck, I'll just merge and release a new version.

[johnnychen94]

Just to make sure if I understand it right, so your idea is to put static method analysis as a part of the test? Something like

JET_reports = report_package(ImageIO)
@test JET_reports.nreported == 0

? This would definitely improve the code quality.

[timholy]

No doubt about it. However, for a thorough analysis, that JET/SnoopCompile integration is going to be handy. Currently:

julia> using JET

shell> cd test
/home/tim/.julia/dev/ImageIO/test

julia> report_file("runtests.jl")
[toplevel-info] virtualized the context of Main (took 0.018 sec)
[toplevel-info] entered into runtests.jl
[toplevel-info]  exited from runtests.jl (took 13.994 sec)
═════ 1 toplevel error found ═════
┌ @ runtests.jl:8 
│ UndefVarError: ret not defined
└─────────────────

I couldn't even figure out where ret is, but I'm guessing it's in Test. And this was on the previous master branch of this package, and JET missed the kwargs problem in this PR. That's basically because it was hidden behind an inference failure.

But if you start a fresh session, use that SnoopCompile branch, and do this:

julia> using JET, SnoopCompile

shell> cd test
/home/tim/.julia/dev/ImageIO/test

julia> tinf = @snoopi_deep include("runtests.jl")
[ Info: Threads.nthreads() = 1, multithread tests will be disabled
Test Summary: | Pass  Total
ImageIO       |   64     64
InferenceTimingNode: 7.032328/12.149364 on Core.Compiler.Timings.ROOT() with 358 direct children

julia> itrigs = inference_triggers(tinf);

julia> for itrig in itrigs
           display(report_callee(itrig))
       end
No errors !

═════ 1 possible error found ═════
┌ @ /home/tim/src/julia-master/usr/share/julia/stdlib/v1.8/Logging/src/ConsoleLogger.jl:76 Base.contractuser(file)
│┌ @ path.jl:500 Base.Filesystem.==(path, home)
││┌ @ strings/basic.jl:324 Base.cmp(a, b)
│││┌ @ strings/basic.jl:302 Base.iterate(Base.zip(a, b))
││││┌ @ iterators.jl:340 Base.Iterators._zip_iterate_all(Base.getproperty(z, :is), Base.map(#5, Base.getproperty(z, :is)))
│││││┌ @ iterators.jl:348 Base.Iterators._zip_isdone(is, ss)
││││││┌ @ iterators.jl:380 Base.Iterators.tail(ss)
│││││││┌ @ iterators.jl:380 Base.Iterators._zip_isdone(Base.Iterators.tail(is), Base.Iterators.tail(ss))
││││││││┌ @ iterators.jl:379 Base.getindex(is, 1)
│││││││││┌ @ tuple.jl:29 Base.getfield(t, i, $(Expr(:boundscheck)))
││││││││││ invalid builtin function call: Base.getfield(t::Tuple{}, i::Int64, $(Expr(:boundscheck)))
│││││││││└───────────────

No errors !

No errors !

═════ 1 possible error found ═════
┌ @ strings/io.jl:142 Base.print(s, x)
│┌ @ strings/io.jl:35 Base.show(io, x)
││┌ @ show.jl:881 Base._show_type(io, Base.inferencebarrier(x))
│││┌ @ show.jl:886 Base.show_typealias(io, x)
││││┌ @ show.jl:723 Base.make_typealias(properx)
│││││┌ @ show.jl:533 Base.modulesof!(Core.apply_type(Base.Set, Base.Module)(), x)
││││││┌ @ show.jl:504 Base.push!(s, Base.getproperty(Base.getproperty(x, :name), :module))
│││││││┌ @ set.jl:59 Base.setindex!(Base.getproperty(s, :dict), Base.nothing, x)
││││││││┌ @ dict.jl:382 Base.ht_keyindex2!(h, key)
│││││││││┌ @ dict.jl:349 Base.rehash!(h, _13)
││││││││││┌ @ error.jl:229 Base.getproperty(Main.Base, :string)(msg)
│││││││││││┌ @ strings/io.jl:183 Base.print_to_string(xs...)
││││││││││││┌ @ strings/io.jl:142 Base.print(s, x)
│││││││││││││┌ @ show.jl:1283 Base.show_unquoted(Base.IOContext(io, Base.=>(:unquote_fallback, false)), ex, 0, -1)
││││││││││││││┌ @ show.jl:1758 #self#(io, ex, indent, prec, 0)
│││││││││││││││┌ @ show.jl:2058 Base.mapany(Base.allow_macroname, args)
││││││││││││││││┌ @ abstractarray.jl:2855 Base.map!(f, Core.apply_type(Base.Vector, Base.Any)(Base.undef, Base.length(A)), A)
│││││││││││││││││┌ @ abstractarray.jl:2845 Base.iterate(Base.zip(Base.eachindex(dest), Base.eachindex(A)))
││││││││││││││││││┌ @ iterators.jl:340 Base.Iterators._zip_iterate_all(Base.getproperty(z, :is), Base.map(#5, Base.getproperty(z, :is)))
│││││││││││││││││││┌ @ iterators.jl:348 Base.Iterators._zip_isdone(is, ss)
││││││││││││││││││││┌ @ iterators.jl:380 Base.Iterators._zip_isdone(Base.Iterators.tail(is), Base.Iterators.tail(ss))
│││││││││││││││││││││┌ @ iterators.jl:380 Base.Iterators._zip_isdone(Base.Iterators.tail(is), Base.Iterators.tail(ss))
││││││││││││││││││││││┌ @ iterators.jl:379 Base.getindex(is, 1)
│││││││││││││││││││││││┌ @ tuple.jl:29 Base.getfield(t, i, $(Expr(:boundscheck)))
││││││││││││││││││││││││ invalid builtin function call: Base.getfield(t::Tuple{}, i::Int64, $(Expr(:boundscheck)))
│││││││││││││││││││││││└───────────────

No errors !

═════ 1 possible error found ═════
┌ @ cartesian.jl:44 Base.Cartesian._nloops(Core.tuple(N, itersym, Core._expr(:->, d, Core._expr(:block, $(QuoteNode(:(#= cartesian.jl:44 =#))), Core._expr(:call, $(Expr(:copyast, :($(QuoteNode(:(Base.axes)))))), arraysym, d)))), args...)
│┌ @ cartesian.jl:52 Base.string("number of arguments must be 1 ≤ length(args) ≤ 3, got ", Base.Cartesian.nargs)
││ variable Base.Cartesian.nargs is not defined: Base.string("number of arguments must be 1 ≤ length(args) ≤ 3, got ", Base.Cartesian.nargs)
│└───────────────────

... (goes on for a long time!)

you get a much more comprehensive report.

CC @aviatesk.