Fix/def declaration within function#9379
Open
Shamik-07 wants to merge 14 commits intomarimo-team:mainfrom
Open
Fix/def declaration within function#9379Shamik-07 wants to merge 14 commits intomarimo-team:mainfrom
Shamik-07 wants to merge 14 commits intomarimo-team:mainfrom
Conversation
feat: go to definition tries local search in current edition and then tries across cells.
…te variables stay local cell bound.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/core/codemirror/go-to-definition/utils.ts">
<violation number="1" location="frontend/src/core/codemirror/go-to-definition/utils.ts:81">
P1: The new local-first short-circuit treats any local symbol occurrence as a successful local definition, which can block cross-cell definition jumps.</violation>
</file>
<file name="frontend/src/core/codemirror/go-to-definition/commands.ts">
<violation number="1" location="frontend/src/core/codemirror/go-to-definition/commands.ts:87">
P2: Class scope is incorrectly treated as an enclosing scope for method symbol resolution, which can send go-to-definition to class-body names instead of module/global definitions.</violation>
</file>
Architecture diagram
sequenceDiagram
participant U as User
participant V as Editor View (CodeMirror)
participant UT as utils.ts
participant C as commands.ts
participant T as Syntax Tree (Lezer)
participant S as Notebook Store (Jotai)
U->>V: Trigger "Go to Definition" (F12/Cmd+Click)
V->>UT: goToDefinitionAtCursorPosition(view)
UT->>UT: getWordUnderCursor(state)
Note right of UT: CHANGED: Now returns both<br/>variable name AND position
UT->>C: NEW: goToVariableDefinition(view, name, position)
C->>T: resolveInner(position)
T-->>C: SyntaxNode
C->>C: NEW: getScopeChain(tree, position)
Note right of C: Build hierarchy of scopes<br/>(Function, Class, Global)
C->>C: NEW: collectMatchingDeclarations(tree)
C->>T: iterate tree nodes
T-->>C: VariableName, ParamList, etc.
C->>C: NEW: findScopedDefinitionPosition()
Note right of C: Filter declarations by lexical<br/>scope and position sensitivity
alt Local Definition Found
C->>V: focus() and setSelection(foundPosition)
C-->>UT: true
else Not Found Locally (Reactive/Global Fallback)
C-->>UT: false
UT->>S: getEditorForVariable(variableName)
Note over UT,S: Checks marimo variablesAtom<br/>for definitions in other cells
alt Found in Other Cell
S-->>UT: Target Editor View
UT->>C: findFirstMatchingVariable(targetView, name)
C->>T: iterate tree (first match)
T-->>C: position
C->>V: focus target view and setSelection
UT-->>V: true
else Not Found Anywhere
S-->>UT: null
UT-->>V: false
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| usagePosition?: number, | ||
| ): boolean { | ||
| if (usagePosition !== undefined) { | ||
| const foundLocally = goToVariableDefinition( |
Contributor
There was a problem hiding this comment.
P1: The new local-first short-circuit treats any local symbol occurrence as a successful local definition, which can block cross-cell definition jumps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/core/codemirror/go-to-definition/utils.ts, line 81:
<comment>The new local-first short-circuit treats any local symbol occurrence as a successful local definition, which can block cross-cell definition jumps.</comment>
<file context>
@@ -69,7 +75,19 @@ export function goToDefinitionAtCursorPosition(view: EditorView): boolean {
+ usagePosition?: number,
): boolean {
+ if (usagePosition !== undefined) {
+ const foundLocally = goToVariableDefinition(
+ view,
+ variableName,
</file context>
Comment on lines
+87
to
+92
| if (SCOPE_CREATING_NODES.has(currentNode.name)) { | ||
| scopeChain.push({ | ||
| id: currentNode.from, | ||
| type: currentNode.name, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Class scope is incorrectly treated as an enclosing scope for method symbol resolution, which can send go-to-definition to class-body names instead of module/global definitions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/core/codemirror/go-to-definition/commands.ts, line 87:
<comment>Class scope is incorrectly treated as an enclosing scope for method symbol resolution, which can send go-to-definition to class-body names instead of module/global definitions.</comment>
<file context>
@@ -22,50 +46,378 @@ function goToPosition(view: EditorView, from: number): void {
+ let currentNode: SyntaxNode | null = tree.resolveInner(usagePosition, 0);
+
+ while (currentNode) {
+ if (SCOPE_CREATING_NODES.has(currentNode.name)) {
+ scopeChain.push({
+ id: currentNode.from,
</file context>
Suggested change
| if (SCOPE_CREATING_NODES.has(currentNode.name)) { | |
| scopeChain.push({ | |
| id: currentNode.from, | |
| type: currentNode.name, | |
| }); | |
| } | |
| const inFunctionLikeScope = scopeChain.some( | |
| (scope) => | |
| scope.type === "FunctionDefinition" || | |
| scope.type === "LambdaExpression", | |
| ); | |
| if ( | |
| SCOPE_CREATING_NODES.has(currentNode.name) && | |
| !(inFunctionLikeScope && currentNode.name === "ClassDefinition") | |
| ) { | |
| scopeChain.push({ | |
| id: currentNode.from, | |
| type: currentNode.name, | |
| }); | |
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes CodeMirror “go to definition” so symbol resolution prefers lexical scope (e.g., inside def blocks) before falling back to reactive/global definitions across cells, and adds tests for the reported regressions.
Changes:
- Enhance go-to-definition to pass usage position and attempt local (scoped) resolution first.
- Implement scoped definition lookup in
goToVariableDefinition(functions, lambdas, comprehensions, etc.). - Add frontend tests covering local-vs-global resolution and private-variable behavior; broaden Altair chart config helper input types.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| marimo/_data/charts.py | Expands add_common_config to accept FacetChart. |
| frontend/src/core/codemirror/go-to-definition/utils.ts | Threads usage position from cursor selection into go-to-definition flow. |
| frontend/src/core/codemirror/go-to-definition/commands.ts | Adds scoped AST-based declaration collection and resolution logic. |
| frontend/src/core/codemirror/go-to-definition/tests/utils.test.ts | Adds tests ensuring local definitions are preferred over reactive globals and private vars stay in-cell. |
| frontend/src/core/codemirror/go-to-definition/tests/commands.test.ts | Adds tests for selecting in-scope local and parameter definitions. |
Comment on lines
+80
to
+88
| if (usagePosition !== undefined) { | ||
| const foundLocally = goToVariableDefinition( | ||
| view, | ||
| variableName, | ||
| usagePosition, | ||
| ); | ||
| if (foundLocally) { | ||
| return true; | ||
| } |
Comment on lines
+380
to
+388
| const match = declarations | ||
| .filter((declaration) => declaration.scopeId === scope.id) | ||
| .filter((declaration) => { | ||
| return POSITION_SENSITIVE_SCOPES.has(scope.type) | ||
| ? declaration.from <= clampedUsagePosition | ||
| : true; | ||
| }) | ||
| .toSorted((left, right) => left.from - right.from)[0]; | ||
|
|
| def add_common_config(chart: alt.Chart | alt.LayerChart) -> alt.Chart: | ||
| def add_common_config( | ||
| chart: alt.Chart | alt.LayerChart | alt.FacetChart, | ||
| ) -> alt.Chart: |
Author
|
@manzt |
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
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.
📝 Summary
Previously, F12/command usage would jump effectively using first match semantics and could jump to an outer/global definition when the cursor was on a local symbol use. Now, the jump follows lexical scope first, and fixes the issue where symbols inside def blocks did not resolve correctly.
Tested with the code snippets to reproduce this issue and added the same to the tests.
Closes #1430
📋 Pre-Review Checklist
✅ Merge Checklist
I have read the CLA Document and I hereby sign the CLA