Keep `str`'s concat error when `__radd__` declines by rawsun007 · Pull Request #8652 · RustPython/RustPython · GitHub
Skip to content

Keep str's concat error when __radd__ declines - #8652

Merged
youknowone merged 1 commit into
RustPython:mainfrom
rawsun007:fix/str-concat-error-message
Sep 5, 2026
Merged

Keep str's concat error when __radd__ declines#8652
youknowone merged 1 commit into
RustPython:mainfrom
rawsun007:fix/str-concat-error-message

Conversation

@rawsun007

@rawsun007 rawsun007 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

"a" + 1 reports the generic binary-op error instead of CPython's concatenation error:

RustPython: TypeError: unsupported operand type(s) for +: 'str' and 'int'
CPython:    TypeError: can only concatenate str (not "int") to str

PyStr::__add__ delegates to the right operand's __radd__ whenever it has one and returns whatever comes back. Every numeric type has __radd__, and it returns NotImplemented for a str left operand, so the result reached the generic handler. CPython instead falls back to str's sq_concat once the reflected call declines, which is where its message comes from.

The correct message was already in the else branch, reachable only for operands with no __radd__ at all — so list, object, bytes and None were already right, while int, float, bool and any class whose __radd__ returns NotImplemented were not. The fix keeps the reflected call, and only falls through to the concat error when it declines. A __radd__ that returns a value still wins, unchanged.

"a" + x before after
1, 1.5, True generic can only concatenate str (not "int"/"float"/"bool") to str
declining __radd__ generic can only concatenate str (not "Declines") to str
[], None, b"b", object() already correct unchanged
__radd__ returning a value works unchanged

Verification:

  • The new snippet in extra_tests/snippets/builtin_str.py passes under both RustPython and CPython 3.14.0, so its expectations are CPython's behaviour rather than my reading of it.
  • Reverting only the str.rs change fails its first assertion.
  • cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi — all 42 test binaries pass.
  • cargo run --release -- -m test test_str test_operator test_descr test_exceptions — SUCCESS for each.
  • cargo fmt --check and ruff format --check clean.

Found by diffing ~70 expressions across both interpreters. Three other divergences turned up and are deliberately not in this PR, since they are unrelated one-line message fixes in different files: '{'.format() and '{0.}'.format(1) produce different ValueError text, and bytes([256]) says byte must be in range(0, 256) where CPython says bytes. Happy to send those separately if wanted.

AI disclosure per the AI policy: found, written and verified with Claude Code (Claude Opus 5), which is also recorded in the commit's Assisted-by: trailer. @rawsun007 reviewed the change and authorised the push.

Summary by CodeRabbit

  • Bug Fixes

    • Improved string concatenation errors to report the specific incompatible operand type.
    • Correctly falls back to the standard string error when a reflected operation declines handling.
    • Preserved reflected-operation results when they provide a valid value.
  • Tests

    • Added coverage for concatenation with incompatible types and reflected operations.

`PyStr::__add__` delegated to the right operand's `__radd__` whenever it had
one, and returned whatever came back. For every numeric type that is a
`NotImplemented`, which then reached the generic binary-op handler:

    >>> "a" + 1
    TypeError: unsupported operand type(s) for +: 'str' and 'int'

CPython falls back to `str`'s `sq_concat` once the reflected call declines, so
it names the concatenation instead:

    TypeError: can only concatenate str (not "int") to str

The specific message was already here, but only reachable for operands with no
`__radd__` at all, so `list`, `object`, `bytes` and `None` were correct while
`int`, `float`, `bool` and any class with a declining `__radd__` were not.

A `__radd__` that returns a value still wins, unchanged.

Verified against CPython 3.14.0: the new snippet asserts the message for
`int`, `float`, `bool`, `list`, `None`, `bytes` and a declining `__radd__`,
and passes under both interpreters. Reverting only the `str.rs` change fails
its first assertion.

Assisted-by: Claude Code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Team

Run ID: 6c8b9343-9445-4d2c-81e5-475fbffa6d43

📥 Commits

Reviewing files that changed from the base of the PR and between bca6c95 and fc0d149.

📒 Files selected for processing (2)
  • crates/vm/src/builtins/str.rs
  • extra_tests/snippets/builtin_str.py

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


📝 Walkthrough

Walkthrough

String concatenation now checks reflected __radd__ results. It returns successful reflected values and reports the string-specific TypeError when reflection returns NotImplemented. Tests cover incompatible operand types and reflected addition behavior.

Changes

String concatenation behavior

Layer / File(s) Summary
Reflected addition fallback and validation
crates/vm/src/builtins/str.rs, extra_tests/snippets/builtin_str.py
PyStr::__add__ returns non-NotImplemented reflected results and otherwise raises the specific string concatenation error. Tests verify messages for incompatible types and both reflected-result outcomes.

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

Merge Risk: ⚪ Minimal · up to fc0d1

String concatenation now retains CPython-compatible error messages when reflected addition declines, while preserving successful reflected additions. The updated behavior is covered for relevant operand and reflection outcomes, with no remaining merge-blocking risk identified.

Suggested reviewers: youknowone, joshuamegnauth54, shaharnaveh

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 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 change: preserving the specific str concatenation error when radd returns NotImplemented.
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 PR with unit tests

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 left a comment

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.

👍 Thank you!

@youknowone
youknowone merged commit 5346842 into RustPython:main Sep 5, 2026
28 of 29 checks passed
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