Fix EOF SyntaxError diagnostics - #8429
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe compiler now reports UTF-32 source positions and converts terminal-backslash continuation errors to ChangesSyntax error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FileExecution
participant Compiler
participant SyntaxErrorConstructor
participant sys_excepthook
participant write_syntaxerror
FileExecution->>Compiler: provide decoded source
Compiler->>SyntaxErrorConstructor: report source location and EOF diagnostic
SyntaxErrorConstructor->>sys_excepthook: create SyntaxError
sys_excepthook->>write_syntaxerror: format traceback-less EOF error
write_syntaxerror-->>sys_excepthook: omit terminal-backslash caret
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_exceptions.py (TODO: 22) dependencies: dependent tests: (no tests depend on exception) [x] test: cpython/Lib/test/test_eof.py dependencies: dependent tests: (no tests depend on eof) [x] lib: cpython/Lib/tokenize.py dependencies:
dependent tests: (150 tests)
Legend:
|
| let SyntaxErrorInfo { msg, narrow_caret } = syntax_error_info; | ||
| let unterminated_triple_quoted_string = | ||
| msg.starts_with("unterminated triple-quoted string literal"); | ||
| let unexpected_eof_error = msg == "unexpected EOF while parsing"; |
There was a problem hiding this comment.
@ShaharNaveh is this currently the best way to detect specific kind of error?
There was a problem hiding this comment.
atm, yes:(
Either ruff don't expose the exact reason, or we need to do a major refactor of how we propagate the errors from the compiler
There was a problem hiding this comment.
how could it be detected after the major refactor? we can't do this message matching forever 😂
There was a problem hiding this comment.
ig you're right...
maybe we can't escape it and just have our own ruff fork/have a rustpython-ast crate
There was a problem hiding this comment.
I am sorry if the last comment was confusing. I'd like to ask what kind of refactor do you have in mind?
|
you seem to fix more tests! please check CI result about failing tests e.g. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/compiler/src/lib.rs (2)
252-269: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the shared diagnostic construction.
Both branches repeat
NormalizedParseDiagnostic::new(..., loc, loc). Extract the diagnostic error and location first. Call the constructor once.As per coding guidelines, when branches differ only in a value but share common logic, extract the differing value first, then call the common logic once.
Suggested refactor
- if !matches!(mode, Mode::Eval) - && terminal_backslash == Some(error.location.start().to_usize()) - { - let loc = source_line_end_location(source_file, error.location.start()); - return Some(NormalizedParseDiagnostic::new( - parser::ParseErrorType::OtherError("unexpected EOF while parsing".to_owned()), - loc, - loc, - )); - } - let loc = source_location(source_file, error.location.start() + TextSize::from(1)); - return Some(NormalizedParseDiagnostic::new( - error.error.clone(), - loc, - loc, - )); + let (diagnostic_error, loc) = if !matches!(mode, Mode::Eval) + && terminal_backslash == Some(error.location.start().to_usize()) + { + ( + parser::ParseErrorType::OtherError("unexpected EOF while parsing".to_owned()), + source_line_end_location(source_file, error.location.start()), + ) + } else { + ( + error.error.clone(), + source_location(source_file, error.location.start() + TextSize::from(1)), + ) + }; + return Some(NormalizedParseDiagnostic::new( + diagnostic_error, + loc, + loc, + ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/compiler/src/lib.rs` around lines 252 - 269, Refactor the diagnostic handling around the terminal-backslash check so each branch computes only its differing error and location values. Then call NormalizedParseDiagnostic::new once with the selected error and location, preserving the existing EOF-specific error/location and default error/location behavior.Source: Coding guidelines
252-263: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle CR-only physical lines in line-range diagnostics.
Python accepts LF, CRLF, and CR as physical-line terminators.
source_line_end_locationcurrently splitssource_text()on\n, so CR-only lines are counted as one line; line-based diagnostics can show the wrong ending column instead of using a newline-aware full-line API such assource_file.to_source_code().full_line_str(...). Add CR-only and mixed line-ending coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/compiler/src/lib.rs` around lines 252 - 263, Update source_line_end_location and its callers to determine physical line boundaries using the source file’s newline-aware full-line API, preserving correct line ranges for LF, CRLF, CR-only, and mixed line endings. Add coverage for CR-only and mixed newline inputs, including the terminal-backslash diagnostic path around NormalizedParseDiagnostic::new.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/compiler/src/lib.rs`:
- Around line 252-269: Refactor the diagnostic handling around the
terminal-backslash check so each branch computes only its differing error and
location values. Then call NormalizedParseDiagnostic::new once with the selected
error and location, preserving the existing EOF-specific error/location and
default error/location behavior.
- Around line 252-263: Update source_line_end_location and its callers to
determine physical line boundaries using the source file’s newline-aware
full-line API, preserving correct line ranges for LF, CRLF, CR-only, and mixed
line endings. Add coverage for CR-only and mixed newline inputs, including the
terminal-backslash diagnostic path around NormalizedParseDiagnostic::new.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b78b4251-e883-41b3-92bc-830b4f846101
⛔ Files ignored due to path filters (2)
Lib/test/test_exceptions.pyis excluded by!Lib/**Lib/test/test_tokenize.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/compiler/src/lib.rs
Summary
Fix CPython compatibility for EOF-related
SyntaxErrordiagnostics and remove the fiveexpectedFailuremarkers fromtest_eof.pyThis aligns RustPython with CPython for:
str, UTF-8bytes, UTF-8 BOMbytes, and latin-1bytes\Before / after (
exec("ä = 5\\")):offsetunexpected EOF while parsingunexpected character after line continuation characterunexpected EOF while parsingThe previous behavior also counted UTF-8 bytes instead of Python characters for
SyntaxError.offset, included an extra newline in unterminated triple-quoted-stringSyntaxError.text, and displayed a caret for a file containing only\.Approach
x = 1 \ q \.end_offsetto-1.SyntaxError.textonly for unterminated triple-quoted-string diagnostics.decode_source_bytes()for script-file execution so BOMs and PEP 263 encoding cookies are handled consistently withcompile(),eval(), andexec().exec(chr(92))continue through the normal traceback path.Testing
@unittest.expectedFailuredecorators fromtest_eof; assertions and test inputs are unchanged.test_eof,test_syntax,test_cmd_line_script, andtest_tracebackall pass: 470 tests total.rustpython-compilerunit tests pass: 13 tests.str, UTF-8 bytes, BOM-prefixed bytes, and latin-1 bytes;unfulfilled_lint_expectationsoutside this change.Summary by CodeRabbit
Bug Fixes