fix(hir): give an imported constructor's instanceof its value

changelog.d/10596-instanceof-imported-fn-ctor.md

@@ -0,0 +1,11 @@
+### Fixed
+
+- **`x instanceof F` no longer folds to `false` for an imported non-class constructor.** Lowering
+  (`crates/perry-hir/src/lower/lower_expr/arm_bin.rs`) only attached a runtime value to an identifier
+  `instanceof` RHS for a local, a module function, or a native module — an imported binding was never
+  consulted, so codegen resolved the bare name to no class id and folded the check to
+  `js_instanceof(v, 0)` (always false). Affected every import form (named, default, CJS
+  `module.exports`/`exports.F`) for a plain ES5-style or factory-built constructor; `ns.F`, a local
+  alias, and the check written inside the defining module all worked already. Codegen
+  (`crates/perry-codegen/src/expr/instance_misc1.rs`) keeps the static class-id fast path for
+  imported classes and every non-compiled-source import, so those emit unchanged LLVM IR.

crates/perry-codegen/src/expr/instance_misc1.rs

@@ -126,6 +126,39 @@ pub(crate) fn builtin_parent_reserved_class_id(name: &str) -> Option<u32> {
     })
 }
 
+/// #10477: HIR hands `x instanceof F` with an IMPORTED `F` to codegen as a
+/// dynamic RHS (the binding's `ExternFuncRef` value), because only codegen
+/// knows what the import resolved to. Returns `true` when the static
+/// `js_instanceof(v, <class id>)` path below still answers the question, so the
+/// value is never materialized:
+///
+/// - an imported CLASS: its value is the INT32 class-ref immediate that
+///   `js_instanceof_dynamic` would only unpack back to the same id. The filter
+///   mirrors `ExternFuncRef`'s value lowering (`dyn_extern_i18n.rs`), so class
+///   metadata that is not this lexical binding never claims it;
+/// - a binding that is not a compiled source-module import (a V8-fallback or
+///   node-submodule import, an FFI `declare function`, an unresolved name): its
+///   value form is a placeholder, and these keep their reserved-id mapping.
+///
+/// Every other import (a function constructor, an exported `const` holding
+/// one, a CJS `module.exports = F`) has no class id, so the static path folded
+/// it to id 0 and the check was always `false`. `name == ty` confines this to
+/// the bare-identifier RHS; a parenthesized or cast RHS was dynamic before.
+fn imported_instanceof_rhs_is_static(ctx: &FnCtx<'_>, ty: &str, ty_expr: &Expr) -> bool {
+    let Expr::ExternFuncRef { name, .. } = ty_expr else {
+        return false;
+    };
+    if name != ty {
+        return false;
+    }
+    let imported_class = ctx.class_ids.contains_key(name)
+        && !ctx.imported_vars.contains(name)
+        && !ctx.namespace_imports.contains(name);
+    imported_class
+        || !ctx.import_function_prefixes.contains_key(name)
+        || ctx.import_function_v8_specifiers.contains_key(name)
+}
+
 fn emit_with_key(ctx: &mut FnCtx<'_>, property: &str) -> (String, String) {
     let key_idx = ctx.strings.intern(property);
     let key_entry = ctx.strings.entry(key_idx);
@@ -334,7 +367,10 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             // #7615 slice 2: `v` is live across the RHS's lowering, so the pair
             // is rooted as a group. The static-RHS path below lowers nothing
             // after `v` and keeps its plain `lower_expr`.
-            if let Some(ty_e) = ty_expr {
+            if let Some(ty_e) = ty_expr
+                .as_deref()
+                .filter(|ty_e| !imported_instanceof_rhs_is_static(ctx, ty, ty_e))
+            {
                 return rooting::with_operands_rooted(ctx, &[e, ty_e], |ctx, vals| {
                     Ok(ctx.block().call(
                         DOUBLE,

crates/perry-codegen/src/expr/instanceof_imported_rhs_tests.rs

@@ -0,0 +1,152 @@
+//! #10477: `x instanceof F` where `F` is an IMPORTED binding.
+//!
+//! HIR cannot tell an imported function constructor from an imported class —
+//! both are `ExternFuncRef` bindings — so it hands every imported RHS to
+//! codegen as a dynamic `ty_expr` and codegen decides. Both directions are
+//! asserted here, because each one silently degrades in a way no other test
+//! sees:
+//!
+//! * a compiled-source import with no class id must reach
+//!   `js_instanceof_dynamic`, which resolves the constructor value and walks
+//!   the prototype chain. Before #10477 it folded to `js_instanceof(v, 0)` —
+//!   a well-formed call that always answers `false`;
+//! * an imported CLASS must keep the static `js_instanceof(v, <class id>)`
+//!   check. Routing it through the dynamic helper would still be correct, so
+//!   only an IR census catches the regression: the class fast path would just
+//!   get slower.
+
+use crate::{compile_module, CompileOptions, ImportedClass};
+use perry_hir::types::Type;
+use perry_hir::{Expr, Module, Stmt};
+
+const DYNAMIC_CALL: &str = "call double @js_instanceof_dynamic(";
+const STATIC_CALL: &str = "call double @js_instanceof(";
+
+fn imported_ref(name: &str) -> Expr {
+    Expr::ExternFuncRef {
+        name: name.to_string(),
+        param_types: Vec::new(),
+        return_type: Type::Any,
+    }
+}
+
+/// `{} instanceof <name>` with the imported binding's value attached, exactly
+/// as `lower_expr/arm_bin.rs` lowers a bare imported identifier RHS.
+fn instanceof_imported(name: &str) -> Module {
+    let mut module = Module::new("instanceof_imported.ts");
+    module.init = vec![Stmt::Expr(Expr::InstanceOf {
+        expr: Box::new(Expr::Object(Vec::new())),
+        ty: name.to_string(),
+        ty_expr: Some(Box::new(imported_ref(name))),
+    })];
+    module
+}
+
+fn imported_class(name: &str, class_id: u32) -> ImportedClass {
+    ImportedClass {
+        name: name.to_string(),
+        local_alias: None,
+        namespace: None,
+        source_prefix: "lib_ts".to_string(),
+        constructor_param_count: 0,
+        has_own_constructor: true,
+        constructor_has_rest: false,
+        has_instance_fields: false,
+        method_names: Vec::new(),
+        proven_this_method_names: Vec::new(),
+        proven_this_tower_method_names: Vec::new(),
+        method_return_types: Vec::new(),
+        method_param_counts: Vec::new(),
+        method_has_rest: Vec::new(),
+        method_has_synthetic_arguments: Vec::new(),
+        method_arguments_length_only: Vec::new(),
+        static_field_names: Vec::new(),
+        static_method_names: Vec::new(),
+        static_method_return_types: Vec::new(),
+        static_method_param_counts: Vec::new(),
+        static_method_has_rest: Vec::new(),
+        static_method_has_user_rest: Vec::new(),
+        static_method_has_synthetic_arguments: Vec::new(),
+        getter_names: Vec::new(),
+        getter_return_types: Vec::new(),
+        setter_names: Vec::new(),
+        parent_name: None,
+        field_names: Vec::new(),
+        field_types: Vec::new(),
+        source_class_id: Some(class_id),
+        return_shape_imports: Vec::new(),
+        object_literal: None,
+    }
+}
+
+fn compile(module: &Module, opts: CompileOptions) -> String {
+    String::from_utf8(compile_module(module, opts).expect("instanceof module compiles"))
+        .expect("LLVM IR is UTF-8")
+}
+
+#[test]
+fn imported_function_constructor_rhs_resolves_the_constructor_value() {
+    let mut opts = CompileOptions {
+        emit_ir_only: true,
+        ..Default::default()
+    };
+    opts.import_function_prefixes
+        .insert("Plain".to_string(), "lib_ts".to_string());
+    let ir = compile(&instanceof_imported("Plain"), opts);
+
+    assert!(
+        ir.contains(DYNAMIC_CALL),
+        "an imported function constructor must resolve its value and walk the \
+         prototype chain:\n{ir}"
+    );
+    assert!(
+        !ir.contains(STATIC_CALL),
+        "the class-id check has no id for a function import — it folds to \
+         `false` (#10477):\n{ir}"
+    );
+}
+
+#[test]
+fn imported_class_rhs_keeps_the_static_class_id_check() {
+    let mut opts = CompileOptions {
+        emit_ir_only: true,
+        ..Default::default()
+    };
+    opts.import_function_prefixes
+        .insert("Klass".to_string(), "lib_ts".to_string());
+    opts.imported_classes.push(imported_class("Klass", 7701));
+    let ir = compile(&instanceof_imported("Klass"), opts);
+
+    assert!(
+        ir.contains(STATIC_CALL),
+        "an imported class must keep the static class-id check:\n{ir}"
+    );
+    assert!(
+        !ir.contains(DYNAMIC_CALL),
+        "the dynamic helper would only unpack the same class id back out:\n{ir}"
+    );
+}
+
+#[test]
+fn unresolved_import_rhs_keeps_its_reserved_builtin_id() {
+    // Not a compiled source module (nothing in `import_function_prefixes`):
+    // the binding's value form is a placeholder, so the reserved-id mapping
+    // stays — `js_instanceof_dynamic` on a placeholder would throw instead of
+    // answering.
+    let ir = compile(
+        &instanceof_imported("Error"),
+        CompileOptions {
+            emit_ir_only: true,
+            ..Default::default()
+        },
+    );
+
+    assert!(
+        ir.contains(STATIC_CALL),
+        "an unresolved import must keep the static reserved-id check:\n{ir}"
+    );
+    assert!(
+        !ir.contains(DYNAMIC_CALL),
+        "nothing resolves this binding to a constructor value:\n{ir}"
+    );
+}

crates/perry-codegen/src/expr/mod.rs

@@ -170,6 +170,8 @@ mod entry_block_alloca_tests;
 mod hit_path_access_tests;
 #[cfg(test)]
 mod index_set_barrier_tests;
+#[cfg(test)]
+mod instanceof_imported_rhs_tests;
 mod record_value;
 mod repsel_gates;
 mod scalar_slot_root;

crates/perry-hir/src/lower/lower_expr/arm_bin.rs

@@ -148,9 +148,18 @@ pub(crate) fn lower_bin_expr(ctx: &mut LoweringContext, bin: &ast::BinExpr) -> R
                 // route through `js_instanceof_dynamic`, which derives the same
                 // `synthetic_class_id_for_function` that `new Foo()` stamps onto
                 // the instance (see js_new_function_construct).
+                //
+                // #10477: an IMPORTED binding is a runtime value too. It has
+                // no class entry in this module unless it is an imported
+                // class, so without its value an imported function
+                // constructor (`import F from "./f.js"`, CJS
+                // `module.exports = F`, decimal.js's `Decimal`) folded to
+                // class_id 0 and `x instanceof F` was always false. Codegen
+                // keeps the static class-id check for imported classes.
                 if ctx.lookup_local(name).is_some()
                     || ctx.lookup_func(name).is_some()
                     || ctx.lookup_native_module(name).is_some()
+                    || ctx.lookup_imported_func(name).is_some()
                 {
                     match lower_expr(ctx, &bin.right) {
                         Ok(e) => Some(Box::new(e)),

crates/perry-hir/src/lower/tests.rs

@@ -15,6 +15,7 @@ fn make_ctx() -> LoweringContext {
     LoweringContext::new("test.ts")
 }
 
+mod instanceof_rhs;
 mod literal_shape;
 
 #[test]

crates/perry-hir/src/lower/tests/instanceof_rhs.rs

@@ -0,0 +1,79 @@
+//! #10477: which `instanceof` right-hand sides carry a runtime value.
+//!
+//! An identifier RHS only reaches `js_instanceof_dynamic` when lowering
+//! attaches the binding's value as `ty_expr`; otherwise codegen resolves the
+//! bare NAME to a class id, and a name with no class entry folds to
+//! `js_instanceof(v, 0)` — always `false`. An imported binding was in exactly
+//! that position: `import { Plain } from "./lib.js"; x instanceof Plain` was
+//! false for every non-class constructor, in every import form, while
+//! `ns.Plain`, `const A = Plain` and the same check inside `lib.js` all worked.
+//!
+//! The builtin case is the other half: `x instanceof Date` must NOT grow a
+//! value, because the reserved class id is what brand-checks a native Date.
+
+use super::*;
+
+fn instanceof_rhs_value(source: &str, function: &str) -> Option<Expr> {
+    let module =
+        perry_parser::parse_typescript(source, "instanceof-rhs.ts").expect("source parses");
+    let hir = crate::lower::lower_module(&module, "instanceof-rhs", "instanceof-rhs.ts")
+        .expect("source lowers");
+    let body = hir
+        .functions
+        .iter()
+        .find(|f| f.name == function)
+        .unwrap_or_else(|| panic!("`{function}` must be lowered: {hir:?}"))
+        .body
+        .clone();
+    match body.first() {
+        Some(Stmt::Return(Some(Expr::InstanceOf { ty_expr, .. }))) => {
+            ty_expr.as_ref().map(|value| (**value).clone())
+        }
+        other => panic!("`{function}` must lower to a returned instanceof: {other:?}"),
+    }
+}
+
+#[test]
+fn imported_binding_instanceof_rhs_carries_its_value() {
+    let rhs = instanceof_rhs_value(
+        r#"
+            import { Plain } from "./lib.js";
+            export function check(x: unknown) { return x instanceof Plain; }
+        "#,
+        "check",
+    );
+    assert!(
+        matches!(&rhs, Some(Expr::ExternFuncRef { name, .. }) if name == "Plain"),
+        "an imported RHS must be lowered to its value so the dynamic path can \
+         resolve the constructor (#10477): {rhs:?}"
+    );
+}
+
+#[test]
+fn builtin_instanceof_rhs_stays_a_static_name() {
+    let rhs = instanceof_rhs_value(
+        r#"
+            export function check(x: unknown) { return x instanceof Date; }
+        "#,
+        "check",
+    );
+    assert!(
+        rhs.is_none(),
+        "an unshadowed builtin RHS must keep the reserved-class-id check: {rhs:?}"
+    );
+}
+
+#[test]
+fn local_function_constructor_instanceof_rhs_carries_its_value() {
+    let rhs = instanceof_rhs_value(
+        r#"
+            function Plain(this: any) {}
+            export function check(x: unknown) { return x instanceof Plain; }
+        "#,
+        "check",
+    );
+    assert!(
+        matches!(&rhs, Some(Expr::FuncRef(_))),
+        "a module-local function constructor keeps its pre-#10477 value RHS: {rhs:?}"
+    );
+}

test-files/fixtures/issue_10477_fn_ctor/cjs_default.cjs

@@ -0,0 +1,7 @@
+function CjsCtor(v) {
+  this.v = v;
+}
+CjsCtor.prototype.get = function () {
+  return this.v;
+};
+module.exports = CjsCtor;

test-files/fixtures/issue_10477_fn_ctor/cjs_named.cjs

@@ -0,0 +1,4 @@
+function Named(v) {
+  this.v = v;
+}
+exports.Named = Named;

test-files/fixtures/issue_10477_fn_ctor/default_fn.ts

@@ -0,0 +1,3 @@
+export default function DefaultFn(this: any, v: number) {
+  this.v = v;
+}

test-files/fixtures/issue_10477_fn_ctor/default_var.ts

@@ -0,0 +1,11 @@
+// `var D = factory(); export default D` — decimal.js's module shape.
+function factory() {
+  function Dec(this: any, v: number): any {
+    if (!(this instanceof Dec)) return new (Dec as any)(v);
+    this.v = v;
+  }
+  Dec.prototype = { constructor: Dec };
+  return Dec;
+}
+var D: any = factory();
+export default D;