feat(resolve): full Node.js '#' subpath-imports (package.json "imports") support

changelog.d/7028-subpath-imports.md

@@ -0,0 +1 @@
+**Full Node.js `#` subpath-imports support (package.json `"imports"`):** the resolver's `#`-specifier handling (from #5039, built for chalk's vendored deps) covered only the happy path; the new `resolve/subpath_imports.rs` implements the spec's PACKAGE_IMPORTS_RESOLVE: package scope discovery walking up to the nearest `package.json` with an `imports` object (stopping at `node_modules` boundaries), exact keys, `*` wildcard patterns with Node's best-match rule (longest prefix, `patternKeyCompare` tie-break), string / fallback-array / conditional-object targets (conditions matched in the same `perry`/`node`/`import`/`module`/`default`/`require` priority order as the `exports` resolver, `node` above `default`), bare-package targets re-entering node_modules resolution (`node:` builtins included), and spec-mandated rejections — the invalid specifiers `#`, `#/…`, and trailing `/`, plus targets or wildcard captures traversing `..`/`node_modules` or escaping the package directory, each with a descriptive error. Targets keep perry's TS-first extension probing, so `"#lib/*": "./src/lib/*"` resolves `#lib/foo` to `src/lib/foo.ts`. Runs before the tsconfig-`paths` fallback (spec resolution outranks aliasing) but falls through to it when no `imports` map governs the importer. `check --check-deps` now resolves `#` imports the same way instead of asking node_modules for a package literally named `#lib/…` — killing the false R003 "Package '#lib/x' not found in node_modules" errors real projects (e.g. sfw) hit.

crates/perry/src/commands/compile.rs

@@ -41,7 +41,9 @@ mod parse_cache;
 mod post_link;
 mod precompile_capture;
 mod reachability;
-mod resolve;
+// pub(crate): commands/deps.rs (the `check --check-deps` dependency checker)
+// reuses the subpath-imports + tsconfig-paths resolvers for `#` specifiers.
+pub(crate) mod resolve;
 mod resources;
 mod sandbox_buildrs;
 mod strip_dedup;

crates/perry/src/commands/compile/optimized_libs/freshness.rs

@@ -297,26 +297,34 @@ pub(crate) fn auto_optimized_cross_features(
 
 /// Feature names a workspace crate's `Cargo.toml` can satisfy in a
 /// `--features <crate>/<name>` request: the `[features]` table keys plus every
-/// optional dependency (an optional dep implicitly defines a same-named
-/// feature unless all its `dep:` references say otherwise — over-including
-/// those keeps this fail-open). `None` when the manifest is missing or
+/// optional dependency whose implicit same-named feature has not been hidden
+/// by a `dep:<name>` reference. `None` when the manifest is missing or
 /// unparseable, so callers skip filtering rather than dropping features a
 /// manifest they couldn't read might well declare.
 fn declared_feature_names(workspace_root: &Path, krate: &str) -> Option<BTreeSet<String>> {
     let manifest_path = workspace_root.join("crates").join(krate).join("Cargo.toml");
     let manifest: toml::Value = toml::from_str(&fs::read_to_string(manifest_path).ok()?).ok()?;
-    let mut names: BTreeSet<String> = manifest
-        .get("features")?
-        .as_table()?
-        .keys()
-        .cloned()
+    let feature_table = manifest.get("features").and_then(|value| value.as_table());
+    let mut names: BTreeSet<String> = feature_table
+        .into_iter()
+        .flat_map(|table| table.keys().cloned())
+        .collect();
+    let hidden_implicit_features: BTreeSet<&str> = feature_table
+        .into_iter()
+        .flat_map(|table| table.values())
+        .filter_map(|value| value.as_array())
+        .flatten()
+        .filter_map(|value| value.as_str())
+        .filter_map(|feature| feature.strip_prefix("dep:"))
         .collect();
     let mut collect_optional = |deps: Option<&toml::Value>| {
         let Some(table) = deps.and_then(|d| d.as_table()) else {
             return;
         };
         for (name, spec) in table {
-            if spec.get("optional").and_then(|o| o.as_bool()) == Some(true) {
+            if spec.get("optional").and_then(|o| o.as_bool()) == Some(true)
+                && !hidden_implicit_features.contains(name.as_str())
+            {
                 names.insert(name.clone());
             }
         }

crates/perry/src/commands/compile/optimized_libs/tests.rs

@@ -673,7 +673,7 @@ fn retain_workspace_declared_features_drops_unknown_names() {
     let dir = tempfile::tempdir().expect("tempdir");
     write_file(
         &dir.path().join("crates/perry-runtime/Cargo.toml"),
-        b"[package]\nname = \"perry-runtime\"\n\n[features]\nfull = []\nregex-engine = []\n\n[dependencies]\nmimalloc = { version = \"0.1\", optional = true }\n",
+        b"[package]\nname = \"perry-runtime\"\n\n[features]\nfull = [\"dep:hidden-allocator\"]\nregex-engine = []\n\n[dependencies]\nmimalloc = { version = \"0.1\", optional = true }\nhidden-allocator = { version = \"0.1\", optional = true }\n",
     );
     write_file(
         &dir.path().join("crates/perry-stdlib/Cargo.toml"),
@@ -684,14 +684,15 @@ fn retain_workspace_declared_features_drops_unknown_names() {
         "perry-runtime/full".to_string(),
         "perry-runtime/alloc-mimalloc".to_string(),
         "perry-runtime/mimalloc".to_string(),
+        "perry-runtime/hidden-allocator".to_string(),
         "perry-stdlib/crypto".to_string(),
         "perry-stdlib/web-fetch".to_string(),
     ];
     let dropped = retain_workspace_declared_features(dir.path(), &mut cross_features);
 
     // `full` and `crypto` are declared features; `mimalloc` is an optional
-    // dep (implicit feature). Only the names the checkout has never heard of
-    // go.
+    // dep with an implicit feature. `hidden-allocator` is referenced through
+    // `dep:`, so Cargo does not expose an implicit same-named feature.
     assert_eq!(
         cross_features,
         vec![
@@ -704,11 +705,32 @@ fn retain_workspace_declared_features_drops_unknown_names() {
         dropped,
         vec![
             "perry-runtime/alloc-mimalloc".to_string(),
+            "perry-runtime/hidden-allocator".to_string(),
             "perry-stdlib/web-fetch".to_string(),
         ]
     );
 }
 
+/// A manifest without an explicit `[features]` table still declares implicit
+/// features for optional dependencies, but must reject every other stale name.
+#[test]
+fn retain_workspace_declared_features_handles_missing_feature_table() {
+    let dir = tempfile::tempdir().expect("tempdir");
+    write_file(
+        &dir.path().join("crates/perry-runtime/Cargo.toml"),
+        b"[package]\nname = \"perry-runtime\"\n\n[dependencies]\nmimalloc = { version = \"0.1\", optional = true }\n",
+    );
+
+    let mut cross_features = vec![
+        "perry-runtime/mimalloc".to_string(),
+        "perry-runtime/stale".to_string(),
+    ];
+    let dropped = retain_workspace_declared_features(dir.path(), &mut cross_features);
+
+    assert_eq!(cross_features, vec!["perry-runtime/mimalloc".to_string()]);
+    assert_eq!(dropped, vec!["perry-runtime/stale".to_string()]);
+}
+
 /// Fail-open: with no readable manifest (release tarball, partial checkout)
 /// there is nothing trustworthy to filter against — every requested feature
 /// must survive.

crates/perry/src/commands/compile/resolve.rs

@@ -38,7 +38,11 @@ use super::CompilationContext;
 use super::{NativeBackend, NativeLibraryManifest};
 
 mod native_library;
-mod tsconfig_paths;
+// pub(crate): the `check --check-deps` dependency checker (commands/deps.rs)
+// consults both resolvers so `#` subpath imports and tsconfig-aliased
+// specifiers stop reporting false R003 "not found in node_modules" errors.
+pub(crate) mod subpath_imports;
+pub(crate) mod tsconfig_paths;
 pub(crate) use native_library::validate_native_library_manifest_value;
 pub(super) use native_library::{
     ergonomic_export_alias, has_perry_native_library, has_perry_native_module,
@@ -857,40 +861,27 @@ fn resolve_exports_with_conditions(
     }
 }
 
-/// Node subpath imports (#5039): resolve a `#`-prefixed specifier through the
-/// importing package's own `package.json` `"imports"` map
-/// (https://nodejs.org/api/packages.html#imports). chalk 5 loads its vendored
-/// dependencies this way (`import ansiStyles from '#ansi-styles'` →
-/// `./source/vendor/ansi-styles/index.js`), so without this every compiled
-/// chalk style table came up empty. The map shares the `exports` value shape
-/// (string / conditional object / `*` patterns), so the same resolver is
-/// reused — with `node` ranked above `default` so conditional pairs like
-/// chalk's `#supports-color` `{ node, default: browser }` pick the node build
-/// for native compilation. Per Node's package-scope rule, only the NEAREST
-/// `package.json` up from the importer is consulted.
-fn resolve_subpath_import(import_source: &str, importer_path: &Path) -> Option<PathBuf> {
-    let mut dir = importer_path.parent();
-    while let Some(d) = dir {
-        let pkg_json = d.join("package.json");
-        if pkg_json.is_file() {
-            let content = std::fs::read_to_string(&pkg_json).ok()?;
-            let json: serde_json::Value = serde_json::from_str(&content).ok()?;
-            let target = resolve_exports_with_conditions(
-                json.get("imports")?,
-                import_source,
-                &["perry", "node", "import", "module", "default", "require"],
-            )?;
-            let base = d.join(target.trim_start_matches("./"));
-            return resolve_with_extensions(&base)
-                .and_then(|p| p.canonicalize().ok())
-                .or_else(|| base.canonicalize().ok());
-        }
-        dir = d.parent();
-    }
-    None
+/// Resolve exports field from package.json for executable module entries.
+///
+/// `node` is ranked ABOVE `default` (and above `import`/`module`) because perry
+/// compiles a Node target: a package whose `exports` offers a `{ node, default }`
+/// conditional pair ships its full Node API under `node` and a reduced
+/// browser/edge build under `default`. Picking `default` drops Node-only
+/// exports — e.g. `unicorn-magic` exposes `toPath`/`traversePathUp` only in its
+/// `node` entry (`./node.js`); resolving `./default.js` left them undefined, so
+/// `npm-run-path` (→ execa) failed to LINK (`undefined symbol
+/// perry_fn_…unicorn_magic…__toPath`). Matches the `resolve_subpath_import`
+/// condition order (chalk's `#supports-color` `{ node, default }`); the two
+/// resolvers must agree.
+pub(super) fn resolve_exports(exports: &serde_json::Value, subpath: &str) -> Option<String> {
+    resolve_exports_with_conditions(
+        exports,
+        subpath,
+        &["perry", "node", "import", "module", "default", "require"],
+    )
 }
 
-/// Like `resolve_exports_with_conditions`, but returns EVERY condition branch's resolution
+/// Like [`resolve_exports`], but returns EVERY condition branch's resolution
 /// in priority order instead of only the first. Callers that check disk
 /// existence (`resolve_package_entry`) walk the list so a pruned target
 /// (Next.js standalone file tracing) falls through to the next condition.
@@ -1248,13 +1239,48 @@ pub(super) fn resolve_import(
         return None; // Native modules are handled by stdlib, not file imports
     }
 
-    // Node subpath imports (`#…`, #5039) resolve through the importing
-    // package's own `"imports"` map and then classify exactly like a relative
-    // import to the mapped file.
+    // Node subpath imports (`#…`, #5039): resolve through the importing
+    // package's own `package.json` `"imports"` map with full
+    // PACKAGE_IMPORTS_RESOLVE semantics (see subpath_imports.rs), then
+    // classify exactly like a relative import to the mapped file. This runs
+    // BEFORE the tsconfig-paths fallback so spec-defined resolution wins over
+    // tsconfig aliasing; when no `imports` field governs the importer the
+    // specifier falls through, so a tsconfig `paths` alias covering `#…`
+    // keeps working.
     let subpath_import_target = if import_source.starts_with('#') {
-        match resolve_subpath_import(import_source, importer_path) {
-            Some(canonical) => Some(canonical),
-            None => return None,
+        use subpath_imports::SubpathImportOutcome;
+        match subpath_imports::resolve_subpath_import(
+            import_source,
+            importer_path,
+            subpath_imports::DEFAULT_CONDITIONS,
+        ) {
+            Ok(SubpathImportOutcome::File(canonical)) => Some(canonical),
+            // Bare-package target: re-enter resolution with the mapped
+            // specifier, which resolves through node_modules per spec (or the
+            // stdlib for `node:` builtins).
+            Ok(SubpathImportOutcome::External(spec)) => {
+                return resolve_import(
+                    &spec,
+                    importer_path,
+                    project_root,
+                    compile_packages,
+                    compile_package_dirs,
+                );
+            }
+            // Not covered by an `imports` map — fall through (the tsconfig
+            // `paths` fallback below may still alias it).
+            Ok(SubpathImportOutcome::NotDefined) => None,
+            // Spec-defined hard errors (`#` / `#/…`, escaping targets, …)
+            // must be surfaced, not silently degraded to tsconfig aliasing.
+            Err(err) => {
+                eprintln!(
+                    "warning: cannot resolve '{}' (imported from {}): {}",
+                    import_source,
+                    importer_path.display(),
+                    err
+                );
+                return None;
+            }
         }
     } else {
         None