Reject surplus positional arguments before converting any by jseop-lim · Pull Request #9242 · RustPython/RustPython

jseop-lim · 2026-10-20T07:43:01Z

Summary

FuncArgs::bind_for rejects leftover positional arguments only after from_args has converted every parameter. A conversion can run user code (__iter__ for PyIter, __index__ for ArgIndex), so a call with too many arguments raises whatever that code raises. CPython checks the count first, in filter_vectorcall and in cfunction_vectorcall_O for hex() and friends. #8957 made the same change for keywords.

AS-IS

>>> class FailingIter:
...     def __iter__(self): raise RuntimeError
...
>>> class FailingIndex:
...     def __index__(self): raise RuntimeError
...
>>> filter(None, FailingIter(), 0)
RuntimeError
>>> hex(FailingIndex(), 1)
RuntimeError

TO-BE

>>> filter(None, FailingIter(), 0)
TypeError: filter expected 2 arguments, got 3
>>> hex(FailingIndex(), 1)
TypeError: hex expected 1 argument, got 2

(matching CPython's exception type; the filter message is identical, the hex one keeps RustPython's shared wording)

Changes

Check the positional count against FromArgs::arity() in bind_for before from_args runs.
Make the default arity() unbounded, since a hand-written impl such as ObjectArgs or ReplaceChanges may take any number of positional arguments itself. Give KwArgs, (), DirFd, OpenDirFd and csv's FormatOptions the bound they actually take, so their existing messages keep a finite count.
Add each flattened field's upper bound in the derived arity(), and sum tuple bounds with saturating_add.
As a side effect, a too-few message for a signature with *args now says expected at least 1 argument instead of expected 1 argument (for example struct.pack() and int.__new__()). Both are still TypeError.
Add a case to extra_tests/snippets/vm_argument_errors.py.

Test plan

Built and run on macOS aarch64 against upstream/main 398dd70, using the CI feature set --no-default-features --features stdlib,importlib,stdio,encodings,sqlite,ssl-rustls-aws-lc,host_env,doc,ucd_3_2_0, plus threading, compared with CPython 3.14.4.

A 69-call probe of builtins, itertools, operator, functools, math and constructors with too many, too few and valid arguments: the patch changes exactly 10 results, all from RuntimeError to TypeError (filter twice, enumerate with 3 arguments, hex, oct, bin, chr, itertools.compress, pairwise and takewhile). The other 59 are unchanged.
rustpython -m test -j 6 --timeout 600 on the unpatched and patched builds: 45,717 run on both, and the same 3 modules fail on both (test_cmd_line, test_dict, test_frame).
pytest -k rustpython in extra_tests: 246 pass on the patched build. On the unpatched build the only failure is the new vm_argument_errors.py case, which also passes under CPython 3.14.4.
cargo test -p rustpython-vm -p rustpython-derive-impl --lib: 128 pass.
cargo fmt --check clean, cargo clippy -p rustpython-vm -p rustpython-stdlib -p rustpython-derive-impl --all-targets reports nothing, and pre-commit run --files passes on the changed files.

I used Claude Code (claude-opus-5-5) to draft the analysis, the patch and this description; I reviewed the change and ran every command above myself.

Summary by CodeRabbit

Bug Fixes
- Improved argument validation so excess positional arguments are rejected before argument conversion, producing clearer and more consistent errors.
- Corrected argument counting for flattened and combined argument parsers, including cases with large argument counts.
- Preserved keyword-only behavior while accurately reporting zero positional arguments for directory-related options.
Compatibility
- Updated CSV formatting options to correctly accept zero or one positional argument.

FuncArgs::bind_for rejected surplus positional arguments only after FromArgs::from_args had converted every declared parameter. A conversion can run user code (__iter__ for PyIter, __index__ for ArgIndex), so filter(None, it, 0), hex(x, 1) and pairwise(it, 0) raised whatever that code raised instead of the TypeError CPython reports before it touches any argument. Check the count against FromArgs::arity() before from_args runs. For that bound to be safe: - The default arity() has no upper bound, since a manual impl may take positional arguments itself. KwArgs, (), DirFd, OpenDirFd and csv's FormatOptions declare the bound they actually have. - A derived struct adds the upper bound of each flattened field. - A tuple sums upper bounds with saturating_add. Assisted-by: Claude Code:claude-opus-5-5

chatgpt-codex-connector · 2026-10-20T07:43:25Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-20T07:51:51.501377Z	`a61c2a9`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-10-20T07:43:39Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: 81c6b10b-84e8-4527-b539-4517fcd53e69

📥 Commits

Reviewing files that changed from the base of the PR and between 398dd70 and a61c2a9.

📒 Files selected for processing: 5

crates/derive-impl/src/from_args.rs
crates/stdlib/src/csv.rs
crates/vm/src/function/argument.rs
crates/vm/src/stdlib/os.rs
extra_tests/snippets/vm_argument_errors.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Argument parsing now uses explicit arity bounds and rejects excess positional arguments before converting them. Generated arity accounts for flattened fields, and tests cover conversion ordering.

Changes: Argument arity validation

Layer / File(s)	Summary
Arity bounds and binding order `crates/vm/src/function/argument.rs`	`FromArgs` defaults to an unbounded upper arity. `KwArgs` and `()` declare zero positional capacity. Tuple upper bounds use saturating addition. `FuncArgs::bind_for` rejects excess positional arguments before conversion.
Composed parser arity `crates/derive-impl/src/from_args.rs`, `crates/stdlib/src/csv.rs`, `crates/vm/src/stdlib/os.rs`, `extra_tests/snippets/vm_argument_errors.py`	Generated arity includes flattened field bounds. `FormatOptions` declares arity `0..=1`; `DirFd` and `OpenDirFd` declare arity `0..=0`. A test checks that excess arguments are counted before conversion.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: youknowone, shaharnaveh

Merge Risk: ⚪ Minimal · up to a61c2

This change rejects calls with too many positional arguments before converting any of them, so user code such as iter no longer runs on calls that were already invalid. No concrete merge-blocking risk was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: rejecting surplus positional arguments before argument conversion.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Navigation Menu

Repository navigation

Reject surplus positional arguments before converting any - #9242

Reject surplus positional arguments before converting any#9242
jseop-lim wants to merge 1 commit into
RustPython:mainfrom
jseop-lim:positional-arity-first

jseop-lim commented Oct 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jseop-lim commented Oct 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

AS-IS

TO-BE

Changes

Test plan

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jseop-lim commented Oct 20, 2026 •

edited by coderabbitai Bot

Loading

chatgpt-codex-connector Bot commented Oct 20, 2026 •

edited

Loading

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading