From 958a668bf0977d42bfd43047b663f343cd13c3ff Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Wed, 16 Sep 2026 08:02:26 +0000 Subject: [PATCH] fix(compile): an un-imported export must not shadow a global intrinsic #10356. When a module imports anything from another native-compiled module, run_pipeline registers every exported class of that module for dispatch -- deliberately, "even when the class name wasn't in the specifier list". The comment argues this is safe because a same-named LOCAL class wins in compile_module. That holds for local classes, but a global intrinsic is not a local class, so nothing outranked the implicit entry. So a module exporting `class Request` made an unrelated `new Request(url, init)` in ANY importer construct that class instead of the global fetch Request -- `.headers` came back undefined. Generated SDKs exporting Request/Response/ Headers are common (hey-api, openapi-typescript, oazapfts); this is OpenCode's TUI bootstrap wall, where packages/sdk/js/src/v2/client.ts imports only OpencodeClient from a gen/sdk.gen.ts that also exports `class Request`. Skip builtin global names in that implicit loop only. An explicit `import { Request } from "./mod.js"` is pushed by the specifier-driven sites above and already wins the name dedup, so it is unaffected -- covered by cell 11 of the test. --- crates/perry-hir/src/analysis.rs | 13 +- .../src/commands/compile/run_pipeline.rs | 15 ++ ..._10356_unimported_export_shadows_global.rs | 173 ++++++++++++++++++ 3 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 crates/perry/tests/issue_10356_unimported_export_shadows_global.rs diff --git a/crates/perry-hir/src/analysis.rs b/crates/perry-hir/src/analysis.rs index 49a6ffa300..0a9c7df79e 100644 --- a/crates/perry-hir/src/analysis.rs +++ b/crates/perry-hir/src/analysis.rs @@ -11,8 +11,19 @@ use crate::walker::{walk_expr_children, walk_expr_children_mut}; mod builtins; pub(crate) use builtins::{ builtin_constructor_length, builtin_global_function_length, builtin_static_function_length, - is_builtin_function, is_builtin_global_value_name, is_builtin_static_function_member, + is_builtin_function, is_builtin_static_function_member, }; +pub(crate) use builtins::is_builtin_global_value_name; + +/// Whether `name` is one of the global constructors / namespaces the runtime +/// installs on `globalThis` (`Request`, `Response`, `Headers`, `URL`, `Map`, +/// …). Public so the compile pipeline can tell a global apart from a +/// user-defined class of the same name — see #10356: implicitly registering +/// an *un-imported* exported class under a global's name shadows the global +/// for every `new` in the importing module. +pub fn is_global_intrinsic_value_name(name: &str) -> bool { + is_builtin_global_value_name(name) +} mod uses_this; pub(crate) use uses_this::{closure_uses_new_target, closure_uses_this, uses_this_stmt}; diff --git a/crates/perry/src/commands/compile/run_pipeline.rs b/crates/perry/src/commands/compile/run_pipeline.rs index ebc7fd963d..16e2fb4457 100644 --- a/crates/perry/src/commands/compile/run_pipeline.rs +++ b/crates/perry/src/commands/compile/run_pipeline.rs @@ -4806,6 +4806,21 @@ pub fn run_with_parse_cache( if !class.is_exported { continue; } + // #10356: this loop registers classes the importing module + // never named in its specifier list. The comment above + // argues that is safe because a same-named LOCAL class + // wins in `compile_module` — true, but a global intrinsic + // is not a local class, so nothing outranks the implicit + // entry and `new Request(...)` in the importer builds the + // exporter's `class Request` instead of the global one. + // (A generated SDK exporting `Request`/`Response`/`Headers` + // is common: hey-api, openapi-typescript, oazapfts.) + // An EXPLICIT `import { Request } from "./mod.js"` is + // pushed by the specifier-driven sites above and already + // wins the name dedup below, so it is unaffected. + if perry_hir::analysis::is_global_intrinsic_value_name(&class.name) { + continue; + } // Dedup across multiple import statements: the same class // may be transitively reachable from several imports, and // the same-class-twice case would produce duplicate diff --git a/crates/perry/tests/issue_10356_unimported_export_shadows_global.rs b/crates/perry/tests/issue_10356_unimported_export_shadows_global.rs new file mode 100644 index 0000000000..6764572926 --- /dev/null +++ b/crates/perry/tests/issue_10356_unimported_export_shadows_global.rs @@ -0,0 +1,173 @@ +//! Regression test for #10356: importing ONE name from a module also bound +//! that module's OTHER exported classes in the importer, so a user-defined +//! `class Request` shadowed the global fetch `Request`. +//! +//! `run_pipeline.rs` registers every exported class of every module an +//! importer touches, deliberately, "even when the class name wasn't in the +//! specifier list" — the stated safety argument being that a same-named LOCAL +//! class wins in `compile_module`. That argument holds for local classes, but +//! a global intrinsic is not a local class, so nothing outranked the implicit +//! entry: `new Request(url, init)` in the importer built the *exporter's* +//! class and `.headers` came back `undefined`. +//! +//! This was OpenCode's TUI bootstrap wall. `packages/sdk/js/src/v2/client.ts` +//! imports only `OpencodeClient` from `gen/sdk.gen.ts`, which also happens to +//! export `class Request extends HeyApiClient`; `rewrite()`'s +//! `new Request(url, request).headers.delete(...)` then threw +//! "Cannot read properties of undefined (reading 'delete')". Generated SDKs +//! exporting `Request`/`Response`/`Headers` are common (hey-api, +//! openapi-typescript, oazapfts). +//! +//! Per ESM a named import binds exactly the names it lists, so `Request` in +//! the importer is the global. bun, node and tsc all agree. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn runtime_dir() -> PathBuf { + std::env::var_os("PERRY_RUNTIME_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| { + perry_bin() + .parent() + .expect("compiler directory") + .to_path_buf() + }) +} + +/// Mirrors `packages/sdk/js/src/v2/gen/sdk.gen.ts`: a module that exports a +/// class named `Request` alongside the one class the importer actually wants. +const MOD_SOURCE: &str = r#" +export class HeyApiClient { + client: any + constructor(config: any) { this.client = config?.client ?? null } +} +export class Request extends HeyApiClient { + readonly kind = "user-sdk-request" +} +export class Response { + readonly kind = "user-sdk-response" +} +export class OpencodeClient { + readonly name = "OpencodeClient" +} +"#; + +/// A second module whose `Request` IS explicitly imported — the control. The +/// fix must not disturb a real named import of a global-shadowing class. +const EXPLICIT_SOURCE: &str = r#" +export class Request { + readonly kind = "explicitly-imported" + constructor(_a?: any, _b?: any) {} +} +"#; + +/// `main.ts` imports only `OpencodeClient`, so every `Request` below is the +/// global one. `shadow.ts` is where an explicit import is exercised. +const MAIN_SOURCE: &str = r#" +import { OpencodeClient } from "./mod.js" +import { explicitKind } from "./shadow.js" + +const c = new OpencodeClient() +console.log("1 import-works:", c.name) + +const url = new URL("http://example.com/x?a=1") +const base = new Request(url, { method: "GET" }) +console.log("2 base.method:", base.method) +console.log("3 base.url:", base.url) +console.log("4 typeof base.headers:", typeof base.headers) + +// The exact OpenCode shape: re-wrap an existing Request, then touch .headers. +const next = new Request(url, base) +console.log("5 next.method:", next.method) +console.log("6 typeof next.headers:", typeof next.headers) +try { + next.headers.delete("x-opencode-directory") + console.log("7 headers.delete:", "ok") +} catch (e: any) { + console.log("7 headers.delete:", "THREW " + e.message) +} +// The leak is visible as a field from a class that was never imported. +console.log("8 kind-leak:", (base as any).kind) + +// `Response` is exported by mod.ts too and is likewise never imported. +console.log("9 response-status:", new Response("hi", { status: 201 }).status) +console.log("10 response-leak:", (new Response("hi") as any).kind) + +// Control: an EXPLICITLY imported class of the same name must still win. +console.log("11 explicit-import:", explicitKind) +"#; + +/// The explicit-import control lives in its own module so `main.ts` keeps the +/// global binding under test unpolluted. +const SHADOW_SOURCE: &str = r#" +import { Request } from "./explicit.js" +export const explicitKind = new Request("http://example.com/", {}).kind +"#; + +/// Byte-for-byte what bun 1.3.14 prints. +const EXPECTED: &str = "\ +1 import-works: OpencodeClient +2 base.method: GET +3 base.url: http://example.com/x?a=1 +4 typeof base.headers: object +5 next.method: GET +6 typeof next.headers: object +7 headers.delete: ok +8 kind-leak: undefined +9 response-status: 201 +10 response-leak: undefined +11 explicit-import: explicitly-imported +"; + +#[test] +fn unimported_export_does_not_shadow_a_global_intrinsic() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::write(root.join("mod.ts"), MOD_SOURCE).unwrap(); + std::fs::write(root.join("explicit.ts"), EXPLICIT_SOURCE).unwrap(); + std::fs::write(root.join("shadow.ts"), SHADOW_SOURCE).unwrap(); + std::fs::write(root.join("main.ts"), MAIN_SOURCE).unwrap(); + + let output = root.join("main_bin"); + let out = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(root.join("main.ts")) + .arg("-o") + .arg(&output) + .arg("--no-cache") + .env("PERRY_NO_AUTO_OPTIMIZE", "1") + .env("PERRY_RUNTIME_DIR", runtime_dir()) + .output() + .expect("run perry compile"); + assert!( + out.status.success(), + "shadowing probe must compile; stdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + + let run = Command::new(&output).output().expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary must run; stdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + let stdout = String::from_utf8(run.stdout).expect("UTF-8 stdout"); + assert!( + !stdout.contains("user-sdk-"), + "no field of an un-imported class may appear on a global-intrinsic \ + instance; stdout:\n{stdout}" + ); + assert_eq!( + stdout, EXPECTED, + "a named import binds exactly the names it lists — the module's other \ + exports must not shadow globals in the importer" + ); +}