fix(hir): give an imported constructor's instanceof its value - #10596
proggeramlug wants to merge 2 commits into
Conversation
x instanceof F was always false when F was an imported non-class constructor function. Lowering only attached a runtime value to an identifier RHS for a local, a module function or a native module, so an imported binding reached codegen as a bare name, resolved to no class id and folded to js_instanceof(v, 0) - false for every instance. Every import form was affected (named, default, CJS module.exports/exports.F), while ns.F, a local alias and the check inside the defining module all worked. Imported bindings now lower to their value and take the prototype-chain path. Codegen keeps the static class-id check for an imported class, and for a binding that is not a compiled source-module import (its value form is a placeholder), so the class fast path and the reserved builtin ids are unchanged.
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis fix preserves runtime values for imported function constructors in ChangesImported
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The imported function-constructor behavior is covered across named, default, CommonJS, inheritance, live-binding, and error cases, with controls for existing class and builtin behavior. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 43.24% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 12 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
CodeRabbit review triage: "No actionable comments were generated in the recent review." The only CI: the only failing check is |
|
Landed via merge train #10631 (v0.5.1595). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Summary
x instanceof Falways returnedfalsewhenFwas an imported non-class constructor function(a plain ES5-style function, a factory-built function, or a CJS
module.exports = F). Named import,default import, and CJS forms were all affected;
ns.F, a local alias of the import, and the samecheck written inside the module that defines
Fall worked correctly.Root cause
crates/perry-hir/src/lower/lower_expr/arm_bin.rs(lower_bin_expr'sinstanceofarm, ~line 148)only attached a lowered runtime value (
ty_expr) to an identifier RHS when the name resolved viactx.lookup_local,ctx.lookup_func, orctx.lookup_native_module. An imported binding matchesnone of those —
ctx.lookup_imported_funcwas never consulted — soty_exprstayedNone.Codegen (
crates/perry-codegen/src/expr/instance_misc1.rs, theExpr::InstanceOfarm oflower)then resolved the bare name through the static class-id path. A function constructor has no class id
there, so the check folded to
js_instanceof(v, 0), which is alwaysfalse. Imported classes wereunaffected because they do have a class id (populated from
opts.imported_classesintoctx.class_ids).The fix
arm_bin.rs: add|| ctx.lookup_imported_func(name).is_some()to the condition that decideswhether an identifier RHS gets its value lowered. An imported binding now takes the same dynamic
js_instanceof_dynamicpath a local/module function constructor already did.instance_misc1.rs: a new helperimported_instanceof_rhs_is_static(ctx, ty, ty_expr)gateswhether the static
js_instanceof(v, <class id>)path is still used despite the RHS nowcarrying a value, so the previously-correct cases stay byte-for-byte unchanged:
ExternFuncRefINT32 class-ref immediate that thedynamic helper would only unpack back to the same id (mirrors how
ExternFuncRefvalue-loweringalready special-cases this in
dyn_extern_i18n.rs);declare function, or an unresolved name) — its value form is a placeholder, so thereserved-builtin-id mapping must stay.
Net effect: a function-constructor import now resolves its value and walks the real prototype chain;
an imported class and every non-compiled-source import emit the identical LLVM IR they did before
(asserted by the codegen unit tests via literal
js_instanceof(/js_instanceof_dynamic(IR-stringchecks).
Tests added
test-files/test_gap_10477_instanceof_imported_function_ctor.ts(+ fixtures undertest-files/fixtures/issue_10477_fn_ctor/) — covers named/default/CJS import forms, prototypereassignment (the bignumber.js/decimal.js shape),
ns.F, a local alias, the in-module check,util.inherits/setPrototypeOfhierarchies,Symbol.hasInstance, factory-built constructors,live rebinding, and imported-class controls.
crates/perry-hir/src/lower/tests/instanceof_rhs.rs(3 tests) andcrates/perry-codegen/src/expr/instanceof_imported_rhs_tests.rs(3 tests) — unit-level coverage ofthe lowering condition and the static/dynamic codegen gate, including literal IR-string assertions
that an imported class and an unresolved import keep the exact static form.
Proof the gap test fails on the pre-fix baseline, passes on this fix: ran both against a pristine
origin/mainbuild (9df5075fbe) and against this branch,PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10477:PARITY_FAIL—Node.js: plain importer-made true/Perry: plain importer-made false(Parity Rate 0.0%)
PASS(Parity Rate 100.0%)Validation
cargo test --release -p perry-hir -p perry-codegen --tests: all green, including the 6 new tests(
instanceof_rhs::*,instanceof_imported_rhs_tests::*).python3 scripts/check_test_registration.py: OK, 335 files checked against 4 registries.Lint (
rustup run stable cargo fmt --all -- --checkthenSKIP_COMPILE_GATES=1 ./scripts/run_lint_gates.sh): 76 of 77 script gates passed, compile tier not run (known red onthis host per project docs). The one failure, "Public benchmark evidence freshness", is known-red
on
mainand unrelated to this change.Gap suite: ran only
test_gap_10477_*locally (see above); did not run the full local suite (thechange is scoped to the
instanceofidentifier-RHS path, not a hot lowering/runtime path used bymost programs) — leaving the full sweep to CI's gap-suite shards.
Performance (
perf stat -e instructions,task-clock, 3 runs each, baseline vs this branch, 10Miterations):
instanceof ImportedClass(already-correct fast path — regression check)instanceof ImportedFn(this fix's behavior)¹ Behavior changed: pre-fix, the check always folded to
falseso the loop body (count++) neverran; post-fix it does the real prototype-chain work and increments 10M times — the instruction
count reflects doing the correct work, not a regression on an unchanged code path. The
already-correct class-instanceof path (the actual regression risk, since
imported_instanceof_rhs_is_static's extra branching runs on everyinstanceofwith an identifierRHS) shows no measurable change. Node wall time for context: ~90-96ms for either 10M-iteration
workload (this shared host's wall time is not a reliable A/B metric; instructions is).
Side findings (not fixed here, filed separately)
new (Headers as any)()) builds thebuiltin, not the user's own function/class of that name, even though the same import resolves
correctly through a local alias (
const A: any = Headers; new A()). Filed as new (X as any)() on an imported builtin-named function/class constructs the BUILTIN, not the user's own (Headers/EventEmitter/Stream); a local alias works #10589 (the gap testadded here works around this by constructing through a local alias).
node:eventsimport'sinstanceofsegfaults — reproduces identically on the pristinebaseline, so it predates and is unrelated to this fix. Not independently re-verified against an
existing issue number in this pass; noted here for visibility.
Not verified
function F(){}; export default F;exports a different function object: importers lose F's prototype methods and statics, and missing arguments arrive as garbage instead of undefined #10434's default-export identity issue fordecimal.js'sexport default <identifier>shape).Note on provenance
This fix was implemented and validated on a build host that was destroyed mid-session before it
could be pushed. It was recovered from a session mirror/transcript and has been fully re-applied,
rebuilt, and re-validated from scratch on a different host against current
mainfor this PR(all numbers above are from this re-validation run, not carried over from the lost session).
Fixes #10477
Summary by CodeRabbit
instanceofchecks for imported function-style constructors.Symbol.hasInstance, and live bindings.TypeErrorinstead of incorrectly returningfalse.instanceofscenarios.