Repository navigation
Accept a negative or __index__ default in operator.length_hint() - #9515
Conversation
length_hint() required its default to be an exact int and converted it to usize, so a default with __index__ raised TypeError and a negative default raised ValueError. Bind the default as isize, which goes through __index__, and return it unchanged when the object has no length hint. Assisted-by: Claude Code:claude-opus-5-5
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Length hint conversion and checks crates/vm/src/stdlib/_operator.rs, extra_tests/snippets/stdlib_operator.py |
length_hint now accepts an isize default and returns PyResult<isize>. Tests check valid and invalid hints, exceptions, and default values. |
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Bug fix · Severity of issue fixed: Low
Suggested reviewers: youknowone
Merge Risk: 
cbe61
-
The change supports negative and index-convertible defaults while preserving available length hints. No concrete merge-blocking issue was identified; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: supporting negative defaults and index defaults in operator.length_hint(). Linked Issues check The changes satisfy the coding requirements in [ #9509].LengthHintArgs.defaultnow usesisize, so argument conversion uses__index__and preserves signed values.vm.length_hint_opt(obj)?propa…Out of Scope Changes check The pull request changes only operator.length_hint()and its focused snippet tests. Removing the unusedPyIntimport supports the implementation change. The added tests directly verify the behavio…Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in [
#9509].LengthHintArgs.defaultnow usesisize, so argument conversion uses__index__and preserves signed values.vm.length_hint_opt(obj)?propagates errors from__length_hint__and from default conversion. The function returns the signed default when no hint exists. The added tests cover an__index__default, an exception from__index__, negative defaults with and without a hint, invalid length hints, and an out-of-range default.
Full details: Out of Scope Changes check
-
Explanation
The pull request changes only
operator.length_hint()and its focused snippet tests. Removing the unusedPyIntimport supports the implementation change. The added tests directly verify the behaviors requested by [#9509]. No unrelated change is present in the reviewed diff.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbe6197b34
ℹ️ 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 () | ||
|
|
||
|
|
||
| assert _operator.length_hint(object(), Index()) == 5 |
There was a problem hiding this comment.
The root AGENTS.md explicitly limits test-file modifications to adding/removing @unittest.expectedFailure decorators and removing associated TODO comments. These new assertions and supporting classes fall outside that allowance. Keep the Rust fix, but remove the added snippet changes unless an explicit exception to the repository policy is approved.
AGENTS.md reference: AGENTS.md:L280-L282
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That section covers the CPython tests in Lib/test. For extra_tests/snippets, AGENTS.md asks for new tests in the most closely related existing file (L119-L120), which is what this does.
operator.length_hint()rejects a negativedefaultand adefaultthat defines__index__#9509Summary
length_hintacceptsdefaultonly as anintinstance and converts it tousize, so a negative default raisesValueErrorand a default with__index__raisesTypeError. CPython convertsdefaulttoPy_ssize_twith_PyNumber_Index(Modules/clinic/_operator.c.h) and returns it unchanged when the object gives no hint.AS-IS
TO-BE
(matching CPython)
Changes
defaultasisizeincrates/vm/src/stdlib/_operator.rs, so it goes through__index__, and returnvm.length_hint_opt(obj)?.map_or(default, …). The exact-intcheck and the now unusedPyIntimport go away.extra_tests/snippets/stdlib_operator.pywith an__index__default, an__index__that raises, a negative default with and without a hint, a__length_hint__returning a non-int, and an out-of-range default.Test plan
Built and run on macOS aarch64 against upstream/main 713e14c, using the CI feature set
--no-default-features --features stdlib,importlib,stdio,encodings,sqlite,ssl-rustls-aws-lc,host_env,doc,ucd_3_2_0, plusthreading, compared with CPython 3.15.2.extra_tests/snippets/stdlib_operator.pyfails on an unpatched build of that commit (at the__index__default) and passes with the patch and under CPython 3.15.2.rustpython -m test test_operator test_iter test_list test_bytes: 569 run, 17 skipped, all pass.pytest -k rustpythoninextra_tests: 248 pass.cargo fmt --checkclean,cargo clippy -p rustpython-vm -- -Dwarningsreports nothing, and the pre-commit hooks pass on both changed files.I used Claude Code (claude-opus-5-5) to draft the analysis, the patch and this description; I reviewed the change and ran every command above myself.
Summary by CodeRabbit
length_hintbehavior to correctly accept integer-like length hints, return the supplied default when no hint is available, and preserve valid hints such as zero.