From 9206f69f990f46b712c6439034aa692478785428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 1 Aug 2026 17:42:20 +0200 Subject: [PATCH] fix(compile): keep the zero-config default additive and surface version-shadowed resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two post-merge follow-ups on #7137's auto-compile default, found while verifying the shipped behavior against Node 26.5.1 with a fixture matrix. 1. The implicit `"*"` made the #3527 expansion block run unconditionally, so every text-mode compile gained "Compile package wildcard: expanded to 0 installed package(s)" — including programs with no node_modules at all. The default is meant to be additive; that put a new line in the stdout of every existing build. An explicitly-spelled wildcard still always reports (that zero is the answer to the question it asked). 2. For any package in `perry.compilePackages`, `resolve_import` searches the project root BEFORE the importer's own ancestors, and `compile_package_dirs` then keeps one directory per package name. Both were narrow while that set held only hand-listed packages. The auto-compile default puts the whole reachable graph in it, so both now apply to every bare specifier. Verified divergence: `dup-pkg@1.0.0` top-level + `dup-pkg@2.0.0` under `sub/node_modules` gives "A-top-level-v1 / B-nested-v2" under node 26.5.1 and "A-top-level-v1 / A-top-level-v1" under perry — the nested importer silently got the other major. Warn once per package, naming both versions, both paths, and the importer that was redirected. Identical versions and copies with no readable version stay quiet; verified zero false positives across the other eight fixtures. Tests: 6 resolve unit tests (including that the nearest-first dir is what Node would pick, which is what the warning compares against) and 2 host_config tests for the report predicate. --- changelog.d/7195-zero-config-followups.md | 25 ++++ .../perry/src/commands/compile/host_config.rs | 49 +++++++- crates/perry/src/commands/compile/resolve.rs | 91 ++++++++++++++ .../compile/resolve/dedup_version_tests.rs | 118 ++++++++++++++++++ 4 files changed, 281 insertions(+), 2 deletions(-) create mode 100644 changelog.d/7195-zero-config-followups.md create mode 100644 crates/perry/src/commands/compile/resolve/dedup_version_tests.rs diff --git a/changelog.d/7195-zero-config-followups.md b/changelog.d/7195-zero-config-followups.md new file mode 100644 index 0000000000..c6bd45a5b2 --- /dev/null +++ b/changelog.d/7195-zero-config-followups.md @@ -0,0 +1,25 @@ +### Fixed + +- **The auto-compile default no longer adds a line to every build's stdout + (#7137 follow-up).** Injecting the implicit `"*"` made the wildcard + expansion block run on every text-mode compile, so even a program with no + `node_modules` at all printed `Compile package wildcard: expanded to 0 + installed package(s)`. A host that spelled a wildcard out (`["*"]`, + `"auto"`, `"@scope/*"`) still always gets the report; the implicit default + reports only when the expansion actually routed or skipped something. + +- **Perry now warns when zero-config resolution hands an importer a + different package version than Node would.** For any package in + `perry.compilePackages`, `resolve_import` searches the project root before + the importer's own ancestors (so a top-level ESM copy beats a nested CJS + one), and `compile_package_dirs` then keeps one directory per package name. + Both were narrow while the set held only hand-listed packages — opting one + in was a deliberate act. The auto-compile default puts the whole reachable + graph in that set, so both now apply to every bare specifier, and a tree + carrying two majors of one package silently gets one of them. Verified: a + project with `dup-pkg@1.0.0` at the top level and `dup-pkg@2.0.0` under + `sub/node_modules` prints `A-top-level-v1 / B-nested-v2` under Node 26.5.1 + and `A-top-level-v1 / A-top-level-v1` under Perry. Perry now emits one + warning per package naming both versions, both paths, and the importer that + was redirected. Identical versions (a genuine duplicate install) and copies + with no readable `version` stay silent. diff --git a/crates/perry/src/commands/compile/host_config.rs b/crates/perry/src/commands/compile/host_config.rs index 6bcb64a666..725cdea905 100644 --- a/crates/perry/src/commands/compile/host_config.rs +++ b/crates/perry/src/commands/compile/host_config.rs @@ -57,6 +57,20 @@ fn should_auto_grant_compile_allow( has_universal_route && !allow_was_explicit && !env_forces_deny } +/// Whether to print the `"*"`-expansion summary. A host that spelled a +/// wildcard out (`["*"]`, `"auto"`, `"@scope/*"`) always gets the report — it +/// asked. The implicit auto-compile default reports only when the expansion +/// actually did something, so a program with no `node_modules` at all does +/// not gain a line of build output just because the default injects `"*"` +/// internally. +fn should_report_wildcard_expansion( + auto_default: bool, + added: usize, + skipped_native: usize, +) -> bool { + !auto_default || added > 0 || skipped_native > 0 +} + pub(super) fn apply_pkg_and_toml_config( args: &CompileArgs, project_root: &Path, @@ -716,7 +730,8 @@ pub(super) fn apply_pkg_and_toml_config( // packages are skipped, so bundled bindings still win). Opt out with an // explicit list, or `compilePackages: false` / `[]` to compile nothing and // restore the V8-free gate's "listed only" behavior. - if !compile_packages_explicit { + let compile_packages_auto_default = !compile_packages_explicit; + if compile_packages_auto_default { ctx.compile_packages.insert("*".to_string()); } // Universal routing with no explicit allow policy ⇒ universal allow. An @@ -792,7 +807,16 @@ pub(super) fn apply_pkg_and_toml_config( // nonsensical `node_modules/*/` substring). ctx.compile_packages .retain(|p| p != "*" && !p.ends_with("/*")); - if let OutputFormat::Text = format { + // Only report the expansion when the host actually asked for a + // wildcard, or when the implicit auto-compile default did something. + // Otherwise every `perry compile foo.ts` — including programs with no + // `node_modules` at all — would gain a new "expanded to 0 installed + // package(s)" line purely because the default now injects `"*"` + // internally, making an additive default non-additive for the stdout + // of every existing build. + let report_expansion = + should_report_wildcard_expansion(compile_packages_auto_default, added, skipped_native); + if matches!(format, OutputFormat::Text) && report_expansion { println!( " Compile package wildcard: expanded to {} installed package(s)", added @@ -1161,6 +1185,27 @@ mod tests { assert!(!should_auto_grant_compile_allow(true, false, true)); assert!(!should_auto_grant_compile_allow(false, false, false)); } + + /// The auto-compile default injects `"*"` for every project, so the + /// expansion block now runs on builds that never asked for a wildcard. + /// It must stay silent there — otherwise an "additive" default adds a + /// line to the stdout of every existing build (including + /// `perry compile foo.ts` with no `node_modules` at all). + #[test] + fn implicit_default_reports_only_when_it_did_something() { + assert!(!should_report_wildcard_expansion(true, 0, 0)); + assert!(should_report_wildcard_expansion(true, 1, 0)); + assert!(should_report_wildcard_expansion(true, 0, 1)); + } + + /// A host that spelled the wildcard out asked for the report and gets it + /// even when the expansion matched nothing — that zero is the answer to + /// their question. + #[test] + fn explicit_wildcard_always_reports() { + assert!(should_report_wildcard_expansion(false, 0, 0)); + assert!(should_report_wildcard_expansion(false, 3, 2)); + } } fn parse_fp_contract_mode(value: &str, source: &str) -> Result { diff --git a/crates/perry/src/commands/compile/resolve.rs b/crates/perry/src/commands/compile/resolve.rs index dbcdf23be9..46803d2e40 100644 --- a/crates/perry/src/commands/compile/resolve.rs +++ b/crates/perry/src/commands/compile/resolve.rs @@ -131,6 +131,8 @@ mod bun_store_tests; #[cfg(test)] mod declaration_map_source_tests; #[cfg(test)] +mod dedup_version_tests; +#[cfg(test)] mod extension_resolution_tests; #[cfg(test)] mod tests; @@ -190,6 +192,83 @@ pub(super) fn extract_compile_package_dir( .map(Path::to_path_buf) } +/// Read a package directory's declared `version`, if it has a readable +/// `package.json` with a string `version` field. +fn package_json_version(package_dir: &Path) -> Option { + let raw = fs::read_to_string(package_dir.join("package.json")).ok()?; + let value: serde_json::Value = serde_json::from_str(&raw).ok()?; + value + .get("version") + .and_then(|v| v.as_str()) + .map(str::to_string) +} + +/// Whether `chosen` is a *different* copy from `found` with a *different* +/// declared version. Identical versions are a genuine duplicate install and +/// collapsing them is intended; differing versions mean the build silently +/// dropped one of them. +pub(super) fn dedup_collapses_distinct_versions(chosen: &Path, found: &Path) -> bool { + if chosen == found { + return false; + } + match (package_json_version(chosen), package_json_version(found)) { + (Some(a), Some(b)) => a != b, + // A copy with no readable version can't be proven distinct; stay + // quiet rather than warning on every unversioned local link. + _ => false, + } +} + +/// The copy plain Node resolution would have picked for `package_name` as +/// imported from `importer_path`: the nearest ancestor `node_modules` that +/// holds the package. Perry's compile-package path deliberately searches the +/// project root first instead (see `search_paths` in `resolve_import`), so +/// the two can disagree. +pub(super) fn node_nearest_package_dir( + package_name: &str, + importer_path: &Path, +) -> Option { + let start = importer_path.parent().unwrap_or(importer_path); + ancestor_node_modules_dirs(start) + .into_iter() + .map(|node_modules| node_modules.join(package_name)) + .find(|candidate| candidate.is_dir()) +} + +/// Warn, at most once per package, when the compile-package resolution path +/// hands an importer a different *version* than Node would have. Covers both +/// the root-first search order and the `compile_package_dirs` first-found +/// dedup, since `chosen` is the directory actually used. +fn warn_on_version_shadowed_resolution(package_name: &str, chosen: &Path, importer_path: &Path) { + static WARNED: OnceLock>> = OnceLock::new(); + let Some(nearest) = node_nearest_package_dir(package_name, importer_path) else { + return; + }; + if !dedup_collapses_distinct_versions(chosen, &nearest) { + return; + } + let warned = WARNED.get_or_init(|| Mutex::new(HashSet::new())); + let Ok(mut warned) = warned.lock() else { + return; + }; + if !warned.insert(package_name.to_string()) { + return; + } + let chosen_version = package_json_version(chosen).unwrap_or_else(|| "?".to_string()); + let nearest_version = package_json_version(&nearest).unwrap_or_else(|| "?".to_string()); + eprintln!( + " warning: `{package_name}` is installed at two different versions and \ + Perry compiles ONE copy per package name. `{importer}` gets \ + {chosen_version} (from {chosen}); Node would have given it \ + {nearest_version} (from {nearest}). Deduplicate the dependency (npm \ + dedupe / a package override), or list the package explicitly in \ + `perry.compilePackages` only where you want it compiled.", + importer = importer_path.display(), + chosen = chosen.display(), + nearest = nearest.display(), + ); +} + /// Check if a file path is inside a package listed in compile_packages pub(super) fn is_in_compile_package(path: &Path, compile_packages: &HashSet) -> bool { compile_packages.iter().any(|pkg_name| { @@ -1473,6 +1552,18 @@ pub(super) fn resolve_import( let effective_dir = compile_package_dirs .get(&package_name) .unwrap_or(&package_dir); + // #7137 follow-up. Two mechanisms route this import away from + // the copy Node would have used: the root-first `search_paths` + // order just above (chosen for compile packages so a top-level + // ESM copy beats a nested CJS one), and this first-found + // `compile_package_dirs` dedup. Both were narrow while + // `compile_packages` held only hand-listed names — opting a + // package in was a deliberate act. The auto-compile default + // puts the WHOLE reachable graph in that set, so both now apply + // to every bare specifier in the project, and a tree carrying + // two majors of one package silently gets one of them. Report + // it when the versions actually differ. + warn_on_version_shadowed_resolution(&package_name, effective_dir, importer_path); // Prefer TypeScript source over compiled JS if let Some(src_entry) = resolve_package_source_entry(effective_dir, subpath.as_deref()) diff --git a/crates/perry/src/commands/compile/resolve/dedup_version_tests.rs b/crates/perry/src/commands/compile/resolve/dedup_version_tests.rs new file mode 100644 index 0000000000..7b9740b175 --- /dev/null +++ b/crates/perry/src/commands/compile/resolve/dedup_version_tests.rs @@ -0,0 +1,118 @@ +use super::*; + +/// #7137 follow-up. Compile-package dedup keeps one directory per package +/// name and routes every importer to it. That is right for a genuine +/// duplicate install and wrong-but-silent when the two copies are different +/// versions — which auto-compile made reachable for a project's entire +/// dependency graph rather than only for hand-listed packages. +/// +/// `dedup_collapses_distinct_versions` is the predicate behind the warning, +/// so it has to be true exactly in the lossy case. +fn write_pkg(dir: &std::path::Path, version: &str) { + std::fs::create_dir_all(dir).expect("mkdir"); + std::fs::write( + dir.join("package.json"), + format!(r#"{{"name":"dup-pkg","version":"{version}"}}"#), + ) + .expect("write package.json"); +} + +#[test] +fn differing_versions_are_reported_as_collapsing() { + let root = tempfile::tempdir().expect("tempdir"); + let chosen = root.path().join("node_modules/dup-pkg"); + let found = root.path().join("sub/node_modules/dup-pkg"); + write_pkg(&chosen, "1.0.0"); + write_pkg(&found, "2.0.0"); + + assert!( + dedup_collapses_distinct_versions(&chosen, &found), + "1.0.0 substituted for 2.0.0 is a silent loss and must be reported" + ); +} + +#[test] +fn identical_versions_are_a_plain_duplicate_install() { + let root = tempfile::tempdir().expect("tempdir"); + let chosen = root.path().join("node_modules/dup-pkg"); + let found = root.path().join("sub/node_modules/dup-pkg"); + write_pkg(&chosen, "1.0.0"); + write_pkg(&found, "1.0.0"); + + assert!( + !dedup_collapses_distinct_versions(&chosen, &found), + "collapsing two copies of the same version is the intended dedup" + ); +} + +#[test] +fn same_directory_is_never_a_collapse() { + let root = tempfile::tempdir().expect("tempdir"); + let only = root.path().join("node_modules/dup-pkg"); + write_pkg(&only, "1.0.0"); + + assert!( + !dedup_collapses_distinct_versions(&only, &only), + "the first copy resolving to itself is not a substitution" + ); +} + +/// A copy with no readable `package.json` cannot be proven distinct — a +/// local symlinked workspace package often has no version at the resolved +/// path. Warning there would be noise on every such build. +#[test] +fn unreadable_version_stays_quiet() { + let root = tempfile::tempdir().expect("tempdir"); + let chosen = root.path().join("node_modules/dup-pkg"); + let found = root.path().join("linked/dup-pkg"); + write_pkg(&chosen, "1.0.0"); + std::fs::create_dir_all(&found).expect("mkdir"); + + assert!( + !dedup_collapses_distinct_versions(&chosen, &found), + "an unversioned copy must not produce a warning" + ); +} + +/// The shadowing is not (only) the `compile_package_dirs` dedup: for a +/// package in `compile_packages`, `resolve_import` searches the PROJECT ROOT +/// before the importer's own ancestors. So a nested copy is passed over even +/// on its first resolution. `node_nearest_package_dir` is what Node would +/// have picked, and is what the warning compares against. +#[test] +fn nearest_dir_is_the_importers_own_node_modules() { + let root = tempfile::tempdir().expect("tempdir"); + let top = root.path().join("node_modules/dup-pkg"); + let nested = root.path().join("sub/node_modules/dup-pkg"); + write_pkg(&top, "1.0.0"); + write_pkg(&nested, "2.0.0"); + let importer = root.path().join("sub/child.ts"); + std::fs::write(&importer, "export {};\n").expect("write importer"); + + let nearest = node_nearest_package_dir("dup-pkg", &importer).expect("nearest copy found"); + assert_eq!( + nearest, nested, + "Node resolves a bare specifier from the importer's nearest node_modules" + ); + assert!( + dedup_collapses_distinct_versions(&top, &nearest), + "compiling the root 1.0.0 for an importer Node would give 2.0.0 is a \ + silent version substitution" + ); +} + +/// An importer with no nearer copy resolves to the same directory Perry +/// chose — nothing was shadowed, so nothing is reported. +#[test] +fn importer_without_a_nearer_copy_is_not_shadowed() { + let root = tempfile::tempdir().expect("tempdir"); + let top = root.path().join("node_modules/dup-pkg"); + write_pkg(&top, "1.0.0"); + let importer = root.path().join("sub/child.ts"); + std::fs::create_dir_all(importer.parent().unwrap()).expect("mkdir"); + std::fs::write(&importer, "export {};\n").expect("write importer"); + + let nearest = node_nearest_package_dir("dup-pkg", &importer).expect("root copy found"); + assert_eq!(nearest, top); + assert!(!dedup_collapses_distinct_versions(&top, &nearest)); +}