code quality improvements from JET analysis

[aviatesk]

I ran JET analysis on this package, and fixed some true positive errors.

run JET on MacroTools

julia> using JET
julia> report_file("src/MacroTools.jl"; analyze_from_definitions = true, annotate_types = true)

Before this PR

═════ 11 possible errors found ═════
┌ @ src/match/match.jl:27 Base.getproperty(Base.getproperty(MacroTools.Base, :match::Symbol)::typeof(match)(r"^@?(.*?)_+(_str)?$", MacroTools.string(s::Symbol)::String)::Union{Nothing, RegexMatch}, :captures::Symbol)
│┌ @ Base.jl:33 Base.getfield(x::Nothing, f::Symbol)
││ type Nothing has no field captures
│└──────────────
┌ @ src/match/types.jl:20 MacroTools.map(MacroTools.totype, ts::Vector{Symbol})
│┌ @ abstractarray.jl:2301 Base.collect_similar(A::Vector{Symbol}, Base.Generator(f::typeof(MacroTools.totype), A::Vector{Symbol})::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})
││┌ @ array.jl:620 Base._collect(cont::Vector{Symbol}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, Base.IteratorEltype(itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})::Base.EltypeUnknown, Base.IteratorSize(itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})::Base.HasShape{1})
│││┌ @ array.jl:710 Base.collect_to_with_first!(Base._similar_for(c::Vector{Symbol}, Base.typeof(v1::Union{Expr, Symbol})::Union{Type{Expr}, Type{Symbol}}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, isz::Base.HasShape{1})::Union{Vector{Expr}, Vector{Symbol}}, v1::Union{Expr, Symbol}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, st::Int64)
││││┌ @ array.jl:715 Base.setindex!(dest::Vector{Symbol}, v1::Expr, i1::Int64)
│││││┌ @ array.jl:853 Base.convert(_::Type{Symbol}, x::Expr)
││││││ no matching method found for call signature: Base.convert(_::Type{Symbol}, x::Expr)
│││││└────────────────
││││┌ @ array.jl:715 Base.setindex!(dest::Vector{Expr}, v1::Symbol, i1::Int64)
│││││┌ @ array.jl:853 Base.convert(_::Type{Expr}, x::Symbol)
││││││ no matching method found for call signature: Base.convert(_::Type{Expr}, x::Symbol)
│││││└────────────────
┌ @ src/utils.jl:21 Base.collect(Base.Generator(#11::MacroTools.var"#11#12", MacroTools.map(MacroTools.esc, xs::Tuple)::Any)::Base.Generator{_A, MacroTools.var"#11#12"} where _A)
│┌ @ array.jl:697 Base.collect_to_with_first!(Base._array_for(Base.typeof(v1::Expr)::Type{Expr}, Base.getproperty(itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, :iter::Symbol)::Any, isz::Any)::Any, v1::Expr, itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, st::Any)
││┌ @ array.jl:721 Base.grow_to!(dest::Any, itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, st::Any)
│││┌ @ dict.jl:153 Base.indexed_iterate(Core.getfield(Base.indexed_iterate(y::Tuple{Expr, Any}, 1)::Tuple{Expr, Int64}, 1)::Expr, 1)
││││┌ @ tuple.jl:89 Base.iterate(I::Expr)
│││││ no matching method found for call signature: Base.iterate(I::Expr)
││││└───────────────
┌ @ src/utils.jl:423 MacroTools.rebuilddef(MacroTools.striplines(dict::Dict)::Dict)
│ variable MacroTools.rebuilddef is not defined: MacroTools.rebuilddef(MacroTools.striplines(dict::Dict)::Dict)
└────────────────────
┌ @ src/utils.jl:455 Core.tuple(splitvar::MacroTools.var"#splitvar#35"(arg::Any)::Union{Nothing, Tuple{Any, Any}}, Core.tuple(is_splat::Bool, default::Any)::Tuple{Bool, Any}...)
│ no matching method found for call signature: Core.tuple(splitvar::MacroTools.var"#splitvar#35"(arg::Any)::Union{Nothing, Tuple{Any, Any}}, Core.tuple(is_splat::Bool, default::Any)::Tuple{Bool, Any}...)
└────────────────────
┌ @ src/utils.jl:457 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}...)
│ no matching method found for call signature: 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}...)
└────────────────────
┌ @ src/structdef.jl:13 MacroTools.parse_error(ex::Any)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└───────────────────────
┌ @ src/structdef.jl:21 MacroTools.parse_error(ex::Any)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└───────────────────────
┌ @ src/structdef.jl:29 MacroTools.parse_error(ex::Any)
│ variable MacroTools.parse_error is not defined: MacroTools.parse_error(ex::Any)
└───────────────────────
┌ @ src/examples/destruct.jl:24 MacroTools.error("Can't destructure fields with default values")
│┌ @ error.jl:33 error(::String)
││ may throw: Base.throw($(Expr(:invoke, MethodInstance for ErrorException(::String), :(Base.ErrorException), Core.Argument(2)))::ErrorException)
│└───────────────

After this PR: only the false positives ramain (Julia's inference or JET itself should be improved)

═════ 4 possible errors found ═════
┌ @ src/match/types.jl:20 MacroTools.map(MacroTools.totype, ts::Vector{Symbol})
│┌ @ abstractarray.jl:2301 Base.collect_similar(A::Vector{Symbol}, Base.Generator(f::typeof(MacroTools.totype), A::Vector{Symbol})::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})
││┌ @ array.jl:620 Base._collect(cont::Vector{Symbol}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, Base.IteratorEltype(itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})::Base.EltypeUnknown, Base.IteratorSize(itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)})::Base.HasShape{1})
│││┌ @ array.jl:710 Base.collect_to_with_first!(Base._similar_for(c::Vector{Symbol}, Base.typeof(v1::Union{Expr, Symbol})::Union{Type{Expr}, Type{Symbol}}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, isz::Base.HasShape{1})::Union{Vector{Expr}, Vector{Symbol}}, v1::Union{Expr, Symbol}, itr::Base.Generator{Vector{Symbol}, typeof(MacroTools.totype)}, st::Int64)
││││┌ @ array.jl:715 Base.setindex!(dest::Vector{Symbol}, v1::Expr, i1::Int64)
│││││┌ @ array.jl:853 Base.convert(_::Type{Symbol}, x::Expr)
││││││ no matching method found for call signature: Base.convert(_::Type{Symbol}, x::Expr)
│││││└────────────────
││││┌ @ array.jl:715 Base.setindex!(dest::Vector{Expr}, v1::Symbol, i1::Int64)
│││││┌ @ array.jl:853 Base.convert(_::Type{Expr}, x::Symbol)
││││││ no matching method found for call signature: Base.convert(_::Type{Expr}, x::Symbol)
│││││└────────────────
┌ @ src/utils.jl:21 Base.collect(Base.Generator(#11::MacroTools.var"#11#12", MacroTools.map(MacroTools.esc, xs::Tuple)::Any)::Base.Generator{_A, MacroTools.var"#11#12"} where _A)
│┌ @ array.jl:697 Base.collect_to_with_first!(Base._array_for(Base.typeof(v1::Expr)::Type{Expr}, Base.getproperty(itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, :iter::Symbol)::Any, isz::Any)::Any, v1::Expr, itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, st::Any)
││┌ @ array.jl:721 Base.grow_to!(dest::Any, itr::Base.Generator{_A, MacroTools.var"#11#12"} where _A, st::Any)
│││┌ @ dict.jl:153 Base.indexed_iterate(Core.getfield(Base.indexed_iterate(y::Tuple{Expr, Any}, 1)::Tuple{Expr, Int64}, 1)::Expr, 1)
││││┌ @ tuple.jl:89 Base.iterate(I::Expr)
│││││ no matching method found for call signature: Base.iterate(I::Expr)
││││└───────────────
┌ @ src/examples/destruct.jl:24 MacroTools.error("Can't destructure fields with default values")
│┌ @ error.jl:33 error(::String)
││ may throw: Base.throw($(Expr(:invoke, MethodInstance for ErrorException(::String), :(Base.ErrorException), Core.Argument(2)))::ErrorException)
│└───────────────

[goretkin]

This change just replaces one runtime error by another, right?

julia> nothing.captures
ERROR: type Nothing has no field captures

julia> (nothing::AbstractMatch).captures
ERROR: TypeError: in typeassert, expected AbstractMatch, got a value of type Nothing

What's the point?

[aviatesk]

AFAIU that runtime error won't happen (the regex always matches bname's argument), thus this annotation.

[aviatesk]

@cstjean can you have a quick review on this ? This doesn't change any fuctionality.

[aviatesk]

The macro with the same name is defined in test file, and this
could be removed.
This definition is also timed, especially rebuilddef is no longer defined.

[aviatesk]

bump, @MikeInnes @cstjean ?

[cstjean]

I approve the splitcombine removal, but don't really know these other functions well enough to review. I'd be curious to know what's the improvement behind each of these changes though...

[aviatesk]

I'd be curious to know what's the improvement behind each of these changes though...

They're simply bugfixes, or attempts to improve runtime performance a bit or avoid invalidation risks by kindly telling some invariants to the compiler.
The latter won't change any current behavior.

[cstjean]

They're simply bugfixes

Can you provide a MWE triggering the bug and/or write a test for it?

[aviatesk]

Can you provide a MWE triggering the bug

MacroTools.splitstructdef(:(non_struct_ex(arg))) maybe ?

[aviatesk]

Tests are added, should be good to go.

[aviatesk]

I'm going to use this example at our JuliaCon workshop and want to preserve these changes for that.

[cstjean]

Should we reopen and merge this? I know it's been forever, but MacroTools hasn't changed much in the last 3 years, I assume we could probably merge it as is.