fix(codegen): argless builtins ignore extra args instead of bailing#5285
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughArgless builtin method lowering arms for ChangesArgless Builtin Extra-Arg Tolerance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/argless_builtin_extra_args.rs (1)
91-129: ⚡ Quick winExpand regression coverage to all modified argless arms.
This file currently verifies only
trimandpop, while the PR also changesshift,toLowerCase/toUpperCase/trimStart/trimEnd,isWellFormed, andtoWellFormed. Adding one compile-success case per method family here would better lock the behavior and prevent partial regressions.🤖 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/tests/argless_builtin_extra_args.rs` around lines 91 - 129, The test file currently only verifies compile-success cases for the argless methods trim and pop, but the PR modifies several other argless methods including shift, toLowerCase, toUpperCase, trimStart, trimEnd, isWellFormed, and toWellFormed. Add a new test function for each of these remaining modified argless methods following the same pattern as string_trim_with_extra_arg_compiles and array_pop_with_extra_arg_compiles—create an AST that calls each method with an extra argument, compile it, and assert that compilation succeeds. This ensures regression coverage for all modified argless arms and prevents partial regressions if only some of them are properly handled.
🤖 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-codegen/tests/argless_builtin_extra_args.rs`:
- Around line 91-129: The test file currently only verifies compile-success
cases for the argless methods trim and pop, but the PR modifies several other
argless methods including shift, toLowerCase, toUpperCase, trimStart, trimEnd,
isWellFormed, and toWellFormed. Add a new test function for each of these
remaining modified argless methods following the same pattern as
string_trim_with_extra_arg_compiles and array_pop_with_extra_arg_compiles—create
an AST that calls each method with an extra argument, compile it, and assert
that compilation succeeds. This ensures regression coverage for all modified
argless arms and prevents partial regressions if only some of them are properly
handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 084df4d3-60e9-4b42-9f2e-caa74338de01
📒 Files selected for processing (3)
crates/perry-codegen/src/lower_array_method.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/tests/argless_builtin_extra_args.rs
JS ignores surplus arguments to argless methods ("x".trim(1) is legal
and returns the trimmed string). perry's codegen bail!'d with "takes no
args, got N" for String.{toLowerCase,toUpperCase,trim,trimStart,trimEnd,
isWellFormed,toWellFormed} and Array.{pop,shift}, rejecting valid JS.
Drop the arg-count bail in all 5 sites; evaluate extra args for their
side effects (ECMA-262 evaluates arguments before the call) then discard,
matching the existing Annex B HTML-wrapper convention in the same file.
Clears the first codegen wall on the claude-code cli.js bundle.
Adds tests/argless_builtin_extra_args.rs covering "x".trim(1) and
[1].pop(99).
bf4dbfc to
69bcb0e
Compare
What
Argless built-in methods (
String.prototypetrim/trimStart/trimEnd/toLowerCase/toUpperCase/isWellFormed/toWellFormed,Array.prototypepop/shift)bail!ed in codegen when called with extra args. But per ECMA-262 those args are simply ignored ("x".trim(1)is legal and returns the trimmed string). Minified bundles do this. Now codegen evaluates the surplus args for side effects (spec requires arg evaluation) and discards them, matching the Annex-B HTML-wrapper convention already in the same file, instead of failing the compile.Why
Surfaced compiling a large real-world minified bundle:
perry-codegen: String.trim takes no args, got 1aborted the whole module.Tests
New
crates/perry-codegen/tests/argless_builtin_extra_args.rs(" x ".trim(1)→js_string_trim;[1].pop(99)compiles).cargo test -p perry-codegen --testsgreen.Summary by CodeRabbit
Bug Fixes
popandshiftand string methodstoLowerCase,toUpperCase,trim,trimStart,trimEnd, andisWellFormedto ignore extra arguments while still evaluating them for side effects, matching JavaScript behavior.Tests