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..59a9a158fe --- /dev/null +++ b/changelog.d/10699-native-binding-import-provenance.md @@ -0,0 +1,23 @@ +**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. + +`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/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-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. 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" + ); +}