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

lowering: Fix captured vars shadowed by an inner global declaration #57648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mlechu
Copy link
Contributor

@mlechu mlechu commented Mar 5, 2025

Bug

As discovered in #57547, lowering isn't resolving references to captured
variables when a global of the same name is declared in any scope in the
current function:

julia> let
            g = 1
            function f()
                let; global g = 2; end;
                return g    # g is not resolved
            end; f()
        end
ERROR: Found raw symbol in code returned from lowering. Expected all symbols to
have been resolved to GlobalRef or slots.

resolve-scopes correctly detects that we're returning the local, but
scope-blocks are removed in this pass. As a result, analyze-vars-lambda finds
more globals than resolve-scopes did when calling find-global-decls (which
does not peek inside nested scopes) on f. analyze-vars-lambda then
calculates the set of captured variables in f to be:

captvars = (current_env ∩ free_vars) - (new_staticparams ∪ wrong_globals)

so g in this case is never captured.

As one might expect from the diff, static parameters were affected too:

julia> let
           function f(x::T) where T
               function g(x)
                   let; global T = 1; end
                   x::T
               end
           end
       end
ERROR: Found raw symbol in code returned from lowering. Expected all symbols to
have been resolved to GlobalRef or slots.

This bug was introduced (revealed, maybe) in #57051---the whole resolution step
used to happen after lowering, so lowering passes disagreeing on scopes would be
less visible.

Fix

Omit globals from the free variable list in analyze-vars-lambda in the first
place. This way we don't need to call the scope-block-dependant
find-global-decls.

After:

julia> let
            g = 1
            function f()
                let; global g = 2; end;
                return g
            end; f()
        end
1

julia> let
           function f(x::T) where T
               function g(x)
                   let; global T = 1; end
                   x::T
               end
           end
       end
(::var"#f#f##1") (generic function with 1 method)

Also in this PR:

  • To handle our new ability to shadow a captured variable with a global, prevent
    tampering with globals in cl-convert-
  • Update the manual slightly

@mlechu mlechu added compiler:lowering Syntax lowering (compiler front end, 2nd stage) bugfix This change fixes an existing bug labels Mar 5, 2025

# global declarations from the top level are not inherited by functions.
# don't allow such a declaration to override an outer local, since it's not
# clear what it should do.
Copy link
Member

Choose a reason for hiding this comment

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

Can you spell out the ambiguity? What might it do / not do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test (and comment) was added a while ago in #35536, which special-cases globals shadowing locals if we're not in a lambda(?). If we know global declarations belong to scope-blocks instead of lambdas, maybe this special case can be removed? @JeffBezanson

;; sp: list of symbol
;; new-sp: list of symbol (static params declared here)
;; methsig: `(call (core svec) ...)
;; tab: table of (name . var-info)
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for this!

return g3
end
f()
end === 1
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is a breaking change to our global behavior up until now, is that right?

On Julia 1.6-1.11:

julia> let
           g3 = 1
           function f()
               let; global g3 = 2; end;
               return g3
           end
           (f(), g3)
       end
(2, 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it; I should have checked.

The new behaviour is more consistent with the documentation on global and Jeff's comments, but if it's too breaking I'll see what I can do. Other options are scoping global declarations to lambdas (discussed, would break the case where g3 = 1 is moved into f) or trying to reimplement the "my scope > inner scopes with globals > outer scopes" resolution.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's breaking, but it was also a bug. There were a few other places in lowering where we accidentally got the scope wrong. That's why I made that an error. My preference would be to try it with the bug fixed. I think the problem with changing the scope semantics to try to match what we did before is that the behavior before was not particularly well defined, so we risk breaking changes in the opposite direction if the try to choose a principled semantics that matches the old behavior.

@topolarity topolarity requested a review from JeffBezanson March 5, 2025 20:12
@topolarity topolarity added the breaking This change will break code label Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code bugfix This change fixes an existing bug compiler:lowering Syntax lowering (compiler front end, 2nd stage)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants