Match CPython's str subscript TypeError message#8319
Conversation
`str.__getitem__` with a non-index key raised the shared sequence
wording ("str indices must be integers or slices or classes that
override __index__ operator, not 'x'") instead of CPython's
str-specific "string indices must be integers, not 'x'" (bpo-44110 /
gh-88276, Python 3.11).
Add `SequenceIndex::try_from_borrowed_object_with_err`, which takes a
caller-supplied error closure, and have the existing
`try_from_borrowed_object` delegate to it with the unchanged shared
message. `str._getitem` now supplies the CPython-accurate message,
which flows through both `__getitem__` and the mapping subscript slot.
Other sequence types keep the shared wording.
Removes the expectedFailure marker on
`string_tests.StringLikeTest.test_subscript`, fixing it for both
test_str and test_userstring.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesString index error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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: [ ] test: cpython/Lib/test/test_set.py (TODO: 5) dependencies: dependent tests: (no tests depend on set) [x] lib: cpython/Lib/xml dependencies:
dependent tests: (34 tests)
[x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
[x] lib: cpython/Lib/test/libregrtest dependencies:
dependent tests: (no tests depend on regrtest) [x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (222 tests)
Legend:
|
There was a problem hiding this comment.
Pull request overview
Aligns RustPython’s str subscripting TypeError wording with CPython by allowing SequenceIndex parsing to reuse the shared index logic while letting callers customize the “non-index object” error message. This removes a now-unnecessary expectedFailure in the CPython-derived string test suite.
Changes:
- Added
SequenceIndex::try_from_borrowed_object_with_errto centralize index parsing while allowing customTypeErrorconstruction for non-index objects. - Updated
strsubscripting to use the CPython-matching message:string indices must be integers, not '<type>'. - Removed
@unittest.expectedFailurefromLib/test/string_tests.py::test_subscriptnow that behavior matches CPython.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Lib/test/string_tests.py | Removes expectedFailure marker for test_subscript now that str subscript error message matches CPython. |
| crates/vm/src/sliceable.rs | Introduces try_from_borrowed_object_with_err and refactors the existing helper to delegate, preserving existing messages for other sequence types. |
| crates/vm/src/builtins/str.rs | Uses the new helper to raise the CPython-accurate TypeError message for invalid str indices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/sliceable.rs`:
- Around line 266-272: Use the class name string in both error-formatting sites:
append `.name()` to `obj.class()` in `crates/vm/src/sliceable.rs` lines 266-272
and to `needle.class()` in `crates/vm/src/builtins/str.rs` lines 663-668,
preserving the exact CPython-compatible wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: de10d7d7-f5ed-4f7f-a75e-8a9988c893a6
⛔ Files ignored due to path filters (1)
Lib/test/string_tests.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/builtins/str.rscrates/vm/src/sliceable.rs
| Self::try_from_borrowed_object_with_err(vm, obj, || { | ||
| vm.new_type_error(format!( | ||
| "{} indices must be integers or slices or classes that override __index__ operator, not '{}'", | ||
| type_name, | ||
| obj.class() | ||
| )) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use .name() to format the class name correctly.
Passing the type object directly to format! may fail to compile or produce an incorrect string representation (like <class 'str'> instead of str, defeating the PR's objective of strictly matching CPython's exact error wording). Consistent with other error formatting in the codebase, call .name() on the class to extract just the type name string.
crates/vm/src/sliceable.rs#L266-L272: append.name()toobj.class().crates/vm/src/builtins/str.rs#L663-L668: append.name()toneedle.class().
📍 Affects 2 files
crates/vm/src/sliceable.rs#L266-L272(this comment)crates/vm/src/builtins/str.rs#L663-L668
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/sliceable.rs` around lines 266 - 272, Use the class name string
in both error-formatting sites: append `.name()` to `obj.class()` in
`crates/vm/src/sliceable.rs` lines 266-272 and to `needle.class()` in
`crates/vm/src/builtins/str.rs` lines 663-668, preserving the exact
CPython-compatible wording.
| pub fn try_from_borrowed_object_with_err( | ||
| vm: &VirtualMachine, | ||
| obj: &PyObject, | ||
| err: impl FnOnce() -> PyBaseExceptionRef, |
There was a problem hiding this comment.
because we always raise type error, err can be error message generator rather than a full type error generator.
| err: impl FnOnce() -> PyBaseExceptionRef, | |
| err_fmt: impl FnOnce() -> String, |
There was a problem hiding this comment.
I prefer to use fn function pointer rather than closure in this place if possible, to let it not to be inlined
Summary
Make
strsubscripting with a non-integer index raise the sameTypeErrormessage as CPython, and drop the
expectedFailuremarker onstring_tests.test_subscript.Before / after (
'abc'['def']):string indices must be integers, not 'str'str indices must be integers or slices or classes that override __index__ operator, not 'str'string indices must be integers, not 'str'Only the error message differed — indexing itself (int / negative / slice)
already behaved correctly. CPython gives
strits own wording inunicode_subscript(see python/cpython#88276, fixed in 3.11), distinct from theshared sequence message used by
list/tuple/bytes.Approach
SequenceIndex::try_from_borrowed_object_with_err, which keeps the sharedindex-parsing logic but lets the caller supply the
TypeErrorraised for anon-index object.
try_from_borrowed_objectnow delegates to it with the existing wording, soevery other sequence caller (
list/tuple/bytes/bytearray/array/mmap/struct-sequence) is byte-for-byte unchanged.
str::_getitem— the single path shared by__getitem__and the mappingsubscript slot — supplies the CPython-accurate message.
Scoped intentionally to
stronly: CPython declined to unify the sequencemessages (python/cpython#75731, rejected), so the other types' wording is left
as-is.
Testing
test_str/test_userstringtest_subscriptnow pass (marker removed).str/object/None/floatindices, and for
UserStringas receiver.list/tuple/bytesmessages verified unchanged;rustpython-vmandrustpython-codegenunit tests pass.Summary by CodeRabbit
TypeErroridentifying the invalid type.