Fix EOF SyntaxError diagnostics - #8429
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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] lib: cpython/Lib/tokenize.py dependencies:
dependent tests: (150 tests)
[ ] 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) Legend:
|
|
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
|
@chestnut1717 Could you please check lint issue? This is cargo fmt issue https://github.com/RustPython/RustPython/actions/runs/31006561252/job/93106068014?pr=8429 You will see below when you click "prek run" in the link: --- a/crates/compiler/src/lib.rs
+++ b/crates/compiler/src/lib.rs
@@ -5287,7 +5287,6 @@ pub fn _compile_symtable(
if let Some(error) =
post_parse_source_error(&source_file, ast.tokens(), &CompileOpts::default())
{
-
return Err(error);
}
let ast = ast.into_syntax(); |
7ad1675 to
e47a40a
Compare
|
I checked it and the reason why CI lint failed is an "one blank line" in |
* Fix EOF SyntaxError diagnostics * Fix Scope EOF SyntaxError location conversion * Fix Additional Unexpected Test * Docs Add Annotation source_location_in_code_points()
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