[Merged by Bors] - Fixed a bug in replace returns

[torfjelde]

On master we have the following behavior for a test-case in Turing.jl:

julia> @macroexpand @model empty_model() = begin x = 1; end
quote
    function empty_model(__model__::DynamicPPL.Model, __varinfo__::DynamicPPL.AbstractVarInfo, __context__::DynamicPPL.AbstractContext; )
        #= REPL[5]:1 =#
        begin
            #= REPL[5]:1 =#
            #= REPL[5]:1 =#
            return (x = 1, __varinfo__)
        end
    end
    begin
        $(Expr(:meta, :doc))
        function empty_model(; )
            #= REPL[5]:1 =#
            return (DynamicPPL.Model)(:empty_model, empty_model, NamedTuple(), NamedTuple())
        end
    end
end

Notice the return statement: it converted the statement x = 1 which returns 1 into an attempt at a NamedTuple{(:x, :__varinfo__)}. On Julia 1.6 we don't really notice much of difference, because first and last will have the same behavior, but on Julia 1.3 the tests would fail in TuringLang/Turing.jl#1726 since "implicit" names in construction of NamedTuple isn't supported.

This PR addresses this issue by simply capturing the return-value in separate variable, which is then combined with __varinfo__ in a Tuple at the end. This should both fail and succeed whenever standard Julia code would.

[torfjelde]

bors try

[devmotion]

Could we just use return (begin $expr end, __varinfo__)?

julia> f(expr) = :(return ($expr, 1.0))
f (generic function with 1 method)

julia> f(:(x = 10))
:(return (x = 10, 1.0))

julia> @eval f(:(x = 10))
:(return (x = 10, 1.0))

julia> @eval $(f(:(x = 10)))
ERROR: syntax: invalid named tuple element "1"
...

julia> f(expr) = :(return (begin $expr end, 1.0))
f (generic function with 1 method)

julia> f(:(x = 10))
:(return (begin
              #= REPL[11]:1 =#
              x = 10
          end, 1.0))

julia> @eval $(f(:(x = 10)))
(10, 1.0)

julia> x
10

[bors]

try

Build succeeded:

• format
• Turing.jl
• docs
• test (1, ubuntu-latest, x64, 2)
• test (1, ubuntu-latest, x64, 1)
• test (1, ubuntu-latest, x86, 2)
• test (1.3, ubuntu-latest, x64, 2)
• test (1, macOS-latest, x64, 2)
• test (1.3, ubuntu-latest, x64, 1)
• test (1, windows-latest, x64, 2)

[torfjelde]

Could we just use return (begin $expr end, __varinfo__)?

julia> f(expr) = :(return ($expr, 1.0))
f (generic function with 1 method)

julia> f(:(x = 10))
:(return (x = 10, 1.0))

julia> @eval f(:(x = 10))
:(return (x = 10, 1.0))

julia> @eval $(f(:(x = 10)))
ERROR: syntax: invalid named tuple element "1"
...

julia> f(expr) = :(return (begin $expr end, 1.0))
f (generic function with 1 method)

julia> f(:(x = 10))
:(return (begin
              #= REPL[11]:1 =#
              x = 10
          end, 1.0))

julia> @eval $(f(:(x = 10)))
(10, 1.0)

julia> x
10

That's a good idea! Is this preferable to a temporary variable? It looks less intuitive to me, though I agree it's nice to not have the intermediate variable. I'm fine with either way though, so if you prefer begin ... end, I'm down for that 👍

[torfjelde]

Btw, Turing tests are passing on both Julia 1.3 and 1.6 locally with this. So once we've made the decision above, we'll be ready for a proper release.

[yebai]

Both approaches look good to me - the intermediate variable looks a bit more intuitive!

[devmotion]

I think the begin - end is a bit simpler (maybe one does not even have to handle the tuple case specifically?) and does not add gensymed variables (I don't know how slow or fast their generation is). On the other hand, probably it's less intuitive. So I don't have a strong opinion 🤷‍♂️

[torfjelde]

I think the begin - end is a bit simpler (maybe one does not even have to handle the tuple case specifically?) and does not add gensymed variables (I don't know how slow or fast their generation is). On the other hand, probably it's less intuitive. So I don't have a strong opinion man_shrugging

I don't think perf is much of a concern here tbh, since this particular usage will be hit at most a couple of times for a given model (and in particular not compared to the heavy usage of gensym in the ~ that we have now 😕 ). I'll leave as is for now then 👍

[torfjelde]

bors r+

[bors]

Pull request successfully merged into master.

Build succeeded:

• docs
• format
• test (1.3, ubuntu-latest, x64, 1)
• test (1.3, ubuntu-latest, x64, 2)
• test (1, macOS-latest, x64, 2)
• test (1, ubuntu-latest, x64, 1)
• test (1, ubuntu-latest, x64, 2)
• test (1, ubuntu-latest, x86, 2)
• test (1, windows-latest, x64, 2)
• Turing.jl