feat: Add line highlighting for syntax errors #7047
Conversation
SamarJyoti496
requested review from
Light2Dark,
dmadisetti and
manzt
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| error = MarimoImportStarError(msg=str(e)) | ||
| else: | ||
| error = MarimoSyntaxError(msg="\n".join(syntax_error)) | ||
| error = MarimoSyntaxError( |
There was a problem hiding this comment.
if there are any tests that check the a MarimoSyntaxError was created, can you also check that lineno was also added
frontend/src/core/cells/cells.ts
Outdated
| // so it prevents downstream recomputations. | ||
| const tracebackStringAtom = atom<string | undefined>((get) => { | ||
| // Single flat atom - proper reactivity | ||
| return atom((get: any) => { |
There was a problem hiding this comment.
this now runs on every notebook change. can we instead at least do keep the intermediate atom that computes cellDataAtom (just returning const data = notebook.cellRuntime[cellId])
|
@SamarJyoti496 you will also need to run |
I have updated that one. Some backend test cases failed, but I'm not sure whether they're related to my changes. I think they're not as they had run successfully before. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds line number tracking to syntax errors in Marimo. The changes enable syntax errors to capture and display the line number where the error occurred, improving debugging capabilities.
- Added an optional
linenofield to theMarimoSyntaxErrorclass with a default value ofNone - Updated the runtime to extract and pass line numbers from Python
SyntaxErrorexceptions - Enhanced the frontend to display syntax error line numbers in the traceback interface
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| marimo/_messaging/errors.py | Added optional lineno field to MarimoSyntaxError struct |
| marimo/_runtime/runtime.py | Extracts lineno from Python SyntaxError and passes it to MarimoSyntaxError |
| frontend/src/core/cells/cells.ts | Refactored createTracebackInfoAtom to handle both runtime and syntax errors, adding traceback info for syntax errors with line numbers |
| packages/openapi/api.yaml | Updated API schema to include optional lineno field for MarimoSyntaxError |
| packages/openapi/src/api.ts | Generated TypeScript types reflecting the new lineno field |
| tests/_messaging/test_messaging_errors.py | Added tests for MarimoSyntaxError with and without lineno |
| tests/_messaging/test_cell_output.py | Updated expected serialization to include lineno: None field |
| tests/_runtime/test_runtime.py | Added assertions to verify lineno is captured correctly in syntax errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lineno = getattr(e, "lineno", None) | ||
| if isinstance(e, ImportStarError): | ||
| error = MarimoImportStarError(msg=str(e)) | ||
| else: | ||
| error = MarimoSyntaxError(msg="\n".join(syntax_error)) | ||
| error = MarimoSyntaxError( | ||
| msg="\n".join(syntax_error), lineno=lineno | ||
| ) |
There was a problem hiding this comment.
The lineno is extracted from the exception but not passed to MarimoImportStarError. Since ImportStarError is a subclass of SyntaxError (as shown in marimo/_ast/errors.py), it should also include the line number information. Consider adding a lineno field to MarimoImportStarError or documenting why it's intentionally excluded.
| const tracebackStringAtom = atom<string | undefined>((get) => { | ||
| const notebook = get(notebookAtom); | ||
| const data = notebook.cellRuntime[cellId]; | ||
| //use existing cellRuntimeAtom for intermediate computation |
There was a problem hiding this comment.
Corrected comment formatting: 'use' should be capitalized and there should be a space after '//'.
| //use existing cellRuntimeAtom for intermediate computation | |
| // Use existing cellRuntimeAtom for intermediate computation |
|
@SamarJyoti496 im going to push some changes for some tests and a bit of cleanup |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2 participants
📝 Summary
Syntax Errors now show red line highlighting in the editor matching the existing behavior for runtime errors.
Fixes #6979
🔍 Description of Changes
Problem: Syntax errors displayed error messages but didn't highlight the problematic line in red, unlike runtime errors.
Solution: Extract line numbers from Python's
SyntaxError.linenoattribute and feed them into the existing traceback.📋 Checklist
Note
Extracts
linenofor Python syntax/import-star errors in the runtime, exposes it via API, and updates frontend traceback logic to highlight exact error lines, with comprehensive tests.linenotoMarimoSyntaxErrorandImportStarError; capture fromSyntaxError.linenoin runtime.packages/openapi) to includelinenoonMarimoSyntaxErrorandImportStarError.createTracebackInfoAtomto usecellRuntimeAtomand parsesyntax/import-starerrors withlinenointoTracebackInfo(skip when queued/running).cellRuntimeoverrides.createTracebackInfoAtomfor testing.linenopropagation (including 0, null, multiline) and new traceback behavior; update serialization expectations to includelineno.Written by Cursor Bugbot for commit 9da2233. This will update automatically on new commits. Configure here.