pypy parity: index/slice coercion, sequence concat, in-place operators, overflow guards#560
pypy parity: index/slice coercion, sequence concat, in-place operators, overflow guards#560youknowone wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 35 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 (14)
✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f625bb1929
ℹ️ 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".
| } else { | ||
| (v as usize).min(char_len as usize) | ||
| } | ||
| crate::sliceobject::adapt_lower_bound(char_len, args[3])? |
There was a problem hiding this comment.
Return false for out-of-range empty-prefix checks
When start and end are both supplied past the string length, leaving this positive end unbounded makes start <= end, and the later nth() calls map both offsets to bytes.len(), producing an empty window. That makes cases such as 'abc'.startswith('', 5, 10) and endswith('', 5, 10) return True, while PyPy's unicodeobject.py:_unwrap_and_compute_idx_params forces start_index = end_index + 1 when start > length, so the match remains False even for an empty prefix. Clamp end to char_len or otherwise preserve the out-of-range-start sentinel before comparing/building the window.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 15bdca6). 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
|
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b1c7ed4a8a77f9e41a9c240c7a5fb4f58d231850/pyre-interpreter/src/objspace/descroperation.rs#L92-L93
Preserve zero left shifts before reserving
When the left operand is zero and the shift count is huge but nonnegative, Python returns zero without needing to allocate the shifted magnitude (e.g. 0 << 10**18). This helper now sizes the reservation from the shift count before checking the value, so both the int and long paths that call it can raise MemoryError for a valid zero result; special-case a zero a before computing/reserving limbs.
ℹ️ 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".
getindex_w (baseobjspace.py:1574) remaps a TypeError raised while coercing a subscript key through __index__ — a non-int __index__ return, or a TypeError raised inside __index__ — to "<descr> indices must be integers or slices, not '<type>'" whenever objdescr is set. The inlined subscript coercions (list/tuple/str/bytes __getitem__, list/bytearray __setitem__, and subscript_index_w on the __delitem__ path) previously let space.index's "__index__ returned non-int (type X)" propagate verbatim. A ValueError from __index__ still propagates; list.insert / list.pop pass no objdescr and keep surfacing the raw error. ord() on a multi-byte bytes argument now reports "bytes of length N" (bytesobject.py:473); bytearray keeps "string of length N" (bytearrayobject.py:213) — the shared branch reported "string" for both. Comment-only: pow3 notes the omitted is_cpytype() pow3_bug_compat_cpyext branch; index_type_error notes the reference pypy3 quotes the %T operand. Assisted-by: Claude
Route integer coercion through the index protocol and align sequence-concatenation errors with pypy across the slice, index, format, round and concat paths. - builtins::getindex_w: clamp an out-of-word overflow to i64::MIN for a negative value (was always i64::MAX), so a large-negative slice bound floors to 0 instead of collapsing to the tail. - normalize_slice / sliceobject::adapt_lower_bound: fold a negative index by the length with saturating_add so an i64::MIN bound does not overflow before flooring at 0. - str_slice_args (startswith/endswith): take start/end bounds through adapt_lower_bound(eval_slice_index) so a non-index bound raises, an __index__ object is honoured, and a positive bound is not upper-clamped. - builtin_round: take ndigits through getindex_w for a float (clamping) and index for an int; a huge-magnitude negative ndigits short-circuits to 0 rather than building an astronomical power of ten. - index_to_bigint (hex/oct/bin): coerce through space.index so a non-int __index__ return raises "__index__ returned non-int (type X)". - formatting number_arg_decimal / number_arg_integer (%d/%i/%o/%x): remap a TypeError from the numeric decoder to the operand-type error naming the original argument. - list/tuple/bytes/bytearray __add__ and descroperation::add: return NotImplemented for a non-sequence operand so the operator raises the generic "unsupported operand type(s) for +", dropping the cpython-only "can only concatenate" message; bytearray __iadd__ on a non-buffer now raises "a bytes-like object is required, not 'X'". Assisted-by: Claude
Add the in-place operator-module functions and the mutable set's in-place operator slots, both previously missing. - operator: iadd/isub/imul/ifloordiv/imod/itruediv/ipow/ilshift/irshift/ iand/ior/ixor route through binary_value's in-place path (space.inplace_X), and iconcat requires both operands to be subscriptable. The extra_init dunder aliases (operator.__iadd__ etc.) now resolve as a result. - set: __isub__/__iand__/__ior__/__ixor__ mirror setobject.py's descr_inplace_sub/_and/_or/_xor — a non-set/-frozenset operand yields NotImplemented, otherwise self is mutated through the matching update helper and returned. The four *_update method bodies are extracted into named functions shared by the app methods and the in-place slots. Assisted-by: Claude
A left shift whose result exceeds allocatable memory (e.g. `1 << 10**18`) drove the underlying big-int `<<` into the infallible allocator and aborted the process. Pre-flight a fallible reservation of the result's 64-bit limb count in checked_bigint_lshift and raise a catchable MemoryError instead, matching long_lshift. int_lshift and long_lshift route through the checked helper; the probe upper-bounds the shift's own allocation so it fails exactly when the real shift would. Assisted-by: Claude
A float or complex power coerces each int operand to a double before the power is computed, so an over-range int base or exponent raises OverflowError up front -- even `1.0 ** huge`, which never reaches the arithmetic, matching float(huge). reject_pow_operand_overflow performs that check and is called on both operands in the two-argument pow / pow_builtin float and complex paths. In float.__pow__/__rpow__ the check runs before float_pow_reject_modulus, so `pow(2.0, huge, 5)` raises OverflowError from the coercion rather than TypeError from the modulus rejection. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35081d2f4d
ℹ️ 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".
| let result_bits = a.bits().saturating_add(shift); | ||
| let limbs = (result_bits / 64).saturating_add(2); |
There was a problem hiding this comment.
Preserve zero for huge left shifts
When the left operand is zero and the shift count is very large but still fits in i64 (for example 0 << 10**18), this preflight sizes the reservation from shift alone and returns MemoryError before doing the shift. PyPy/CPython return 0 for zero shifted by any non-negative count, so this changes valid code that used to be allocation-free; short-circuit zero before the reservation in this helper so both the int and long callers keep that behavior.
Useful? React with 👍 / 👎.
Drains the remaining low-risk pypy-parity items from the 3-way (cpython/pypy/pyre) sweep — the integer-coercion, sequence-concatenation, in-place-operator, and numeric-overflow paths. Each behavior was probed against the installed
pypy3(7.3.20, thecheck.pyoracle) and now matches byte-for-byte. Where cpython and pypy disagree (so acheck.pybaseline test cannot cover it), the behavior was verified by hand againstpypy3.objspace: remap __index__ TypeError; ord names bytes nounCarries over the two PR #552 review fixes that had not yet landed:
getindex_wremaps aTypeErrorraised by a subscript's__index__(a non-int return, or aTypeErrorfrom inside__index__) to<descr> indices must be integers or slices, not '<type>'; aValueErrorstill propagates. Applied to the list/tuple/str/bytes get, list/bytearray set, and delitem sites.ord(b"ab")reportsbytes of length 2whilebytearray(b"ab")keepsstring of length 2.pypy parity: index coercion and sequence-concat errorsbuiltins::getindex_w— an out-of-word overflow now clamps toi64::MINfor a negative value (was alwaysi64::MAX), so a large-negative slice bound floors to0instead of collapsing to the tail:[1,2,3][-(10**30):]→[1, 2, 3].normalize_slice/sliceobject::adapt_lower_bound— fold a negative index by the length withsaturating_addso ani64::MINbound does not overflow before flooring at 0.str_slice_args(startswith/endswith) — take thestart/endbounds throughadapt_lower_bound(eval_slice_index(...)), so a non-index bound raisesslice indices must be integers or None or have an __index__ method, an__index__object is honoured, and a positive bound is not upper-clamped.builtin_round— takendigitsthroughgetindex_w(float: clamping; int:space.index); a huge-magnitude negativendigitsshort-circuits to0rather than building an astronomical power of ten (round(3.14159, 2**63)→3.14159).index_to_bigint(hex/oct/bin) — coerce throughspace.indexso a non-int__index__return raises__index__ returned non-int (type X).number_arg_decimal/number_arg_integer(%d/%i/%o/%x) — remap aTypeErrorfrom the numeric decoder to the operand-type error naming the original argument (%d format: a real number is required, not BadIdx).list/tuple/bytes/bytearray__add__anddescroperation::add— returnNotImplementedfor a non-sequence operand so the operator raises the genericunsupported operand type(s) for +, dropping the cpython-onlycan only concatenatemessage;bytearray.__iadd__on a non-buffer now raisesa bytes-like object is required, not 'X'.fix warningsRemoves three unused-binding warnings in the rtyper (
classdesc.rs,rclass.rs,rpbc.rs).operator/set: in-place operator functions and set slotsoperatormodule gains the in-place functionsiadd,isub,imul,imatmul,ifloordiv,imod,itruediv,ipow,ilshift,irshift,iand,ior,ixor(each dispatching the matching in-place binary op) andiconcat(which requires both operands to be subscriptable, else raises'X' object can't be concatenated). The__iadd__→iaddalias table now resolves to real callables.setgains the__isub__,__iand__,__ior__,__ixor__slots, sos &= tandoperator.iand(s, t)update in place; a non-set right operand yieldsNotImplemented.int: raise MemoryError from an unallocatably-large left shift1 << (10**18)drove the big-int<<into the infallible global allocator and aborted the process. A newchecked_bigint_lshiftpre-flights a fallible reservation of the result's 64-bit limb count and raises a catchableMemoryErrorinstead;intandlongleft shifts route through it. The shift count is carried asu64so a 32-bitusizetarget (wasm) does not truncate a10**18shift.float/complex pow: over-range int operand overflowsfloat/complexpower coerces eachintoperand to a double before the power is computed, so an over-rangeintbase or exponent raisesOverflowErrorup front — even1.0 ** huge, which never reaches the arithmetic, matchingfloat(huge). Infloat.__pow__/__rpow__the check runs before the ternary-modulus rejection, sopow(2.0, huge, 5)raisesOverflowErrorfrom the coercion rather thanTypeErrorfrom the modulus.Verification
__index__objects, huge/negative bounds, non-seq concat,%-format, in-place set/operator ops,1 << 10**18, over-range float/complex pow) are byte-identical topypy3across all cases; the cpython≠pypy edges (3-arg pow ordering, lshift OOM message) were hand-verified againstpypy3.hex/round/%-format /startswith/ in-place-op / small-shift / ordinary-pow paths are byte-identical topypy3(no regression).operator_set_inplace_ops,int_lshift_memoryerror,float_pow_overflow_exp(each restricted to cpython==pypy cases).check.py: dynasm 190/190, cranelift 190/190, wasm 189/189 (3/3 backends).Deferred (own follow-up slices)
int.__lshift__('a', 1)returnsNotImplementedwhere pypy raisesTypeError "'int' object expected, got 'str' instead"— needs a systematic unbound-descriptor self-type check across every typed-self builtin; cpython≠pypy so not baseline-testable.x &= yaugmented-assignment error symbol — a type mismatch reports the binary-op symbol&rather than&=(e.g.int += str). Needs anOption-returning binop path so the augmented-assign layer can re-emit with theX=symbol.🤖 Generated with Claude Code