fix(codegen): String method on a nullish receiver must throw, not coerce#6453
Merged
proggeramlug merged 1 commit intoJul 16, 2026
Merged
Conversation
📝 WalkthroughWalkthroughString method lowering now uses receiver-aware coercion that preserves member-access TypeErrors for nullish receivers. The runtime declaration and implementation accept the method name for diagnostics, and a regression test covers nullish and valid string receivers. ChangesString method receiver coercion
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/builtins/numbers.rs (1)
676-702: 📐 Maintainability & Code Quality | 🔵 TrivialRebuild the static wrapper crates. This runtime change should also rebuild
perry-runtime-staticandperry-stdlib-staticso the linked archives don’t go stale.🤖 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/perry-runtime/src/builtins/numbers.rs` around lines 676 - 702, Rebuild the static wrapper crates perry-runtime-static and perry-stdlib-static after updating js_string_coerce_method_this, ensuring their linked archives include the runtime change and do not remain stale.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/perry/tests/issue_string_method_nullish_receiver.rs`:
- Around line 60-61: Move or duplicate coverage for string method access on a
nullish receiver into cargo-test-visible unit tests, such as the perry-codegen
unit test suite, rather than relying only on
string_method_on_nullish_receiver_throws_member_access_type_error in the
integration tests. Preserve assertions that the operation throws the expected
member-access type error.
---
Nitpick comments:
In `@crates/perry-runtime/src/builtins/numbers.rs`:
- Around line 676-702: Rebuild the static wrapper crates perry-runtime-static
and perry-stdlib-static after updating js_string_coerce_method_this, ensuring
their linked archives include the runtime change and do not remain stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e6c49f-36eb-4223-be4a-7228ed0fa875
📒 Files selected for processing (4)
crates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-runtime/src/builtins/numbers.rscrates/perry/tests/issue_string_method_nullish_receiver.rs
The inline lowering for `String.prototype` char-access/case/split methods
(`charAt`/`charCodeAt`/`codePointAt`/`toUpperCase`/`toLowerCase`/`split`/…)
applied `ToString(this)` to a non-string receiver via `js_string_coerce`
without a `RequireObjectCoercible` guard. `ToString` maps `undefined`→
`"undefined"` and `null`→`"null"`, so:
(undefined as any).codePointAt(0) // => 117 ("undefined".codePointAt(0))
(undefined as any).toUpperCase() // => "UNDEFINED"
(null as any).charAt(0) // => "n"
where V8/Node throw `TypeError: Cannot read properties of undefined
(reading 'codePointAt')` — the member access `x.codePointAt` reads the
method off `x` first (ECMA-262 §13.3), before the call. The general
property-get path (used for e.g. `slice`, `indexOf`) already threw
correctly; only the optimistically-inlined char-access/case/split path
routed through `lower_string_method` skipped the guard.
Fix: the non-string-receiver coercion branch now calls a new
`js_string_coerce_method_this(value, prop_name, prop_len)` runtime helper,
which performs `RequireObjectCoercible(this)` — throwing the V8-shaped
`Cannot read properties of {undefined|null} (reading '<method>')` via
`js_throw_type_error_property_access` — before `ToString`. A statically
string-typed receiver still skips the guard (fast path, unchanged).
Regression test compiles + runs the nullish-receiver shapes and asserts the
member-access TypeError message (and that a real string receiver still
works).
Claude-Session: https://claude.ai/code/session_01XRHKpxgDnVB3GJdsV9ud7g
proggeramlug
force-pushed
the
fix/string-method-nullish-receiver
branch
from
July 16, 2026 02:10
e54704e to
44e4186
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/builtins/numbers.rs (1)
686-686: 📐 Maintainability & Code Quality | 🔵 TrivialRebuild static wrapper crates.
As per path instructions, when changing runtime or stdlib code, remember to rebuild the corresponding static wrapper crates (
perry-runtime-staticandperry-stdlib-static). The runtime and stdlib crates themselves emit only rlibs and otherwise may leave stale archives linked during local testing or builds.🤖 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/perry-runtime/src/builtins/numbers.rs` at line 686, After modifying the runtime or stdlib, rebuild the corresponding static wrapper crates per the repository instructions: perry-runtime-static and perry-stdlib-static. Ensure their generated archives are refreshed so local tests and builds do not link stale artifacts.Source: Path instructions
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@crates/perry-runtime/src/builtins/numbers.rs`:
- Line 686: After modifying the runtime or stdlib, rebuild the corresponding
static wrapper crates per the repository instructions: perry-runtime-static and
perry-stdlib-static. Ensure their generated archives are refreshed so local
tests and builds do not link stale artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 35b9ab43-a324-4abf-bcfc-9ea2063387d3
📒 Files selected for processing (4)
crates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-runtime/src/builtins/numbers.rscrates/perry/tests/issue_string_method_nullish_receiver.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/perry-codegen/src/lower_string_method.rs
- crates/perry/tests/issue_string_method_nullish_receiver.rs
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The inline lowering for
String.prototypechar-access/case/split methods (charAt/charCodeAt/codePointAt/toUpperCase/toLowerCase/split/…) appliedToString(this)to a non-string receiver viajs_string_coercewithout aRequireObjectCoercibleguard.ToStringmapsundefined→"undefined"andnull→"null", so:where V8/Node throw
TypeError: Cannot read properties of undefined (reading 'codePointAt')— the member accessx.codePointAtreads the method offxfirst (ECMA-262 §13.3), before the call. The general property-get path (used for e.g.slice,indexOf) already threw correctly; only the optimistically-inlined char-access/case/split path routed throughlower_string_methodskipped the guard.Fix
The non-string-receiver coercion branch now calls a new
js_string_coerce_method_this(value, prop_name, prop_len)runtime helper, which performsRequireObjectCoercible(this)— throwing the V8-shapedCannot read properties of {undefined|null} (reading '<method>')viajs_throw_type_error_property_access— beforeToString. A statically string-typed receiver still skips the guard (fast path, unchanged).Test
crates/perry/tests/issue_string_method_nullish_receiver.rscompiles + runs the nullish-receiver shapes (mirroringcell.value.codePointAt(0)wherecell.valueisundefined) and asserts the member-accessTypeErrormessage forcodePointAt/charCodeAt/charAt/toUpperCase/spliton bothundefinedandnull, plus that a real string receiver still works.https://claude.ai/code/session_01XRHKpxgDnVB3GJdsV9ud7g
Summary by CodeRabbit
Bug Fixes
String.prototypemethod calls onnull/undefinedreceivers to throw the expected member-accessTypeError(instead of coercing to strings).Tests
charAt,codePointAt,charCodeAt,toUpperCase,split) and asserting exact error message text.