diff --git a/frontend/src/core/codemirror/go-to-definition/__tests__/commands.test.ts b/frontend/src/core/codemirror/go-to-definition/__tests__/commands.test.ts index 85e7ef0e5f9..8dbf2b01935 100644 --- a/frontend/src/core/codemirror/go-to-definition/__tests__/commands.test.ts +++ b/frontend/src/core/codemirror/go-to-definition/__tests__/commands.test.ts @@ -253,6 +253,73 @@ a = 10`; `); }); + test("from-import alias is the binding, not the imported name", async () => { + const code = `\ +from math import sin as my_sin +print(my_sin)`; + view = createEditor(code); + const usagePosition = code.lastIndexOf("my_sin"); + const result = goToVariableDefinition(view, "my_sin", usagePosition); + + expect(result).toBe(true); + await tick(); + // The alias `my_sin` (after `as`) is the real binding. + expect(renderEditorView(view)).toMatchInlineSnapshot(` + " + from math import sin as my_sin + ^ + print(my_sin) + " + `); + }); + + test("module path in from-import is not a local definition", async () => { + const code = `\ +from math import sin +print(math)`; + view = createEditor(code); + const usagePosition = code.lastIndexOf("math"); + // `math` is a module reference in the from-clause, not a binding in this + // cell, so the scoped resolver should return false and let the caller fall + // through to cross-cell resolution. + const result = goToVariableDefinition(view, "math", usagePosition); + + expect(result).toBe(false); + expect(view.state.selection.main.head).toBe(0); + }); + + test("imported name without `as` is a local definition", async () => { + const code = `\ +from math import sin +print(sin)`; + view = createEditor(code); + const usagePosition = code.lastIndexOf("sin"); + const result = goToVariableDefinition(view, "sin", usagePosition); + + expect(result).toBe(true); + await tick(); + expect(renderEditorView(view)).toMatchInlineSnapshot(` + " + from math import sin + ^ + print(sin) + " + `); + }); + + test("imported name shadowed by `as` is not a binding", async () => { + const code = `\ +from math import sin as my_sin +print(sin)`; + view = createEditor(code); + const usagePosition = code.lastIndexOf("sin"); + // `sin` here refers to nothing in this cell (it was renamed to `my_sin`), + // so the scoped resolver should return false. + const result = goToVariableDefinition(view, "sin", usagePosition); + + expect(result).toBe(false); + }); + test("selects outer-scope function declaration", async () => { view = createEditor(`\ def x(): diff --git a/frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts b/frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts index 63e11cc2e26..1a3c195da75 100644 --- a/frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts +++ b/frontend/src/core/codemirror/go-to-definition/__tests__/utils.test.ts @@ -96,4 +96,51 @@ output = _x + 10`; await tick(); expect(view.state.selection.main.head).toBe(code.indexOf("_x = 10")); }); + + test("falls through to cross-cell when in-cell occurrence is only a module path in a from-import", async () => { + // Regression: ImportStatement used to register every VariableName child + // (the module path and pre-`as` names) as in-cell declarations, so the + // local-first short-circuit would steal F12 from cross-cell resolution. + const moduleCell = cellId("module-cell"); + const usageCell = cellId("usage-cell"); + const moduleCode = `mymodule = 100`; + const usageCode = `\ +from mymodule import something +print(mymodule)`; + + const moduleView = createEditor(moduleCode, moduleCode.length); + const usageView = createEditor( + usageCode, + usageCode.lastIndexOf("mymodule"), + ); + views.push(moduleView, usageView); + + const notebook = initialNotebookState(); + notebook.cellHandles[moduleCell] = { + current: { editorView: moduleView, editorViewOrNull: moduleView }, + } as never; + notebook.cellHandles[usageCell] = { + current: { editorView: usageView, editorViewOrNull: usageView }, + } as never; + + store.set(notebookAtom, notebook); + store.set(variablesAtom, { + [variableName("mymodule")]: { + dataType: "int", + declaredBy: [moduleCell], + name: variableName("mymodule"), + usedBy: [usageCell], + value: "100", + }, + }); + + const result = goToDefinitionAtCursorPosition(usageView); + + expect(result).toBe(true); + await tick(); + // Cross-cell jump: moduleView's cursor should land on `mymodule = 100`. + expect(moduleView.state.selection.main.head).toBe( + moduleCode.indexOf("mymodule"), + ); + }); }); diff --git a/frontend/src/core/codemirror/go-to-definition/commands.ts b/frontend/src/core/codemirror/go-to-definition/commands.ts index 5ba4f47bd33..902be41c7da 100644 --- a/frontend/src/core/codemirror/go-to-definition/commands.ts +++ b/frontend/src/core/codemirror/go-to-definition/commands.ts @@ -306,33 +306,52 @@ function collectMatchingDeclarations( break; } case "ImportStatement": { + // The grammar emits one ImportStatement for both `import x [as y]` and + // `from m import x [as y], ...`. Direct children include the keywords + // (`from`/`import`/`as`), commas, dots, and every VariableName from the + // module path AND the import list. We only want the names that actually + // bind in the current scope: the post-`as` alias if present, otherwise + // the imported name itself. Names before `import` (the from-path) and + // the original name when an alias follows it are NOT bindings. const subCursor = node.cursor(); subCursor.firstChild(); - do { - if ( - subCursor.name === "VariableName" && - state.doc.sliceString(subCursor.from, subCursor.to) === variableName - ) { - addDeclaration(declarations, currentScope, subCursor.from); + let pastImport = false; + // Buffer the most recent post-`import` VariableName so we can defer + // committing it until we know whether `as` follows. + let pending: { from: number; matches: boolean } | null = null; + const commit = () => { + if (pending?.matches) { + addDeclaration(declarations, currentScope, pending.from); } - } while (subCursor.nextSibling()); - break; - } - case "ImportFromStatement": { - const subCursor = node.cursor(); - subCursor.firstChild(); - let foundImport = false; + pending = null; + }; do { if (subCursor.name === "import") { - foundImport = true; - } else if ( - foundImport && - subCursor.name === "VariableName" && - state.doc.sliceString(subCursor.from, subCursor.to) === variableName - ) { - addDeclaration(declarations, currentScope, subCursor.from); + pastImport = true; + continue; + } + if (!pastImport) { + continue; + } + if (subCursor.name === "as") { + // Next VariableName is the alias and replaces `pending`. + pending = null; + continue; + } + if (subCursor.name === "VariableName") { + // Flush any previous pending name (no `as` followed it). + commit(); + pending = { + from: subCursor.from, + matches: + state.doc.sliceString(subCursor.from, subCursor.to) === + variableName, + }; + } else if (subCursor.name === ",") { + commit(); } } while (subCursor.nextSibling()); + commit(); break; } case "TryStatement": @@ -415,10 +434,14 @@ export function goToVariableDefinition( usagePosition?: number, ): boolean { const { state } = view; + // When the caller knows the usage position, trust the scoped lookup. Falling + // back to first-match would defeat the local-vs-cross-cell decision in + // goToDefinition: if the symbol only appears as a module path in an import, + // scoped resolution returns null and we want the caller to try other cells. const from = - (usagePosition !== undefined + usagePosition !== undefined ? findScopedDefinitionPosition(state, variableName, usagePosition) - : null) ?? findFirstMatchingVariable(state, variableName); + : findFirstMatchingVariable(state, variableName); if (from === null) { return false; diff --git a/frontend/src/core/codemirror/reactive-references/__tests__/analyzer.test.ts b/frontend/src/core/codemirror/reactive-references/__tests__/analyzer.test.ts index cd45cfcdc09..7b140da2760 100644 --- a/frontend/src/core/codemirror/reactive-references/__tests__/analyzer.test.ts +++ b/frontend/src/core/codemirror/reactive-references/__tests__/analyzer.test.ts @@ -640,6 +640,60 @@ def run(polars): `); }); + test("set comprehension target shadows outer global", () => { + // Regression: SCOPE_CREATING_NODES used "SetComprehension" instead of the + // grammar's "SetComprehensionExpression", so set comprehensions never + // created a scope and their for-target was treated as reactive. + expect(runHighlight(["x"], "result = {x for x in range(5)}")) + .toMatchInlineSnapshot(` + " + result = {x for x in range(5)} + " + `); + }); + + test("from-import module path stays reactive", () => { + // Regression: ImportStatement collected every VariableName child, so the + // module name in `from m import y` was wrongly treated as a local binding. + expect( + runHighlight( + ["math"], + ` +def f(): + from math import sin as my_sin + return math + my_sin(1)`, + ), + ).toMatchInlineSnapshot(` + " + def f(): + from math import sin as my_sin + return math + my_sin(1) + ^^^^ + " + `); + }); + + test("from-import: aliased imported name is not a binding", () => { + // Regression: `sin` in `from math import sin as my_sin` was incorrectly + // registered as a local binding, hiding genuine reactive uses of `sin`. + expect( + runHighlight( + ["sin"], + ` +def f(): + from math import sin as my_sin + return sin + my_sin(1)`, + ), + ).toMatchInlineSnapshot(` + " + def f(): + from math import sin as my_sin + return sin + my_sin(1) + ^^^ + " + `); + }); + test("lambda inside function with outer global", () => { expect( runHighlight( diff --git a/frontend/src/core/codemirror/reactive-references/analyzer.ts b/frontend/src/core/codemirror/reactive-references/analyzer.ts index 64aecb2a833..ca1bb1f78d5 100644 --- a/frontend/src/core/codemirror/reactive-references/analyzer.ts +++ b/frontend/src/core/codemirror/reactive-references/analyzer.ts @@ -16,7 +16,7 @@ const SCOPE_CREATING_NODES = new Set([ "FunctionDefinition", "LambdaExpression", "ArrayComprehensionExpression", - "SetComprehension", + "SetComprehensionExpression", "DictionaryComprehensionExpression", "ComprehensionExpression", "ClassDefinition", @@ -152,7 +152,7 @@ export function findReactiveVariables(options: { } case "ArrayComprehensionExpression": case "DictionaryComprehensionExpression": - case "SetComprehension": + case "SetComprehensionExpression": case "ComprehensionExpression": { // Domprehension variables - look for VariableName or TupleExpression after 'for' const subCursor = node.cursor(); @@ -276,49 +276,52 @@ export function findReactiveVariables(options: { break; } case "ImportStatement": { - // Handle import x + // The grammar emits a single ImportStatement for both `import x [as y]` + // and `from m import x [as y], ...`. Direct children mix keywords, + // module-path names (before `import`), imported names, and aliases. + // Only post-`import` names that aren't shadowed by a following `as` + // (and the alias itself when `as` is present) bind in the current + // scope. const subCursor = node.cursor(); subCursor.firstChild(); - do { - if (subCursor.name === "VariableName") { - const varName = options.state.doc.sliceString( - subCursor.from, - subCursor.to, - ); - - const currentScope = - currentScopeStack[currentScopeStack.length - 1] ?? -1; - if (!allDeclarations.has(currentScope)) { - allDeclarations.set(currentScope, new Set()); - } - allDeclarations.get(currentScope)?.add(varName); + const currentScope = + currentScopeStack[currentScopeStack.length - 1] ?? -1; + if (!allDeclarations.has(currentScope)) { + allDeclarations.set(currentScope, new Set()); + } + const scope = allDeclarations.get(currentScope); + let pastImport = false; + let pending: string | null = null; + const commit = () => { + if (pending !== null) { + scope?.add(pending); } - } while (subCursor.nextSibling()); - - break; - } - case "ImportFromStatement": { - // Handle from x import y as z - const subCursor = node.cursor(); - subCursor.firstChild(); - let foundImport = false; + pending = null; + }; do { if (subCursor.name === "import") { - foundImport = true; - } else if (foundImport && subCursor.name === "VariableName") { - const varName = options.state.doc.sliceString( + pastImport = true; + continue; + } + if (!pastImport) { + continue; + } + if (subCursor.name === "as") { + // Drop the imported name; the next VariableName is the alias. + pending = null; + continue; + } + if (subCursor.name === "VariableName") { + commit(); + pending = options.state.doc.sliceString( subCursor.from, subCursor.to, ); - // Add to the current innermost scope - const currentScope = - currentScopeStack[currentScopeStack.length - 1] ?? -1; - if (!allDeclarations.has(currentScope)) { - allDeclarations.set(currentScope, new Set()); - } - allDeclarations.get(currentScope)?.add(varName); + } else if (subCursor.name === ",") { + commit(); } } while (subCursor.nextSibling()); + commit(); break; } @@ -424,6 +427,12 @@ export function findReactiveVariables(options: { const nodeName = cursor.name; const nodeStart = cursor.from; + // Names inside an import statement are module paths, imported names, or + // aliases — none of them are reactive *uses* of an outer-cell global. + if (nodeName === "ImportStatement") { + return; + } + const isNewScope = SCOPE_CREATING_NODES.has(nodeName); let currentScopeStack = scopeStack;