Skip to content

Fix string addition and sequence subclass dispatch - #8822

Merged
youknowone merged 1 commit into
RustPython:mainfrom
seanabreau:codex/str-add-dispatch
Sep 26, 2026
Merged

youknowone merged 1 commit into
RustPython:mainfrom
seanabreau:codex/str-add-dispatch

Conversation

@seanabreau

@seanabreau seanabreau commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A str subclass that declines addition still gets concatenated:

class S(str):
    def __add__(self, other):
        return NotImplemented

S("a") + "b"

CPython raises TypeError; RustPython returns "ab". Direct calls also differ: str.__add__("a", 1) returns NotImplemented instead of raising TypeError.

Use the sequence concatenation slot for strings and leave reflected calls to the operator dispatcher. This removes the extra str.__radd__ method and prevents a declining right operand from being called twice. Clear inherited sequence concatenation when a subclass overrides __add__, and restore it when the native method becomes visible again.

The regression covers direct calls, operator.add, operator.iadd, reflected-call counts, and inherited or dynamically replaced overrides for strings, bytes, tuples, and lists. The existing exact-string bytecode fast paths remain unchanged.

Related: #7293 introduced the numeric string-addition slot; #8652 fixed concatenation errors when a reflected call declines. This preserves those error messages while correcting dispatch.

Validation

  • The new regression passes on CPython 3.14.7 and 3.16, fails on unmodified RustPython main (393689d), and passes with this patch.
  • test_str, test_operator, test_descr, test_exceptions, test_list, test_tuple, and test_bytes: 941 tests run, 59 skipped; all modules pass.
  • extra_tests: 464 passed. Four failures also reproduce without the patch: the default build lacks _sqlite3, and three host-side multiprocessing tests return non-None values, which pytest rejects.
  • C-API tests: 112 passed with debug assertions enabled.
  • Formatting and configured commit hooks pass.
  • Workspace Rust tests and doctests: 1,310 passed, 16 ignored (cargo test --release --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi). The local macOS run needs the installed library directories in DYLD_FALLBACK_LIBRARY_PATH.
  • Workspace and C-API Clippy pass.
  • The contributor manually ran the regression successfully.

AI assistance

Codex assisted with investigation, implementation, regression tests, validation, and this description.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected string addition so non-string operands raise the appropriate concatenation error without calling their reflected addition method.
    • Fixed how concatenation behavior is resolved for sequence types, including when subclasses override or remove addition methods.
    • Improved consistency for direct and in-place addition, including cases where subclasses return NotImplemented.

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
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 50f932f3-876f-4116-8a60-241361c6f0d9

📥 Commits

Reviewing files that changed from the base of the PR and between 393689d and 6a72599.

📒 Files selected for processing (3)
  • crates/vm/src/builtins/str.rs
  • crates/vm/src/types/slot.rs
  • extra_tests/snippets/builtin_str_add.py
💤 Files with no reviewable changes (1)
  • crates/vm/src/builtins/str.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

String addition now uses sequence-slot dispatch. The slot update resolves SqConcat independently of SqInplaceConcat. Tests cover direct and operator-based addition across strings and built-in type subclasses.

Changes

String Addition Dispatch

Layer / File(s) Summary
Sequence concatenation slot resolution
crates/vm/src/types/slot.rs
SqConcat now uses an MRO lookup to store a native SeqConcat function or clear the slot when the lookup finds a Python method or no matching method.
String addition behavior and tests
crates/vm/src/builtins/str.rs, extra_tests/snippets/builtin_str_add.py
PyStr::__add__ no longer calls a non-string operand’s __radd__. The string number-slot add implementation was removed. Tests cover direct operators, operator functions, reflected addition, and subclass overrides.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to 6a725

No actionable merge-blocking issue remains; the changed addition path preserves single-call and reflected-dispatch behavior.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6a725

The changed behavior affects addition across built-in sequences and their subclasses. The inspected dispatch paths show no new privileged operation or trust-boundary bypass, but security coverage of the affected code is incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The identified exposure is Python-level addition behavior for strings and sequence subclasses. No privileged-resource transition was identified in the inspected slot-resolution and operator-consumer paths.

Trust Boundaries and Controls

  • observed — Numeric dispatch handles reflected operands before the addition operator considers the left operand’s native sequence slot; direct string addition performs its own operand-type check.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: fixing string addition and sequence subclass dispatch.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@youknowone
youknowone merged commit f3adc69 into RustPython:main Sep 26, 2026
21 checks passed
@youknowone

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants