Skip to content

Match CPython's str subscript TypeError message #8319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kim-jaedeok wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −8
Open

Match CPython's str subscript TypeError message #8319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Lib/test/string_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,7 +1301,6 @@ def test___contains__(self):
self.checkequal(False, 'asd', '__contains__', 'asdf')
self.checkequal(False, '', '__contains__', 'asdf')

@unittest.expectedFailure # TODO: RUSTPYTHON
def test_subscript(self):
self.checkequal('a', 'abc', '__getitem__', 0)
self.checkequal('c', 'abc', '__getitem__', -1)
Expand Down
8 changes: 7 additions & 1 deletion crates/vm/src/builtins/str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,13 @@ impl PyStr {
}

fn _getitem(&self, needle: &PyObject, vm: &VirtualMachine) -> PyResult {
let item = match SequenceIndex::try_from_borrowed_object(vm, needle, "str")? {
let index = SequenceIndex::try_from_borrowed_object_with_err(vm, needle, || {
vm.new_type_error(format!(
"string indices must be integers, not '{}'",
needle.class()
))
})?;
let item = match index {
SequenceIndex::Int(i) => self.getitem_by_index(vm, i)?.to_pyobject(vm),
SequenceIndex::Slice(slice) => self.getitem_by_slice(vm, slice)?.to_pyobject(vm),
};
Expand Down
26 changes: 20 additions & 6 deletions crates/vm/src/sliceable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// export through sliceable module, not slice.
use crate::{
PyObject, PyResult, VirtualMachine,
builtins::{int::PyInt, slice::PySlice},
builtins::{PyBaseExceptionRef, int::PyInt, slice::PySlice},
};
use core::ops::Range;
use malachite_bigint::BigInt;
Expand Down Expand Up @@ -262,6 +262,24 @@ impl SequenceIndex {
vm: &VirtualMachine,
obj: &PyObject,
type_name: &str,
) -> PyResult<Self> {
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()
))
})
Comment on lines +266 to +272

@coderabbitai coderabbitai Bot Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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() to obj.class().
  • crates/vm/src/builtins/str.rs#L663-L668: append .name() to needle.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.

}

/// Like [`Self::try_from_borrowed_object`], but lets the caller supply the
/// `TypeError` raised for a non-index object. Used by types whose CPython
/// message differs from the shared sequence wording (e.g. `str`, whose
/// `unicode_subscript` says "string indices must be integers, not ...").
pub fn try_from_borrowed_object_with_err(
vm: &VirtualMachine,
obj: &PyObject,
err: impl FnOnce() -> PyBaseExceptionRef,

@youknowone youknowone Jul 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we always raise type error, err can be error message generator rather than a full type error generator.

Suggested change
err: impl FnOnce() -> PyBaseExceptionRef,
err_fmt: impl FnOnce() -> String,

@youknowone youknowone Jul 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use fn function pointer rather than closure in this place if possible, to let it not to be inlined

) -> PyResult<Self> {
if let Some(i) = obj.downcast_ref::<PyInt>() {
// TODO: number protocol
Expand All @@ -276,11 +294,7 @@ impl SequenceIndex {
.map_err(|_| vm.new_index_error("cannot fit 'int' into an index-sized integer"))
.map(Self::Int)
} else {
Err(vm.new_type_error(format!(
"{} indices must be integers or slices or classes that override __index__ operator, not '{}'",
type_name,
obj.class()
)))
Err(err())
}
}
}
Expand Down
Loading