Add error case for unnamed arguments

[phipsgabler]

Model arguments without names (f(x, ::T)) don't really make sense -- we can't put them in the args named tuple.

What currently happens is a bit weird, since we produce a syntactically invalid macro output:

julia> @model function m1(x::T) where T 1 end
m1 (generic function with 1 method)

julia> @model function m1(::T) where T 1 end
ERROR: syntax: invalid "::" syntax around REPL[71]:1
Stacktrace:
 [1] top-level scope
   @ REPL[71]:1

julia> @macroexpand @model function m1(::T) where T 1 end
quote
    $(Expr(:meta, :doc))
    function m1(::T; ) where T
        #= REPL[72]:1 =#
        var"##evaluator#285" = ((__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext, ::T) where T->begin
                    begin
                        #= REPL[72]:1 =#
                        #= REPL[72]:1 =#
                        1
                    end
                end)
        return (Model)(:m1, var"##evaluator#285", NamedTuple{($(QuoteNode(:(::T))),)}((::T,)), NamedTuple())
    end
end

With this PR, it is caught during build_model_info:

julia> @macroexpand @model function m1(::T) where T 1 end
ERROR: LoadError: ArgumentError: unnamed arguments are not supported
Stacktrace:
  [1] macro expansion
    @ ~/git/DynamicPPL.jl/src/compiler.jl:44 [inlined]
  [2] macro expansion
    @ ~/.julia/packages/MacroTools/BunBL/src/match/macro.jl:18 [inlined]
  [3] (::DynamicPPL.var"#157#159")(arg::Expr)
    @ DynamicPPL ~/git/DynamicPPL.jl/src/compiler.jl:230
  [4] iterate
    @ ./generator.jl:47 [inlined]
  [5] _collect(c::Vector{Expr}, itr::Base.Generator{Vector{Expr}, DynamicPPL.var"#157#159"}, #unused#::Base.EltypeUnknown, isz::Base.HasShape{1})
    @ Base ./array.jl:691
  [6] collect_similar(cont::Vector{Expr}, itr::Base.Generator{Vector{Expr}, DynamicPPL.var"#157#159"})
    @ Base ./array.jl:606
  [7] map(f::Function, A::Vector{Expr})
    @ Base ./abstractarray.jl:2294
  [8] build_model_info(input_expr::Expr)
    @ DynamicPPL ~/git/DynamicPPL.jl/src/compiler.jl:271
  [9] model(mod::Module, linenumbernode::LineNumberNode, expr::Expr, warn::Bool)
    @ DynamicPPL ~/git/DynamicPPL.jl/src/compiler.jl:217
 [10] var"@model"(__source__::LineNumberNode, __module__::Module, expr::Any, warn::Any) (repeats 2 times)
    @ DynamicPPL ~/git/DynamicPPL.jl/src/compiler.jl:213
 [11] #macroexpand#50
    @ ./expr.jl:112 [inlined]
 [12] top-level scope
    @ REPL[102]:1
in expression starting at REPL[102]:1

(The alternative would be to generate some symbol, but I don't see how that is useful.)

[devmotion]

This should not error, I think? We might want to use them as

@model demo(::T=Float64) where T
    x = Vector{T}(undef, 10)
    x .~ Normal()
    return x
end

Here T is ensured to be of the correct type depending on the sampler and AD backend that is used - e.g., HMC + ForwardDiff will give you a dual number etc.

[devmotion]

I thought we always supported this but maybe we dropped it accidentally at some point 🤔

[phipsgabler]

OK, interesting. But how would those arguments be stored in the model?

Perhaps it was caused by to_namedtuple_expr having changed?
https://github.com/TuringLang/DynamicPPL.jl/blame/b8e448c6be4057afce558c572c817c443c4c7795/src/compiler.jl#L264

[devmotion]

Ah wait, I take this back 😄 One always had to use ::Type{T} which is supported (regardless of if it is provided with or without name).

[devmotion]

The example would still make sense (I think) but it is not supported and type won't be adjusted (since then we would have to instantiate values at some point...).

[phipsgabler]

Right, and it can't currently reasonably be supported, since we'd have to make up a name for the argument. The PR doesn't change that behaviour, it just improves the error.

Supporting that kind of argument can IMHO reasonably be done by gensyming something and perhaps emit a warning, but that's a separate issue.

[phipsgabler]

@yebai's opionon is that we might just generate names. Now, the normal lowering uses the pattern #unused#@_i for unused slotnames:

julia> bar(::Int, ::Bool) = nothing
bar (generic function with 1 method)

julia> @code_warntype bar(1, true)
Variables
  #self#::Core.Const(bar)
  #unused#@_2::Int64
  #unused#@_3::Bool

Body::Nothing
1 ─     return Main.nothing

I think if we do just that and rename all argument names, we should get everything for free, right? I'm basing this on two assumptions:

1. The argument names stored in the model are only there for internal purposes, and not exposed to the user (they could, in pretty-printing, but we can special-case this)
2. It does not make sense to "condition on a type parameter" anyway, so we don't need to treat the Type{TV} case specially and convert it to T behaving like a name.

(And if you want to see some #unused#-related weirdness, have a look here :D)

[phipsgabler]

Thinking once more about it -- this, too, is one of the cases that are automatically resolved if the prob macro is gotten rid of in favour of condition, and args are no more.

[yebai]

@phipsgabler Can you clarify why the unnamed argument issue will disappear if we remove the prob macro?

[phipsgabler]

@yebai The only reason args is stored in the model is the ability to reconstruct a model instance with only some of the arguments replaced. This is what the prob-macro needs to evaluate with a different conditioning, and we should now be able to do explicitely with condition.

And the origin for the above bug, or rather the special treatment of ::Type{T}, is that the args are a named tuple. And an unnamed argument has not name associated, so the current hack is to behave like it were written T::Type and store T = ... in args. Without that requirement, no special treatment of arguments is needed at all (simiarly for #325).

[yebai]

@devmotion @phipsgabler I think #394 fixed the issue here. Please reopen if otherwise.

[devmotion]

Ha I didn't remember this PR 😅