Skip to content

Commit f3adc69

Browse files
authored
Fix string addition and sequence subclass dispatch (#8822)
Use sequence concatenation for str and clear inherited concat slots when Python subclasses override __add__. Cover direct calls, reflected dispatch, and dynamic overrides against CPython. Codex assisted with investigation, implementation, tests, validation, and the PR description. Assisted-by: Codex:gpt-6
1 parent 393689d commit f3adc69

3 files changed

Lines changed: 103 additions & 25 deletions

File tree

‎crates/vm/src/builtins/str.rs‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -678,16 +678,6 @@ impl PyStr {
678678
}
679679
.to_pyobject(vm))
680680
} else {
681-
// hack to get around not distinguishing number add from seq concat
682-
if let Some(radd) = vm.get_method(other.to_owned(), identifier!(vm, __radd__)) {
683-
let result = radd?.call((zelf,), vm)?;
684-
// CPython reaches `str`'s sq_concat once the reflected call declines, so a
685-
// `__radd__` returning NotImplemented must still report the concat error
686-
// rather than the generic binary-op one.
687-
if !result.is(&vm.ctx.not_implemented) {
688-
return Ok(result);
689-
}
690-
}
691681
Err(vm.new_type_error(format!(
692682
r#"can only concatenate str (not "{}") to str"#,
693683
other.class().slot_name()
@@ -1694,20 +1684,6 @@ impl AsMapping for PyStr {
16941684
impl AsNumber for PyStr {
16951685
fn as_number() -> &'static PyNumberMethods {
16961686
static AS_NUMBER: PyNumberMethods = PyNumberMethods {
1697-
add: Some(|a, b, vm| {
1698-
let Some(a) = a.downcast_ref::<PyStr>() else {
1699-
return Ok(vm.ctx.not_implemented());
1700-
};
1701-
let Some(b) = b.downcast_ref::<PyStr>() else {
1702-
return Ok(vm.ctx.not_implemented());
1703-
};
1704-
let bytes = a.as_wtf8().py_add(b.as_wtf8());
1705-
Ok(unsafe {
1706-
let kind = a.kind() | b.kind();
1707-
PyStr::new_str_unchecked(bytes.into(), kind)
1708-
}
1709-
.to_pyobject(vm))
1710-
}),
17111687
remainder: Some(|a, b, vm| {
17121688
if let Some(a) = a.downcast_ref::<PyStr>() {
17131689
a.__mod__(b.to_owned(), vm).to_pyresult(vm)

‎crates/vm/src/types/slot.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1564,7 +1564,21 @@ impl PyType {
15641564
SlotAccessor::SqLength => {
15651565
update_sub_slot!(as_sequence, length, sequence_len_wrapper, SeqLength)
15661566
}
1567-
SlotAccessor::SqConcat | SlotAccessor::SqInplaceConcat if !ADD => {
1567+
SlotAccessor::SqConcat => {
1568+
// Python __add__ overrides use nb_add, not the inherited sq_concat.
1569+
let concat = match self.lookup_slot_in_mro(name, ctx, |sf| {
1570+
if let SlotFunc::SeqConcat(f) = sf {
1571+
Some(*f)
1572+
} else {
1573+
None
1574+
}
1575+
}) {
1576+
SlotLookupResult::NativeSlot(func) => Some(func),
1577+
SlotLookupResult::PythonMethod | SlotLookupResult::NotFound => None,
1578+
};
1579+
self.slots.as_sequence.concat.store(concat);
1580+
}
1581+
SlotAccessor::SqInplaceConcat if !ADD => {
15681582
// Sequence concat uses sq_concat slot - no generic wrapper needed
15691583
// (handled by number protocol fallback)
15701584
accessor.inherit_from_mro(self);
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import operator
2+
3+
from testutils import assert_raises
4+
5+
6+
class Declines(str):
7+
def __add__(self, other):
8+
return NotImplemented
9+
10+
11+
class Reflected:
12+
def __init__(self, result):
13+
self.calls = 0
14+
self.result = result
15+
16+
def __radd__(self, other):
17+
self.calls += 1
18+
return self.result
19+
20+
21+
class ReflectedStr(str):
22+
def __radd__(self, other):
23+
return "reflected"
24+
25+
26+
def add(a, b):
27+
return a + b
28+
29+
30+
def iadd(a, b):
31+
a += b
32+
return a
33+
34+
35+
for operation in (add, iadd, operator.add, operator.iadd):
36+
assert operation("a", "b") == "ab"
37+
assert operation("a", Declines("b")) == "ab"
38+
assert_raises(TypeError, operation, Declines("a"), "b")
39+
assert operation("a", ReflectedStr("b")) == "reflected"
40+
for result in ("reflected", NotImplemented):
41+
other = Reflected(result)
42+
if result is NotImplemented:
43+
assert_raises(TypeError, operation, "a", other)
44+
else:
45+
assert operation("a", other) == result
46+
assert other.calls == 1
47+
48+
assert str.__add__("a", "b") == "ab"
49+
assert str.__add__("a", ReflectedStr("b")) == "ab"
50+
assert_raises(TypeError, str.__add__, "a", 1)
51+
for result in ("reflected", NotImplemented):
52+
other = Reflected(result)
53+
assert_raises(TypeError, str.__add__, "a", other)
54+
assert other.calls == 0
55+
56+
assert not hasattr(str, "__radd__")
57+
58+
assert str.__add__(Declines("a"), "b") == "ab"
59+
60+
61+
for base, left, right in (
62+
(str, "a", "b"),
63+
(bytes, b"a", b"b"),
64+
(tuple, (1,), (2,)),
65+
(list, [1], [2]),
66+
):
67+
68+
class Override(base):
69+
def __add__(self, other):
70+
return NotImplemented
71+
72+
class Child(Override):
73+
pass
74+
75+
for cls in (Override, Child):
76+
assert_raises(TypeError, operator.add, cls(left), right)
77+
78+
Override.__add__ = base.__add__
79+
for cls in (Override, Child):
80+
assert operator.add(cls(left), right) == left + right
81+
82+
Override.__add__ = lambda self, other: "override"
83+
for cls in (Override, Child):
84+
assert operator.add(cls(left), right) == "override"
85+
86+
del Override.__add__
87+
for cls in (Override, Child):
88+
assert operator.add(cls(left), right) == left + right

0 commit comments

Comments
 (0)