make splitdef use a mutable struct

[rfourquet]

Basically the same as FluxML/MacroTools.jl#116.
The motivation is not at all speed, but

1. def.name instead of def[:name] would be nicer (make splitdef return a NamedTuple FluxML/MacroTools.jl#116 (comment))
2. the keys order can be controlled:

 julia> splitdef(:(function f(x::T; y) where T end))
Dict{Symbol, Any} with 6 entries:
  :args        => Any[:(x::T)]
  :body        => quote…
  :kwargs      => Any[:y]
  :head        => :function
  :whereparams => Any[:T]
  :name        => :f

Here it would be nicer to have :head and :name first, then :args and :kwargs.

A mutable struct is nicer than a named tuple because named tuples don't print on multiple lines, and anyway it's nicer to be able to mutate the object.
For backward compatibility, such a SplitDef object could be made <: AbstractDict.
I volunteer to implement this if there is support.

[omus]

I can get behind this but there are a couple of things to call out:

• Key existence is used to check the presence of components. We'd need to use use nothing for this in a struct
• We may want to have a custom show to avoid showing fields that are set to nothing
• For backwards compatibility we can just have a method to convert to a dictionary and have a deprecation. Alternatively, we could just define getindex on the new struct

[rfourquet]

I had roughly the same ideas for your first two points, but it depends if you implied SplitDef <: AbstractDict. If so, then non-existant fields (nothing) would not be included in keys, and then the default dictionary show method might be enough?
If it's not made <: AbstractDict, definitely a custom show method is needed, and for backward compatibility, getindex and a couple of other method would be useful (e.g. haskey). Probably <: AbstractDict would be the least disruptive.

[rfourquet]

While working through this, I see a design choice to make that I would like to discuss before continuing. I made a struct like this:

Base.@kwdef mutable struct SplitDef <: AbstractDict{Symbol, Any}
    head::Symbol = :function
    name::Union{Symbol, Nothing} = nothing
    params::Union{Vector, Nothing} = nothing
    args::Union{Vector, Nothing} = nothing
    kwargs::Union{Vector, Nothing} = nothing
    rtype::Any = nothing
    whereparams::Union{Vector, Nothing} = nothing
    body::Any = nothing
end

where nothing means not set. But there can be ambiguity for body, where Expr(:function, :(f(x)), nothing)) has an explicit nothing as body. But currently, ExprTools considers that a nothing body represents a "generic function definition" (i.e. function f end). I see two solutions:

1. have the field body::Union{Some,Nothing} instead of ::Any, but this complicates the logic
2. have args be the field which discriminates between "generic function definition" (nothing) or not (Vector, possibly empty)

I currently much prefer 2). Setting an empty vector instead of nothing for f() = 1 might be somewhat breaking though. But there is some conceptual appeal to this, as a 0-arguments method has an argument list of size 0, as opposed to not possibly having arguments (like generic function definitions). Also, this would beg the question: should .kwargs be set too to [] instead of nothing in this case?

Do you see alternatives?

[omus]

Do you see alternatives?

Approaching this from the perspective of the user of splitdef we need a way to consistently determine whether the expression defined a given feature. If we end up going using Union{Some,Nothing} we'd need to apply this approach for all the fields. This would also true for option 2.

I would suggestion with the struct approach we extend hasproperty as this check (currently we use haskey). Using that as the interface as to whether a expression contains that feature provides us with an abstraction layer and then it doesn't matter which solution we choose. We could even use an additional field as part of the struct such as generic::Bool to solve this problem