Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable update not reflected outside nested try blocks #42022

Closed
tkf opened this issue Aug 27, 2021 · 1 comment · Fixed by #42081
Closed

Variable update not reflected outside nested try blocks #42022

tkf opened this issue Aug 27, 2021 · 1 comment · Fixed by #42081
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference

Comments

@tkf
Copy link
Member

tkf commented Aug 27, 2021

Minimal example:

assertstring(::AbstractString) = nothing

function test_trytry()
    x = nothing
    try
        try
            x = "/"
            assertstring(x)
            error("err")
        catch
            rethrow()
        end
    catch
    end
    assertstring(x)
end

test_trytry()

throws

ERROR: LoadError: MethodError: no method matching assertstring(::Nothing)

As a sanity check, using Ref instead of a slot works:

function test_trytry_ref()
    x = Ref{Any}(nothing)
    try
        try
            x[] = "/"
            assertstring(x[])
            error("err")
        catch
            rethrow()
        end
    catch
    end
    assertstring(x[])
end

test_trytry_ref()  # no error

Looking at @code_typed optimize=false test_trytry():

CodeInfo(
1 ─      (x = Main.nothing)::Core.Const(nothing)
2 ─ %2 = $(Expr(:enter, #7))
3 ─      $(Expr(:enter, #5))
4 ─      (x = "/")::Core.Const("/")
│        Main.assertstring(x::Core.Const("/"))::Any
│        Main.error("err")::Union{}
│        Core.Const(:($(Expr(:leave, 1))))::Union{}
└──      Core.Const(:(goto %12))::Union{}
5 ┄      $(Expr(:leave, 1))
6 ─      Main.rethrow()::Union{}
│        Core.Const(:($(Expr(:pop_exception, :(%3)))))::Union{}
│        Core.Const(:($(Expr(:leave, 1))))::Union{}
└──      Core.Const(:(goto %16))::Union{}
7 ┄      $(Expr(:leave, 1))
8 ─      $(Expr(:pop_exception, :(%2)))::Any
│        Main.assertstring(x::Core.Const(nothing))::Union{}
└──      Core.Const(:(return %16))::Union{}
) => Union{}

Note Main.assertstring(x::Core.Const(nothing))::Union{} in BB 8. Does this indicate that there are some problems in the inference?

@tkf tkf added the compiler:inference Type inference label Aug 27, 2021
@vtjnash vtjnash added the bug Indicates an unexpected problem or unintended behavior label Aug 27, 2021
@vtjnash vtjnash self-assigned this Aug 27, 2021
@vtjnash
Copy link
Member

vtjnash commented Aug 27, 2021

ah, that is a remarkable find. I see what is wrong: we only propagate changes to the nearest exception handler, but we must propagate to all of them

aviatesk added a commit that referenced this issue Sep 3, 2021
* inference: propagate variable changes to all exception frames

Fix #42022

* Update test/compiler/inference.jl

* Update test/compiler/inference.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>

* fixup! inference: propagate variable changes to all exception frames

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
KristofferC pushed a commit that referenced this issue Sep 3, 2021
* inference: propagate variable changes to all exception frames

Fix #42022

* Update test/compiler/inference.jl

* Update test/compiler/inference.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>

* fixup! inference: propagate variable changes to all exception frames

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
(cherry picked from commit e83b317)
KristofferC pushed a commit that referenced this issue Sep 8, 2021
* inference: propagate variable changes to all exception frames

Fix #42022

* Update test/compiler/inference.jl

* Update test/compiler/inference.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>

* fixup! inference: propagate variable changes to all exception frames

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
(cherry picked from commit e83b317)
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
…ang#42081)

* inference: propagate variable changes to all exception frames

Fix JuliaLang#42022

* Update test/compiler/inference.jl

* Update test/compiler/inference.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>

* fixup! inference: propagate variable changes to all exception frames

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
…ang#42081)

* inference: propagate variable changes to all exception frames

Fix JuliaLang#42022

* Update test/compiler/inference.jl

* Update test/compiler/inference.jl

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>

* fixup! inference: propagate variable changes to all exception frames

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants