Raise the shared argument-binding errors from the slots that count for themselves - #8634
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (39)
📝 WalkthroughWalkthroughThe change centralizes argument-error formatting, allows class names in ChangesArgument binding and error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: ✨ 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 |
…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
7f66fe7 to
47c36a7
Compare
Merging this PR will improve performance by 21.81%
|
| 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)
…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
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:
operator.attrgetter()attrgetter expected 1 argument, got 0.attrgetter expected 1 argument, got 0typing.TypeVar('T', bogus=1)TypeVar() got unexpected keyword argument(s): bogustypevar() 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 0GenericAlias expected 2 arguments, got 0weakref.ref,frozenset,itemgetter,islice,start_new_thread,TextIOWrapper,AttributeError,NameError,ParamSpec,TypeVarTupleandTypeAliasTypeall follow.FrameLocalsProxyandmin/maxalso now count arguments before reading keywords, which is the orderframelocalsproxy_newandmin_maxcheck 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 avm.new_*_error, so these are too:The keyword one takes an
Optionbecause a parser sometimes has no name of its own —_PyArg_Parser.fnamebeing NULL, which is what_csv's dialect parser wants.bind_forandcheck_kwargs_empty_fortake anything aCalleeconverts from, so the slots passSelf::NAMEthere too, andCalleestays with the binding machinery that actually needs to carry a name around.Measured
Against
python3.143.14.6, side by side:@unittest.expectedFailuredecorators removed (test_bytes,string_tests,test_range).extra_tests/snippets/vm_argument_errors.pypasses under both interpreters.cargo fmt --checkclean.Four divergences are left and need more than a name:
weakref.proxyis a function of its own there rather than a type,frozensetnames the subclass being constructed, andTextIOWrappernames its first parameter (missing required argument 'buffer' (pos 1)) and counts before converting.Summary by CodeRabbit
Bug Fixes
TypeErrormessages for incorrect argument counts across built-in constructors and standard-library functions.Tests