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

Fix scope of hoisted signature-local variables #56712

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Fix scope of hoisted signature-local variables #56712

merged 1 commit into from
Dec 2, 2024

Conversation

Keno
Copy link
Member

@Keno Keno commented Nov 29, 2024

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 c6c3d72, 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

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 c6c3d72, 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
@Keno Keno requested a review from JeffBezanson November 29, 2024 07:35
@Keno Keno merged commit f1b0b01 into master Dec 2, 2024
7 checks passed
@Keno Keno deleted the kf/56711 branch December 2, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local variable inside closure signature gets hoisted to wrong scope
2 participants