From f1b0b010dd20591a013c71d8f3f7a09503e55baf Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Mon, 2 Dec 2024 18:02:41 -0500
Subject: [PATCH] Fix scope of hoisted signature-local variables (#56712)

When we declare inner methods, e.g. the `f` in

```
function fs()
   f(lhs::Integer) = 1
   f(lhs::Integer, rhs::(local x=Integer; x)) = 2
   return f
end
```

we must hoist the definition of the (appropriately mangled) generic
function `f` to top-level, including all variables that were used in the
signature definition of `f`. This situation is a bit unique in the
language because it uses inner function scope, but gets executed in
toplevel scope. For example, you're not allowed to use a local of the
inner function in the signature definition:

```
julia> function fs()
          local x=Integer
          f(lhs::Integer, rhs::x) = 2
          return f
       end
ERROR: syntax: local variable x cannot be used in closure declaration
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1
```

In particular, the restriction is signature-local:
```
julia> function fs()
          f(rhs::(local x=Integer; x)) = 1
          f(lhs::Integer, rhs::x) = 2
          return f
       end
ERROR: syntax: local variable x cannot be used in closure declaration
Stacktrace:
 [1] top-level scope
   @ REPL[4]:1
```

There's a special intermediate form `moved-local` that gets generated
for this definition. In c6c3d72d1cbddb3d27e0df0e739bb27dd709a413, this
form stopped getting generated for certain inner methods. I suspect this
happened because of the incorrect assumption that the set of moved
locals is being computed over all signatures, rather than being a
per-signature property.

The result of all of this was that this is one of the few places where
lowering still generated a symbol as the lhs of an assignment for a
global (instead of globalref), because the code that generates the
assignment assumes it's a local, but the later pass doesn't know this.
Because we still retain the code for this from before we started using
globalref consistently, this wasn't generally causing a problems, except
possibly leaking a global (or potentially assigning to a global when
this wasn't intended). However, in follow on work, I want to make use of
knowing whether the LHS is a global or local in lowering, so this was
causing me trouble.

Fix all of this by putting back the `moved-local` where it was dropped.

Fixes #56711
---
 src/julia-syntax.scm | 1 +
 test/syntax.jl       | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
index 72e97da3c2daa..852d5eb4d6f86 100644
--- a/src/julia-syntax.scm
+++ b/src/julia-syntax.scm
@@ -4279,6 +4279,7 @@ f(x) = yt(x)
                    (if (or exists (and short (pair? alldefs)))
                        `(toplevel-butfirst
                          (null)
+                         ,@(map (lambda (v) `(moved-local ,v)) moved-vars)
                          ,@sp-inits
                          ,@mk-method
                          (latestworld))
diff --git a/test/syntax.jl b/test/syntax.jl
index d9d311ac6615d..0fb752bae480f 100644
--- a/test/syntax.jl
+++ b/test/syntax.jl
@@ -4033,3 +4033,11 @@ end
 @test isa(create_inner_f_no_methods(), Function)
 @test length(methods(create_inner_f_no_methods())) == 0
 @test Base.invoke_in_world(first(methods(create_inner_f_one_method)).primary_world, create_inner_f_one_method()) == 1
+
+# Issue 56711 - Scope of signature hoisting
+function fs56711()
+    f(lhs::Integer) = 1
+    f(lhs::Integer, rhs::(local x_should_not_be_defined=Integer; x_should_not_be_defined)) = 2
+    return f
+end
+@test !@isdefined(x_should_not_be_defined)