Skip to content

Commit 02fc173

Browse files
committed
Add failing tests for go-to-definition scope bugs
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.
1 parent 4a8eb4b commit 02fc173

2 files changed

Lines changed: 114 additions & 4 deletions

File tree

frontend/src/core/codemirror/go-to-definition/__tests__/commands.test.ts

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,116 @@ def my_func(a):
147147
`);
148148
});
149149

150+
test("selects the comprehension target inside a set comprehension", async () => {
151+
const code = `\
152+
x = 100
153+
s = {x for x in range(10)}`;
154+
view = createEditor(code);
155+
// Go-to-definition on the `x` before `for` (the expression part of the
156+
// comprehension).
157+
const usagePosition = code.indexOf("{x") + 1;
158+
const result = goToVariableDefinition(view, "x", usagePosition);
159+
160+
expect(result).toBe(true);
161+
await tick();
162+
// Should jump to the comprehension target `x` (after `for`), not the
163+
// outer `x = 100`. The Lezer Python grammar emits
164+
// `SetComprehensionExpression`, but the code looks for `SetComprehension`,
165+
// so the comprehension never creates a scope and the for-target is not
166+
// collected — `findScopedDefinitionPosition` returns null and the
167+
// fallback `findFirstMatchingVariable` lands on `x = 100`.
168+
expect(renderEditorView(view)).toMatchInlineSnapshot(`
169+
"
170+
x = 100
171+
s = {x for x in range(10)}
172+
^
173+
"
174+
`);
175+
});
176+
177+
test("selects the comprehension target inside a dict comprehension", async () => {
178+
const code = `\
179+
x = 100
180+
d = {x: x for x in range(10)}`;
181+
view = createEditor(code);
182+
const usagePosition = code.indexOf("{x") + 1;
183+
const result = goToVariableDefinition(view, "x", usagePosition);
184+
185+
expect(result).toBe(true);
186+
await tick();
187+
// Positive control: `DictionaryComprehensionExpression` matches the grammar
188+
// and is in SCOPE_CREATING_NODES, so this should jump to the comprehension
189+
// target `x` (after `for`).
190+
expect(renderEditorView(view)).toMatchInlineSnapshot(`
191+
"
192+
x = 100
193+
d = {x: x for x in range(10)}
194+
^
195+
"
196+
`);
197+
});
198+
199+
test("skips enclosing class scope when resolving from inside a method", async () => {
200+
const code = `\
201+
x = 100
202+
class Foo:
203+
x = 10
204+
def method(self):
205+
return x`;
206+
view = createEditor(code);
207+
// Go-to-definition on the `x` inside `return x`.
208+
const usagePosition = code.lastIndexOf("x");
209+
const result = goToVariableDefinition(view, "x", usagePosition);
210+
211+
expect(result).toBe(true);
212+
await tick();
213+
// Should jump to `x = 100` at module scope. In Python, methods do NOT see
214+
// their enclosing class body's names — class scopes are skipped in LEGB
215+
// lookup once a function boundary has been crossed. `getScopeChain` walks
216+
// straight up and pushes the `ClassDefinition` onto the chain, so the
217+
// method's lookup finds the class-body `x = 10` instead.
218+
expect(renderEditorView(view)).toMatchInlineSnapshot(`
219+
"
220+
x = 100
221+
^
222+
class Foo:
223+
x = 10
224+
def method(self):
225+
return x
226+
"
227+
`);
228+
});
229+
230+
test("resolves a global forward-reference from inside a function", async () => {
231+
const code = `\
232+
def foo():
233+
return a
234+
235+
a = 10`;
236+
view = createEditor(code);
237+
// Go-to-definition on the `a` inside `return a`.
238+
const usagePosition = code.indexOf("return a") + "return ".length;
239+
const result = goToVariableDefinition(view, "a", usagePosition);
240+
241+
expect(result).toBe(true);
242+
await tick();
243+
// Should jump to `a = 10` at the bottom. Python allows forward references
244+
// from within nested functions to module-level names. POSITION_SENSITIVE_SCOPES
245+
// includes `"global"`, so the global declaration is filtered out (its `from`
246+
// is after the usage), the lookup returns null, and the fallback
247+
// `findFirstMatchingVariable` lands on the `a` inside `return a` — i.e.
248+
// go-to-definition jumps to itself.
249+
expect(renderEditorView(view)).toMatchInlineSnapshot(`
250+
"
251+
def foo():
252+
return a
253+
254+
a = 10
255+
^
256+
"
257+
`);
258+
});
259+
150260
test("selects outer-scope function declaration", async () => {
151261
view = createEditor(`\
152262
def x():

frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,11 @@ def test():
5656

5757
const notebook = initialNotebookState();
5858
notebook.cellHandles[globalCell] = {
59-
current: { editorView: globalView },
60-
} as never;
59+
current: { editorView: globalView, editorViewOrNull: globalView },
60+
};
6161
notebook.cellHandles[localCell] = {
62-
current: { editorView: localView },
63-
} as never;
62+
current: { editorView: localView, editorViewOrNull: localView },
63+
};
6464

6565
store.set(notebookAtom, notebook);
6666
store.set(variablesAtom, {

0 commit comments

Comments
 (0)