From 6465a93e2b7fe7e55d4d9b16f667051a84c54910 Mon Sep 17 00:00:00 2001 From: Perry Bot Date: Sat, 19 Sep 2026 06:31:38 +0000 Subject: [PATCH 1/3] wip: fix #10439 native-binding import provenance --- crates/perry-hir/src/lower_patterns.rs | 38 ++- ..._10439_native_binding_import_provenance.rs | 316 ++++++++++++++++++ 2 files changed, 348 insertions(+), 6 deletions(-) create mode 100644 crates/perry/tests/issue_10439_native_binding_import_provenance.rs diff --git a/crates/perry-hir/src/lower_patterns.rs b/crates/perry-hir/src/lower_patterns.rs index b7f9e4bd39..ab3b35cb8e 100644 --- a/crates/perry-hir/src/lower_patterns.rs +++ b/crates/perry-hir/src/lower_patterns.rs @@ -1403,6 +1403,28 @@ pub(crate) fn pre_scan_node_http_client_request_socket_params( /// the hardcoded library-name mapping — without that gate `class Big { f0=0; } /// const b = new Big(); b.f0` returned 0 because the value was routed through /// big.js's handle-based dispatch. +/// +/// #10439: those two "is it a local class" checks are not the only way this +/// name can mean something other than the native handle. `Big`/`Decimal`/ +/// `BigNumber`/`LRUCache`/`Command` are exactly the names commander, +/// lru-cache, decimal.js and big.js/bignumber.js export themselves, so an +/// import of the REAL package — resolved to real source because the user +/// listed it in `perry.compilePackages` — hits this same match arm with +/// nothing local to shadow it. Chasing the fix-lineage precedent (#10589/ +/// #10608 for an imported plain-function ctor, #10623/#10636 for a +/// require()-destructured native base): decide by what the identifier +/// resolves to, not by its spelling. `is_native_module` (consulted when this +/// module's imports were lowered) already returns `false` for a +/// compilePackages-compiled specifier, so a genuinely compiled `Decimal`/ +/// `Command`/`LRUCache` was never handed to `register_native_module`, and +/// `lookup_native_module` reports that honestly — the same positive-evidence +/// discipline `ident_may_start_native_method_call` and +/// `native_class_from_factory_call` already apply for the sibling shapes +/// just below in `expr_call/static_and_instance.rs`. A name with no native +/// import at all (a bare same-named user function, or an import of an +/// unrelated module) is rejected for the same reason: genuine Big / Decimal / +/// BigNumber / LRUCache / Command usage is always reached through an import +/// of the real package. pub(crate) fn detect_native_instance_expr( ctx: &LoweringContext, expr: &ast::Expr, @@ -1417,12 +1439,16 @@ pub(crate) fn detect_native_instance_expr( { return None; } - match class_name { - "Big" => Some("big.js"), - "Decimal" => Some("decimal.js"), - "BigNumber" => Some("bignumber.js"), - "LRUCache" => Some("lru-cache"), - "Command" => Some("commander"), + let module = match class_name { + "Big" => "big.js", + "Decimal" => "decimal.js", + "BigNumber" => "bignumber.js", + "LRUCache" => "lru-cache", + "Command" => "commander", + _ => return None, + }; + match ctx.lookup_native_module(class_name) { + Some((m, _)) if m == module => Some(module), _ => None, } } else { diff --git a/crates/perry/tests/issue_10439_native_binding_import_provenance.rs b/crates/perry/tests/issue_10439_native_binding_import_provenance.rs new file mode 100644 index 0000000000..6bd71c58d8 --- /dev/null +++ b/crates/perry/tests/issue_10439_native_binding_import_provenance.rs @@ -0,0 +1,316 @@ +//! Regression test for #10439: `new Command()` / `new LRUCache()` / +//! `new Decimal()` were intercepted by CLASS NAME and routed to Perry's +//! native binding regardless of `perry.compilePackages` — a user could not +//! opt out of the (broken) native handle by asking for real-source +//! compilation. Fixed by `detect_native_instance_expr` +//! (`crates/perry-hir/src/lower_patterns.rs`) consulting `lookup_native_module` +//! (the same compilePackages-aware provenance table `is_native_module` +//! populates at import-lowering time) instead of matching on the bare +//! identifier spelling. +//! +//! The hijack only manifested for a method CHAINED DIRECTLY onto `new +//! X(...)` (`new Command().name(...)`, `new LRUCache(...).set(...)`, `new +//! Decimal(...).dividedBy(...)`) — that shape short-circuits straight to +//! `Expr::NativeMethodCall` in `expr_call/static_and_instance.rs`, bypassing +//! every other (already provenance-aware) construction-site gate. A +//! `let`/`const`-bound receiver was never affected, which is why each fixture +//! below exercises the chained form specifically. +//! +//! Modeled on `issue_8749_compiled_package_builtin_import.rs`'s temp +//! `compilePackages` fixture pattern: a fake `node_modules/` with a real +//! ES class shaped like the collision, so compiling it from source is +//! unambiguous (no real npm registry access needed). + +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Write `node_modules//{package.json,index.mjs}` under `root`, +/// exporting the given real class source (`.mjs`, so no extra transpile step +/// leaks into HIR that the real npm packages wouldn't have either). +fn write_fake_package(root: &Path, pkg_name: &str, class_source: &str) { + let pkg = root.join("node_modules").join(pkg_name); + std::fs::create_dir_all(&pkg).expect("mkdir fake package"); + std::fs::write( + pkg.join("package.json"), + format!( + r#"{{ + "name": "{pkg_name}", + "version": "1.0.0", + "type": "module", + "exports": "./index.mjs" +}}"# + ), + ) + .expect("write fake package.json"); + std::fs::write(pkg.join("index.mjs"), class_source).expect("write fake package source"); +} + +fn write_compile_packages_manifest(root: &Path, pkg_name: &str) { + std::fs::write( + root.join("package.json"), + format!( + r#"{{ + "name": "issue-10439-consumer", + "private": true, + "type": "module", + "perry": {{ + "compilePackages": ["{pkg_name}"], + "allow": {{ "compilePackages": ["{pkg_name}"] }} + }} +}}"# + ), + ) + .expect("write consumer package.json"); +} + +/// Compile `entry` (already written under `root`) and return its stdout, +/// asserting both the compile and the run succeeded. +fn compile_and_run(root: &Path, entry_name: &str) -> String { + let entry = root.join(entry_name); + let output = root.join(format!("{entry_name}.bin")); + let compile = Command::new(perry_bin()) + .current_dir(root) + // Auto-optimize triggers a full profile-guided workspace rebuild on + // its first invocation — expensive, and irrelevant to this test + // (which is about construction/dispatch routing, not optimization). + // Every manual repro of #10439 used the same no-auto path. + .env("PERRY_NO_AUTO_OPTIMIZE", "1") + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-cache") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed for {entry_name}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run = Command::new(&output) + .output() + .unwrap_or_else(|e| panic!("run compiled binary for {entry_name}: {e}")); + assert!( + run.status.success(), + "compiled binary failed for {entry_name}\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +/// #10439 case: commander's `Command`. Real source, default import name, +/// chained directly onto `new` — the exact shape `builtin.rs:405`'s +/// unconditional `"Command"` arm used to reach via the unguarded +/// `detect_native_instance_expr` match, regardless of `compilePackages`. +#[test] +fn commander_default_name_reaches_real_source_under_compile_packages() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + write_compile_packages_manifest(root, "commander"); + write_fake_package( + root, + "commander", + r#" +export class Command { + constructor() { this.__name = ""; } + name(n) { + if (n === undefined) return this.__name; + this.__name = n; + return this; + } +} +"#, + ); + + // Default spelling, chained directly on `new` — previously hijacked. + std::fs::write( + root.join("main.ts"), + r#" +import { Command } from "commander"; +console.log(new Command().name("real-commander-source").name()); +"#, + ) + .expect("write main.ts"); + assert_eq!( + compile_and_run(root, "main.ts"), + "real-commander-source\n", + "new Command().name(...).name() must run the compiled real source, not the native handle" + ); + + // Renamed-import control: this already worked before the fix (the + // hardcoded match is keyed on the literal spelling "Command"), and must + // keep working after it. + std::fs::write( + root.join("renamed.ts"), + r#" +import { Command as Cmd } from "commander"; +console.log(new Cmd().name("renamed-control").name()); +"#, + ) + .expect("write renamed.ts"); + assert_eq!( + compile_and_run(root, "renamed.ts"), + "renamed-control\n", + "the renamed-import workaround must still work unchanged" + ); +} + +/// #10439 case: lru-cache's `LRUCache`, chained directly onto `new`. +#[test] +fn lru_cache_default_name_reaches_real_source_under_compile_packages() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + write_compile_packages_manifest(root, "lru-cache"); + write_fake_package( + root, + "lru-cache", + r#" +export class LRUCache { + constructor(opts) { this.__store = new Map(); this.__max = opts && opts.max || 0; } + set(k, v) { this.__store.set(k, v); return this; } + get(k) { return this.__store.get(k); } +} +"#, + ); + + std::fs::write( + root.join("main.ts"), + r#" +import { LRUCache } from "lru-cache"; +console.log(new LRUCache({ max: 3 }).set("a", 1).get("a")); +"#, + ) + .expect("write main.ts"); + assert_eq!( + compile_and_run(root, "main.ts"), + "1\n", + "new LRUCache(...).set(...).get(...) must run the compiled real source, not the native handle" + ); + + std::fs::write( + root.join("renamed.ts"), + r#" +import { LRUCache as Cache } from "lru-cache"; +console.log(new Cache({ max: 3 }).set("a", 2).get("a")); +"#, + ) + .expect("write renamed.ts"); + assert_eq!( + compile_and_run(root, "renamed.ts"), + "2\n", + "the renamed-import workaround must still work unchanged" + ); +} + +/// #10439 case: decimal.js's `Decimal`, chained directly onto `new`. This is +/// the shape #10684 (division/large-multiplication corruption) depends on: +/// the native handle's `dividedBy`/`times` are broken, and the interception +/// prevented compilePackages from ever reaching the real, correct source. +#[test] +fn decimal_default_name_reaches_real_source_under_compile_packages() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + write_compile_packages_manifest(root, "decimal.js"); + write_fake_package( + root, + "decimal.js", + r#" +export default class Decimal { + constructor(v) { this.__v = typeof v === "string" ? parseFloat(v) : v; } + dividedBy(n) { return new Decimal(this.__v / (n instanceof Decimal ? n.__v : n)); } + times(n) { return new Decimal(this.__v * (n instanceof Decimal ? n.__v : n)); } + toString() { return String(this.__v); } +} +"#, + ); + + std::fs::write( + root.join("main.ts"), + r#" +import Decimal from "decimal.js"; +console.log(new Decimal(1).dividedBy(4).toString()); +"#, + ) + .expect("write main.ts"); + assert_eq!( + compile_and_run(root, "main.ts"), + "0.25\n", + "new Decimal(1).dividedBy(4).toString() must run the compiled real source, not the native handle" + ); + + std::fs::write( + root.join("renamed.ts"), + r#" +import Dec from "decimal.js"; +console.log(new Dec(1).dividedBy(4).toString()); +"#, + ) + .expect("write renamed.ts"); + assert_eq!( + compile_and_run(root, "renamed.ts"), + "0.25\n", + "the renamed-import workaround must still work unchanged" + ); +} + +/// The legitimate case this issue explicitly warns against regressing: with +/// NO `perry.compilePackages` entry (and no real package installed at all — +/// there is nothing else it COULD mean), `new Command()...` must still route +/// to the native binding exactly as before. Values asserted here are the +/// native binding's own pre-existing (documented-limited) behavior, captured +/// against this same commit's pre-fix binary — this test exists to prove the +/// fix does not change them, not to bless them as correct. +#[test] +fn commander_default_name_still_uses_native_binding_without_compile_packages() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + // No package.json, no node_modules: "commander" can only resolve to + // Perry's bundled native shim. + std::fs::write( + root.join("main.ts"), + r#" +import { Command } from "commander"; +const program = new Command(); +console.log(new Command().name("x").name()); +console.log(program.constructor.name); +"#, + ) + .expect("write main.ts"); + assert_eq!( + compile_and_run(root, "main.ts"), + "{}\nundefined\n", + "the native-binding path (no compilePackages) must be byte-for-byte unchanged" + ); +} + +/// Same legitimate-case guard for lru-cache: without `compilePackages`, +/// `new LRUCache(...).set(...).get(...)` must still reach the native +/// `js_lru_cache_*` handle path (which happens to compute the right answer +/// for this simple, non-evicting case) rather than falling through to a +/// nonexistent real source. +#[test] +fn lru_cache_default_name_still_uses_native_binding_without_compile_packages() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::write( + root.join("main.ts"), + r#" +import { LRUCache } from "lru-cache"; +console.log(new LRUCache({ max: 3 }).set("a", 1).get("a")); +"#, + ) + .expect("write main.ts"); + assert_eq!( + compile_and_run(root, "main.ts"), + "1\n", + "the native-binding path (no compilePackages) must be byte-for-byte unchanged" + ); +} From 08325f1e6fdefcbcb2f9d6aef2c48f9cbcc02a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 19 Sep 2026 08:43:56 +0200 Subject: [PATCH 2/3] changelog: add fragment for #10699 (native-binding import provenance) --- .../10699-native-binding-import-provenance.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog.d/10699-native-binding-import-provenance.md diff --git a/changelog.d/10699-native-binding-import-provenance.md b/changelog.d/10699-native-binding-import-provenance.md new file mode 100644 index 0000000000..e8acb86373 --- /dev/null +++ b/changelog.d/10699-native-binding-import-provenance.md @@ -0,0 +1,12 @@ +**A `perry.compilePackages` copy of commander, lru-cache, or decimal.js is no +longer overridden by their bundled native bindings.** `new Command()`, +`new LRUCache()`, and `new Decimal()` chained directly onto a method call +(`new Command().name(...)`, `new LRUCache(...).set(...)`, +`new Decimal(...).dividedBy(...)`) matched those class names unconditionally +and routed straight to the native handle, even when the user asked for the +real package to be compiled from source — the only way to opt out was to +rename the import. Construction and method dispatch now resolve through the +same compilePackages-aware provenance table `is_native_module` already +consults, so a compiled copy of the real package runs its own code at its +documented import name. The (unmodified) native binding still installs when +the package is not opted into `compilePackages`. Fixes #10439. From 087fc89d95bb5d60c5c5fde03935158aef408ed4 Mon Sep 17 00:00:00 2001 From: Perry Bot Date: Sat, 19 Sep 2026 09:22:10 +0000 Subject: [PATCH 3/3] fix(test): remove stale ambient-dispatch assertion in fluent_chain_lowering native_fluent_chain_still_dispatches_through_native_methods asserted the pre-fix, spelling-based, no-import native dispatch that this PR's own detect_native_instance_expr change deliberately eliminates. With no import at all, `new Decimal(1)` (or Command/LRUCache/Big/BigNumber) now correctly falls through to an unresolved-global reference -- matching Node's ReferenceError on a genuinely undefined global -- instead of silently reaching the native handle by name. The test predates this change and was never updated for it, so it went red on this same commit without this PR's diff touching that file: only the sweep's `cargo test --workspace` would have caught it, hours later and attributed to a time window rather than this PR. Removed with the rationale recorded inline, matching the identical resolution three PRs stacked on this branch (#10704, #10708, #10712) each carried independently -- landing it here so none of them has to repeat it. crates/perry-hir/tests/fluent_chain_lowering.rs now runs 2/2; the crate's full test suite (`cargo test -p perry-hir --tests`) is green. --- .../10699-native-binding-import-provenance.md | 11 +++++ .../perry-hir/tests/fluent_chain_lowering.rs | 40 +++++++++---------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/changelog.d/10699-native-binding-import-provenance.md b/changelog.d/10699-native-binding-import-provenance.md index e8acb86373..59a9a158fe 100644 --- a/changelog.d/10699-native-binding-import-provenance.md +++ b/changelog.d/10699-native-binding-import-provenance.md @@ -10,3 +10,14 @@ same compilePackages-aware provenance table `is_native_module` already consults, so a compiled copy of the real package runs its own code at its documented import name. The (unmodified) native binding still installs when the package is not opted into `compilePackages`. Fixes #10439. + +`crates/perry-hir/tests/fluent_chain_lowering.rs`'s +`native_fluent_chain_still_dispatches_through_native_methods` asserted the +pre-fix, ambient/no-import, spelling-based dispatch this change deliberately +tightens (a bare `new Decimal(1)` with no import now correctly falls through +to an unresolved-global reference, matching Node's `ReferenceError`, instead +of silently reaching the native handle). That test predates this change and +was never updated for it, so it went red on this same commit without this +diff touching its file — caught by the sweep's `cargo test --workspace`, +not by any diff-scoped gate. Removed here, with the rationale recorded +inline, rather than left for a descendant PR to patch around a third time. diff --git a/crates/perry-hir/tests/fluent_chain_lowering.rs b/crates/perry-hir/tests/fluent_chain_lowering.rs index cd3d9923c4..facc07e529 100644 --- a/crates/perry-hir/tests/fluent_chain_lowering.rs +++ b/crates/perry-hir/tests/fluent_chain_lowering.rs @@ -107,23 +107,23 @@ fn uppercase_imported_builder_chain_stays_generic() { ); } -#[test] -fn native_fluent_chain_still_dispatches_through_native_methods() { - let module = lower_result( - r#" - export const out = new Decimal(1).plus(2).times(3).toString(); - "#, - ) - .expect("native fluent chain should lower"); - let debug = format!("{module:#?}"); - assert!( - debug.contains("module: \"decimal.js\""), - "Decimal chain should dispatch through decimal.js native methods: {debug}" - ); - for method in ["plus", "times", "toString"] { - assert!( - debug.contains(&format!("method: \"{method}\"")), - "Decimal chain should preserve native method {method}: {debug}" - ); - } -} +// `native_fluent_chain_still_dispatches_through_native_methods` removed here +// (was `new Decimal(1).plus(2).times(3).toString()`, no import). +// +// It asserted ambient/no-import, spelling-based native dispatch: +// `detect_native_instance_expr` used to match a bare `Decimal`/`Big`/ +// `BigNumber`/`LRUCache`/`Command` identifier by spelling alone, with no +// import required. This commit tightens that (the #10439 fix this PR makes) +// to require `ctx.lookup_native_module(class_name)` to actually resolve to +// the expected module -- deciding by what the identifier resolves to, not +// by its bare spelling. This test was never updated for that change and +// went red on this same commit; verified against this commit's parent, +// where it still passes (with no import, `new Decimal(1)` on that side +// resolves an unknown ambient identifier by name rather than raising). +// +// With no import, `new Decimal(1)` (or `Command`/`LRUCache`/...) now lowers +// to an unresolved-global reference instead -- correct, Node-matching +// behavior (a real ReferenceError on a genuinely undefined global), not a +// regression. Deleted rather than re-pointed at a still-present native name +// because none of them retain this ambient no-import dispatch any more; +// asserting it would assert the same already-fixed bug.