Repository navigation
builtins, typedef: run check_user_subclass for type.__new__, and refuse extra arguments to object.__init_subclass__ - #1115
Conversation
WalkthroughThe interpreter now uses parser-compatible validation for ChangesType construction and subclass initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
youknowone
changed the title
check_user_subclass for type.__new__, and refuse extra arguments to object.__init_subclass__
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 628a07a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f2044077
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .as_str() | ||
| .ok() |
There was a problem hiding this comment.
When object.__init_subclass__ receives a valid string keyword containing a lone surrogate, such as object.__init_subclass__(**{'\ud800': 1}), as_str() fails and .ok() silently drops the keyword. The resulting empty unknown list lets the call return None, whereas the previous predicate rejected this keyword and other Python implementations raise TypeError. Count and reject Wtf8 keyword names without requiring conversion to Rust str.
AGENTS.md reference: AGENTS.md:L232-L232
Useful? React with 👍 / 👎.
…_ast` caller names `type` `descr__new__` tests `space.is_w(w_typetype, space.w_type)` and returns `space.type(w_name)` on the positive branch, raising in the `else`. `type_descr_new` tested the negation and raised first; invert it so the two read in the same order. `_ast`'s `register_module` is the only caller that builds the `type.__new__` argument list by hand rather than reaching it through an attribute lookup, so record why it has to name `type` itself. The header records the matching invariant: the `(name, bases, dict)` triple may not be keyed off argument types, because scanning for the first `str` lets a non-type metatype slip past `precheck_for_new`. `type_new_metatype_guard.py` gains the three-argument metatype rows, the `super().__new__` metaclass path, and the `_ast` construction path. Assisted-by: Claude
…s metaclass the type layout `allocate_instance` runs `W_TypeObject.check_user_subclass` (typeobject.py:555-567), which tests three things: that the metatype is a type, that it is a subtype of `type`, and that its layout typedef matches. The construction path inlined the middle test and reformatted its message; call the function instead. The third test is not decoration here. The five `_ctypes` metaclasses were built through `make_builtin_type_with_base`, which records the general instance layout, so `check_user_subclass` answered `type.__new__(PyCSimpleType) is not safe` and `import ctypes` failed at `class py_object(_SimpleCData)`. Instances of a metaclass are type objects — `type` itself is set up with `TYPE_TYPE` for that reason — so the five now name `TYPE_TYPE` too. Upstream has no interp-level ctypes metaclass: `class _CDataMeta(type)` is app-level (basics.py:48) and inherits `type`'s instance layout from its base. A zero-argument call never reaches `descr__new__`, because `w_typetype` is a declared parameter and the argument parser refuses it first. Measured under pypy 7.3.20 that refusal reads `type.__new__() missing 1 required positional argument: 'typetype'`; `type.__new__(): not enough arguments` is cpython's `tp_new_wrapper`. `type_new_metatype_guard.py` gains the zero-argument row, the three-argument not-a-subtype rows, the empty-bases/one-base pair that pins when the `(object,)` default is applied, and the `ctypes` construction path. Every row passes under `pypy3` and `python3.14` unmodified. Assisted-by: Claude
…ss__` `descr___init_subclass__(space, w_cls)` (objectobject.py:139) declares one parameter and no `__args__`, so the argument parser refuses everything past the bound class before the body runs. pyre's default checked only for keywords, so `object.__init_subclass__(1)` returned `None` where upstream raises. Report the refusals through the `ArgErr` shapes the parser builds (argument.py:529-627) rather than a fixed string, which also settles four details the string did not carry: keywords are collected before the positional overflow is judged, so `__init_subclass__(1, x=1)` names the keyword; a second unknown keyword switches to `got 2 unexpected keyword arguments`; the "did you forget 'self'" hint is conditional on `given == num_argnames + 1`, so it appears for one extra argument and not for two; and the message names the defining type, reading `object.__init_subclass__()` even when the call arrives through a subclass or a class statement's keywords. Measured against pypy 7.3.20. cpython words all of these differently and names the receiving class's `__qualname__`, so `object_init_subclass_arity.py` asserts per runtime; every row passes under `pypy3` and `python3.14` unmodified. `__subclasshook__` declares `__args__` instead and keeps swallowing everything; the fixture pins that contrast so the refusal reads as signature-driven rather than a rule about `object`'s classmethods. Assisted-by: Claude
…age per implementation `type_new_metatype_guard.py`, `object_init_subclass_arity.py` and the message block in `builtin_new_argument_count.py` selected an expected message with `sys.implementation.name`, so each runtime was compared against its own literal. Every message assertion is now a containment on the substring the runtimes share, or absent where they share nothing and only the refusal is asserted. The first two files no longer branch on the runtime at all; `builtin_new_argument_count.py` keeps the branch that covers the one-argument form of `type.__new__`, where the outcome differs rather than its wording. Dropping the full-message comparison also removes the reason the class statements in `object_init_subclass_arity.py` ran through `exec` in a bare namespace, and its `__subclasshook__` rows now assert the one-argument call both runtimes answer. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 628a07ae58
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // `pos[0]` is the class the classmethod bound, so an argument | ||
| // of its own puts the count at two. | ||
| if pos.len() > 1 { | ||
| return refuse(crate::argument::ArgErr::TooManyMethod { |
There was a problem hiding this comment.
When the underlying callable is extracted with object.__dict__['__init_subclass__'].__func__ and invoked as f(object, 1), upstream argument matching has methodcall=False and therefore emits ArgErrTooMany; this unconditional TooManyMethod instead appends the misleading “Did you forget 'self'” hint. Preserve the call-mode flag or route this through the actual signature matcher rather than always selecting the method-call variant.
AGENTS.md reference: AGENTS.md:L232-L232
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 18967-18979: Update the unknown-key collection in the kwargs
handling block to retain all string keys, including lone-surrogate names,
instead of filtering through as_str().ok(). Preserve the __pyre_kw__ exclusion
and use the existing WTF-8 error-reporting path when converting or displaying
these names, so object.__init_subclass__ still rejects unexpected keywords.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a4b9557-5056-4d5d-bf77-9c44a22ad88d
📒 Files selected for processing (2)
pyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/typedef.rs
| let unknown: Vec<String> = match kwargs { | ||
| Some(kw) => unsafe { | ||
| pyre_object::w_dict_items(kw) | ||
| .into_iter() | ||
| .filter(|(k, _)| pyre_object::is_str(*k)) | ||
| .filter_map(|(k, _)| { | ||
| pyre_object::w_str_get_wtf8(k) | ||
| .as_str() | ||
| .ok() | ||
| .map(str::to_string) | ||
| }) | ||
| .filter(|name| name != "__pyre_kw__") | ||
| .collect() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pypy3 - <<'PY'
key = "\ud800"
try:
class SurrogateKeyword(**{key: 1}):
pass
except TypeError as exc:
print(repr(str(exc)))
else:
raise SystemExit("PyPy accepted an unexpected surrogate keyword")
PYRepository: youknowone/pyre
Length of output: 196
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate and inspect the referenced code section plus nearby parser behavior symbols.
wc -l pyre/pyre-interpreter/src/typedef.rs
sed -n '18930,19015p' pyre/pyre-interpreter/src/typedef.rs
# Search for the unknown keyword handling and related parser entries.
rg -n "unknown.*\\{|__pyre_kw__|w_str_get_wtf8|unknown: Vec<String>|as_str\\(\\)\\.ok\\(\\)" pyre/pyre-interpreter/src/typedef.rs pyre -g '*.rs'Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the call parser path that currently preserves WTF8 keys and produces UnknownKwds.
sed -n '3860,4005p' pyre/pyre-interpreter/src/call.rs
sed -n '4595,4645p' pyre/pyre-interpreter/src/builtins.rs
sed -n '680,715p' pyre/pyre-interpreter/src/call.rs
# Inspect Wtf8 conversion behavior around str conversion.
sed -n '240,280p' pyre/pyre-object/src/unicodeobject.rs
sed -n '550,635p' pyre/pyre-object/src/unicodeobject.rs
rg -n "struct Wtf8|Wtf8Buf|fn as_str|impl .*Wtf8|is_legal|contains.*surrogate|WTF8" pyre/pyre-object/src/unicodeobject.rs pyre/pyre-object/src/rutf8.rsRepository: youknowone/pyre
Length of output: 17640
🌐 Web query:
Python TypeError unexpected keyword argument object.__init_subclass__ surrogate unicode character
💡 Result:
The error "TypeError: init_subclass takes no keyword arguments" occurs when keyword arguments passed during a class definition are not correctly consumed by an init_subclass method in the inheritance chain [1][2]. When no subclass implementation consumes these arguments, they are ultimately passed to object.init_subclass, which explicitly forbids keyword arguments to ensure that errors are not passed silently [3][4][5]. Regarding your specific mention of "surrogate unicode character": while not a standard part of this Python error message, it is possible that hidden or non-printable characters (such as surrogate pairs or control characters) are present in your source code, potentially within the class definition or keyword argument names, causing unexpected behavior or parsing issues during class creation. To resolve this error, follow these steps: 1. Consume Keyword Arguments: Ensure your base class explicitly defines kwargs in its init_subclass method, extracts the specific arguments it needs, and passes the remaining arguments to super().init_subclass(kwargs) [2][6]. class Base: def init_subclass(cls, custom_param=None, kwargs): super.init_subclass(kwargs) # Pass remaining kwargs up cls.custom_param = custom_param class Child(Base, custom_param="value"): pass 2. Check for Typos: If you are not intending to pass custom arguments, check your class definition for misspelled arguments (e.g., using meta instead of metaclass) [1][7]. These typos are passed as unexpected keyword arguments, triggering the error [1][7]. 3. Inspect Source Code: If you suspect "surrogate unicode character" issues, inspect your code file using a hex editor or an IDE that displays hidden/special characters. Ensure your file is saved with proper encoding (UTF-8) and that there are no invalid characters in the class signature. 4. Mypy False Positives: If you are using Mypy and receiving errors about unexpected keyword arguments for object.init_subclass, note that this is a known issue in Mypy when handling class decorators or complex metaclass inheritance [8][9]. You may need to ignore the error or update your Mypy configuration [9].
Citations:
- 1: https://stackoverflow.com/questions/63448679/typeerror-init-subclass-takes-no-keyword-arguments
- 2: https://runebook.dev/en/docs/python/reference/datamodel/object.__init_subclass__
- 3: https://gopy.tamnd.com/docs/annotations/python/python_typeobject5_detail
- 4: https://github.com/python/peps/blob/main/peps/pep-0487.rst
- 5: https://discuss.python.org/t/allow-object-init-subclass-to-take-keyword-arguments/48343
- 6: object.__init_subclass__ should take keyword arguments but doesn't python/cpython#116697
- 7: https://exchangetuts.com/typeerror-init-subclass-takes-no-keyword-arguments-1641668224087603
- 8: https://stackoverflow.com/questions/67673887/mypy-error-unexpected-keyword-argument-for-init-subclass
- 9: False positive on class definition keyword arguments python/mypy#11036
🌐 Web query:
PyPy object.__init_subclass__() got unexpected keyword argument "\ud800"
💡 Result:
The error “PyPy object.init_subclass got unexpected keyword argument …” isn’t directly about “\ud800”. It indicates that at class-creation time, some keyword(s) were provided in the class header (e.g., class X(..., something=...): ...), but the keyword(s end up being passed up to object.init_subclass, and the base object.init_subclass accepts no keyword arguments, so Python raises a TypeError [1]. Why this happens on PyPy (common cause) 1) init_subclass keyword arguments require cooperative filtering down the MRO. PEP 487 explains that object has an empty init_subclass endpoint and that init_subclass methods must process keyword arguments and pass only the remaining ones to super().init_subclass(kwargs) [1]. If your init_subclass doesn’t consume the keyword(s it’s meant to accept (or a superclass in the MRO doesn’t), then object.init_subclass will eventually receive the leftover keywords and error. 2) Metaclass/ABC + init_subclass interactions can trigger keyword-argument plumbing bugs or mismatches. Both CPython issues and PyPy issues document failures when mixing init_subclass keyword arguments with standard-library metaclasses/ABC machinery, because the metaclass path that calls type.new/type.init may not forward/handle kwargs the way init_subclass expects [2][3], and PyPy has also reported incorrect handling in cases involving abc/collections.abc ancestors [4][5]. How to fix (practical guidance) - Ensure the class hierarchy consumes the keyword(s before calling super().init_subclass. Example pattern: - Define init_subclass(cls, your_kw, kwargs): ...; super().init_subclass(kwargs) so the keyword is not forwarded to object. - If you’re combining ABC/metaclasses (or collections.abc ABCs) with init_subclass keywords, avoid passing those parameters as class-header keywords unless you control and can update the relevant metaclass/ancestor init_subclass to accept and forward them correctly. In older implementations, metaclass/ABC plumbing may not fully cooperate with init_subclass keywords [2][3][4][5]. About the specific “\ud800” - \ud800 looks like a Unicode escape (a code unit). The mechanism that raises this error is still the same: a keyword argument reached object.init_subclass that it doesn’t accept. So you should focus on the keyword name/value plumbing rather than treating \ud800 as a special PyPy-only keyword issue. If you paste the minimal reproduction (your class ... line and the init_subclass signature(s) involved), I can tell you exactly which keyword(s are leaking to object.init_subclass and where to consume/filter them.
Citations:
- 1: https://peps.pythonlang.net/pep-0487/
- 2: https://bugs.python.org/issue29581
- 3: https://stackoverflow.com/questions/42281697/typeerror-when-combining-abcmeta-with-init-subclass-in-python-3-6
- 4: https://www.mail-archive.com/pypy-issue@python.org/msg03226.html
- 5: https://www.mail-archive.com/pypy-issue@python.org/msg03238.html
Keep lone-surrogate keyword names in unknown.
as_str().ok() drops valid Python str keys that are not valid UTF-8, such as "\ud800". That key reaches __init_subclass__ without an unknown entry, so object.__init_subclass__ accepts unexpected keyword arguments instead of reporting TypeError. Keep w_str_get_wtf8(k) in the unknown-key set and report the keyword name through the WTF-8 error path.
🤖 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 `@pyre/pyre-interpreter/src/typedef.rs` around lines 18967 - 18979, Update the
unknown-key collection in the kwargs handling block to retain all string keys,
including lone-surrogate names, instead of filtering through as_str().ok().
Preserve the __pyre_kw__ exclusion and use the existing WTF-8 error-reporting
path when converting or displaying these names, so object.__init_subclass__
still rejects unexpected keywords.
Source: Coding guidelines
1 participant
Summary
Four changes around
type.__new__andobject.__init_subclass__, and a rework of the parity fixtures that cover them.type.__new__runs the allocation check whole._create_new_typereaches the instance throughspace.allocate_instance(W_TypeObject, w_typetype), which runsW_TypeObject.check_user_subclass(typeobject.py:555-567) on the way in.mainhad one of that function's three arms hand-inlined; this calls the function instead._precheck_for_newsettles only that the metatype is a type at all, so the layout arm is what refuses a metatype whose instances are not laid out as type objects — without ittype.__new__(int, 'A', (), {})builds a class whose metatype isint.The one-argument form is tested positively.
descr__new__checksspace.is_w(w_typetype, space.w_type)and raises in theelse(typeobject.py:890-896); the condition was inverted.Five
_ctypesmetaclasses record the type instance layout. They derive fromtype, so their instances are type objects, and the recorded layout is whatcheck_user_subclassreads;make_builtin_type_with_basenamed the general instance layout instead.typeitself is bootstrapped withTYPE_TYPEfor the same reason. Upstream never faces the question — its ctypes metaclasses are app-level (class _CDataMeta(type),basics.py:48) and inherittype's layout from their base. Calling the allocation check without this refusesimport ctypes.object.__init_subclass__refuses an extra positional argument.descr___init_subclass__(space, w_cls)(objectobject.py:139) declares one parameter and no__args__, so everything past the bound class is refused by the argument parser before the body runs. The default here only scanned for keywords and returnedNoneotherwise, soobject.__init_subclass__(1)answeredNone— a wrong answer behind what looked like a wording difference. The refusal is routed through the parser's ownArgErrshapes (argument.py:529-627) rather than a new fixed string, which is what gets the ordering (keywords are judged before positional overflow), the singular/plural split, and the conditional "Did you forget 'self'" hint right.Parity fixtures
type_new_metatype_guard.py,object_init_subclass_arity.pyandbuiltin_new_argument_count.pyselected an expected message withsys.implementation.name, so each runtime was compared against its own literal and nothing was compared between them.Every message assertion is now a containment on the substring the runtimes share, or absent where they share nothing and only the refusal is asserted. The first two no longer branch on the runtime at all.
builtin_new_argument_count.pykeeps one branch, for the one-argument form oftype.__new__, where the runtimes differ on the outcome rather than on its spelling.Two things follow from dropping the full-message comparison: the class statements in
object_init_subclass_arity.pyno longer needexecin a bare namespace (that existed only to keep one runtime's__qualname__out of a literal), and its__subclasshook__rows now assert the one-argument call every runtime answers.#1114landedcomplex.__new__coverage inbuiltin_new_argument_count.pywhile this was in flight, including two more per-runtime message blocks. Its coverage is kept; its message assertions are folded into the same containment form, which is also what the rebase conflict in that file was. Two of them turn out to be worth keeping in full —complex.__new__(int): int is not a subtype of complexis worded identically everywhere — andcomplex.__new__(1)collapses toX is not a type object (, since one reference quotes the rejected name and the other names the call it came from first.The remaining
sys.implementationuses inextra_tests/parity_testswere checked and left: they gate on an attribute a type does not have, a behaviour a runtime dropped, or a module it does not ship — none of them compare messages.Self-review
Assisted-bySummary by CodeRabbit
Bug Fixes
Tests
bool,complex, and ctypes.