feat: Node-style primordials for runtime builtins - #415
Conversation
📝 WalkthroughWalkthroughBuiltins now receive a frozen, per-isolate 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: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
e31477b to
bce9ece
Compare
|
Orchestrator review pass (folded into the commit):
Deferred (pre-existing, unchanged by this PR): |
bce9ece to
1c9f00c
Compare
1c9f00c to
c4fd06d
Compare
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.
c4fd06d to
e79fea2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 winFix 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 tochild[SymbolHasInstance]on a function that does not already own that property fails silently in sloppy mode (it would throw in strict mode instead). The custominstanceofoverride 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
- child[SymbolHasInstance] = function (instance) { - return instance instanceof this.__extended; - } + ObjectDefineProperty(child, SymbolHasInstance, { + value: function (instance) { + return instance instanceof this.__extended; + }, + configurable: true, + });🤖 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 `@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.
🧹 Nitpick comments (2)
eslint.config.mjs (1)
14-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
Symbol.hasInstanceto the captured-statics lint list.
primordials.jscapturesSymbolHasInstance: Symbol.hasInstanceas a static-like binding, butcapturedStaticshere does not include an entry for it. Every other captured static (JSON.stringify,Object.*,Reflect.construct) is protected byno-restricted-properties; a bareSymbol.hasInstancereference in a future builtin change would not be caught.Add
['Symbol', 'hasInstance', 'SymbolHasInstance']tocapturedStaticsfor parity with the rest of the enforcement mechanism.♻️ Proposed addition
const 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'], ];🤖 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 `@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.NativeScript/runtime/js/promise-proxy.js (1)
56-63: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: 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
-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));🤖 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 `@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.
🤖 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.
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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d5baefd-2e45-46d0-9e09-7bfc5f6f6105
📒 Files selected for processing (17)
NativeScript/runtime/BuiltinLoader.cppNativeScript/runtime/BuiltinLoader.hNativeScript/runtime/Caches.hNativeScript/runtime/js/README.mdNativeScript/runtime/js/blob-url.jsNativeScript/runtime/js/class-extends.jsNativeScript/runtime/js/error-events.jsNativeScript/runtime/js/events.jsNativeScript/runtime/js/inline-functions.jsNativeScript/runtime/js/primordials.jsNativeScript/runtime/js/promise-proxy.jsNativeScript/runtime/js/smart-stringify.jsNativeScript/runtime/js/ts-helpers.jsTestRunner/app/tests/PrimordialsTests.jsTestRunner/app/tests/index.jseslint.config.mjstools/js2c-inputs.xcfilelist
2 participants
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
Promiseproxy traps, console's smart-stringify,__extends, theURL.searchParamsaccessor. 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.jsis a new builtin that captures exactly the intrinsics the other builtins need into a frozen, null-prototype namespace, Node-style.The
(binding, primordials)contractBuiltins are now compiled as function bodies with two fixed parameters:
primordialsis built lazily on the firstRunBuiltinof an isolate — which happens during runtime init, before any user code — and cached inCaches::Primordials. Consequences:smart-stringifyis only compiled on the first object logged, which can be well after user code has run.bindingis unchanged; most builtins still receiveundefinedfor it.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:captured.call(recv, …)alternative, which is why uncurrying is used uniformly rather than mixing styles.JSON.stringifyetc.) are free.Reflect.applyis measurably worse and is not used;FunctionPrototypeApplycovers those sites.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.mjsdeclaresprimordialsand adds two rules overNativeScript/runtime/js, both verified to fire:no-restricted-propertiesfor every captured static (JSON.stringify,Object.defineProperty/assign/freeze/create/keys/setPrototypeOf/getOwnPropertyDescriptor,Array.from,Reflect.construct,Reflect.apply).no-restricted-globalsfor every captured constructor (Error,Map,Proxy,TypeError). Aconst { Proxy } = primordialsdestructure shadows the global, so the rule only fires on an unguarded reference.Each message names the replacement.
primordials.jsitself 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 inNativeScript/runtime/js/README.md.Deliberately left as live global lookups:
Reflect.decorateininline-functions.js(reflect-metadatainstalls it after runtime init, and the TypeScript__decorateshim must see it),Blob/Fileinblob-url.js(provided by the app layer, not the runtime), andString(x)/instanceof Arrayconversions where tamper-immunity is not affected.ts-helpersbuilds native super calls with the capturedReflect.constructrather thanbind-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 afinally:dispatchEventdelivers to every listener (function andhandleEvent) withArray.prototype.slice/indexOf/push/spliceandFunction.prototype.callreplacedaddEventListener/removeEventListenerandoncesemantics under the same tamperingreportErrorstill reaches anerrorlistener with a correctErrorEventconsole.logof a circular object stays non-fatal withJSON.stringifyand the replacer'sArray.prototype.indexOf/pushreplaced (the smart-stringify path; its output is not reachable from JS andJsonStringifyObjectswallows a throwing stringify, so this spec is a crash/throw check rather than an output assertion — noted in the test)_super.call(this, x)over an Objective-C base withArray.prototype.slice/concat,Function.prototype.bindandReflect.constructreplaced__extendsand__tsEnumwith theObjectstatics replacedResult: 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__tsEnumandArrayFrom(arguments)ininline-functions.jsstill go through the array iterator protocol, andString(x)conversions inevents.js/error-events.jsread the live globalString.Summary by CodeRabbit
New Features
Bug Fixes
Tests