Conversation
…ber, String, Boolean Bare calls to these compiled to LoadGlobal -> undefined -> "not a function". They are now recognized at compile time (shadowable by a local of the same name, like external functions) and dispatched via a new CallBuiltin instruction to builtins::call_global_function. Test262: built-ins/parseInt 0% -> 73%, parseFloat 0% -> 76%.
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
|
Hi @TheUncharted! I'd love to see this and many of the other fantastic fixes by @jtippett merged into this project, unless you have any strong objections. Thank you both for the contributions; this is a great project! |
TheUncharted
left a comment
TheUncharted
left a comment
There was a problem hiding this comment.
The basic builtin tests pass (72/72), and I found no malicious code or direct host-capability escape. I am requesting changes for four focused issues documented inline: lexical/first-class name resolution, serialized instruction ordering, JavaScript conversion semantics, and resource bounds for long parsing operations. The name-resolution and coercion reproductions were executed against this exact PR head; the snapshot and resource findings are grounded in the serialization/allocation code paths.
|
|
||
| fn parse_int(args: &[Value]) -> Value { | ||
| let s = args.first().map(|v| v.to_js_string()).unwrap_or_default(); | ||
| let chars: Vec<char> = s.trim_start().chars().collect(); |
There was a problem hiding this comment.
This parser clones the full input string and then materializes Vec<char>, but neither temporary allocation is charged to ResourceTracker. The subsequent scan runs inside one opcode, while the VM checks elapsed time only between opcodes. A sufficiently large guest- or host-provided string can therefore amplify memory and occupy the worker beyond configured limits before control returns to the VM.
I deliberately did not run an unbounded stress payload during review. This finding is based on the allocation and control-flow path here.
Please avoid materializing Vec<char> by parsing with an iterator/byte indices, impose an input-size/resource preflight, and periodically check the execution deadline during long scans. parseFloat needs equivalent bounds. Add a bounded test with low memory/time limits that returns a controlled limit error.
| Return, | ||
| CallExternal(String, usize), | ||
| /// Call a global builtin function by name (e.g. parseInt, isNaN, Number). | ||
| CallBuiltin(String, usize), |
There was a problem hiding this comment.
Instruction derives serde serialization and the complete CompiledProgram is stored in postcard snapshots. Inserting CallBuiltin here shifts the enum tags for every following opcode (Jump onward), so a snapshot created by Zapcode 1.5.3 may decode old instructions as different instructions after an upgrade.
Example scenario:
const response = await externalTool();
response ? 1 : 2;The persisted snapshot contains the post-suspension jump bytecode. After this insertion, those old enum tags no longer identify the same opcodes.
Please append CallBuiltin at the end of the enum to preserve existing tags. Longer term, snapshots should use an explicit format version and legacy fixture tests, but preserving enum order is the minimal fix required in this PR.
| if self.resolve_local(name).is_none() && is_global_builtin_fn(name) { | ||
| for arg in args { | ||
| self.compile_expr(arg)?; | ||
| } |
There was a problem hiding this comment.
This compile-time rewrite bypasses normal lexical/global resolution. resolve_local() sees only this function's local slots, not a binding captured from an enclosing scope. The builtin also is not registered as a first-class global value.
Both cases were verified at this PR head:
const Number = value => value + 1;
const call = () => Number(41);
call();Expected: 42; actual: 41.
const parse = parseInt;
parse("42");Expected: 42; actual: TypeError("undefined is not a function").
Please register these as normal builtin function Values and invoke them through ordinary lexical/global call resolution. That lets captured/local bindings shadow them and allows assignment, callbacks, and object properties. Please add both cases as regression tests.
| Some(v) => Value::String(Arc::from(v.to_js_string().as_str())), | ||
| None => Value::String(Arc::from("")), | ||
| }, | ||
| "Number" => match args.first() { |
There was a problem hiding this comment.
These conversions use the current generic to_number()/Rust parsing behavior rather than ECMAScript coercion. I verified:
[Number(""), Number(" 42 "), Number("0x10")]Expected: [0, 42, 16]; actual: [NaN, NaN, NaN].
I also verified:
parseInt("10", 4294967298)Expected: 2 because JavaScript applies ToInt32 to the radix; actual: NaN.
Please implement shared ECMAScript-style ToNumber/ToString helpers and use ToInt32 for the radix. At minimum, cover empty/trimmed strings, numeric prefixes, signed zero, Infinity, and radix conversion in regression tests. If exact ECMAScript behavior is intentionally out of scope, these functions need explicit documented subset semantics rather than presenting them as standard globals.
3 participants
Summary
Agent-generated code constantly reaches for
parseInt/parseFloat/isNaN/isFiniteand theNumber/String/Booleanconversion functions; today a bare call compiles toLoadGlobal → undefined → "x is not a function". All seven are pure computation with no sandbox surface.Changes
CallBuiltininstruction dispatching tobuiltins::call_global_function.tests/builtins.rs.Test plan
cargo test)make lintcleanbuilt-ins/parseInt0% → 73%,parseFloat0% → 76%Related issues
None open. We run this patch in production via ex_zapcode.