Skip to content

Commit

Permalink
Fixed a bug in replace returns (#352)
Browse files Browse the repository at this point in the history
On master we have the following behavior for a test-case in Turing.jl:

```julia
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.
  • Loading branch information
torfjelde committed Dec 15, 2021
1 parent 28c6004 commit 57c50f1
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 10 deletions.
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "DynamicPPL"
uuid = "366bfd00-2699-11ea-058f-f148b4cae6d8"
version = "0.17.0"
version = "0.17.1"

[deps]
AbstractMCMC = "80f14c24-f653-4e6a-9b94-39d6b0f70001"
Expand Down
19 changes: 10 additions & 9 deletions src/compiler.jl
Original file line number Diff line number Diff line change
Expand Up @@ -518,16 +518,17 @@ function replace_returns(e::Expr)
end

if Meta.isexpr(e, :return)
# NOTE: `return` always has an argument. In the case of
# an empty `return`, the lowered expression will be `return nothing`.
# Hence we don't need any special handling for empty returns.
retval_expr = if length(e.args) > 1
Expr(:tuple, e.args...)
else
e.args[1]
# We capture the original return-value in `retval` and return
# a `Tuple{typeof(retval),typeof(__varinfo__)}`.
# If we don't capture the return-value separately, cases such as
# `return x = 1` will result in `(x = 1, __varinfo__)` which will
# mistakenly attempt to construct a `NamedTuple` (which fails on Julia 1.3
# and is not our intent).
@gensym retval
return quote
$retval = $(e.args...)
return $retval, __varinfo__
end

return :(return ($retval_expr, __varinfo__))
end

return Expr(e.head, map(replace_returns, e.args)...)
Expand Down
7 changes: 7 additions & 0 deletions test/compiler.jl
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,13 @@ end
end

@testset "return value" begin
# Make sure that a return-value of `x = 1` isn't combined into
# an attempt at a `NamedTuple` of the form `(x = 1, __varinfo__)`.
@model empty_model() = return x = 1
empty_vi = VarInfo()
retval_and_vi = DynamicPPL.evaluate!!(empty_model(), empty_vi, SamplingContext())
@test retval_and_vi isa Tuple{Int,typeof(empty_vi)}

# Even if the return-value is `AbstractVarInfo`, we should return
# a `Tuple` with `AbstractVarInfo` in the second component too.
@model demo() = return __varinfo__
Expand Down

2 comments on commit 57c50f1

@torfjelde
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JuliaRegistrator
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registration pull request created: JuliaRegistries/General/50616

After the above pull request is merged, it is recommended that a tag is created on this repository for the registered package version.

This will be done automatically if the Julia TagBot GitHub Action is installed, or can be done manually through the github interface, or via:

git tag -a v0.17.1 -m "<description of version>" 57c50f1f46aa5e51ec2125f3ae4883c61e59e9c3
git push origin v0.17.1

Please sign in to comment.