Skip to content

Raise the shared argument-binding errors from the slots that count for themselves - #8634

Merged
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:RustPython-3
Sep 1, 2026
Merged

youknowone merged 2 commits into
RustPython:mainfrom
youknowone:RustPython-3

Conversation

@youknowone

@youknowone youknowone commented Aug 31, 2026

Copy link
Copy Markdown
Member

Follow-up to #8630, which taught the argument binder to name the function it was binding for. Two commits.

Raise the shared errors from the slots that count for themselves

Sites all over the tree checked their own argument counts and keywords and wrote the message by hand, so they had drifted apart:

before after / CPython 3.14.6
operator.attrgetter() attrgetter expected 1 argument, got 0. attrgetter expected 1 argument, got 0
typing.TypeVar('T', bogus=1) TypeVar() got unexpected keyword argument(s): bogus typevar() got an unexpected keyword argument 'bogus'
csv.reader([], bogus=1) reader() got an unexpected keyword argument ''bogus' is an invalid keyword argument for this function' this function got an unexpected keyword argument 'bogus'
GenericAlias() expected 2 arguments, got 0 GenericAlias expected 2 arguments, got 0

weakref.ref, frozenset, itemgetter, islice, start_new_thread, TextIOWrapper, AttributeError, NameError, ParamSpec, TypeVarTuple and TypeAliasType all follow. FrameLocalsProxy and min/max also now count arguments before reading keywords, which is the order framelocalsproxy_new and min_max check in.

Give those errors the constructors the rest of them have

Raising one meant writing Callee::of::<Self>(vm).arity_error(1..=3, 0, vm) — the vm twice, and a type to know about before you could say what you meant. Every other error in the tree is a vm.new_*_error, so these are too:

return Err(vm.new_arity_type_error(Self::NAME, 1..=3, 0));
return Err(vm.new_unexpected_keyword_type_error(Some("typevar"), &key));

The keyword one takes an Option because a parser sometimes has no name of its own — _PyArg_Parser.fname being NULL, which is what _csv's dialect parser wants. bind_for and check_kwargs_empty_for take anything a Callee converts from, so the slots pass Self::NAME there too, and Callee stays with the binding machinery that actually needs to carry a name around.

Measured

Against python3.14 3.14.6, side by side:

  • 22 of the 26 calls these commits touch produce the identical string (21 after the first commit; the second changed no message).
  • The broader 43-call set stands at 23 exact matches.
  • Three @unittest.expectedFailure decorators removed (test_bytes, string_tests, test_range).
  • extra_tests/snippets/vm_argument_errors.py passes under both interpreters.
  • 133 suites / 25,316 tests SUCCESS; clippy (CI flags) clean; cargo fmt --check clean.

Four divergences are left and need more than a name: weakref.proxy is a function of its own there rather than a type, frozenset names the subclass being constructed, and TextIOWrapper names its first parameter (missing required argument 'buffer' (pos 1)) and counts before converting.

Summary by CodeRabbit

  • Bug Fixes

    • Standardized TypeError messages for incorrect argument counts across built-in constructors and standard-library functions.
    • Improved reporting of unexpected keyword arguments, including the relevant callable or class name.
    • Made validation order more consistent, producing clearer errors for invalid calls.
  • Tests

    • Added coverage confirming consistent argument-count and unexpected-keyword error messages across callable and constructor implementations.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Team

Run ID: d56bb0c6-4f4e-470f-a31d-42cb3d31e66a

📥 Commits

Reviewing files that changed from the base of the PR and between 3ae7166 and 47c36a7.

📒 Files selected for processing (39)
  • crates/stdlib/src/contextvars.rs
  • crates/stdlib/src/csv.rs
  • crates/vm/src/builtins/bool.rs
  • crates/vm/src/builtins/bytes.rs
  • crates/vm/src/builtins/classmethod.rs
  • crates/vm/src/builtins/complex.rs
  • crates/vm/src/builtins/float.rs
  • crates/vm/src/builtins/frame_locals_proxy.rs
  • crates/vm/src/builtins/genericalias.rs
  • crates/vm/src/builtins/int.rs
  • crates/vm/src/builtins/range.rs
  • crates/vm/src/builtins/set.rs
  • crates/vm/src/builtins/singletons.rs
  • crates/vm/src/builtins/slice.rs
  • crates/vm/src/builtins/staticmethod.rs
  • crates/vm/src/builtins/str.rs
  • crates/vm/src/builtins/tuple.rs
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/builtins/weakproxy.rs
  • crates/vm/src/builtins/weakref.rs
  • crates/vm/src/exception_group.rs
  • crates/vm/src/exceptions.rs
  • crates/vm/src/function/argument.rs
  • crates/vm/src/function/mod.rs
  • crates/vm/src/stdlib/_ctypes/simple.rs
  • crates/vm/src/stdlib/_io.rs
  • crates/vm/src/stdlib/_operator.rs
  • crates/vm/src/stdlib/_thread.rs
  • crates/vm/src/stdlib/_typing.rs
  • crates/vm/src/stdlib/builtins.rs
  • crates/vm/src/stdlib/itertools.rs
  • crates/vm/src/stdlib/os.rs
  • crates/vm/src/stdlib/posix.rs
  • crates/vm/src/stdlib/time.rs
  • crates/vm/src/stdlib/typevar.rs
  • crates/vm/src/types/slot.rs
  • crates/vm/src/types/structseq.rs
  • crates/vm/src/vm/vm_new.rs
  • extra_tests/snippets/vm_argument_errors.py

📝 Walkthrough

Walkthrough

The change centralizes argument-error formatting, allows class names in FuncArgs binding, and adds VM helpers for arity and unexpected-keyword errors. Built-in and standard-library constructors now use class names for binding. Tests cover the updated errors.

Changes

Argument binding and error handling

Layer / File(s) Summary
Argument binding and error helpers
crates/vm/src/function/*, crates/vm/src/vm/vm_new.rs
FuncArgs accepts names convertible to Callee. Shared message helpers and VM TypeError constructors handle arity and unexpected keywords.
Class-aware constructor binding
crates/stdlib/src/contextvars.rs, crates/vm/src/builtins/*, crates/vm/src/stdlib/*, crates/vm/src/types/*, crates/vm/src/exception_group.rs
Constructors and call slots use Self::NAME or another named callee for argument binding.
Standardized argument validation
crates/stdlib/src/csv.rs, crates/vm/src/builtins/{frame_locals_proxy,set}.rs, crates/vm/src/exceptions.rs, crates/vm/src/stdlib/*
Arity and unexpected-keyword failures use the new VM helpers.
Argument error coverage
extra_tests/snippets/vm_argument_errors.py
Tests cover argument-count and unexpected-keyword errors across multiple slots and constructors.

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

Suggested reviewers: shaharnaveh

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

…r themselves

Sites all over the tree checked their own argument counts and keywords and
wrote the message by hand, so they drifted: `attrgetter expected 1 argument,
got 0.` carried a full stop, `TypeVar() got unexpected keyword argument(s):
bogus` named neither the keyword the way its counterpart does nor the
function the way `typevar()` is named, and `_csv`'s dialect parser handed a
whole formatted sentence to `InvalidKeywordArgument`, which then wrapped it
in another one.

`Callee::arity_error` and `Callee::unexpected_keyword` are now public, and
these sites raise through them, so there is one place the wording lives.
`FrameLocalsProxy` counts its arguments before reading its keywords, and
`min`/`max` do too, which is the order `framelocalsproxy_new` and `min_max`
check in. `weakref.ref`, `GenericAlias`, `frozenset`, `attrgetter`,
`itemgetter`, `islice`, `start_new_thread`, `TextIOWrapper`, `AttributeError`,
`NameError`, `TypeVar`, `ParamSpec`, `TypeVarTuple` and `TypeAliasType` all
follow.

Of 25 calls measured against CPython 3.14.6, 21 now produce the identical
string. The four left over need what `Callee` cannot carry: `weakref.proxy`
is a function of its own there rather than a type, `frozenset` names the
subclass being constructed, and `TextIOWrapper` names its first parameter
and counts before converting.

Assisted-by: Claude
Raising one of these meant writing `Callee::of::<Self>(vm).arity_error(1..=3,
0, vm)` — the vm twice, and a type to know about before you could say what
you meant. Every other error in the tree is a `vm.new_*_error`.

So the message texts move out of `Callee` into functions the vm can call, and
the vm gets `new_arity_type_error` and `new_unexpected_keyword_type_error`
alongside `new_unsupported_bin_op_error` and the rest. A slot names itself
with the `NAME` its class already carries:

    return Err(vm.new_arity_type_error(Self::NAME, 1..=3, 0));
    return Err(vm.new_unexpected_keyword_type_error(Some("typevar"), &key));

The keyword one takes the name as an `Option` because the parser sometimes
has none of its own, which is what `_PyArg_Parser.fname` being NULL means and
what `_csv`'s dialect parser wants.

`bind_for` and `check_kwargs_empty_for` take anything a `Callee` converts
from, so the slots pass `Self::NAME` there too. `Callee` itself is left to the
binding machinery, which is the only place that needs to carry a name around
rather than use one.

No message changes: 22 of 26 calls still match CPython 3.14.6 exactly, and the
43-call set still stands at 23.

Assisted-by: Claude
@codspeed

codspeed Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will improve performance by 21.81%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 64 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
gc_collect.py[rustpython] 202.1 ms 158.5 ms +27.48%
gc_traversal.py[rustpython] 841.3 ms 722.7 ms +16.4%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing youknowone:RustPython-3 (47c36a7) with main (3ae7166)

Open in CodSpeed

@youknowone
youknowone marked this pull request as ready for review September 1, 2026 13:15
@youknowone
youknowone merged commit 3167896 into RustPython:main Sep 1, 2026
28 of 29 checks passed
@youknowone
youknowone deleted the RustPython-3 branch September 1, 2026 13:16
youknowone added a commit that referenced this pull request Sep 16, 2026
…r themselves (#8634)

* Raise the shared argument-binding errors from the slots that count for themselves

Sites all over the tree checked their own argument counts and keywords and
wrote the message by hand, so they drifted: `attrgetter expected 1 argument,
got 0.` carried a full stop, `TypeVar() got unexpected keyword argument(s):
bogus` named neither the keyword the way its counterpart does nor the
function the way `typevar()` is named, and `_csv`'s dialect parser handed a
whole formatted sentence to `InvalidKeywordArgument`, which then wrapped it
in another one.

`Callee::arity_error` and `Callee::unexpected_keyword` are now public, and
these sites raise through them, so there is one place the wording lives.
`FrameLocalsProxy` counts its arguments before reading its keywords, and
`min`/`max` do too, which is the order `framelocalsproxy_new` and `min_max`
check in. `weakref.ref`, `GenericAlias`, `frozenset`, `attrgetter`,
`itemgetter`, `islice`, `start_new_thread`, `TextIOWrapper`, `AttributeError`,
`NameError`, `TypeVar`, `ParamSpec`, `TypeVarTuple` and `TypeAliasType` all
follow.

Of 25 calls measured against CPython 3.14.6, 21 now produce the identical
string. The four left over need what `Callee` cannot carry: `weakref.proxy`
is a function of its own there rather than a type, `frozenset` names the
subclass being constructed, and `TextIOWrapper` names its first parameter
and counts before converting.

Assisted-by: Claude

* Give the argument-binding errors the constructors the rest of them have

Raising one of these meant writing `Callee::of::<Self>(vm).arity_error(1..=3,
0, vm)` — the vm twice, and a type to know about before you could say what
you meant. Every other error in the tree is a `vm.new_*_error`.

So the message texts move out of `Callee` into functions the vm can call, and
the vm gets `new_arity_type_error` and `new_unexpected_keyword_type_error`
alongside `new_unsupported_bin_op_error` and the rest. A slot names itself
with the `NAME` its class already carries:

    return Err(vm.new_arity_type_error(Self::NAME, 1..=3, 0));
    return Err(vm.new_unexpected_keyword_type_error(Some("typevar"), &key));

The keyword one takes the name as an `Option` because the parser sometimes
has none of its own, which is what `_PyArg_Parser.fname` being NULL means and
what `_csv`'s dialect parser wants.

`bind_for` and `check_kwargs_empty_for` take anything a `Callee` converts
from, so the slots pass `Self::NAME` there too. `Callee` itself is left to the
binding machinery, which is the only place that needs to carry a name around
rather than use one.

No message changes: 22 of 26 calls still match CPython 3.14.6 exactly, and the
43-call set still stands at 23.

Assisted-by: Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant