From f3adc6975490a07480623c1e57addcb98ecc0bf7 Mon Sep 17 00:00:00 2001 From: sean abreau <92339393+seanabreau@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:16:40 -0700 Subject: [PATCH] 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 --- crates/vm/src/builtins/str.rs | 24 ------- crates/vm/src/types/slot.rs | 16 ++++- extra_tests/snippets/builtin_str_add.py | 88 +++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 25 deletions(-) create mode 100644 extra_tests/snippets/builtin_str_add.py diff --git a/crates/vm/src/builtins/str.rs b/crates/vm/src/builtins/str.rs index 76bf7f6cd..d530cdbf2 100644 --- a/crates/vm/src/builtins/str.rs +++ b/crates/vm/src/builtins/str.rs @@ -678,16 +678,6 @@ impl PyStr { } .to_pyobject(vm)) } else { - // hack to get around not distinguishing number add from seq concat - if let Some(radd) = vm.get_method(other.to_owned(), identifier!(vm, __radd__)) { - let result = radd?.call((zelf,), vm)?; - // CPython reaches `str`'s sq_concat once the reflected call declines, so a - // `__radd__` returning NotImplemented must still report the concat error - // rather than the generic binary-op one. - if !result.is(&vm.ctx.not_implemented) { - return Ok(result); - } - } Err(vm.new_type_error(format!( r#"can only concatenate str (not "{}") to str"#, other.class().slot_name() @@ -1694,20 +1684,6 @@ impl AsMapping for PyStr { impl AsNumber for PyStr { fn as_number() -> &'static PyNumberMethods { static AS_NUMBER: PyNumberMethods = PyNumberMethods { - add: Some(|a, b, vm| { - let Some(a) = a.downcast_ref::() else { - return Ok(vm.ctx.not_implemented()); - }; - let Some(b) = b.downcast_ref::() else { - return Ok(vm.ctx.not_implemented()); - }; - let bytes = a.as_wtf8().py_add(b.as_wtf8()); - Ok(unsafe { - let kind = a.kind() | b.kind(); - PyStr::new_str_unchecked(bytes.into(), kind) - } - .to_pyobject(vm)) - }), remainder: Some(|a, b, vm| { if let Some(a) = a.downcast_ref::() { a.__mod__(b.to_owned(), vm).to_pyresult(vm) diff --git a/crates/vm/src/types/slot.rs b/crates/vm/src/types/slot.rs index 28eb5fd78..7465a6f9e 100644 --- a/crates/vm/src/types/slot.rs +++ b/crates/vm/src/types/slot.rs @@ -1564,7 +1564,21 @@ impl PyType { SlotAccessor::SqLength => { update_sub_slot!(as_sequence, length, sequence_len_wrapper, SeqLength) } - SlotAccessor::SqConcat | SlotAccessor::SqInplaceConcat if !ADD => { + SlotAccessor::SqConcat => { + // Python __add__ overrides use nb_add, not the inherited sq_concat. + let concat = match self.lookup_slot_in_mro(name, ctx, |sf| { + if let SlotFunc::SeqConcat(f) = sf { + Some(*f) + } else { + None + } + }) { + SlotLookupResult::NativeSlot(func) => Some(func), + SlotLookupResult::PythonMethod | SlotLookupResult::NotFound => None, + }; + self.slots.as_sequence.concat.store(concat); + } + SlotAccessor::SqInplaceConcat if !ADD => { // Sequence concat uses sq_concat slot - no generic wrapper needed // (handled by number protocol fallback) accessor.inherit_from_mro(self); diff --git a/extra_tests/snippets/builtin_str_add.py b/extra_tests/snippets/builtin_str_add.py new file mode 100644 index 000000000..075939ae8 --- /dev/null +++ b/extra_tests/snippets/builtin_str_add.py @@ -0,0 +1,88 @@ +import operator + +from testutils import assert_raises + + +class Declines(str): + def __add__(self, other): + return NotImplemented + + +class Reflected: + def __init__(self, result): + self.calls = 0 + self.result = result + + def __radd__(self, other): + self.calls += 1 + return self.result + + +class ReflectedStr(str): + def __radd__(self, other): + return "reflected" + + +def add(a, b): + return a + b + + +def iadd(a, b): + a += b + return a + + +for operation in (add, iadd, operator.add, operator.iadd): + assert operation("a", "b") == "ab" + assert operation("a", Declines("b")) == "ab" + assert_raises(TypeError, operation, Declines("a"), "b") + assert operation("a", ReflectedStr("b")) == "reflected" + for result in ("reflected", NotImplemented): + other = Reflected(result) + if result is NotImplemented: + assert_raises(TypeError, operation, "a", other) + else: + assert operation("a", other) == result + assert other.calls == 1 + +assert str.__add__("a", "b") == "ab" +assert str.__add__("a", ReflectedStr("b")) == "ab" +assert_raises(TypeError, str.__add__, "a", 1) +for result in ("reflected", NotImplemented): + other = Reflected(result) + assert_raises(TypeError, str.__add__, "a", other) + assert other.calls == 0 + +assert not hasattr(str, "__radd__") + +assert str.__add__(Declines("a"), "b") == "ab" + + +for base, left, right in ( + (str, "a", "b"), + (bytes, b"a", b"b"), + (tuple, (1,), (2,)), + (list, [1], [2]), +): + + class Override(base): + def __add__(self, other): + return NotImplemented + + class Child(Override): + pass + + for cls in (Override, Child): + assert_raises(TypeError, operator.add, cls(left), right) + + Override.__add__ = base.__add__ + for cls in (Override, Child): + assert operator.add(cls(left), right) == left + right + + Override.__add__ = lambda self, other: "override" + for cls in (Override, Child): + assert operator.add(cls(left), right) == "override" + + del Override.__add__ + for cls in (Override, Child): + assert operator.add(cls(left), right) == left + right