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

Side effects due to assignment are visible before the right hand side is evaluated #57484

Open
xal-0 opened this issue Feb 20, 2025 · 4 comments
Labels
bug Indicates an unexpected problem or unintended behavior compiler:lowering Syntax lowering (compiler front end, 2nd stage)

Comments

@xal-0
Copy link
Member

xal-0 commented Feb 20, 2025

Basic case

It's reasonable to expect _, _ = ... to evaluate the right hand side before
setting the variables on the left. Some cases follow this intuition, for
example the swapping idiom:

julia> x = 1
       y = 2
       x, y = y, x
(2, 1)

Other cases, including those with a ref on the LHS, also work:

julia> x = Ref(1)
       x[], y = 2, x[]
(2, 1)

However, others don't:

julia> x = 1
       f() = x
       x, y = 2, f()
(2, 2)

The lowering for x, y = 2, x:

1 ─ %1  = 2
│   %2  = Main.x
│         $(Expr(:globaldecl, :(Main.x)))
│         $(Expr(:latestworld))
│   %5  =   builtin Core.get_binding_type(Main, :x)
│         #s1 = %1
│   %7  = #s1
│   %8  =   dynamic %7 isa %5
└──       goto #3 if not %8
2 ─       goto #4
3 ─ %11 = #s1
└──       #s1 = Base.convert(%5, %11)
4 ┄ %13 = #s1
│           dynamic Base.setglobal!(Main, :x, %13)
│         $(Expr(:globaldecl, :(Main.y)))
│         $(Expr(:latestworld))
│   %17 =   builtin Core.get_binding_type(Main, :y)
│         @_2 = %2
│   %19 = @_2
│   %20 =   dynamic %19 isa %17
└──       goto #6 if not %20
5 ─       goto #7
6 ─ %23 = @_2
└──       @_2 = Base.convert(%17, %23)
7 ┄ %25 = @_2
│           dynamic Base.setglobal!(Main, :y, %25)
│   %27 =   dynamic Core.tuple(%1, %2)
└──       return %27

The lowering for x, y = 2, f():

1 ─       $(Expr(:globaldecl, :(Main.x)))
│         $(Expr(:latestworld))
│   %3  =   builtin Core.get_binding_type(Main, :x)
│         #s2 = 2
│   %5  = #s2
│   %6  =   dynamic %5 isa %3
└──       goto #3 if not %6
2 ─       goto #4
3 ─ %9  = #s2
└──       #s2 = Base.convert(%3, %9)
4 ┄ %11 = #s2
│           dynamic Base.setglobal!(Main, :x, %11)
│   %13 = Main.f
│   %14 =   dynamic (%13)()
│         $(Expr(:globaldecl, :(Main.y)))
│         $(Expr(:latestworld))
│   %17 =   builtin Core.get_binding_type(Main, :y)
│         @_2 = %14
│   %19 = @_2
│   %20 =   dynamic %19 isa %17
└──       goto #6 if not %20
5 ─       goto #7
6 ─ %23 = @_2
└──       @_2 = Base.convert(%17, %23)
7 ┄ %25 = @_2
│           dynamic Base.setglobal!(Main, :y, %25)
│   %27 =   dynamic Core.tuple(2, %14)
└──       return %27

Multiple const assignments

In 1.12 we have the related issue of constants becoming visible mid-evaluation
because of a latestworld. For example, in const x, y = 2, f():

1 ─ %1  = 2
│         x = %1
│         $(Expr(:const, :(Main.x), :(x)))
│         $(Expr(:latestworld))
│   %5  = Main.f
│   %6  =   dynamic (%5)()
│         y = %6
│         $(Expr(:const, :(Main.y), :(y)))
│         $(Expr(:latestworld))
│   %10 =   dynamic Core.tuple(2, %6)
└──       return %10

Typeasserts

Typeasserts present yet another problem; convert can fail and leave only some
of the variables assigned:

julia> x = 0
       y = "a"
       z = 0
0

julia> x, y::String, z::Int = 1, 2, 3
ERROR: cannot set type for global Main.y. It already has a value or is already set to a different type.
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1

julia> x, y, z
(1, "a", 0)

Lowered:

1 ──       $(Expr(:globaldecl, :(Main.x)))
│          $(Expr(:latestworld))
│    %3  =   builtin Core.get_binding_type(Main, :x)
│          #s3 = 1
│    %5  = #s3
│    %6  =   dynamic %5 isa %3
└───       goto #3 if not %6
2 ──       goto #4
3 ── %9  = #s3
└───       #s3 = Base.convert(%3, %9)
4 ┄─ %11 = #s3
│            dynamic Base.setglobal!(Main, :x, %11)
│    %13 = 2
│    %14 = 3
│    %15 = Main.String
│          $(Expr(:globaldecl, :(Main.y), :(%15)))
│          $(Expr(:latestworld))
│          $(Expr(:globaldecl, :(Main.y)))
│          $(Expr(:latestworld))
│    %20 =   builtin Core.get_binding_type(Main, :y)
│          @_2 = %13
│    %22 = @_2
│    %23 =   dynamic %22 isa %20
└───       goto #6 if not %23
5 ──       goto #7
6 ── %26 = @_2
└───       @_2 = Base.convert(%20, %26)
7 ┄─ %28 = @_2
│            dynamic Base.setglobal!(Main, :y, %28)
│    %30 = Main.Int
│          $(Expr(:globaldecl, :(Main.z), :(%30)))
│          $(Expr(:latestworld))
│          $(Expr(:globaldecl, :(Main.z)))
│          $(Expr(:latestworld))
│    %35 =   builtin Core.get_binding_type(Main, :z)
│          @_3 = %14
│    %37 = @_3
│    %38 =   dynamic %37 isa %35
└───       goto #9 if not %38
8 ──       goto #10
9 ── %41 = @_3
└───       @_3 = Base.convert(%35, %41)
10 ┄ %43 = @_3
│            dynamic Base.setglobal!(Main, :z, %43)
│    %45 =   dynamic Core.tuple(1, %13, %14)
└───       return %45
@xal-0 xal-0 added bug Indicates an unexpected problem or unintended behavior compiler:lowering Syntax lowering (compiler front end, 2nd stage) labels Feb 20, 2025
@xal-0
Copy link
Member Author

xal-0 commented Feb 20, 2025

The basic inconsistency is present in at least as far back as 0.3:

julia> x = [1]
       f() = x[1]
       x[1], y = 2, f()
(2,1)

julia> x = 1
       f() = x
       x, y = 2, f()
(2,2)

@oscardssmith
Copy link
Member

tagging @c42f since she will probably be interested.

@adienes
Copy link
Contributor

adienes commented Feb 21, 2025

looks related to #18300

@c42f
Copy link
Member

c42f commented Feb 24, 2025

Thanks @oscardssmith :)

I think I spotted+fixed the worst of these already while reading and contemplating the existing code! It's certainly clear that side effects from the rhs should happen before any assignments because whatever lowering optimizations we do should be equivalent to constructing the tuple on the rhs and the destructuring it. See the commit message here:

c42f/JuliaLowering.jl@9e7d9e4

It's less clear to me that the type assert or const cases are wrong - one might argue that the commas on the left hand side are just syntax sugar for a normal list of assignments. And that having those happen in order (along with whatever side effects for fancy assignment syntax) is actually the desired behavior?

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:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

No branches or pull requests

4 participants