fix(codegen): any-typed startsWith/endsWith/lastIndexOf must dynamic-dispatch, not the static String builtin#5666
Merged
proggeramlug merged 2 commits intoJun 25, 2026
Conversation
…dispatch An any-typed receiver may be an object with its own same-named method (e.g. zod's z.string().startsWith() returns a refined schema, not a boolean). Forcing the static String builtin returned a primitive, so a chained .describe()/ .optional() threw. Drop these three from the any-typed string-method fallback so they dynamic-dispatch; a genuine string receiver is still served by the runtime is_string() arm. Same shape as the indexOf/includes fix (PerryTS#1341).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates property-call lowering, adds symbol-keyed descriptor storage and ChangesLowered call dispatch
Symbol-keyed property hooks
Optimized libs fresh-path merge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
crates/perry-codegen/src/lower_call/property_get.rs (1)
135-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the earlier method-list comment to avoid contradicting this fallback.
Lines 95-96 still say
startsWith/endsWithare unambiguous string-only methods, but this new block says they must not be string-forced. Please remove or rewrite that earlier mention so the dispatch policy is consistent.🤖 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-codegen/src/lower_call/property_get.rs` around lines 135 - 147, Update the earlier method-list comment in property_get::lower_call to match the new dispatch behavior for startsWith and endsWith. The current note in the string-only branch contradicts the later fallback logic in js_native_call_method handling, so remove or rewrite that mention to say these methods should not be forced through the static string path and must be dispatched at runtime like indexOf/includes when the receiver is Any-typed.
🤖 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-runtime/src/object/instanceof.rs`:
- Around line 130-140: Update the `instanceof` handling in
`object/instanceof.rs` so the `Symbol.hasInstance` lookup in the object path
does not silently fall through when an own property exists but is not callable.
In the `type_ref` branch that uses `js_object_has_own_symbol`, make the
`js_object_get_symbol_property` result either invoke `js_native_call_value` when
`value_is_callable(cb)` is true, or raise a `TypeError` for any other
non-null/non-undefined own value instead of continuing. Apply the same rule in
the class-static-symbol path so both code paths enforce callable own
`@@hasInstance` consistently.
- Around line 765-789: The `instanceof` handling in `object/instanceof.rs`
should prioritize the registered `@@hasInstance` hook before the ordinary
class-chain fast path, since `new C() instanceof C` can currently short-circuit
too early. Update the logic around the `class_static_symbol_lookup` /
`js_native_call_value` hook check so it runs before any class-chain return in
the `instanceof` evaluation flow, while preserving the existing OWN-only lookup
and truthy/falsy return behavior.
In `@crates/perry-runtime/src/object/object_ops/define_property.rs`:
- Around line 365-390: The Symbol define-property fallback in define_property.rs
is treating generic descriptors as data descriptors and overwriting existing
closure values with undefined. Update the non-accessor branch in the object
define path to mirror the ordinary Symbol handling: only call
js_object_set_symbol_property when the descriptor actually supplies a value,
preserve the existing fn[sym] for generic redefines like { enumerable: true },
and make sure writable/enumerable/configurable attributes are recorded before
returning, consistent with the other Symbol property paths.
- Around line 297-305: The `define_property` handling in
`desc_read_field`/`js_class_register_static_symbol` is skipping explicit
`undefined` values by checking `is_undefined()`, so `Object.defineProperty(C,
sym, { value: undefined })` never creates the own symbol entry. Update the
`descriptor_value` handling to gate on whether the `value` field is present in
the descriptor, not whether it is undefined, and pass the field bits through to
`js_class_register_static_symbol` even when the stored value is `undefined`.
---
Nitpick comments:
In `@crates/perry-codegen/src/lower_call/property_get.rs`:
- Around line 135-147: Update the earlier method-list comment in
property_get::lower_call to match the new dispatch behavior for startsWith and
endsWith. The current note in the string-only branch contradicts the later
fallback logic in js_native_call_method handling, so remove or rewrite that
mention to say these methods should not be forced through the static string path
and must be dispatched at runtime like indexOf/includes when the receiver is
Any-typed.
🪄 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: 8ae08397-bba5-4134-a391-3393a024a34f
📒 Files selected for processing (4)
crates/perry-codegen/src/lower_call/property_get.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/object_ops/define_property.rscrates/perry/src/commands/compile/optimized_libs/driver.rs
… defineProperty This PR's commit bundles the same instanceof.rs / define_property.rs changes as PerryTS#5667; apply the identical review fixes here: - instanceof.rs: consult an own `@@hasInstance` (registered hook + defineProperty form) BEFORE the class-chain fast path; a present-but-non-callable own value throws `TypeError`, only `undefined`/`null` falls through (shared `dispatch_own_has_instance` helper, both dynamic and class-static paths). - define_property.rs: gate symbol value-writes on descriptor-field presence so `{ value: undefined }` is preserved and a generic redefine like `{ enumerable: true }` does not clobber an existing entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
force-pushed
the
fix/codegen-anytyped-string-method-dispatch
branch
from
45abacf to
b796c30
Compare
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.
Problem
When the receiver of
.startsWith()/.endsWith()/.lastIndexOf()is any-typed (its TS type was erased at bundle time), codegen routed the call to the staticString.prototypebuiltin, which returns a boolean/number. But an any-typed receiver may be an object with its own same-named method that returns something else.Minimal repro (zod):
ZodString.startsWith()returns a refined schema, not a boolean, so a chained.describe()/.optional()then threw on a primitive. This blocks compiled programs that build zod schemas with these string refinements.Fix
Drop
startsWith/endsWith/lastIndexOffrom the any-typed string-method fallback inlower_call/property_get.rs, so they dynamic-dispatch: an object's own method wins, while a genuine any-typed string receiver is still serviced by the runtime'sjsval.is_string()arm ofjs_native_call_method.This is exactly the fix already applied to
indexOf/includes(#1341) for the same reason (an any-typed receiver that is actually an array).Testing
Regression-tested against Node: typed and any-typed
startsWith/endsWith/lastIndexOf, plus arrayincludes/indexOf, all still match.z.string().startsWith("./").describe("x")now returns aZodStringas in Node.Summary by CodeRabbit
instanceofbehavior to honorSymbol.hasInstancewith correct precedence, own-property lookup, and proper handling of non-callable cases.definePropertyforSymbolkeys on classes and functions so descriptors are applied correctly forvalue,get, andset.startsWith,endsWith, andlastIndexOfonAny-typed string receivers to prevent incorrect string-only routing.