Turning around escape in model macro

[phipsgabler]

Instead of escaping the whole generated code and gensyming internal names ourselves, escape only what is necessary and hygiene do its work.

Implements #308. Just a first try, I'm not even sure yet what to keep.

[phipsgabler]

Can anyone help me think? I don't know how I broke macro expansion:

julia> # Can't do that!  Why?
julia> @macroexpand @model function demo4(n, ::Type{TV} = Vector{Float64}) where TV
                 m ~ Normal()
                 x = TV(undef, n)
            @show __varinfo__
           
             for i = eachindex(x)
           
                 x[i] ~ Normal(m, 1.0)
             end
         end
:($(Expr(:error, "malformed expression")))

julia> # Actually _calling_ the macro does work...
julia> var"@model"(LineNumberNode(1, nothing), Main, :(function demo4(n, ::Type{TV} = Vector{Float64}) where TV
                 m ~ Normal()
                 x = TV(undef, n)
            @show __varinfo__
           
             for i = eachindex(x)
           
                 x[i] ~ Normal(m, 1.0)
             end
         end))
:(#= /home/philipp/git/DynamicPPL.jl/src/compiler.jl:517 =# (Base).@__doc__ function demo4(n, ::Type{TV} = Vector{Float64}; ) where TV
          #= line 1 =#
          evaluator = ((__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext, n, ::Type{TV}) where TV->begin
                      begin
                          #= REPL[127]:1 =#
                          #= REPL[127]:2 =#
                          begin
                              vn = (VarName){:m}()
                              inds = ()
                              isassumption = begin
                                      let vn = (VarName){:m}()
                                          if (DynamicPPL.contextual_isassumption)(__context__, vn)
                                              if !((DynamicPPL.inargnames)(vn, __model__)) || (DynamicPPL.inmissings)(vn, __model__)
                                                  true
                                              else
                                                  m === missing
                                              end
                                          else
                                              false
                                          end
                                      end
                                  end
                              if isassumption
                                  m = (DynamicPPL.tilde_assume!)(__context__, (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(Normal()), vn)..., inds, __varinfo__)
                              else
                                  if !((DynamicPPL.inargnames)(vn, __model__))
                                      m = (DynamicPPL.getvalue_nested)(__context__, vn)
                                  end
                                  (DynamicPPL.tilde_observe!)(__context__, (DynamicPPL.check_tilde_rhs)(Normal()), m, vn, inds, __varinfo__)
                              end
                          end
                          #= REPL[127]:3 =#
                          x = TV(undef, n)
                          #= REPL[127]:4 =#
                          begin
                              Base.println("__varinfo__ = ", Base.repr(begin
                                          #= show.jl:955 =#
                                          local var"#310#value" = __varinfo__
                                      end))
                              var"#310#value"
                          end
                          #= REPL[127]:6 =#
                          for i = eachindex(x)
                              vn = (VarName){:x}(((i,),))
                              inds = ((i,),)
                              isassumption = begin
                                      let vn = (VarName){:x}(((i,),))
                                          if (DynamicPPL.contextual_isassumption)(__context__, vn)
                                              if !((DynamicPPL.inargnames)(vn, __model__)) || (DynamicPPL.inmissings)(vn, __model__)
                                                  true
                                              else
                                                  #= /home/philipp/git/DynamicPPL.jl/src/compiler.jl:88 =# @views(x[i]) === missing
                                              end
                                          else
                                              false
                                          end
                                      end
                                  end
                              if isassumption
                                  x[i] = (DynamicPPL.tilde_assume!)(__context__, (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(Normal(m, 1.0)), vn)..., inds, __varinfo__)
                              else
                                  if !((DynamicPPL.inargnames)(vn, __model__))
                                      x[i] = (DynamicPPL.getvalue_nested)(__context__, vn)
                                  end
                                  (DynamicPPL.tilde_observe!)(__context__, (DynamicPPL.check_tilde_rhs)(Normal(m, 1.0)), #= /home/philipp/git/DynamicPPL.jl/src/compiler.jl:88 =# @views(x[i]), vn, inds, __varinfo__)
                              end
                          end
                      end
                  end)
          return (Model)(:demo4, evaluator, NamedTuple{(:n, :TV)}((n, TV)), NamedTuple{(:TV,)}((Vector{Float64},)))
      end)

julia> # So does evaluation!
julia> var"@model"(LineNumberNode(1, nothing), Main, :(function demo4(n, ::Type{TV} = Vector{Float64}) where TV
                 m ~ Normal()
                 x = TV(undef, n)
            @show __varinfo__
           
             for i = eachindex(x)
           
                 x[i] ~ Normal(m, 1.0)
             end
         end)) |> eval
demo4 (generic function with 2 methods)

[devmotion]

It would be great to not have to escape everything. I actually tried to change it at some point but it turned out to be quite annoying and not completely straightforward (I don't remember why) so I did not complete or push these local changes.

[phipsgabler]

So, what about the following approach:

• All variable names introduced by us (intermediate values in tilde expansion etc.) are still manually gensymed, to prevent used code from accidentially capturing them.
• The INTERNALNAMES and function arguments are all bound, because they are function arguments. So we just interpolate them as is; hygiene can rename them consistently, but we want the user to be able to capture them.
• Only the function name needs to be escaped, since it should leak the name of the model constructor to the outside.

I think that's all sound. It remains to check

• how things work out with inner macros (esp. submodels)
• how well reading from outside scope works; but in theory, that should already be handled by default hygiene.

julia> @macroexpand @model function demo4(n, x::Int)
           m ~ Normal()
           x = Vector(undef, n)
           @show __varinfo__
           for i in eachindex(x)
               x[i] ~ Normal(m, 1.0)
           end
       end
quote
    $(Expr(:meta, :doc))
    function demo4(var"#402#n", var"#403#x"::DynamicPPL.Int; )
        #= REPL[113]:1 =#
        var"#401###evaluator#390" = ((var"#413#__model__"::Model, var"#414#__varinfo__"::AbstractVarInfo, var"#415#__context__"::DynamicPPL.AbstractContext, var"#402#n", var"#403#x"::DynamicPPL.Int)->begin
                    begin
                        #= REPL[113]:1 =#
                        #= REPL[113]:2 =#
                        begin
                            var"#404###vn#384" = (VarName){:m}()
                            var"#405###inds#385" = ()
                            var"#406###isassumption#386" = begin
                                    if (DynamicPPL.contextual_isassumption)(var"#415#__context__", var"#404###vn#384")
                                        if !((DynamicPPL.inargnames)(var"#404###vn#384", var"#413#__model__")) || (DynamicPPL.inmissings)(var"#404###vn#384", var"#413#__model__")
                                            true
                                        else
                                            var"#407#m" === DynamicPPL.missing
                                        end
                                    else
                                        false
                                    end
                                end
                            if var"#406###isassumption#386"
                                var"#407#m" = (DynamicPPL.tilde_assume!)(var"#415#__context__", (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal()), var"#404###vn#384")..., var"#405###inds#385", var"#414#__varinfo__")
                            else
                                if !((DynamicPPL.inargnames)(var"#404###vn#384", var"#413#__model__"))
                                    var"#407#m" = (DynamicPPL.getvalue_nested)(var"#415#__context__", var"#404###vn#384")
                                end
                                (DynamicPPL.tilde_observe!)(var"#415#__context__", (DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal()), var"#407#m", var"#404###vn#384", var"#405###inds#385", var"#414#__varinfo__")
                            end
                        end
                        #= REPL[113]:3 =#
                        var"#403#x" = DynamicPPL.Vector(DynamicPPL.undef, var"#402#n")
                        #= REPL[113]:4 =#
                        begin
                            Base.println("__varinfo__ = ", Base.repr(begin
                                        #= show.jl:955 =#
                                        local var"#408##400#value" = var"#414#__varinfo__"
                                    end))
                            var"#408##400#value"
                        end
                        #= REPL[113]:5 =#
                        for var"#409#i" = DynamicPPL.eachindex(var"#403#x")
                            var"#410###vn#387" = (VarName){:x}(((var"#409#i",),))
                            var"#411###inds#388" = ((var"#409#i",),)
                            var"#412###isassumption#389" = begin
                                    if (DynamicPPL.contextual_isassumption)(var"#415#__context__", var"#410###vn#387")
                                        if !((DynamicPPL.inargnames)(var"#410###vn#387", var"#413#__model__")) || (DynamicPPL.inmissings)(var"#410###vn#387", var"#413#__model__")
                                            true
                                        else
                                            (Base.maybeview)(var"#403#x", var"#409#i") === DynamicPPL.missing
                                        end
                                    else
                                        false
                                    end
                                end
                            if var"#412###isassumption#389"
                                var"#403#x"[var"#409#i"] = (DynamicPPL.tilde_assume!)(var"#415#__context__", (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal(var"#407#m", 1.0)), var"#410###vn#387")..., var"#411###inds#388", var"#414#__varinfo__")
                            else
                                if !((DynamicPPL.inargnames)(var"#410###vn#387", var"#413#__model__"))
                                    var"#403#x"[var"#409#i"] = (DynamicPPL.getvalue_nested)(var"#415#__context__", var"#410###vn#387")
                                end
                                (DynamicPPL.tilde_observe!)(var"#415#__context__", (DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal(var"#407#m", 1.0)), (Base.maybeview)(var"#403#x", var"#409#i"), var"#410###vn#387", var"#411###inds#388", var"#414#__varinfo__")
                            end
                        end
                    end
                end)
        return (Model)($(QuoteNode(:($(Expr(:escape, :demo4))))), var"#401###evaluator#390", DynamicPPL.NamedTuple{(:n, :x)}((var"#402#n", var"#403#x")), DynamicPPL.NamedTuple())
    end
end

Note that a lot of names are "double gensymed", since they are gensyms undergoing hygiene.

[devmotion]

Sounds good, we should rely on the default hygiene as much as possible. It seems in the example the argument types should be DynamicPPL.Model and DynamicPPL.AbstractVarInfo though?

[github-actions]

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

	$(DynamicPPL.inmissings)($vn, __model__)
	$(DynamicPPL.inmissings)($vn, __model__)

[devmotion]

It seems something went wrong and the deprecated internal names where reintroduced when you merged the master branch.

[phipsgabler]

Right, I didn't realize you had removed them already :D

[phipsgabler]

I think this looks good:

julia> @macroexpand @model function demo4(n, ::Type{T}; σ) where {T}
           m ~ Normal()
           x = Vector{T}(undef, n)
           for i in eachindex(x)
               x[i] ~ Normal(m, σ)
           end
           return (;m, x, ℓ = getlogp(__varinfo__))
       end
quote
    function demo4(var"#176#__model__"::Model, var"#177#__varinfo__"::AbstractVarInfo, var"#178#__context__"::DynamicPPL.AbstractContext, var"#179#n", ::DynamicPPL.Type{var"#182#T"}, var"#181#σ"; ) where var"#182#T"
        #= REPL[11]:1 =#
        begin
            #= REPL[11]:1 =#
            #= REPL[11]:2 =#
            begin
                var"#167###vn#295" = (VarName){:m}()
                var"#168###inds#296" = ()
                var"#169###isassumption#297" = begin
                        if (DynamicPPL.contextual_isassumption)(var"#178#__context__", var"#167###vn#295")
                            if !((DynamicPPL.inargnames)(var"#167###vn#295", var"#176#__model__")) || (DynamicPPL.inmissings)(var"#167###vn#295", var"#176#__model__")
                                true
                            else
                                var"#170#m" === DynamicPPL.missing
                            end
                        else
                            false
                        end
                    end
                if var"#169###isassumption#297"
                    var"#170#m" = (DynamicPPL.tilde_assume!)(var"#178#__context__", (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal()), var"#167###vn#295")..., var"#168###inds#296", var"#177#__varinfo__")
                else
                    if !((DynamicPPL.inargnames)(var"#167###vn#295", var"#176#__model__"))
                        var"#170#m" = (DynamicPPL.getvalue_nested)(var"#178#__context__", var"#167###vn#295")
                    end
                    (DynamicPPL.tilde_observe!)(var"#178#__context__", (DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal()), var"#170#m", var"#167###vn#295", var"#168###inds#296", var"#177#__varinfo__")
                end
            end
            #= REPL[11]:3 =#
            var"#171#x" = DynamicPPL.Vector{var"#182#T"}(DynamicPPL.undef, var"#179#n")
            #= REPL[11]:4 =#
            for var"#172#i" = DynamicPPL.eachindex(var"#171#x")
                var"#173###vn#298" = (VarName){:x}(((var"#172#i",),))
                var"#174###inds#299" = ((var"#172#i",),)
                var"#175###isassumption#300" = begin
                        if (DynamicPPL.contextual_isassumption)(var"#178#__context__", var"#173###vn#298")
                            if !((DynamicPPL.inargnames)(var"#173###vn#298", var"#176#__model__")) || (DynamicPPL.inmissings)(var"#173###vn#298", var"#176#__model__")
                                true
                            else
                                (Base.maybeview)(var"#171#x", var"#172#i") === DynamicPPL.missing
                            end
                        else
                            false
                        end
                    end
                if var"#175###isassumption#300"
                    var"#171#x"[var"#172#i"] = (DynamicPPL.tilde_assume!)(var"#178#__context__", (DynamicPPL.unwrap_right_vn)((DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal(var"#170#m", var"#181#σ")), var"#173###vn#298")..., var"#174###inds#299", var"#177#__varinfo__")
                else
                    if !((DynamicPPL.inargnames)(var"#173###vn#298", var"#176#__model__"))
                        var"#171#x"[var"#172#i"] = (DynamicPPL.getvalue_nested)(var"#178#__context__", var"#173###vn#298")
                    end
                    (DynamicPPL.tilde_observe!)(var"#178#__context__", (DynamicPPL.check_tilde_rhs)(DynamicPPL.Normal(var"#170#m", var"#181#σ")), (Base.maybeview)(var"#171#x", var"#172#i"), var"#173###vn#298", var"#174###inds#299", var"#177#__varinfo__")
                end
            end
            #= REPL[11]:7 =#
            return (; m = var"#170#m", x = var"#171#x", ℓ = DynamicPPL.getlogp(var"#177#__varinfo__"))
        end
    end
    begin
        $(Expr(:meta, :doc))
        function demo4(var"#183#n", ::DynamicPPL.Type{var"#185#T"}; σ) where var"#185#T"
            #= REPL[11]:1 =#
            return (Model)(:demo4, demo4, DynamicPPL.NamedTuple{(:n, :T, :σ)}((var"#183#n", var"#185#T", σ)), DynamicPPL.NamedTuple())
        end
    end
end

julia> @model function demo4(n, ::Type{T}; σ) where {T}
           m ~ Normal()
           x = Vector{T}(undef, n)
           for i in eachindex(x)
               x[i] ~ Normal(m, σ)
           end
           return (;m, x, ℓ = getlogp(__varinfo__))
       end
demo4 (generic function with 2 methods)

julia> methods(demo4)
# 2 methods for generic function "demo4":
[1] demo4(__model__::Model, __varinfo__::AbstractVarInfo, __context__::DynamicPPL.AbstractContext, n, ::Type{var"#201#T"}, σ) where var"#201#T" in Main at REPL[12]:1
[2] demo4(n, ::Type{var"#204#T"}; σ) where var"#204#T" in Main at REPL[12]:1

julia> demo4(2, Float64; σ = 1.0)
Model{typeof(demo4), (:n, :T, :σ), (), (), Tuple{Int64, DataType, Float64}, Tuple{}, DefaultContext}(:demo4, demo4, (n = 2, T = Float64, σ = 1.0), NamedTuple(), DefaultContext())

julia> demo4(2, Float64; σ = 1.0)()
(m = 0.1242670570655595, x = [0.38638092732182805, -0.08489833515866964], ℓ = -2.820763671492404)

• Hygiene being applied to __internalnames__ is OK, since it doesn't change meaning (they're all bound function arguments), and the user can still refer to them before hygiene.
• Keyword arguments are not subject to hygiene, so this does exactly what we want (relevant only in the generator, since they become regulat arguments in the evaluator).
• Nicely, the docstrings of the methods still refer to the names before hygiene!

[devmotion]

I managed to fix the error by making sure that the arguments and where clause of the modeldef function are escaped correctly.

[devmotion]

I also noticed that the example above is not what we want. Eg., Normal must be evaluated in the module where @model is called but not in DynamicPPL. Additionally, not only __varinfo__ etc. but also tilde and other functions from DynamicPPL should be handled correctly by macro hygiene automatically.

[phipsgabler]

tilde etc. (basically everything interpolated as $(DynamicPPL.X)) are not subject to hygiene -- they are object references in the expression tree (Expr(:call, DynamicPPL.tilde, ...)), not GlobalRefs (Expr(:call, GlobalRef(DynamicPPL, :tilde), ...)). Just printed the same.

But you're right about the global variables needed to be resolved in calling scope. I have to think about that.

The bug is resolved to some extent by escaping arguments and where-params in that the syntax error does not occur anymore, but then we'd also have to escape all occurences of them in the body (due to the problem admitted in the previous paragraph), which is tedious and should not be necessary in the first place, IMO! I am convinced there is some underlying problem in either hygiene or MacroTools, or how we change the meaning of ::Type{T}=X.

[devmotion]

tilde etc. (basically everything interpolated as $(DynamicPPL.X)) are not subject to hygiene -- they are object references in the expression tree (Expr(:call, DynamicPPL.tilde, ...)), not GlobalRefs (Expr(:call, GlobalRef(DynamicPPL, :tilde), ...)). Just printed the same.

Sure, but often this is sufficient if you construct an unescaped macro expression. E.g., in the rule definition macros in ChainRulesCore (https://github.com/JuliaDiff/ChainRulesCore.jl/blob/3b46ac592fcb6bce8377f45ba2f9f5a4321a0bdf/src/rule_definition_tools.jl#L172-L177 and similar lines) rely on this behaviour - the interpolation was removed on purpose when the macro was changed to an unescaped version. However, I guess here it might be problematic in the function body if e.g. users use variables of name check_tilde_rhs etc.

But you're right about the global variables needed to be resolved in calling scope. I have to think about that.

I believe there's no way around this issue other than escaping all user-provided statements in the function body. The question is what we gain in the end then, in particular if we still interpolate DynamicPPL.tilde etc. expressions.

The bug is resolved to some extent by escaping arguments and where-params in that the syntax error does not occur anymore, but then we'd also have to escape all occurences of them in the body (due to the problem admitted in the previous paragraph), which is tedious and should not be necessary in the first place, IMO! I am convinced there is some underlying problem in either hygiene or MacroTools, or how we change the meaning of ::Type{T}=X.

The interesting thing is that it is only necessary to escape it in modeldef, not for the evaluator. But in modeldef we only have the Model(...) statement and so one only has to escape the vals in to_namedtuple_expr as well. It seems that the way we construct the arguments in evaluatordef is better than what is returned by MacroTools (the whereparams are the same, so it seems to be the arguments). I guess we have to transform the representation of ::Type{TV} = Vector{Float64} in some way.

[phipsgabler]

Hygiene on ::Type{T}=default-style arguments might have a bigger problem than I thought: JuliaLang/julia#42220.

[phipsgabler]

OK, so there's a few difficulties...

1. the hygiene bug mentioned above makes it pretty impossible to use the ::Type syntax without escaping. This has to be resolved in Julia.
2. As David rightly noted, there's still the problem that expansion/hygiene happens relative to the DynamicPPL module, not the caller. Which leads to results like DynamicPPL.Normal.
3. Even without escaping, we still need the drop_escape calls for hygiene to work correctly; otherwise, we have things like

   for i = eachindex(x)
       var"##vn#388" = (VarName){:x}((Setfield.compose)((Setfield.IndexLens)((i,))))

   becoming

   for var"##i#387" = eachindex(x)
       var"##vn#388" = (VarName){:x}((Setfield.compose)((Setfield.IndexLens)((i,))))

sooo... I guess it isn't really worth it? @torfjelde @devmotion, what do you think?

[devmotion]

Yes, unfortunately it seems somewhat between impossible and very difficult/annoying, so I think it's maybe not worth it.

Just wanted to mention that the fix for

the hygiene bug mentioned above makes it pretty impossible to use the ::Type syntax without escaping. This has to be resolved in Julia.

(JuliaLang/julia#42220) was backported to Julia 1.6.4, so it's fixed in the latest stable release 🙂

[phipsgabler]

Yes, unfortunately it seems somewhat between impossible and very difficult/annoying, so I think it's maybe not worth it.

I came to the same conclusion. We are just doing things for which hygiene is not the right thing: messing around within the passed expression.