Dispatch __index__ for indexing, slicing and integer arguments - #654
Open
rewitt94 wants to merge 6 commits into
Open
Dispatch __index__ for indexing, slicing and integer arguments#654rewitt94 wants to merge 6 commits into
__index__ for indexing, slicing and integer arguments#654rewitt94 wants to merge 6 commits into
Conversation
Merging this PR will degrade performance by 4.54%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | list_comp__monty |
282.7 µs | 298.2 µs | -5.2% |
| ❌ | loop_mod_13_limits__monty |
399.5 µs | 417.1 µs | -4.21% |
| ❌ | loop_mod_13__monty |
397.8 µs | 415.3 µs | -4.2% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rewitt94:dispatch-index-dunder (1a75d6d) with main (0ce2497)
Footnotes
-
16 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty/src/types/bytes.rs">
<violation number="1" location="crates/monty/src/types/bytes.rs:824">
P2: A failing or raising `__index__` bound now copies the entire `sub` bytes object before the bound is validated, so an invalid call can perform a large untracked allocation that the old path avoided. Deferring materialization until after index conversion, while preserving the type check/order with a non-borrowing bytes handle, would keep this error path within the resource-safety model.</violation>
</file>
<file name="crates/monty/test_cases/refcount__index_dunder.py">
<violation number="1" location="crates/monty/test_cases/refcount__index_dunder.py:10">
P3: The comment claims this dunder returns a heap-allocated LongInt so a missed release is observable, but `10**30 // 10**30 + 1` evaluates to 2 — an inline Int, no heap allocation. As written the test cannot catch a leak of a `__index__` return (e.g. the overflow path in `narrow_index_to_i64` returns before `drop_with`), so the stated coverage intent isn't met; correct the value/comment or drop the LongInt claim.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let (sub, start, end) = match pos.as_slice() { | ||
| [sub_value] => { | ||
| let sub = extract_bytes_only(sub_value, vm)?; | ||
| let sub = extract_bytes_only(sub_value, vm)?.to_owned(); |
Contributor
There was a problem hiding this comment.
P2: A failing or raising __index__ bound now copies the entire sub bytes object before the bound is validated, so an invalid call can perform a large untracked allocation that the old path avoided. Deferring materialization until after index conversion, while preserving the type check/order with a non-borrowing bytes handle, would keep this error path within the resource-safety model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/bytes.rs, line 824:
<comment>A failing or raising `__index__` bound now copies the entire `sub` bytes object before the bound is validated, so an invalid call can perform a large untracked allocation that the old path avoided. Deferring materialization until after index conversion, while preserving the type check/order with a non-borrowing bytes handle, would keep this error path within the resource-safety model.</comment>
<file context>
@@ -815,18 +815,22 @@ fn parse_bytes_sub_args(
let (sub, start, end) = match pos.as_slice() {
[sub_value] => {
- let sub = extract_bytes_only(sub_value, vm)?;
+ let sub = extract_bytes_only(sub_value, vm)?.to_owned();
(sub, 0, len)
}
</file context>
Comment on lines
+10
to
+13
| def __index__(self): | ||
| return 10**30 // 10**30 + 1 | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
P3: The comment claims this dunder returns a heap-allocated LongInt so a missed release is observable, but 10**30 // 10**30 + 1 evaluates to 2 — an inline Int, no heap allocation. As written the test cannot catch a leak of a __index__ return (e.g. the overflow path in narrow_index_to_i64 returns before drop_with), so the stated coverage intent isn't met; correct the value/comment or drop the LongInt claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/test_cases/refcount__index_dunder.py, line 10:
<comment>The comment claims this dunder returns a heap-allocated LongInt so a missed release is observable, but `10**30 // 10**30 + 1` evaluates to 2 — an inline Int, no heap allocation. As written the test cannot catch a leak of a `__index__` return (e.g. the overflow path in `narrow_index_to_i64` returns before `drop_with`), so the stated coverage intent isn't met; correct the value/comment or drop the LongInt claim.</comment>
<file context>
@@ -0,0 +1,38 @@
+
+
+class WideIndex:
+ def __index__(self):
+ return 10**30 // 10**30 + 1
+
</file context>
Suggested change
| def __index__(self): | |
| return 10**30 // 10**30 + 1 | |
| # The dunder's return value is owned by the coercion, which must release it once | |
| # narrowed to an `i64`. NOTE: `10**30 // 10**30 + 1` folds to 2, an inline | |
| # `Value::Int` — not a heap-allocated LongInt — so a missed release of the return | |
| # is NOT observable here; a LongInt return beyond i64 would raise out-of-range | |
| # before the coercion release. Return a genuinely heap-allocated LongInt or drop | |
| # the LongInt claim. |
A class defining `__index__` was rejected everywhere CPython accepts one:
`seq[obj]`, `seq[obj:]`, `range(obj)`, `'x'.center(obj)`. Monty's error
messages already promised the protocol ("slice indices must be integers
or None or have an `__index__` method") while nothing ever called it, and
the type checker accepts such code — so this failed only at runtime.
`instance_index` joins the existing sync-dunder callers in `instance.rs`;
no new dispatch machinery was needed. It validates the result is a real
int, which is both CPython's `__index__ returned non-int` check and what
stops a class returning another such instance from recursing.
`as_int`/`as_index` take `&mut VM` to call it. Every other consumer of
those already had one, so the fallout was five sites: `repeat_times`, a
heap borrow held across a bound read in `parse_bytes_sub_args`, and two
closures in `parse_bytes_justify_args` (now free functions). CPython's
left-to-right argument order is preserved at both bytes sites.
Four hand-rolled coercions that duplicated the fast path are folded into
the shared ones, so they pick up `LongInt` and `__index__` together:
the interned `str`/`bytes` subscripts, `str::optional_index`, and
`str::extract_int_arg` — whose wording was not CPython's, so
`'x'.center('a')` now reports `'str' object cannot be interpreted as an
integer` rather than `expected int`, and `'x'.center(10**30)` raises
`OverflowError` rather than `TypeError: integer too large`.
Sequence repetition (`'ab' * obj`) is NOT covered: each of the eight
`py_mul_impl`s owns its coercion, which is the binary-operator surface
rather than the indexing one. Documented in limitations/classes.md along
with `slice()` storing coerced bounds instead of the objects passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four sites hand-rolled the same dispatch: test the value for `HeapData::Instance`, then call `instance_index`. Three of them (`slice::value_to_option_i64`, `str::optional_index`, `from_value::resolve_index_dunder`) imported `instance_index` directly and so had to know how a user class is represented on the heap, which is otherwise `value.rs`/`instance.rs` business. Add `Value::try_index`, which answers "is there an `__index__` here, and what did it return" and yields `Ok(None)` for both "not an instance" and "no `__index__`" — the two cases every caller already treated alike, since each raises its own wording. The callers lose the import, the heap probe and, in the two recursive ones, an early return. `as_int`/`as_index` fold their instance arm into their existing fallback and drop `index_dunder_as_i64`'s `on_missing` parameter, which only existed to carry a message the caller can now raise itself; what remains is `narrow_index_to_i64`. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`value_to_option_i64` only took `Int`/`Bool` directly, so every `LongInt` bound fell through to `try_index`, which answers `None` for anything that isn't an `Instance` — turning a slice bound beyond `i64` into `TypeError: slice indices must be integers or None or have an __index__ method`. That hit two ways. A literal bound (`[1, 2, 3][10**30:]`) was already rejected before this branch. The `__index__` path then inherited the same hole: the dunder's result is fed back through this function, so a class returning `10**30` was rejected too, despite the protocol being the whole point of the branch. CPython clamps in both cases, so `[1, 2, 3][10**30:]` is `[]`. Add `Value::long_int_to_i64_saturating`, which pins an out-of-range `LongInt` to the bound it overflowed past, and take it in the arm ahead of the `try_index` dispatch — so it catches literal bounds on the way in and `__index__` results on the way back through the recursion. Deliberately separate from `narrow_index_to_i64`, which must keep raising for plain indexing and integer arguments. One divergence remains, now documented: since bounds are coerced at construction, `slice(10**30).stop` reads back as `i64::MAX` rather than `10**30`. Slicing itself matches CPython. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`as_int` had no `Bool` arm, so `True`/`False` fell through to `try_index`, which only dispatches for user instances and answered `None` — making `range(True)` raise `TypeError: 'bool' object cannot be interpreted as an integer` where CPython gives `[0]`. `bool` is an `int` subclass in CPython, so it satisfies these arguments directly rather than via the protocol. The gap predates this branch, but the `__index__` work makes it stand out: `as_int` now advertises the full `PyNumber_Index` contract while rejecting the one built-in type that most obviously satisfies it. `as_index`, the subscript-side twin, already had the arm — this brings the two in line. Reaches every "cannot be interpreted as an integer" consumer, so `'x'.center(True)`, `s.find(sub, True)`, `expandtabs(True)`, `itertools.repeat(x, True)` and `round(f, True)` all match CPython now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`extract_int_arg` routes through `as_int`, which enforces C `ssize_t`, but CPython declares `tabsize` as a C `int`. So `'a\tb'.expandtabs(2**31)` raises `OverflowError: Python int too large to convert to C int` there and was accepted here — spending 30 seconds building the 2GB string that a tabsize that large implies. `expandtabs` is the method that motivated `StringBuilder`; this closes the same amplification one step earlier, at the argument rather than at the allocation. CPython reports `... to C int` at every magnitude, including for a value arriving through `__index__` — a path this branch newly opened. Matching that means the overflow error has to be the caller's to pick, so `as_int` grows an `as_int_with_overflow` variant and keeps its own `ssize_t` wording. `extract_c_int_arg` layers the i32 range check on top; consumers narrower than `i64` still bound the result themselves. Also switch the argument to `defer_drop!`. The old trailing `drop_with` left `tabsize`'s reference behind whenever the conversion raised, which was unreachable while the conversion only rejected non-ints and is not anymore. `refcount__str_arg_error_paths.py` covers it — with the previous `drop_with` restored it reports `leaked 1: BigIndex`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase port: `as_int` takes `&mut VM` since it may run a user `__index__`, and `islice_index` — added to main after this branch forked — still passed `&VM`. Threading the mutable borrow through makes the bound conversion able to raise, so it returns `RunResult` and only a type mismatch stays `Invalid`; a raising `__index__` propagates instead of being reworded as "bad indices". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rewitt94
force-pushed
the
dispatch-index-dunder
branch
from
August 12, 2026 14:57
cc14c52 to
1a75d6d
Compare
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
__index__is Python's "this object is an integer" protocol (PEP 357), called wherever an exact integer is required for a non-arithmetic purpose: sequence subscripts, slice bounds,range(), and integer arguments like'x'.center(n). It is deliberately narrower than__int__— a lossy conversion thatfloatalso defines — which is whyseq[3.7]is aTypeErrorwhile a class defining__index__indexes fine.The gap. Raised by cubic on pydantic/monty#635 as an
islicebug, but notislice-specific: Monty dispatched ten dunders, not__index__, so a class defining it was rejected everywhere CPython accepts an index, though the error messages already promised the protocol andmonty -talready accepted the code.The fix.
instance_indexjoins the existinginstance_call_dunder_syncdispatch and validates an int result;as_int/as_indexnow take&mut VM. Four hand-rolled int fast paths fold into them, picking upLongIntand correcting twostrargument errors to match CPython.Not covered. Sequence repetition (
'ab' * obj) andslice()storing coerced bounds rather than the original object — both documented inlimitations/classes.md.Testing. New
class__index_dunder.py/refcount__index_dunder.pycases dual-run against CPython; suite green underref-count-returnandmemory-model-checks.Unrelated, pre-existing. An exception raised in any sync dunder escapes
try/exceptwhen the receiver is a variable (1 in c, but not1 in C()). Reproduces on cleanmain; not fixed here.