Align all remaining error messages with CPython - #7993
JamesClarke7283 wants to merge 27 commits into
Conversation
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_generators.py (TODO: 7) dependencies: dependent tests: (no tests depend on generator) [ ] test: cpython/Lib/test/test_unicodedata.py (TODO: 28) dependencies: dependent tests: (no tests depend on unicode) [ ] lib: cpython/Lib/json dependencies:
dependent tests: (13 tests)
[ ] test: cpython/Lib/test/test_str.py (TODO: 4) dependencies: dependent tests: (no tests depend on str) [ ] test: cpython/Lib/test/test_posix.py (TODO: 3) dependencies: dependent tests: (102 tests)
[x] lib: cpython/Lib/ssl.py dependencies:
dependent tests: (54 tests)
[ ] test: cpython/Lib/test/test_cmd_line.py (TODO: 24) dependencies: dependent tests: (no tests depend on cmd_line) [x] lib: cpython/Lib/pdb.py dependencies:
dependent tests: (1 tests)
[x] lib: cpython/Lib/inspect.py dependencies:
dependent tests: (97 tests)
[x] lib: cpython/Lib/plistlib.py dependencies:
dependent tests: (25 tests)
[x] lib: cpython/Lib/codecs.py dependencies:
dependent tests: (161 tests)
[x] test: cpython/Lib/test/test_format.py dependencies: dependent tests: (no tests depend on format) [x] lib: cpython/Lib/pydoc.py dependencies:
dependent tests: (5 tests)
[ ] test: cpython/Lib/test/test_syntax.py (TODO: 2) dependencies: dependent tests: (no tests depend on syntax) [x] lib: cpython/Lib/contextlib.py dependencies:
dependent tests: (83 tests)
[x] test: cpython/Lib/test/test_context.py (TODO: 7) dependencies: dependent tests: (11 tests) [x] lib: cpython/Lib/socket.py dependencies:
dependent tests: (101 tests)
[x] test: cpython/Lib/test/test_select.py (TODO: 3) dependencies: dependent tests: (109 tests)
[ ] test: cpython/Lib/test/test_pyrepl (TODO: 21) dependencies: dependent tests: (no tests depend on pyrepl) [x] lib: cpython/Lib/threading.py dependencies:
dependent tests: (166 tests)
[x] lib: cpython/Lib/codeop.py dependencies:
dependent tests: (105 tests)
[x] lib: cpython/Lib/hashlib.py dependencies:
dependent tests: (146 tests)
[ ] test: cpython/Lib/test/test_bytes.py (TODO: 15) dependencies: dependent tests: (no tests depend on bytes) [x] test: cpython/Lib/test/test_resource.py (TODO: 3) dependencies: dependent tests: (4 tests)
[x] test: cpython/Lib/test/test_dict.py (TODO: 3) dependencies: dependent tests: (no tests depend on dict) [ ] test: cpython/Lib/test/test_termios.py (TODO: 1) dependencies: dependent tests: (8 tests)
[ ] lib: cpython/Lib/asyncio dependencies:
dependent tests: (7 tests)
[x] test: cpython/Lib/test/test_marshal.py (TODO: 3) dependencies: dependent tests: (25 tests)
[x] lib: cpython/Lib/pty.py dependencies:
dependent tests: (4 tests)
[x] test: cpython/Lib/test/test_coroutines.py (TODO: 14) dependencies: dependent tests: (7 tests) [ ] test: cpython/Lib/test/test_extcall.py (TODO: 7) dependencies: dependent tests: (no tests depend on extcall) [x] lib: cpython/Lib/enum.py dependencies:
dependent tests: (16 tests)
[ ] lib: cpython/Lib/unittest dependencies:
dependent tests: (408 tests)
[ ] test: cpython/Lib/test/test_sys.py (TODO: 7) dependencies: dependent tests: (241 tests)
[ ] test: cpython/Lib/test/test_mmap.py (TODO: 24) dependencies: dependent tests: (2 tests)
[ ] lib: cpython/Lib/sqlite3 dependencies:
dependent tests: (2 tests)
[ ] test: cpython/Lib/test/test_builtin.py (TODO: 14) dependencies: dependent tests: (no tests depend on builtin) [ ] test: cpython/Lib/test/test_structseq.py dependencies: dependent tests: (no tests depend on structseq) [x] lib: cpython/Lib/ast.py dependencies:
dependent tests: (151 tests)
[x] lib: cpython/Lib/struct.py dependencies:
dependent tests: (179 tests)
[x] test: cpython/Lib/test/test_asyncgen.py (TODO: 3) dependencies: dependent tests: (no tests depend on asyncgen) [x] lib: cpython/Lib/lzma.py dependencies:
dependent tests: (102 tests)
[x] lib: cpython/Lib/datetime.py dependencies:
dependent tests: (67 tests)
[x] lib: cpython/Lib/types.py dependencies:
dependent tests: (57 tests)
[x] test: cpython/Lib/test/test_descr.py (TODO: 30) dependencies: dependent tests: (no tests depend on descr) [x] test: cpython/Lib/test/test_cmd_line_script.py (TODO: 13) dependencies: dependent tests: (no tests depend on cmd_line_script) Legend:
|
|
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:
📝 WalkthroughWalkthroughTightens symbol validation (rejecting ChangesSymbol validation and parse error alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
4718-4729: ⚡ Quick winExtract duplicated type parameter validation into a helper method.
The type parameter
__debug__validation logic is duplicated identically in both function and class compilation paths. Extract this into a shared helper to improve maintainability and prevent future divergence.♻️ Proposed refactor to eliminate duplication
Add a helper method to the compiler struct:
fn validate_type_params_no_debug(&self, type_params: Option<&ast::TypeParams>) -> CompileResult<()> { if let Some(params) = type_params { for tp in ¶ms.type_params { let tp_name = match tp { ast::TypeParam::TypeVar(t) => &t.name, ast::TypeParam::TypeVarTuple(t) => &t.name, ast::TypeParam::ParamSpec(t) => &t.name, }; if tp_name.as_str() == "__debug__" { return Err(self.error(CodegenErrorType::Assign("__debug__"))); } } } Ok(()) }Then replace the duplicated blocks with a single call in each location:
- // Reject `def f[__debug__](): ...` type parameter (mirrors class defs). - if let Some(params) = type_params { - for tp in ¶ms.type_params { - let tp_name = match tp { - ast::TypeParam::TypeVar(t) => &t.name, - ast::TypeParam::TypeVarTuple(t) => &t.name, - ast::TypeParam::ParamSpec(t) => &t.name, - }; - if tp_name.as_str() == "__debug__" { - return Err(self.error(CodegenErrorType::Assign("__debug__"))); - } - } - } + // Reject `def f[__debug__](): ...` type parameter (mirrors class defs). + self.validate_type_params_no_debug(type_params)?;Apply the same simplification in the class compilation path (lines 5307-5318).
Also applies to: 5307-5318
🤖 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/codegen/src/compile.rs` around lines 4718 - 4729, Extract the duplicated "__debug__" type-parameter check into a new helper on the compiler impl (e.g. fn validate_type_params_no_debug(&self, type_params: Option<&ast::TypeParams>) -> CompileResult<()>) that iterates params.type_params, extracts the name from ast::TypeParam variants, and returns Err(self.error(CodegenErrorType::Assign("__debug__"))) if a name equals "__debug__", otherwise Ok(()). Replace the duplicated validation blocks in the function compilation path and the class compilation path with a single call to this helper, keeping existing types (ast::TypeParams, CompileResult) and error construction unchanged.
🤖 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.
Inline comments:
In `@crates/compiler/src/lib.rs`:
- Around line 986-1025: The helper parenthesized_param_message currently only
inspects the current physical line, missing multiline cases; update it to scan
backwards from line_start through previous lines to find a preceding "def " or
"lambda" token and then evaluate parenthesis depth across that full span.
Specifically, for "def " (use def_idx search but on the concatenated slice from
the found def position to the error start) walk characters from that def
position counting '(' and ')' to determine if the error position is inside the
function parameter list (return the CPython message if inside), and for "lambda"
search backwards similarly to find a lambda token without a ':' between it and
the error (consider spanning newlines). Keep the existing return messages and
reuse parenthesized_param_message, but replace single-line checks with these
backward-scanning, depth-aware checks so multiline parameter lists are detected.
- Around line 1397-1419: chunk_has_bare_assignment currently treats any
alphanumeric LHS as an identifier; update it to reject literals/keywords by
first checking the trimmed lhs start and exact content: ensure lhs is non-empty
and its first byte is ASCII alphabetic or b'_' (reject if it starts with a
digit, quote, '(' '[' '{', etc.), then ensure the whole lhs matches the
identifier shape (bytes().all(...) as you already do) and is not one of the
Python literal/keyword tokens like "True", "False", or "None" (use a small
static set and compare lhs.eq_ignore_ascii_case or exact match as appropriate).
Return false for those cases so chunk_has_bare_assignment only returns true for
real identifiers.
- Around line 1682-1698: The current is_in_case_pattern only checks the current
line for a "case " header so it misses multi-line case headers; change the logic
to scan backwards from range.start() to find the nearest preceding non-empty
line whose trimmed start begins with "case " (use the existing start and
line_start variables as anchors) and treat the location as being in a case
pattern if such a "case " header exists with indentation (case_indent) less than
the current line and there is no '=' token between the header start and the
range.start(); update the code around is_in_case_pattern, keeping references to
start, line_start, rest, and case_indent, to iterate previous lines (instead of
only checking source[line_start..]) and ensure multiline headers are recognized
before falling back to the match-search logic.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 4718-4729: Extract the duplicated "__debug__" type-parameter check
into a new helper on the compiler impl (e.g. fn
validate_type_params_no_debug(&self, type_params: Option<&ast::TypeParams>) ->
CompileResult<()>) that iterates params.type_params, extracts the name from
ast::TypeParam variants, and returns
Err(self.error(CodegenErrorType::Assign("__debug__"))) if a name equals
"__debug__", otherwise Ok(()). Replace the duplicated validation blocks in the
function compilation path and the class compilation path with a single call to
this helper, keeping existing types (ast::TypeParams, CompileResult) and error
construction unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c64ba8a6-f09d-45ff-8156-2a8997556f5b
⛔ Files ignored due to path filters (4)
Lib/test/test_genexps.pyis excluded by!Lib/**Lib/test/test_named_expressions.pyis excluded by!Lib/**Lib/test/test_patma.pyis excluded by!Lib/**Lib/test/test_syntax.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/compiler/src/lib.rscrates/vm/src/stdlib/_ast.rscrates/vm/src/vm/vm_new.rs
4383c21 to
a84b1a4
Compare
|
@coderabbitai review The duplicated |
|
✅ Actions performedReview triggered.
|
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
tysm for working on this!
On one hand I can't argue with the value this brings, and on the other it feels a bit ridiculous because we are just reimplementing the lexer & parser atp.
I'll wait for @youknowone inputs
05843ed to
19195ed
Compare
|
@ShaharNaveh @youknowone is this okay to merge? |
19195ed to
669fc00
Compare
|
I am sorry about late review of this. To be honest, I felt hard to review this changes and couldn't justify reimplementing the lexer & parser for this. |
now its not reimplementing the parser, just doing the error messages |
|
Post-merge self-review of the remaining diff ( 1. Error priority. The def f():
global x
print(x)
nonlocal x
# CPython: name 'x' is used prior to nonlocal declaration (line 4)
# before fix: name 'x' is nonlocal and global (line 2)
def f():
x = 1
def g():
nonlocal x
x = 2
global x
# CPython: name 'x' is assigned to before global declaration
# before fix: name 'x' is nonlocal and globalFix: moved the 2. Directive location overwrite. def f():
global x
global x
nonlocal x
# CPython: error at line 2; before fix: line 3Fix: only store the location when Validation: 785/785 codegen unit tests pass (added tests for both behaviors), clippy/rustfmt clean, |
|
The two fixes above are now pushed in d944e0a ( |
Why this touches
|
Runtime error-message alignment (d1aad65, 5605a25, 94c901e)Following the parser-side alignment earlier in this PR, these commits complete the runtime error messages: VM, builtins, protocol layers, native stdlib modules, and the format-string machinery — all verified message-by-message against the CPython 3.14.7 sources, with no parser changes. How it was verified
What changedArity errors — new helpers in
A CPython quirk reproduced faithfully: keyword rejections use class-qualified names ( Constructor argument errors for Semantic messages — concat errors with Unraisable reports — Format strings — PEP 649 — attached async for — The stdlib commit (d1aad65) continues the module-by-module alignment from 0e12017 across the native bindings (sqlite3, array, binascii, csv, fcntl, json, locale, lzma, math, mmap, openssl, pystruct, resource, select, socket, ssl, termios, zlib) plus the shared cformat/marshal strings. Test impact
|
|
@JamesClarke7283 can you please fix the merge/rebase |
On it |
fca3d8b to
684929d
Compare
Rebased onto current mainHistory is now linear on top of upstream/main (no merge commits), and the conflicts against the recent landings were resolved as follows:
The only remaining test_socket failures are UDPLITE ones — this kernel has no |
|
@JamesClarke7283 can you please run: assuming that |
Continues RustPython#7928/RustPython#7933/RustPython#7988. Translates many more ruff ParseErrorType variants to CPython's exact wording in CompileError::from_ruff_parse_error, and routes ast.parse() / compile(PyCF_ONLY_AST) through the same path so those messages match too (previously they leaked raw ruff strings). Adds a few codegen/symtable checks. Covered: aug-assign/delete/set/dict/f-string/t-string targets; "cannot use {kind} as import target"; string-prefix incompatibility and "invalid character 'X' (U+XXXX)"; parenthesized def/lambda params; missing default/argument value; dict ':' / value syntax; "'elif' block follows an 'else' block"; raise-from; comprehension 'if'; ternary statement keywords; match "case ... as <target>" -> "cannot use {kind} as pattern target" and "case ... as _"; __debug__ as def/class/type-param/except name; "name 'x' is nonlocal and global"; generic type-parameter wording. Lib/test: drop the now-passing "# TODO: RUSTPYTHON; Wrong error message" doctest markers and @expectedfailure decorators. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`eval` calls the host `print` function, so rust-lld reported it as an undefined symbol and the wasm32-unknown-unknown build failed to link (`kv_get`/`kv_put` are unused, so they were GC'd and did not error). Annotate the `extern "C"` block with `#[link(wasm_import_module = "env")]` so the linker emits the host functions as wasm imports from the `env` module, matching the wasmer host runtime in wasm-runtime/src/main.rs. Verified: `cargo build` (the CI "check wasm32-unknown without js" step) now links and produces the .wasm. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
test_dictcomps.test_illegal_assignment, test_fstring.test_invalid_string_prefixes and test_unicode_identifiers.test_invalid now pass thanks to this PR's error-message alignment, so their @unittest.expectedFailure markers caused "unexpected success" failures in CI. Remove the obsolete markers (same cleanup already applied to test_syntax/test_genexps/test_named_expressions/test_patma). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- Report "Invalid star expression" for bare leading `*` in set/dict displays and non-call parenthesised groups (`{*}`, `(*,)`)
- Collapse double-comma in dict/set/list displays (`{1:2,, 3}`, `[1,, 2]`) to "invalid syntax"
- Add is_bare_star_first_in_group helper
- Drop stale "Is this intended to be part of the string?" uppercase-message entry
pdb's `_exec_in_closure` wraps the debugger input in a generated `nonlocal <var>` scope, so a user's `global g` conflicts with it. CPython rejects that with "name 'g' is nonlocal and global", which pdb catches to fall back to a plain exec. Now that the symbol table raises the same error, test_pdb_closure produces CPython's output and the `+EXPECTED_FAILURE` marker inverts it into a failure. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Follows the SyntaxError work with the non-syntax `wrong error message`
markers. Each message was compared against CPython 3.14.7 directly.
- structseq: accept CPython's second `dict` argument, reporting "got
duplicate or unexpected field name(s)" when a key duplicates a
positional field or names none, and raise "readonly attribute" from
the field descriptors as member descriptors do.
- posix_spawn: validate `scheduler` in the body so a wrong type says
"scheduler must be a tuple or None".
- socket.sendto: bind by hand to report "sendto() takes 2 or 3 arguments
(N given)" and "socket.sendto() takes no keyword arguments".
- bz2: report libbzip2's "Invalid data stream", and make a decompressor
unusable after a failure instead of resuming from inconsistent state.
- import: resolve `__import__` against the running frame's builtins, so
`exec(code, {"__builtins__": {}})` raises ImportError, and pass None
rather than () as the from-list of a plain import.
- symboltable: name the variable as written, not mangled, in
"assignment expression cannot rebind comprehension iteration variable".
- _pydatetime: raise the message CPython's C _datetime uses when
subtracting a naive and an aware datetime; the pure-Python module is
the only implementation here.
test_hashlib and test_ast stay marked: both need the callee's name, or
non-string keyword keys, to reach argument binding, which is a change to
the calling convention rather than to a message.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The authorizer denied every statement. SQLite passes NULL for the arguments an action does not use — all four are NULL for SQLITE_SELECT — and `ptr_to_str` raised MemoryError on NULL, which the trampoline swallowed as SQLITE_DENY before the callback ever ran. Those arguments now reach the callback as None, matching CPython's callback trace. That was also the reason the denial message differed: RustPython stopped at the non-column SQLITE_SELECT check, which SQLite reports as the generic "not authorized", where CPython reached the column check and got "access to t2.c1 is prohibited". Also: - Bound the argument count before handing it to SQLite, so create_function and create_window_function report "'narg' must be between -1 and 1000, not -100" instead of a generic creation failure. - Raise ValueError for every invalid `autocommit`, without the ", not X" suffix CPython does not use; a non-integer raised TypeError before. A working authorizer makes the "concurrent mutation" tests reachable, and they hang: they call back into the connection from inside a callback, which deadlocks on the connection mutex. Skipped with that reason until the locking is re-entrant; CI builds with `sqlite`, so leaving them running would hang the suite. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
CPython's clinic-generated signatures name the callee and the position of
`data`, and check a duplicated argument before an unknown keyword and an
unknown keyword before the data/string conflict. The generic binder knows
none of that, so the constructors bind by hand:
hashlib.md5(b'', data=b'') argument for openssl_md5() given by name
('data') and position (1)
hashlib.md5(_=None) openssl_md5() got an unexpected keyword
argument '_'
`hashlib.blake2b` resolves to `_blake2.blake2b` rather than the openssl
constructor, so the two share an implementation that takes the name to
report.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Continues the runtime error-message alignment from 0e12017 across the native stdlib modules: argument validation for the sqlite3, array, binascii, csv, fcntl, json, locale, lzma, math, mmap, openssl, pystruct, resource, select, socket, ssl, termios and zlib bindings, plus the shared %-formatting (cformat) and marshal format strings in compiler-core. Each message was matched against the CPython 3.14.7 sources, including argument-by-name wording and the order CPython validates in. Assisted-by: Claude:Claude Opus 5
Completes the runtime error-message alignment for the VM, builtins and
protocol layers, all verified against CPython 3.14.7:
- Arity errors: new helpers in function::argument mirror CPython's
three message styles (_PyArg_CheckPositional, METH_O/noargs wrappers
and the clinic forms). Builtin functions (abs/chr/len/map/pow/round/
format/filter/...) and methods across dict, list, set, tuple, str,
bytes, bytearray, int, float, slice, range and property now report
CPython's exact wording, including class-qualified names for keyword
rejections but bare names for positional counts. Native-method arity
errors no longer count the receiver, and the generic binder renders
exact/singular forms when min == max.
- Constructor argument errors for str/bytes/bytearray/int/float/set/
range/slice/complex/enumerate/type, including duplicate name-and-
position and missing-required-argument wording.
- Semantic messages: concat errors use tp_name semantics (double quotes,
module-qualified names), sequence-repeat reports "can't multiply
sequence by non-int", str.join reports "can only join an iterable",
__index__ conversions, attribute set/delete errors (read-only and
no-__dict__ suffixes), NoneType immutability, raise vs gen.throw
wording, unbound-method and wrapper-descriptor messages, and the
str.translate table "must be" wording.
- Unraisable reports: __del__ failures report "Exception ignored while
calling deallocator <repr>" and generator-close failures report
"Exception ignored while closing generator <repr>" with the synthetic
GeneratorExit error carrying a traceback.
- Format strings: "Single '{'/'}' encountered", "unmatched '{' in
format spec", "Unknown conversion specifier" (validated at format
time so _string.formatter_parser stays lenient), and
"Invalid format specifier '<spec>' for object of type '<T>'".
- PEP 649: attached __annotate__ functions get the
"<outer>.__annotate__" qualname (gh-137814).
- async for: GET_AITER validates __aiter__/__anext__ presence and
GET_ANEXT awaits via _PyCoro_GetAwaitableIter with from-cause errors.
capi callers updated for the new method signatures.
Assisted-by: ZCode:GLM-5.3
Assisted-by: Claude:Claude Opus 5
Removes 37 stale TODO: RUSTPYTHON markers across 26 test files now that the corresponding error messages match CPython 3.14: async-for errors (test_coroutines), str()/Template/concat errors (test_str, test_tstring, string_tests), unraisable reports (test_exceptions, test_generators), __annotate__ qualnames (test_type_annotations), constructor arity (test_range, test_posix, test_sqlite3, test_struct), find-family messages (test_bytes), attribute errors (test_class, test_descr, test_descrtut), exec/eval arguments (test_pdb, test_extcall), map (test_itertools), marshal readers, lzma filter specs, enum, json scanstring (C variant only; the pure-Python scanner still lacks the OverflowError, so that variant keeps a scoped marker), mmap resize and pdb's exec/eval doctests. Assisted-by: ZCode:GLM-5.3
- posix: qualify the PyTuple path in the posix_spawn scheduler check so
macOS (where the bare import is not in scope) compiles.
- coroutine: gate the Radium import on not(threading); without the
feature lasti is a Cell and its load() comes from the trait. Fixes the
wasm build and miri.
- marshal (compiler-core): drop the duplicate NullObject Display arm
left by the conflict resolution and the now-unnecessary usize cast;
both were -Dwarnings clippy errors.
- socket: getaddrinfo's IDNA path no longer clones the host str
(redundant_clone), passing it by value.
- str: port upstream's char-aware count() body behind this branch's
FuncArgs front - an empty needle is counted in characters (chars + 1),
not encoded byte positions, so "가나다".count("") is 4 again. Also
drops a duplicate #[inline] and a redundant #[must_use] that tripped
-Dwarnings, and py_split_str takes an #[expect(too_many_arguments)]
like its anystr counterpart.
Verified with the CI commands: workspace clippy -Dwarnings with the CI
feature set and excludes, the sandbox-mode checks, the snippets suite
(builtin_str green), and test_str/test_marshal/test_socket (only the
UDPLITE tests fail here - this kernel has no IPPROTO_UDPLITE).
Assisted-by: ZCode:GLM-5.3
- socket: getaddrinfo's port accepts str, bytes and bytearray service names like CPython's setipaddr; anything else (floats, lists) raises OSError "Int or String expected". The int shortcut (decimal string) stays. This un-breaks asyncio's create_connection, whose resolution path fed the service name through getaddrinfo on all three CI operating systems. - socket: the PyInt import moved into the linux-only sendmsg_afalg, so macOS/Windows builds no longer see an unused import under -Dwarnings. - mmap: the flags field is now cfg(linux/netbsd), matching its only reader (the mremap expansion check), so macOS clippy no longer flags it as never read. - test_threading: test_join_daemon_thread_in_finalization stays an expected failure - it passed once on CI runners but fails deterministically here because daemon-thread shutdown ordering differs; the marker documents the dependency. Assisted-by: ZCode:GLM-5.3
- mmap: the re-bound `flags` in py_new is only stored on linux/netbsd (the mremap expansion check is its only reader), so allow the unused variable on other unixes. - os: dir_fd_and_fd_invalid is only called from the unix chown; allow dead_code off unix instead of gating the definition. With these, clippy -Dwarnings passes with the CI feature set on every platform: the previous run had only these two jobs failing. Assisted-by: ZCode:GLM-5.3
- array: the intermediate int/unsigned-int range conversions are no-ops on Windows (c_long is i32, c_ulong is u32 there); allow the useless-conversion lint there while keeping CPython's two-step error order on LP64 platforms (verified against CPython for H/I overflow, negative, and 2**40 inputs). - socket: PyIter moved from the module import list into the unix-only sendmsg, removing the unused import on Windows. Assisted-by: ZCode:GLM-5.3
c_ulong is u32 there, which is the Self of the impl, so clippy's use_self fires; on LP64 platforms it stays raw::c_ulong to keep CPython's "Python int too large to convert to C unsigned long" step. Assisted-by: ZCode:GLM-5.3
Reviewing the whole PR against CPython 3.14 surfaced a batch of
methods whose argument errors still went through the generic binder
("Expected type 'int' but 'float' found."), plus three re-entrancy
safety regressions that the new conversion order exposed:
- bytes/bytearray expandtabs, hex(bytes_per_sep), zfill (bytes) and
the padding family (center/ljust/rjust width) now convert with
PyNumber_AsSsize_t semantics ("'float' object cannot be
interpreted as an integer", OverflowError at the ssize_t bounds).
- find-family start/end and startswith/endswith bounds convert as
slice indices ("slice indices must be integers or None or have an
__index__ method"), clamp like _PyEval_SliceIndex instead of
erroring at the bounds, and - for bytes/bytearray - are validated
before the needle so a bad index reports first, as CPython's
parsing order does. The needle itself is also converted after the
slice indices.
- bytes/bytearray decode() reports "decode() argument 'encoding'/
'errors' must be str, not X" and join() reports "can only join an
iterable".
- hex(sep) measures the separator with PyObject_Length semantics
("object of type 'float' has no len()", validated before the
empty-input and bytes_per_sep==0 early returns), and the fillchar
length error names type and length the way CPython's stringlib
does ("center(): argument 2 must be a byte string of length 1,
not a bytes object of length 2").
- gh-143195 / gh-142560 re-entrancy: bytearray's find/index/rfind/
rindex/count/__contains__/split/rsplit/hex and memoryview's hex
now run their argument conversions under an export guard, so a
re-entrant __len__/__index__/__buffer__ that resizes the buffer
raises BufferError ("Existing exports of data: object cannot be
re-sized" / "memoryview has 1 exported buffer") as in CPython.
memoryview.release() refuses while exports are live.
Un-marks the tests those fixes make pass (test_hex_use_after_free,
test_search_methods_reentrancy_raises_buffererror).
Assisted-by: ZCode:GLM-5.3
CPython's clinic signature for bytes/bytearray/str.expandtabs is
`tabsize: int`, so the value goes through PyLong_AsInt and anything
outside the C int range raises OverflowError. anystr::ExpandTabsArgs
converted with PyNumber_AsSsize_t instead, so on a 64-bit build
bytes(b"\ta").expandtabs(2**31)
tried to build a 2 GiB result rather than reporting
OverflowError: Python int too large to convert to C int
which is what extra_tests/snippets/builtin_bytes.py::test_huge_size
expects, and what str.expandtabs already did.
str.rs carried its own copy of ExpandTabsArgs that converted with the
int converter, which is how the two drifted apart; it now shares the
one in anystr, so bytes, bytearray and str convert identically.
Assisted-by: Claude:Claude Opus 5
#[pyfunction]/#[pymethod] derive __text_signature__ from the Rust parameter list, and a function that takes FuncArgs to check its own arity has no parameters to report, so func_sig emits "(*args, **kwargs)". Every builtin this branch rewrote that way - len, abs, hash, chr, callable, bin, ord, divmod, isinstance, issubclass and the rest of the 27 - stopped reporting the signature that RustPython#8512 had just made accurate: inspect.signature(len) (*args, **kwargs) # was (obj, /) Add `text_signature = "..."`, which overrides the derived parameter list, and give the affected builtins CPython's own, verified against CPython 3.14.7. round declares (number, ndigits=None) and so has a signature now, where before its destructuring pattern left it with none; builtin_signature.py keeps sum as the signature-less case and asserts round's instead. The derived signature is still used wherever no override is given, so genuinely variadic builtins such as breakpoint keep reporting (*args, **kwargs). Assisted-by: Claude:Claude Opus 5
394d53d to
b8b7e33
Compare
main_and_subinterpreter_run_sections_overlap parked each worker on a condvar from inside its run section, with the thread still ATTACHED, and held it there until both had arrived. An attached thread blocked that way runs no bytecode, so it never reaches the safepoint check_signals uses to self-suspend, and stop_the_world - which loops until every non-requester thread is SUSPENDED - cannot finish. Any collection landing in that window wedges both workers until the test's 30 s deadline gives up and reports the run sections as serialized. That is what CI hit on macos-latest: the suite ran 31.46 s and the test failed with entered < 2, while ubuntu and windows passed the same commit. Reproduced deterministically by parking a worker attached and requesting a stop-the-world: it never returns (90 s+ in futex_wait). Parking the same worker inside allow_threads instead, it returns at once. Wrap only the wait in allow_threads, so the thread detaches while parked and re-attaches after, as any blocking call inside a run section must. The counter is still incremented while attached, so the test proves what it did before - both interpreters inside run sections at once - and now finishes in 0.04 s rather than leaning on the deadline. Assisted-by: Claude:Claude Opus 5
419a0b2 moved PlaySound behind rustpython_host_env::winsound, which took the last uses of TryFromBorrowedObject, crate::exceptions and ToWideString with it. The imports stayed, so clippy (windows-latest) fails the build under -Dwarnings with three unused-import errors. main is red on this too, not just this branch. Assisted-by: Claude:Claude Opus 5
Running 3839 error-raising expressions through this build and CPython
3.14.7 and diffing the results turned up defects that reading the diff
does not show:
- _pad passed a hardcoded "center" to ByteInnerPaddingOptions::get_value,
so bytes/bytearray ljust and rjust named the wrong method in their
fillchar error. The short form also dropped the "()" CPython prints:
"ljust() argument 2 must be a byte string of length 1, not int".
- pow() only checked its second required argument, so pow() and
pow(mod=3) reported "'exp' (pos 2)" where CPython reports
"'base' (pos 1)", and pow(exp=2) slipped past the check entirely.
Both positions are now checked, as compile() already did.
- bytes and bytearray index/rindex raised "substring not found";
CPython raises "subsection not found" for bytes-likes and keeps
"substring not found" for str.
- hex()'s bytes_per_sep converted with the Py_ssize_t converter, but the
clinic declares it an int, so b"ab".hex(":", 2**31) returned a value
where CPython raises OverflowError, and 2**63 named the wrong C type.
- sequence_repeat_count reported "repeated bytes are too long" for every
sequence; PyNumber_AsSsize_t says "cannot fit 'int' into an
index-sized integer".
- isinstance()/issubclass() appended ", not <type>" to messages CPython
ends at "union" / "class".
Also drops eight insta .snap.new artifacts committed under a duplicated
crates/stdlib/crates/stdlib/ path; the accepted snapshots already live in
crates/stdlib/src/snapshots/.
Net 171 -> 149 differing cases against CPython 3.14.7, no regressions.
Assisted-by: Claude:Claude Opus 5
|
@ShaharNaveh its ready now, all tests passed, ready to merge when you are. (: |
Resolve the conflicts with upstream main (287dcd9). Where both sides reworked the same code, upstream's mechanism is kept and this branch's CPython-aligned messages are layered on top of it: - argument binding: upstream's `Callee`/`bind_for` replaces the receiver-splitting `take_front`; the `check_*` helpers stay for the METH_O/NOARGS/FASTCALL wording at the call sites - marshal: this branch's EOF/NULL/depth error taxonomy plus upstream's `DataTooShort`, mapped to CPython's three EOFError wordings - compression: upstream's shared zlib/bz2/lzma engines, with the bufsize/zdict/filter-spec validation and the lzma EOF wording re-applied on top - compiler diagnostics: this branch's rules converted to upstream's `CpythonDiagnostic` return type - str/list/dict/type: upstream's sized extraction, typed `__bases__` tuple and `try_from_str_subscript`, keeping this branch's arity checks Co-Authored-By: Claude Fable 5.1 <[email protected]>
The clippy (macos-latest) job of the previous run died in dtolnay/rust-toolchain with "unable to sync download to disk: Input/output error (os error 5)" before clippy ran, a runner disk failure rather than anything in the tree. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Merging this PR will degrade performance by 35.78%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
…nd specialize Two CI failures after the merge with main: - extra_tests/snippets/vm_argument_errors.py expects the keyword a function did not take to be reported by name, and main's `bind_for` does that when told the callee. The sites this branch rewrote to check their own counts still called `bind`, so `"".split(bogus=1)` came back as "this function got an unexpected keyword argument". Every such site that accepts keywords now binds through `bind_for` with the name CPython prints (split, rsplit, replace, splitlines, expandtabs, encode, hex, decode, translate, sort, compile, eval, exec, __import__, open, and the map, enumerate and bytearray constructors). While there, enumerate() counts its keywords toward "takes at most 2 arguments" and map() rejects an unknown keyword before counting positionals, as CPython does. - CodSpeed flagged loop_append.py at -27%. A `FuncArgs` signature marks a method FASTCALL|KEYWORDS, which the CALL fast path does not take, so `list.append` gives up the METH_O path in exactly the loop the benchmark measures (191 ms -> 141 ms per 10^6 appends here). `append` takes its argument typed again; its arity errors are now worded by main's generic binder rather than as METH_O. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…itself
main now reports a __text_signature__ for every generated method, and a
method that takes `FuncArgs` to check its own argument count has no Rust
parameter list to derive one from, so the 172 builtin methods this branch
rewrote that way advertised "(*args, **kwargs)". test_pydoc caught it on
str.lower on all three platforms:
'lower(*args, **kwargs) method of builtins.str instance'
!= 'lower() method of builtins.str instance'
Each of them now carries CPython 3.14's own __text_signature__, read from
CPython and checked back against it for all 172 (dict.update is left alone:
CPython gives it no signature). float.fromhex names its $type the way
CPython does. The two pydoc METH_O tests pass with set.add's signature in
place, so their expected-failure markers go.
Co-Authored-By: Claude Fable 5.1 <[email protected]>
There was a problem hiding this comment.
you put too many changes including design decision into this single PR. that's not reviewable.
you decided to add text_signature to #[pymethod]. this is design decision. why do you think this is a proper design decision to put in "align error message" patch. have you dicussed it to anyone? did you provide any rationale why this is good design? even if you did everything, why do you think putting this kind of design change into a huge patch with everything else is a good idea? I am sorry if this is not the case, but i feel like you don't put enough review effort to your patches as I expect to reasonable patch writers. This is what I already talked to you in another PR with zstd.
please make everything to contain single feature, single response, single review point as much as possible. Please read our AI policy in PR template and don't forget everyone has their own AI, usually easier to communicate. I will not review this size + this quality patch.
The git history is a mess I agree. I think that the easiest approach to resolve this would be to: maybe there's an easier approach to this, but this should work |
84558d4 to
43eec46
Compare
3 participants
Summary
Continues the parser error-message alignment from #7928 / #7933 / #7988. Translates many more ruff
ParseErrorTypevariants to CPython 3.14.5's exact wording inCompileError::from_ruff_parse_error(using the source slice / parsed AST kind), plus a few codegen/symtable checks.crates/vm/src/stdlib/_ast.rsnow routes its parse errors throughfrom_ruff_parse_error, soast.parse()/compile(..., PyCF_ONLY_AST)produce the same CPython-aligned messages as the exec path (they previously leaked raw ruff strings — and this fixes a regression where the var-param message diverged on the AST path).What's aligned
cannot use {attribute,subscript,tuple,list,literal,function call} as import target;import X from Y→ "Did you mean to use 'from ... import ...' instead?"'u' and 'b' prefixes are incompatible);invalid character 'X' (U+XXXX):and value syntax'elif' block follows an 'else' block;raise from; comprehensionif/ unparenthesized target; ternary statement-keyword hintscase … as <target>→cannot use {kind} as pattern target, andcase … as _→cannot use '_' as a target__debug__as adef/class/type-param/except-handler name →cannot assign to __debug__name 'x' is nonlocal and global; generic type-parameter wording (… cannot be used within the definition of a generic)This drops the now-passing
# TODO: RUSTPYTHON; Wrong error messagedoctest markers and@expectedFailuredecorators acrosstest_syntax.py,test_genexps.py,test_named_expressions.py, andtest_patma.py.Not covered (intentionally left marked)
A few
test_syntax.pycases need ruff-parser or deeper codegen changes and remain marked rather than emitting a silently-wrong message: the type-commentbare *case,class C(x for x in L)andf((x)=2)(ruff accepts these without a parse error), anddict(...); x $ y(duplicate-keyword vs lexer ordering). Runtime-error messages in unrelated subsystems (struct-sequence, format-spec, datetime,__import__, ast__replace__) are out of scope for this parser-focused change.Verification
cargo clippy -p rustpython-compiler -p rustpython-codegen -- -D warningsclean;cargo fmt.ast.parse().test_syntax test_genexps test_named_expressions test_patma test_type_params test_scope test_grammar test_compile test_exceptionsall pass (961 tests).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
_as a capture/store targetChores