Replace MacroTools with ExprTools

[onetonfoot]

This pull replaces MacroTools with ExprTools and adds some test for build_model_info.
The generated expression for typed arguments are still of the form

:(T::Type{<:Float64})

Since the generic expression

:(T::Type{T})

Wouldn't be valid Julia, because T is used twice.

In a later pull I could address this and support for expressions of the form t::Type{T}. I think the compiler currently only works with ::Type{T}.

[devmotion]

The generated expression for typed arguments are still of the form
:(T::Type{<:Float64})
Since the generic expression
:(T::Type{T})
Wouldn't be valid Julia, because T is used twice.

IMO ideally we do not change the function signature provided by the user at all, but just add our internal variables to it (see also #109). We just have to extract the names of the argument (positional and keyword arguments) and their default values. In the case of ::Type{T} = Vector{Float64} the name would be T and the default value would be Vector{Float64}. If a user uses t::Type{T} = Vector{Float64} instead and makes use of t inside of the model definition, that would not matter - we would not touch the function signature (hence t is still available) but we would extract T = Vector{Float64} as default (which is still correct).

[devmotion]

What's the motivation for IsEqual? To me it seems it would be much easier (also to read) if we just check fn(y) if needed. In functions such as all or any you could even just provide fn as first argument directly. In calls such as expr == Expr(:(::), IsEqual(issymbol), IsEqual(x->true)) you rely on the fact that the internal implementation of (==)(::Expr, ::Expr) will check == recursively, which is not guaranteed in general. IMO it would be safer and less prone to surprises if one explicitly checks the parts of an expression one is interested in.

Suggested change

	"""
	IsEqual(fn)
	Takes a funciton of the form `fn(x)::Bool`
	"""
	struct IsEqual
	fn::Function
	end
	(==)(x::IsEqual, y) = x.fn(y)
	(==)(y , x::IsEqual) = x == y

[onetonfoot]

I found it to be more readable since the shape of the Expr your checking is clearly declared.

For example

function get_type(expr)
    if expr == Expr(:(::), Expr(:curly, :Type, IsEqual(issymbol)))
        expr.args[1].args[2]
    elseif expr == Expr(:(::), IsEqual(issymbol) , Expr(:curly, :Type, IsEqual(issymbol)))
        expr.args[2].args[2]
    else
        nothing
    end
end

Vs

function get_type(expr)
    type_expr = if Meta.isexpr(expr, :(::), 2)
        expr.args[1]
    elseif Meta.isexpr(expr, :(::), 1)
        expr.args[2]
    else
        nothing
    end
    isnothing(type_expr) ? nothing : type_expr[2]
end

But yeah it could be refactored into a functions instead if that's the preferred style.

I played with this idea a bit more and the recursion seems to work ok.

[devmotion]

I don't have a problem with the style per se (although I'm not sure if it will make it easier for people to understand DynamicPPL's source code). The main problem I see is just that the expression matching happens implicitly, by relying on how ==(::Expr, ::Expr) is implemented which we can't control.

[onetonfoot]

Yeah it assumes your familiar with the S-Expr form which is probably the case if you're writing the functions but may not be so if your reading the source or just calling said functions.

I'd think ==(::Expr, ::Expr) would remain stable since it's quite a fundamental to the language but you're right in that it's not under our control which could be a problem long term if it changes across Julia versions.

I'll update the pull to remove the use of IsEqual.

[phipsgabler]

Maybe it's a good idea if we write a couple of generic helper function s in the style of splitdef. Something like f, (arg1, arg2) = splitcall(expr), t, T = splittyping(expr), etc., in a form that allow tuple unpacking and mark absence with nothing.

[onetonfoot]

Yeah I agree, I did initially try this by replacing the offending line with Expr(:kw, Texpr, Tval) but I kept getting this error.

LoadError: syntax: "Type{TV}" is not a valid function argument name

So I suspect somewhere in the code generation a function is assuming the Expr is of the form
t::Type{T}, and hence an Expr of the form ::Type{T} fails.

It's difficult for me to locate the offending function since, I'm not familiar with the code generation yet.

[devmotion]

IMO ideally we do not change the function signature provided by the user at all, but just add our internal variables to it (see also #109). We just have to extract the names of the argument (positional and keyword arguments) and their default values.

I guess that this would also improve readability of the code since we just have to implement some methods that can extract the names and values of the arguments, and then just map them over the positional and keyword arguments. There would be no need to deal with where clauses or anything else in the function signature.

[phipsgabler]

get_symbol is a too ambiguous name, IMO. What about get_argument_name?

[phipsgabler]

Similarly here: I propose get_argument_type for consistency.

[onetonfoot]

How about getarg_type and getarg_symbol? Since is closer to getargs_dottile.

[phipsgabler]

With what I proposed above, you could actually subsume it under splittyping, with a helper splitcurly:

t, T = splittypeing(:(t::Type{T}))
@assert T !== nothing
_Type, Ts = splitcurly(T)
@assert Ts == (:T,)

Although this might get out of hand quickly -- now that I write it, I start to reimplement pattern matching in my head...

[phipsgabler]

If you remove IsEqual, I'd also get rid of this. If it stays, let's at least use it consistenly across the file instead of isa Symbol.

[phipsgabler]

Maybe it's a good idea if we write a couple of generic helper function s in the style of splitdef. Something like f, (arg1, arg2) = splitcall(expr), t, T = splittyping(expr), etc., in a form that allow tuple unpacking and mark absence with nothing.

[yebai]

This PR is mostly superseded by changes in #134 and the reduced dependency footprint of MacroTools.