| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughBuiltins now receive a frozen, per-isolate primordials snapshot. Runtime builtins use captured intrinsic operations, lint rules enforce the usage pattern, build inputs include the snapshot module, and tests cover behavior when application code replaces intrinsic methods. ChangesPrimordials runtime integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuiltinLoader
participant Caches
participant kPrimordials
participant CompiledBuiltin
BuiltinLoader->>Caches: Check cached Primordials
BuiltinLoader->>kPrimordials: Create frozen intrinsic snapshot
kPrimordials-->>Caches: Cache Primordials
BuiltinLoader->>CompiledBuiltin: Invoke with binding and primordials
Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
|
Orchestrator review pass (folded into the commit):
Deferred (pre-existing, unchanged by this PR): child[Symbol.hasInstance] = ... in ts-helpers is likely a silent no-op in sloppy mode (inherited non-writable Function.prototype[@@hasInstance]), and instanceof Array in ObjCClass could become ArrayIsArray — both worth their own look since fixing them changes behavior. |
Sorry, something went wrong.
The runtime's builtins install globals and leave closures behind that keep running for the lifetime of the app: event dispatch, the Promise proxy traps, console's smart-stringify, __extends. Those closures reached for intrinsics (Array.prototype.slice, JSON.stringify, Object.create, ...) through the live globals, so app code replacing one could break or observe runtime internals. primordials.js captures the intrinsics the other builtins need into a frozen, null-prototype namespace and BuiltinLoader passes it to every builtin as a second fixed parameter next to `binding`. It is built lazily on the first RunBuiltin of an isolate — during runtime init, before user code — and cached in Caches, so workers snapshot their own realm and late-compiling builtins still see pristine intrinsics. Instance methods are uncurried Node-style, `uncurryThis(fn)` being Function.prototype.bind.bind(Function.prototype.call): ArrayPrototypeSlice(list, 1) rather than list.slice(1). Benchmarked under jitless (which is how the runtime always runs on device) uncurried calls cost +5-12% per op over a raw method call but beat the captured-and-.call() alternative, so they are used uniformly. ESLint gains no-restricted-properties for the captured statics and no-restricted-globals for the captured constructors, each pointing at the replacement; uncurried instance-method use stays a review rule since the receiver cannot be matched.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)NativeScript/runtime/js/ts-helpers.js (1)27-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the silent no-op when assigning child[SymbolHasInstance].
This file has no "use strict" pragma, so it runs in sloppy mode. Function.prototype[Symbol.hasInstance] is a non-writable, non-configurable inherited property. A plain assignment to child[SymbolHasInstance] on a function that does not already own that property fails silently in sloppy mode (it would throw in strict mode instead). The custom instanceof override for extended native classes never actually installs.
Use ObjectDefineProperty (already imported) to force an own property instead of a plain assignment. This mirrors the PR's own deferred-issue note about this exact line.
🐛 Proposed fix🤖 Prompt for AI Agents- child[SymbolHasInstance] = function (instance) { - return instance instanceof this.__extended; - } + ObjectDefineProperty(child, SymbolHasInstance, { + value: function (instance) { + return instance instanceof this.__extended; + }, + configurable: true, + });Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/js/ts-helpers.js` around lines 27 - 38, Update the child[SymbolHasInstance] assignment inside extend to use the already imported ObjectDefineProperty, defining an own SymbolHasInstance property on child with the existing custom instanceof function as its value.
eslint.config.mjs (1)🤖 Prompt for all review comments with AI agentsNativeScript/runtime/js/promise-proxy.js (1)14-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add Symbol.hasInstance to the captured-statics lint list.
primordials.js captures SymbolHasInstance: Symbol.hasInstance as a static-like binding, but capturedStatics here does not include an entry for it. Every other captured static (JSON.stringify, Object.*, Reflect.construct) is protected by no-restricted-properties; a bare Symbol.hasInstance reference in a future builtin change would not be caught.
Add ['Symbol', 'hasInstance', 'SymbolHasInstance'] to capturedStatics for parity with the rest of the enforcement mechanism.
♻️ Proposed addition🤖 Prompt for AI Agentsconst capturedStatics = [ ['JSON', 'stringify', 'JSONStringify'], ['Object', 'assign', 'ObjectAssign'], ['Object', 'create', 'ObjectCreate'], ['Object', 'defineProperty', 'ObjectDefineProperty'], ['Object', 'freeze', 'ObjectFreeze'], ['Object', 'getOwnPropertyDescriptor', 'ObjectGetOwnPropertyDescriptor'], ['Object', 'keys', 'ObjectKeys'], ['Object', 'setPrototypeOf', 'ObjectSetPrototypeOf'], ['Reflect', 'construct', 'ReflectConstruct'], // No ReflectApply primordial: apply goes through the uncurried // Function.prototype.apply, which is one property read cheaper. ['Reflect', 'apply', 'FunctionPrototypeApply'], + ['Symbol', 'hasInstance', 'SymbolHasInstance'], ];Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.mjs` around lines 14 - 27, Update the capturedStatics list to include the Symbol.hasInstance mapping as ['Symbol', 'hasInstance', 'SymbolHasInstance'], matching the existing captured-static enforcement entries.56-63: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Optional: avoid an unnecessary bound-function allocation on the synchronous path.
Line 60 calls FunctionPrototypeBind(orig, target, x)(), creating and discarding a bound function to invoke it once. FunctionPrototypeCall(orig, target, x) achieves the same result without the extra allocation. This does not need Function.prototype.bind at all since the call happens immediately in the same tick. This is not a regression from this PR; the pattern predates the primordials conversion.
♻️ Proposed refactor🤖 Prompt for AI Agents-const { FunctionPrototypeBind, Proxy } = primordials; +const { FunctionPrototypeBind, FunctionPrototypeCall, Proxy } = primordials; @@ return typeof orig === 'function' ? function(x) { if (!originIsRuntimeLoop || runloop === CFRunLoopGetCurrent()) { - FunctionPrototypeBind(orig, target, x)(); + FunctionPrototypeCall(orig, target, x); return target; } CFRunLoopPerformBlock(runloop, kCFRunLoopDefaultMode, FunctionPrototypeBind(orig, target, x));Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/js/promise-proxy.js` around lines 56 - 63, In the synchronous branch of the function returned by the promise proxy, replace the immediate FunctionPrototypeBind invocation with FunctionPrototypeCall(orig, target, x) so the original function is invoked with the same receiver and argument without allocating a bound function. Leave the asynchronous CFRunLoopPerformBlock path unchanged.
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Outside diff comments: In `@NativeScript/runtime/js/ts-helpers.js`: - Around line 27-38: Update the child[SymbolHasInstance] assignment inside extend to use the already imported ObjectDefineProperty, defining an own SymbolHasInstance property on child with the existing custom instanceof function as its value. --- Nitpick comments: In `@eslint.config.mjs`: - Around line 14-27: Update the capturedStatics list to include the Symbol.hasInstance mapping as ['Symbol', 'hasInstance', 'SymbolHasInstance'], matching the existing captured-static enforcement entries. In `@NativeScript/runtime/js/promise-proxy.js`: - Around line 56-63: In the synchronous branch of the function returned by the promise proxy, replace the immediate FunctionPrototypeBind invocation with FunctionPrototypeCall(orig, target, x) so the original function is invoked with the same receiver and argument without allocating a bound function. Leave the asynchronous CFRunLoopPerformBlock path unchanged.
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d5baefd-2e45-46d0-9e09-7bfc5f6f6105
📥 CommitsReviewing files that changed from the base of the PR and between 7a4d97f and e79fea2.
📒 Files selected for processing (17)
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Stacked on #411 — review that one first; this PR targets feat/js-builtins.
What this adds
The runtime's builtin JavaScript installs globals and leaves closures behind that run for the lifetime of the app: event dispatch, the Promise proxy traps, console's smart-stringify, __extends, the URL.searchParams accessor. Until now those closures reached for intrinsics (Array.prototype.slice, JSON.stringify, Object.create, …) through the live globals, so app code replacing one could break runtime internals or observe them.
NativeScript/runtime/js/primordials.js is a new builtin that captures exactly the intrinsics the other builtins need into a frozen, null-prototype namespace, Node-style.
The (binding, primordials) contract
Builtins are now compiled as function bodies with two fixed parameters:
primordials is built lazily on the first RunBuiltin of an isolate — which happens during runtime init, before any user code — and cached in Caches::Primordials. Consequences:
Why uncurrying, uniformly
Instance methods are exposed uncurried, exactly as Node does it:
The runtime always runs V8 jitless on device, so the usual "uncurrying is free once optimized" argument does not apply and the cost had to be measured. Benchmarked on Node 24 / V8 13.6, arm64 host, --jitless:
Plain constructor calls made once at init time (new Map() while bootstrapping) may stay direct — the rule targets code in closures that outlive init.
Enforcement
eslint.config.mjs declares primordials and adds two rules over NativeScript/runtime/js, both verified to fire:
Each message names the replacement. primordials.js itself is exempted via a per-file override. Uncurried instance-method use cannot be matched by receiver, so that stays a review rule — documented as such in NativeScript/runtime/js/README.md.
Deliberately left as live global lookups: Reflect.decorate in inline-functions.js (reflect-metadata installs it after runtime init, and the TypeScript __decorate shim must see it), Blob/File in blob-url.js (provided by the app layer, not the runtime), and String(x)/instanceof Array conversions where tamper-immunity is not affected.
ts-helpers builds native super calls with the captured Reflect.construct rather than bind-then-new, which keeps that path off the array iterator protocol as well.
Tests
New TestRunner/app/tests/PrimordialsTests.js (7 specs) tampers with the intrinsics and checks the runtime keeps working. Each spec keeps the tampered window synchronous and assertion-free, restoring the originals in a finally:
Result: 912 specs, 0 failures (905 baseline + 7), independently reproduced on a second simulator. ESLint clean; every builtin verified to compile as a (binding, primordials) function body.
Known follow-ups (not in this PR)
Pre-existing live-intrinsic reads that primordials does not yet cover: for...of ObjectKeys(...) in __tsEnum and ArrayFrom(arguments) in inline-functions.js still go through the array iterator protocol, and String(x) conversions in events.js/error-events.js read the live global String.
Summary by CodeRabbit
New Features
Bug Fixes
Tests