FluxML / MacroTools.jl

MacroTools provides a library of tools for working with Julia code and expressions.
https://fluxml.ai/MacroTools.jl/stable/
Other
308 stars 77 forks source link

improve code quality and type instabilities #162

Closed aviatesk closed 2 years ago

aviatesk commented 2 years ago

I used JET and fixed some actual errors and type instabilities. (this PR is the final output of my demo at our workshop)

/cc @pfitzseb Could you review this PR, please ? :)

DhairyaLGandhi commented 2 years ago

Looks good, thanks! Could you also link to the errors in this PR so it's easier to refer back?

aviatesk commented 2 years ago

All are caught by static analysis:

julia> using JET

julia> report_package("MacroTools")
[toplevel-info] virtualized the context of Main (took 0.023 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/MacroTools.jl
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/match/match.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/match/match.jl (took 0.231 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/match/types.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/match/types.jl (took 0.292 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/match/union.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/match/union.jl (took 0.008 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/match/macro.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/match/macro.jl (took 0.076 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/utils.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/utils.jl (took 7.825 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/structdef.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/structdef.jl (took 0.159 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/examples/destruct.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/examples/destruct.jl (took 0.038 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/examples/threading.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/examples/threading.jl (took 0.112 sec)
[toplevel-info] entered into /Users/aviatesk/julia/packages/MacroTools/src/examples/forward.jl
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/examples/forward.jl (took 0.062 sec)
[toplevel-info]  exited from /Users/aviatesk/julia/packages/MacroTools/src/MacroTools.jl (took 11.243 sec)
[toplevel-info] analyzing from top-level definitions ... 156/156
═════ 11 possible errors found ═════
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/match/match.jl:27 Base.getproperty(Base.getproperty(MacroTools.Base, :match)(r"^@?(.*?)_+(_str)?$", MacroTools.string(s)), :captures)
│┌ @ Base.jl:42 Base.getfield(x, f)
││ type Nothing has no field captures
│└──────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/match/types.jl:20 MacroTools.map(MacroTools.totype, ts)
│┌ @ abstractarray.jl:2853 Base.collect_similar(A, Base.Generator(f, A))
││┌ @ array.jl:698 Base._collect(cont, itr, Base.IteratorEltype(itr), Base.IteratorSize(itr))
│││┌ @ array.jl:788 Base.collect_to_with_first!(Base._similar_for(c, Base.typeof(v1), itr, isz), v1, itr, st)
││││┌ @ array.jl:793 Base.setindex!(dest, v1, i1)
│││││┌ @ array.jl:937 Base.convert(_, x)
││││││ no matching method found for call signature (Tuple{typeof(convert), Type{Symbol}, Expr}): Base.convert(_::Type{Symbol}, x::Expr)
│││││└────────────────
││││┌ @ array.jl:793 Base.setindex!(dest, v1, i1)
│││││┌ @ array.jl:937 Base.convert(_, x)
││││││ no matching method found for call signature (Tuple{typeof(convert), Type{Expr}, Symbol}): Base.convert(_::Type{Expr}, x::Symbol)
│││││└────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/utils.jl:21 Base.collect(Base.Generator(#11, MacroTools.map(MacroTools.esc, xs)))
│┌ @ array.jl:775 Base.collect_to_with_first!(Base._array_for(Base.typeof(v1), Base.getproperty(itr, :iter), isz), v1, itr, st)
││┌ @ array.jl:799 Base.grow_to!(dest, itr, st)
│││┌ @ dict.jl:153 Base.indexed_iterate(Core.getfield(Base.indexed_iterate(y, 1), 1), 1)
││││┌ @ tuple.jl:91 Base.iterate(I)
│││││ no matching method found for call signature (Tuple{typeof(iterate), Expr}): Base.iterate(I::Expr)
││││└───────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/utils.jl:423 MacroTools.rebuilddef(MacroTools.striplines(dict))
│ variable MacroTools.rebuilddef is not defined: MacroTools.rebuilddef(MacroTools.striplines(dict::Dict)::Dict)
└──────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/utils.jl:455 Core.tuple(splitvar(arg), Core.tuple(is_splat, default)...)
│ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Core.tuple(splitvar::MacroTools.var"#splitvar#35"(arg::Any)::Union{Nothing, Tuple{Any, Any}}, Core.tuple(is_splat::Bool, default::Any)::Tuple{Bool, Any}...)
└──────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/utils.jl:457 Core.tuple(splitvar(arg_expr2), Core.tuple(is_splat, MacroTools.nothing)...)
│ no matching method found for call signature (Tuple{typeof(iterate), Nothing}): Core.tuple(splitvar::MacroTools.var"#splitvar#35"(arg_expr2::Any)::Union{Nothing, Tuple{Any, Any}}, Core.tuple(is_splat::Bool, MacroTools.nothing)::Tuple{Bool, Nothing}...)
└──────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/structdef.jl:13 MacroTools.parse_error(ex)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└─────────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/structdef.jl:21 MacroTools.parse_error(ex)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└─────────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/structdef.jl:29 MacroTools.parse_error(ex)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└─────────────────────────────────────────────────────────────────
┌ @ /Users/aviatesk/julia/packages/MacroTools/src/examples/destruct.jl:24 MacroTools.error("Can't destructure fields with default values")
│┌ @ error.jl:33 error(::String)
││ may throw: Base.throw(Base.ErrorException(s::String)::ErrorException)
│└───────────────

This PR will remove all the errors reported above except some false positives due to limitations of JET.

DhairyaLGandhi commented 2 years ago

Yup, I'm good with it too. Do we need to worry about uses of @splitcombine in the wild?

aviatesk commented 2 years ago

The previous @splitcombine was simply not functional, so I guess there is no one using it now ?

aviatesk commented 2 years ago

@DhairyaLGandhi bump ?

aviatesk commented 2 years ago

Thanks @cstjean , could you also tag a new version ?

cstjean commented 2 years ago

Does it fix anyone's concrete problem?

I don't mind tagging if you make a PR bumping the patch version...