Repository navigation
Interpreter correctness: attribute/error-message parity, str width - #614
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
youknowone
changed the title
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 4f59f2b). 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
|
youknowone
changed the title
youknowone
changed the title
youknowone
changed the title
15ce68b to
9bf6392
Compare
youknowone
changed the title
![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: 9bf639211b
ℹ️ 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".
| return PyError::attribute_error_with_context( | ||
| format!("'{tp_name}' object attribute '{name}' is read-only"), | ||
| obj, | ||
| name, | ||
| ); |
There was a problem hiding this comment.
When w_descr is present, this is the PyPy raiseattrerror(..., w_descr) read-only path, not a failed lookup path; PyPy raises a plain AttributeError here while only the missing-attribute branch uses the attribute-error context helper. With this call, setting or deleting a class descriptor on a dict-less object, e.g. a __slots__ = () instance whose class defines f, leaves e.name and e.obj populated even though CPython/PyPy expose them as None for the read-only error, so user code catching the exception can observe the wrong exception state.
Useful? React with 👍 / 👎.
object.__delattr__'s terminal miss built the AttributeError from the raw ob_type name, which for a user instance resolves to the base object type rather than the instance's actual type: `del Slotted().b` reported "'object' object has no attribute 'b'". Route the terminal through raiseattrerror, the same helper object.__setattr__ and the module/type miss branches already use. The receiver's own type is named and the name/obj context is attached. `del Slotted().b` now reports "'Slotted' object has no attribute 'b'", matching both CPython and PyPy. Assisted-by: Claude
object.__setattr__ / object.__delattr__ on a dict-less receiver (a
__slots__ instance, an int/str, ...) reported a genuine miss
("'X' object has no attribute 'Y'") when the name actually resolved to a
non-settable descriptor on the type: a method, or a data descriptor with
no reachable __set__/__delete__.
descr__setattr__/descr__delattr__ pass the looked-up descriptor to
raiseattrerror, which reports "'X' object attribute 'Y' is read-only" when
a descriptor is present. Thread that descriptor through the setattr and
delattr terminals (the general-object misses only; the module/type misses
keep their existing behaviour, which already matches on the common path).
del (5).bit_length and setattr(Slotted(), 'm', 1) now raise
"... is read-only", matching both CPython and PyPy.
Assisted-by: Claude
Exception.add_note() with a non-str argument raised the bare
"note must be a string". interp_exceptions.py:257-260 raises
"note must be a str, not %T", naming the rejected argument's type.
ValueError('x').add_note(5) now raises "note must be a str, not int",
matching PyPy.
Assisted-by: Claude
divmod(int, 0) computed floordiv() first, so a zero divisor surfaced
floordiv's "integer division or modulo by zero" rather than divmod's own
message. intobject.py:356-360 _divmod raises "integer divmod by zero".
Check for a zero int/long divisor before the floordiv/mod split; a float
operand still falls through to the float path ("float modulo").
Assisted-by: Claude
The str padding methods read the width argument with raw `w_int_get_value(args[1])`, which does not check the object is an int. A non-int width (str, float, None) read arbitrary storage as an i64, producing a huge width that drove `Wtf8Buf::with_capacity` and the fill loop into an effectively unbounded allocation/hang; a bignum width was misread as garbage. Parse the width through `space_index_w`, matching the sibling `expandtabs`: it accepts int/bool/__index__, rejects a non-index with "'X' object cannot be interpreted as an integer", and propagates an exception raised inside __index__. `space_index_w` itself did not handle long (bigint) objects: their `__index__` returns the long, which failed the int/bool result checks and fell through to a spurious TypeError. Add a long branch that returns the machine-word value or raises OverflowError "int too large to convert to int" when it does not fit, mirroring `int_w`. This also fixes `'x'.expandtabs(10**100)` and the bytes pad methods, which already route width through `space_index_w`. Assisted-by: Claude
The read-only branch of `raiseattrerror` used `attribute_error_with_context`, populating the AttributeError's `name` and `obj`. Both CPython and PyPy expose those as `None` for the read-only error (only the missing-attribute path carries the lookup context). Raise a plain `attribute_error` so `e.name` / `e.obj` read back as `None`, matching the cited `descroperation.py` raiseattrerror. Assisted-by: Claude
9bf6392 to
4f59f2b
Compare
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f59f2bde6
ℹ️ 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".
| // module/type misses above — so the receiver's own type is named and the | ||
| // obj/name context is attached, rather than the bare `object` base. | ||
| // `w_descr` carries a found-but-non-data descriptor so the miss is read-only. | ||
| Err(raiseattrerror(obj, name, w_descr)) |
There was a problem hiding this comment.
When the receiver has a writable instance dict but the key is absent, this still passes the class-level descriptor into raiseattrerror, selecting the read-only branch. For example, class C: x = 1; c = C(); del c.x should be a normal missing-attribute delete from the instance dict, but after getdict_backing materializes the dict and removed is false, this line reports x as read-only; only dict-less misses should carry the descriptor context to the terminal error.
Useful? React with 👍 / 👎.
Interpreter-level correctness fixes, each verified against CPython 3.14
and PyPy 3.11 and gated on both backends (dynasm 216/216, cranelift
216/216).
del inst.missingon a dict-less user instance named thebase
objecttype; now reports the receiver's actual type viaraiseattrerror.on a dict-less receiver reported "has no attribute"; now reports
"'X' object attribute 'Y' is read-only", matching CPython and PyPy.
The read-only error is a plain
AttributeError—e.name/e.objread back as
None, as on CPython/PyPy (only the missing-attributepath carries the lookup context).
Exception.add_note(5)said "note must be a string";now "note must be a str, not int", naming the offending type.
divmod(1, 0)surfaced floordiv's "integer division ormodulo by zero"; now the dedicated "integer divmod by zero".
zfill/center/ljust/rjustread the width argument witha raw unchecked int accessor, so a non-int width (str/float/None) read
arbitrary storage as a huge width and drove an effectively unbounded
allocation/hang; a bignum width was misread. Width now parses through
space_index_w(accepts int/bool/__index__, rejects a non-index),and
space_index_wgained a long branch (OverflowError when a bigintdoes not fit) that also fixes
expandtabsand the bytes pad methods.Two earlier commits were dropped when this branch rebased onto
main:the REPL
SystemExitfix landed as #604 and the explicit-__class__preservation as part of #562 — both verified equivalent, no regression.
— commented by Claude