Add failing tests for set-comprehension and forward-ref scope bugs#1
Merged
Shamik-07 merged 1 commit intoMay 11, 2026
Conversation
Three known bugs in the new scope-aware go-to-definition path.
Set comprehensions never create a scope because SCOPE_CREATING_NODES
uses "SetComprehension" but the Lezer Python grammar emits
"SetComprehensionExpression" — go-to-def on the expression `x` in
`{x for x in ...}` falls back to first-match and lands on an outer
`x` instead of the for-target. A dict-comprehension test uses the
grammar-correct name and passes as a positive control.
`getScopeChain` walks straight up the syntax tree and pushes any
ClassDefinition ancestor onto the chain, but in Python a method
body does not see its enclosing class scope. Once a function or
lambda boundary has been crossed walking outward, class scopes must
be skipped. With the current behavior, go-to-def on `x` inside a
method finds the class-body `x` instead of the module-level `x`.
POSITION_SENSITIVE_SCOPES includes "global", so a module-level
declaration whose position is after the usage is filtered out. From
inside a function that forward-references a later top-level name,
the lookup returns null and the fallback first-match lands on the
usage itself. Class-scope semantics need the position filter; module
scope does not.
The utils.test.ts mocks also drop the `as never` casts in favor of
fully-populated CellHandle literals, so a future field addition will
surface as a typecheck error.
manzt
force-pushed
the
fix/def_declaration-tests
branch
from
f93dd97 to
02fc173
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two failing tests that demonstrate the correctness issues from the review on marimo-team#9379.
1. Set comprehension typo.
SCOPE_CREATING_NODESuses"SetComprehension", but the Lezer Python grammar emitsSetComprehensionExpression. As a result the comprehension never creates a scope, the for-target isn't collected, and go-to-def on the expressionxin{x for x in range(10)}falls back to first-match and lands on an outerx. A dict-comprehension test is included as a positive control — same shape, but it uses the grammar-correct name and passes. The same typo exists pre-existing inreactive-references/analyzer.ts, worth fixing both spots.2. Global forward reference.
POSITION_SENSITIVE_SCOPESincludes"global", so a module-level declaration whose position is after the usage is filtered out:Go-to-def on
ainsidefoofilters outa = 10, the lookup returns null, and the fallback first-match lands on the usage itself. Removing"global"from the set keeps class-scope semantics intact while letting nested functions resolve later-declared globals.Snapshots encode the expected behavior, so the tests fail on the current branch and will go green once both items are addressed.