Store StopIteration.value in a struct field instead of the instance dict#8301
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStopIteration now stores ChangesStopIteration storage and construction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VirtualMachine
participant invoke_exception
participant PyStopIteration
VirtualMachine->>VirtualMachine: build args from value
VirtualMachine->>invoke_exception: create StopIteration with args
invoke_exception->>PyStopIteration: initialize instance
PyStopIteration-->>VirtualMachine: return exception
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pydoc.py dependencies:
dependent tests: (5 tests)
[x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
[x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (222 tests)
[ ] test: cpython/Lib/test/test_set.py (TODO: 5) dependencies: dependent tests: (no tests depend on set) Legend:
|
| self.ctx.exceptions.stop_iteration.to_owned(), | ||
| Some(dict), | ||
| ) | ||
| exc.unwrap() |
There was a problem hiding this comment.
I'm not suggesting we fix this in this PR, but I'm leaving it here for the record.
The reason Err is returned in invoke_exception is because the conversion to PyBaseException fails. However, since PyStopException has PyException as its base, it seems like this error shouldn't occur. That said, having to call unwrap due to the code structure feels a bit awkward, and there seems to be room for improvement to resolve this at the type level.
There was a problem hiding this comment.
at least let's use expect instead of unwrap.
There was a problem hiding this comment.
Thanks, I agree. invoke_exception returns Err only when the downcast to
PyBaseException fails, and that can't happen for a type whose base is
PyException, so the unwrap() is asserting what the type system already
knows. Solving it at the type level would be cleaner.
Also related: invoke_exception goes through a full PyType::call, and
new_stop_iteration is on a hot path, so I wonder if constructing via
py_new directly would be better here. The // TODO: fast-path built-in exceptions comment in invoke_exception seems to point at the same thing.
I'll keep both out of this PR and open a follow-up issue. I'd like to work
on it if that direction sounds good to you.
| self.ctx.exceptions.stop_iteration.to_owned(), | ||
| Some(dict), | ||
| ) | ||
| exc.unwrap() |
There was a problem hiding this comment.
at least let's use expect instead of unwrap.
| pub struct PyStopIteration { | ||
| base: PyException, | ||
| value: PyAtomicRef<Option<PyObject>>, | ||
| } |
|
I created #8320 while reviewing this PR. if you are interested in, please look in it |
Summary
StopIteration.valuewas stored in the instance__dict__, soStopIteration().__dict__was non-empty and clearing the dict lost the value, unlike CPython. It's now a struct field onPyStopIteration(#[repr(C)]+ base field, the same approach asOSErrorandSystemExitin #8282), exposed via#[pygetset]— writable, deletable, and independent fromargs, with an empty__dict__as in CPython.new_stop_iterationis also routed through the full constructor path instead of building aPyBaseExceptionwith a manual dict. This fixes a crash caused by the added payload (the old path allocated a base-sized object) and removes the per-instance dict allocation on this hot path.Test plan
valueforStopIteration()/(5)/(1, 2), write/delete,dict.clear(),vars(), empty__dict__, and generatorreturnvalue delivery (incl.yield from).test_generators,test_generator_stop,test_yield_from: all pass.test_exceptions/extra_tests/snippets/builtin_exceptions.py: no regressions (the singletest_badisinstancefailure is pre-existing onmain).cargo fmt/cargo clippy: clean (no new warnings).test_member_descriptor: it now passes becauseStopIteration.valueis a real descriptor whose pydoc summary line renders asvalue(it was previously a plainNoneclass attribute).Note this does not add
__doc__support to getset descriptors —StopIteration.value.__doc__still differs from CPython, which is a separate pre-existing gap inPyGetSet.Assisted-by: Claude Code:claude-fable-5
Summary by CodeRabbit
Summary by CodeRabbit
StopIterationexception handling and initialization to make the exception’svaluereliably accessible and updatable.StopIterationis constructed, ensuring the return payload is consistently preserved whether a value is provided or not.