Fix/def declaration within function by Shamik-07 · Pull Request #9379 · marimo-team/marimo

Shamik-07 · 2026-04-24T22:14:30Z

📝 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

For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

I have read the contributor guidelines.
Documentation has been updated where applicable, including docstrings for API changes.
Tests have been added for the changes made.

I have read the CLA Document and I hereby sign the CLA

…erChart

…finition.

feat: go to definition tries local search in current edition and then tries across cells.

…te variables stay local cell bound.

vercel · 2026-04-24T22:14:36Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
marimo-docs	Ready	Preview, Comment	May 12, 2026 9:31pm

github-actions · 2026-04-24T22:14:42Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

Shamik-07 · 2026-04-24T22:16:09Z

I have read the CLA Document and I hereby sign the CLA

Shamik-07 · 2026-04-24T22:16:35Z

recheck

cubic-dev-ai

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.}

Copilot

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.

-def add_common_config(chart: alt.Chart | alt.LayerChart) -> alt.Chart:
+def add_common_config(
+    chart: alt.Chart | alt.LayerChart | alt.FacetChart,
+) -> alt.Chart:


Shamik-07 · 2026-05-04T21:46:59Z

@manzt
Do these copilot reviews need to be addressed ?

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

Looks good. Left a few inline comments on edge cases I think aren't covered yet. Pushed failing tests for them at fix/def_declaration-tests.

manzt · 2026-05-11T19:24:59Z

+    notebook.cellHandles[globalCell] = {
+      current: { editorView: globalView },
+    } as never;
+    notebook.cellHandles[localCell] = {
+      current: { editorView: localView },
+    } as never;


Should not need to type cast here:

Suggested change

notebook.cellHandles[globalCell] = {

current: { editorView: globalView },

} as never;

notebook.cellHandles[localCell] = {

current: { editorView: localView },

} as never;

notebook.cellHandles[globalCell] = {

current: { editorView: globalView, editorViewOrNull: globalView },

} as never;

notebook.cellHandles[localCell] = {

current: { editorView: localView, editorViewOrNull: localView },

};

manzt · 2026-05-11T19:33:57Z

+
+    case "ArrayComprehensionExpression":
+    case "DictionaryComprehensionExpression":
+    case "SetComprehension":


Same comment as above, I believe this is SetComprehensionExpression (like the others) and will never get hit. A test with a set comprehension should have caught this.

manzt · 2026-05-11T19:34:52Z

+  "FunctionDefinition",
+  "LambdaExpression",
+  "ArrayComprehensionExpression",
+  "SetComprehension",


I don't believe Lezer Python emits SetComprehensionExpression, so this string doesn't match anything. Do we have a test covering this case?

manzt · 2026-05-11T19:45:23Z

+  "ClassDefinition",
+]);
+
+const POSITION_SENSITIVE_SCOPES = new Set(["ClassDefinition", "global"]);


I don't think "global" should be here. Python resolves module names lazily, so a forward ref:

def foo(): return a a = 10

should resolve to a = 10. We should have a test for this.

manzt · 2026-05-11T19:48:18Z

+  return from;
+}
+
+function getScopeChain(tree: Tree, usagePosition: number): ScopeContext[] {


Class scopes shouldn't be in the chain once we've crossed a function/lambda walking outward. Methods don't see their enclosing class's names

x = 100 class Foo: x = 10 def method(self): return x # should resolve to module `x = 100`, not class `x = 10`

Should be easy to add a test for this as well.

Add failing tests for set-comprehension and forward-ref scope bugs

…'s python grammar emits this. fix: remove global from the position sensive scopes as python resolves module names lazily.

… function.

Shamik-07 · 2026-05-11T23:06:47Z

Looks good. Left a few inline comments on edge cases I think aren't covered yet. Pushed failing tests for them at fix/def_declaration-tests.

Thank you very much for helping out.
I have addressed your comments. Please let me know if there's something else.

Screen.Recording.2026-05-11.at.19.03.24.mov

Shamik-07 added 5 commits April 24, 2026 17:48

fix: error: Argument 1 to add_common_config has incompatible type Lay…

2aa7825

…erChart

feat: adding command matches to the outer and inner function scope de…

4ad8b3d

…finition.

feat: get word under cursor returns both word and position

0a1c771

feat: go to definition tries local search in current edition and then tries across cells.

test: checks nearest local parameters and checks across cells.

8a170b1

test: local definitions are preferred over reactive gloabls and priva…

bee4c91

…te variables stay local cell bound.

vercel Bot deployed to Preview April 24, 2026 22:15 View deployment

Shamik-07 mentioned this pull request Apr 24, 2026

go to declaration does not work within def #1430

Open

cubic-dev-ai Bot reviewed Apr 24, 2026

View reviewed changes

Comment thread frontend/src/core/codemirror/go-to-definition/utils.ts

Comment thread frontend/src/core/codemirror/go-to-definition/commands.ts

mscolnick requested a review from Copilot April 24, 2026 22:36

Copilot started reviewing on behalf of mscolnick April 24, 2026 22:37 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

mscolnick requested a review from manzt April 27, 2026 13:24

Merge branch 'main' into fix/def_declaration

0fa0c14

vercel Bot deployed to Preview April 27, 2026 21:38 View deployment

Merge branch 'main' into fix/def_declaration

af8a66e

vercel Bot deployed to Preview April 28, 2026 18:59 View deployment

Shamik-07 added 2 commits April 29, 2026 11:20

Merge branch 'main' into fix/def_declaration

6612d01

Merge branch 'main' into fix/def_declaration

2767c77

vercel Bot deployed to Preview April 30, 2026 17:32 View deployment

Merge branch 'main' into fix/def_declaration

264c7e8

vercel Bot deployed to Preview May 1, 2026 20:23 View deployment

Merge branch 'main' into fix/def_declaration

d3875e6

vercel Bot deployed to Preview May 4, 2026 20:06 View deployment

Merge branch 'main' into fix/def_declaration

3475ee0

vercel Bot deployed to Preview May 5, 2026 21:11 View deployment

Merge branch 'main' into fix/def_declaration

5f9c696

vercel Bot deployed to Preview May 6, 2026 22:19 View deployment

Merge branch 'main' into fix/def_declaration

b8b9669

vercel Bot deployed to Preview May 7, 2026 21:21 View deployment

Merge branch 'main' into fix/def_declaration

4a8eb4b

vercel Bot deployed to Preview May 8, 2026 18:10 View deployment

manzt mentioned this pull request May 11, 2026

Add failing tests for set-comprehension and forward-ref scope bugs Shamik-07/marimo#1

Merged

manzt reviewed May 11, 2026

View reviewed changes

Merge pull request #1 from marimo-team/fix/def_declaration-tests

bf12fd1

Add failing tests for set-comprehension and forward-ref scope bugs

vercel Bot deployed to Preview May 11, 2026 21:41 View deployment

Shamik-07 added 4 commits May 11, 2026 18:23

fix: renaming SetComprehension to SetComprehensionExpression as Lezer…

7c634fa

…'s python grammar emits this. fix: remove global from the position sensive scopes as python resolves module names lazily.

fix: skipping class scopes after a function boundary in getscopechain…

345e6e4

… function.

test: fixing the code comments in commands test.

2853f19

Merge branch 'main' into fix/def_declaration

b2551f5

vercel Bot deployed to Preview May 11, 2026 22:58 View deployment

Shamik-07 requested a review from manzt May 11, 2026 23:06

docs: renaming pnpm build watch column name as production esque.

9638f27

vercel Bot deployed to Preview May 11, 2026 23:26 View deployment

Merge branch 'main' into fix/def_declaration

1a458cd

vercel Bot deployed to Preview May 12, 2026 21:31 View deployment

Conversation

Shamik-07 commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

📋 Pre-Review Checklist

✅ Merge Checklist

Uh oh!

vercel Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shamik-07 commented Apr 24, 2026

Uh oh!

Shamik-07 commented Apr 24, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Shamik-07 commented May 4, 2026

Uh oh!

manzt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shamik-07 commented May 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Shamik-07 commented Apr 24, 2026 •

edited

Loading

vercel Bot commented Apr 24, 2026 •

edited

Loading

github-actions Bot commented Apr 24, 2026 •

edited

Loading

manzt left a comment •

edited

Loading