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)); +}