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.

Sign up for GitHub

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

Jump to bottom

comprehensions temporarily rebind global variables #92

Closed
alandonovan opened this issue Jan 6, 2020 · 2 comments
Closed

comprehensions temporarily rebind global variables #92

alandonovan opened this issue Jan 6, 2020 · 2 comments

Comments

@alandonovan
Copy link
Contributor

Yet another scoping bug in the Java implementation: comprehensions save and restore the existing binding but in between they clobber it, and this clobbering is visible:

$ cat a.star
x = 1
def f():
    return x
print([f() for x in [2]])

$ python2 a.star
[2]
$ python3.8 a.star
[1]
$ starlark a.star
[1]
$ blaze-bin/third_party/bazel/src/main/java/com/google/devtools/starlark/Starlark a.star
[2]

The solution is static scope resolution.

A related comprehension scoping issue: #84
bazel-io pushed a commit to bazelbuild/bazel that referenced this issue Jan 14, 2020
@adonovan @copybara-github
bazel syntax: remove the toplevel environment hack
c855244 
Currently, the value of CallFrame.locals (formerly
StarlarkThread.lexicalFrame) for the <toplevel> function is an alias
for StarlarkThread.globalFrame, which is the Module of the <toplevel>
function, necessitating various hacks.

This change causes every function, including top-level ones such as
<toplevel> and <expr>, to have a local environment block.
Variables are bound in this block by load statements and by
comprehensions. We cannot yet assume that the variables bound
by load and by comprehensions have been properly resolved, so
we (in effect) do the resolution to Local during evaluation.
(This requires keeping track of whether evaluation is within a comprehension.)

This fixes issue bazelbuild/starlark#92

Now that CallFrame.locals is never an alias for Module, we can
specialize its type from Frame to LexicalFrame, then inline that type
completely and throw away Frame. CallFrame.locals is now just a map.
Updating it does not throw MutabilityException.

Also, inline Eval.updateAndExport.

PiperOrigin-RevId: 289668675
@ndmitchell
Copy link
Contributor

Curious what the resolution of this issue should be. Does it need an additional test case in the repo? It sounds like a clear bug, so no spec change required.

@laurentlb
Copy link
Contributor

Yes, it was a bug. It has been fixed in the Java implementation.

luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
bazel syntax: remove the toplevel environment hack
a3ceb4b 
    Currently, the value of CallFrame.locals (formerly
    StarlarkThread.lexicalFrame) for the <toplevel> function is an alias
    for StarlarkThread.globalFrame, which is the Module of the <toplevel>
    function, necessitating various hacks.

    This change causes every function, including top-level ones such as
    <toplevel> and <expr>, to have a local environment block.
    Variables are bound in this block by load statements and by
    comprehensions. We cannot yet assume that the variables bound
    by load and by comprehensions have been properly resolved, so
    we (in effect) do the resolution to Local during evaluation.
    (This requires keeping track of whether evaluation is within a comprehension.)

    This fixes issue bazelbuild/starlark#92

    Now that CallFrame.locals is never an alias for Module, we can
    specialize its type from Frame to LexicalFrame, then inline that type
    completely and throw away Frame. CallFrame.locals is now just a map.
    Updating it does not throw MutabilityException.

    Also, inline Eval.updateAndExport.

    PiperOrigin-RevId: 289668675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@laurentlb @ndmitchell @alandonovan