diff --git a/crates/perry/src/commands/compile/collect_modules.rs b/crates/perry/src/commands/compile/collect_modules.rs index 01be91422..ffe6eda0c 100644 --- a/crates/perry/src/commands/compile/collect_modules.rs +++ b/crates/perry/src/commands/compile/collect_modules.rs @@ -18,7 +18,7 @@ use perry_transform::{ }; use std::collections::{HashMap, HashSet}; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use crate::commands::progress::{ProgressSnapshot, VerboseProgress}; use crate::OutputFormat; @@ -125,11 +125,6 @@ pub(super) fn collect_js_module_imports(file_path: &std::path::Path, source: &st specs.push(cap[1].to_string()); } - let parent = match file_path.parent() { - Some(p) => p, - None => return Vec::new(), - }; - let mut out: Vec = Vec::new(); let mut seen: HashSet = HashSet::new(); for spec in specs { @@ -140,25 +135,80 @@ pub(super) fn collect_js_module_imports(file_path: &std::path::Path, source: &st // so the most common case (top-level package brings in submodules) // is covered. Inside a package's `node_modules` tree, all // sibling imports are relative-path anyway. - if !(spec.starts_with("./") || spec.starts_with("../") || spec.starts_with('/')) { + if !(super::resolve::is_relative_specifier(&spec) || spec.starts_with('/')) { continue; } - let candidate = if spec.starts_with('/') { - PathBuf::from(&spec) + let resolved_path = if spec.starts_with('/') { + super::resolve::resolve_absolute_import_paths(&spec) } else { - parent.join(&spec) + super::resolve::resolve_relative_import_paths(&spec, file_path) }; - if let Some(resolved) = super::resolve::resolve_with_extensions(&candidate) { - if let Ok(canon) = resolved.canonicalize() { - if seen.insert(canon.clone()) { - out.push(canon); - } + if let Some(resolved) = resolved_path { + if seen.insert(resolved.canonical_path.clone()) { + out.push(resolved.source_path); } } } out } +struct ResolvedImport { + canonical_path: PathBuf, + source_path: PathBuf, + kind: ModuleKind, +} + +fn cached_resolve_import_with_lexical_base( + import_source: &str, + lexical_importer_path: &Path, + canonical_importer_path: &Path, + ctx: &mut CompilationContext, +) -> Option { + // Module collection keys and reads use canonical paths, but source text + // relative specifiers are written against the importer path the user + // compiled. On platforms where /tmp is a symlink, resolving imports from + // the canonical /private/tmp path can make a valid "../.." edge point at a + // nonexistent sibling and leave imported classes unresolved. + let resolved = cached_resolve_import_from_base(import_source, lexical_importer_path, ctx); + if resolved.is_some() || lexical_importer_path == canonical_importer_path { + return resolved; + } + cached_resolve_import_from_base(import_source, canonical_importer_path, ctx) +} + +fn cached_resolve_import_from_base( + import_source: &str, + importer_path: &Path, + ctx: &mut CompilationContext, +) -> Option { + let (canonical_path, kind) = cached_resolve_import(import_source, importer_path, ctx)?; + let source_path = source_visible_resolved_path(import_source, importer_path, &canonical_path); + Some(ResolvedImport { + canonical_path, + source_path, + kind, + }) +} + +fn source_visible_resolved_path( + import_source: &str, + importer_path: &Path, + canonical_path: &Path, +) -> PathBuf { + let resolved = if import_source.starts_with('/') { + super::resolve::resolve_absolute_import_paths(import_source) + } else if super::resolve::is_relative_specifier(import_source) { + super::resolve::resolve_relative_import_paths(import_source, importer_path) + } else { + None + }; + + resolved + .filter(|path| path.canonical_path == canonical_path) + .map(|path| path.source_path) + .unwrap_or_else(|| canonical_path.to_path_buf()) +} + /// Issue #841: Node.js submodules that Perry knows about at the /// resolver level (no perry-stdlib backing, no compiled-source backing) /// but for which we still want to provide a minimal import surface so @@ -428,7 +478,7 @@ fn collect_module_one( // also walked. Template-literal / variable specifiers can't be // resolved statically and are skipped (V8 will surface the // resolution failure at runtime, same as today). - let transitive_paths = collect_js_module_imports(&canonical, &source); + let transitive_paths = collect_js_module_imports(entry_path, &source); ctx.js_modules.insert( specifier.clone(), JsModule { @@ -1118,8 +1168,12 @@ fn collect_module_one( continue; } - if let Some((resolved_path, kind)) = cached_resolve_import(&import.source, &canonical, ctx) + if let Some(resolved) = + cached_resolve_import_with_lexical_base(&import.source, entry_path, &canonical, ctx) { + let resolved_path = resolved.canonical_path; + let source_path = resolved.source_path; + let kind = resolved.kind; import.resolved_path = Some(resolved_path.to_string_lossy().to_string()); import.module_kind = kind; if let Some(sidecar) = @@ -1241,7 +1295,7 @@ fn collect_module_one( pkg_dir = dir.parent(); } } - pending.push(resolved_path); + pending.push(source_path); } ModuleKind::Interpreted => { // Perry native extension packages (ioredis, ethers, ws, mysql2, dotenv) @@ -1354,7 +1408,7 @@ fn collect_module_one( OutputFormat::Json => {} } - pending.push(resolved_path); + pending.push(source_path); } ModuleKind::NativeRust => { // Native Rust modules are handled by stdlib @@ -1466,9 +1520,12 @@ fn collect_module_one( collected: Some(ctx.native_modules.len() + ctx.js_modules.len()), ..Default::default() }); - if let Some((resolved_path, kind)) = - cached_resolve_import(src.as_str(), &canonical, ctx) + if let Some(resolved) = + cached_resolve_import_with_lexical_base(src.as_str(), entry_path, &canonical, ctx) { + let resolved_path = resolved.canonical_path; + let source_path = resolved.source_path; + let kind = resolved.kind; if let Some(sidecar) = declaration_sidecar_for_resolved_import(src.as_str(), &resolved_path) { @@ -1557,7 +1614,7 @@ fn collect_module_one( } match kind { - ModuleKind::NativeCompiled => pending.push(resolved_path), + ModuleKind::NativeCompiled => pending.push(source_path), ModuleKind::Interpreted => { // JS runtime (V8) support was removed, so interpreted // node_modules dependencies are not followed. A direct diff --git a/crates/perry/src/commands/compile/collect_modules/tests.rs b/crates/perry/src/commands/compile/collect_modules/tests.rs index 007225bab..6c5da96cf 100644 --- a/crates/perry/src/commands/compile/collect_modules/tests.rs +++ b/crates/perry/src/commands/compile/collect_modules/tests.rs @@ -2,8 +2,8 @@ //! Split out of `collect_modules.rs` to keep that file under the file-size gate. use super::{ - collect_modules, env_defines_for_lowering, expand_dynamic_import_glob, - refuse_compile_package_native_addon, + collect_js_module_imports, collect_modules, env_defines_for_lowering, + expand_dynamic_import_glob, refuse_compile_package_native_addon, }; use crate::commands::compile::{CompilationContext, DefineValue}; use crate::commands::progress::VerboseProgress; @@ -137,6 +137,134 @@ console.log(got); ); } +#[cfg(unix)] +#[test] +fn symlinked_entry_resolves_relative_imports_from_lexical_path() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let real_parent = root.join("real-parent"); + let alias_parent = root.join("alias-parent"); + let real_app = real_parent.join("app"); + let alias_app = alias_parent.join("app"); + let alias_outside = alias_parent.join("outside"); + let real_outside = real_parent.join("outside"); + + std::fs::create_dir_all(&real_app).expect("mkdir real app"); + std::fs::create_dir_all(&alias_outside).expect("mkdir alias outside"); + std::fs::create_dir_all(&real_outside).expect("mkdir real outside"); + std::os::unix::fs::symlink(&real_app, &alias_app).expect("symlink app"); + + let dep = alias_outside.join("dep.ts"); + let decoy_dep = real_outside.join("dep.ts"); + let child = alias_app.join("child.ts"); + let entry = alias_app.join("entry.ts"); + + std::fs::write( + &dep, + r#" +export class ExternalCtor { + value: string; + constructor(value: string) { + this.value = value; + } + marker(): string { + return this.value; + } +} +"#, + ) + .expect("write dep"); + std::fs::write( + &decoy_dep, + r#" +export class ExternalCtor { + value: string; + constructor(value: string) { + this.value = "decoy:" + value; + } + marker(): string { + return this.value; + } +} +"#, + ) + .expect("write decoy dep"); + std::fs::write( + &child, + r#" +import { ExternalCtor } from "../outside/dep"; + +export function makeValue(): string { + const value = new ExternalCtor("ready"); + return value.marker(); +} +"#, + ) + .expect("write child"); + std::fs::write( + &entry, + r#" +import { makeValue } from "./child"; + +console.log(makeValue()); +"#, + ) + .expect("write entry"); + + let mut ctx = CompilationContext::new(alias_parent.to_path_buf()); + ctx.entry_canonical = Some(entry.canonicalize().unwrap()); + let mut visited = HashSet::new(); + let mut next_class_id: perry_hir::ClassId = 1; + let progress = VerboseProgress::new(OutputFormat::Text, 0); + + collect_modules( + &entry, + &mut ctx, + &mut visited, + OutputFormat::Text, + None, + &mut next_class_id, + false, + &progress, + None, + ) + .expect("collect modules"); + + let dep_canonical = dep.canonicalize().expect("canonical dep"); + let decoy_canonical = decoy_dep.canonicalize().expect("canonical decoy dep"); + assert!( + ctx.native_modules.contains_key(&dep_canonical), + "relative imports from a symlinked entry must resolve from the lexical path; collected modules: {:?}", + ctx.native_modules.keys().collect::>() + ); + assert!( + !ctx.native_modules.contains_key(&decoy_canonical), + "source-visible lexical imports must win over the canonical sibling decoy; collected modules: {:?}", + ctx.native_modules.keys().collect::>() + ); +} + +#[test] +fn js_import_scan_follows_bare_dot_and_dotdot_specifiers() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let app = root.join("app"); + std::fs::create_dir_all(&app).expect("mkdir app"); + std::fs::write(root.join("index.js"), "export const parent = 1;\n").expect("write parent"); + std::fs::write(app.join("index.js"), "export const current = 1;\n").expect("write current"); + let child = app.join("child.js"); + std::fs::write(&child, "import '.';\nexport * from '..';\n").expect("write child"); + + let imports = collect_js_module_imports(&child, "import '.';\nexport * from '..';\n"); + let collected = imports + .into_iter() + .map(|path| path.canonicalize().expect("canonical import")) + .collect::>(); + + assert!(collected.contains(&app.join("index.js").canonicalize().unwrap())); + assert!(collected.contains(&root.join("index.js").canonicalize().unwrap())); +} + fn write_compile_package_fixture( root: &std::path::Path, package_name: &str, diff --git a/crates/perry/src/commands/compile/resolve.rs b/crates/perry/src/commands/compile/resolve.rs index 04fad398b..c0d03fa17 100644 --- a/crates/perry/src/commands/compile/resolve.rs +++ b/crates/perry/src/commands/compile/resolve.rs @@ -31,7 +31,7 @@ use anyhow::{anyhow, Result}; use perry_hir::ModuleKind; use std::collections::{HashMap, HashSet}; use std::fs; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use super::CompilationContext; #[cfg(test)] @@ -894,13 +894,76 @@ pub(super) fn resolve_relative_import_path( import_source: &str, importer_path: &Path, ) -> Option { + resolve_relative_import_paths(import_source, importer_path) + .map(|resolved| resolved.canonical_path) +} + +pub(super) struct ResolvedPath { + pub source_path: PathBuf, + pub canonical_path: PathBuf, +} + +pub(super) fn resolve_relative_import_paths( + import_source: &str, + importer_path: &Path, +) -> Option { if !is_relative_specifier(import_source) { return None; } let parent = importer_path.parent()?; let resolved = parent.join(import_source); - let path = resolve_with_extensions(&resolved)?; - path.canonicalize().ok() + // Source import specifiers are resolved against the path as written by the + // program. If that path contains a symlinked component such as /tmp, asking + // the filesystem about "a/../b" can follow the symlink before applying ".." + // and accidentally probe the canonical sibling. + let lexical = normalize_path_lexically(&resolved); + let source_path = resolve_with_extensions(&lexical).or_else(|| { + if lexical == resolved { + None + } else { + resolve_with_extensions(&resolved) + } + })?; + let canonical_path = source_path.canonicalize().ok()?; + Some(ResolvedPath { + source_path, + canonical_path, + }) +} + +pub(super) fn resolve_absolute_import_paths(import_source: &str) -> Option { + if !import_source.starts_with('/') { + return None; + } + let source_path = resolve_with_extensions(&PathBuf::from(import_source))?; + let canonical_path = source_path.canonicalize().ok()?; + Some(ResolvedPath { + source_path, + canonical_path, + }) +} + +fn normalize_path_lexically(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + if matches!( + normalized.components().next_back(), + Some(Component::Normal(_)) + ) { + normalized.pop(); + } else { + normalized.push(component.as_os_str()); + } + } + Component::Prefix(_) | Component::RootDir | Component::Normal(_) => { + normalized.push(component.as_os_str()); + } + } + } + normalized } /// True for ECMAScript relative-import specifiers. Besides the obvious `./x` diff --git a/crates/perry/src/commands/compile/resolve/tests.rs b/crates/perry/src/commands/compile/resolve/tests.rs index 1bc0faa42..8e8bef1e8 100644 --- a/crates/perry/src/commands/compile/resolve/tests.rs +++ b/crates/perry/src/commands/compile/resolve/tests.rs @@ -1363,6 +1363,27 @@ mod manifest_parse_tests { } } +#[cfg(test)] +mod lexical_path_tests { + use super::*; + + #[test] + fn lexical_normalization_preserves_leading_parent_segments() { + assert_eq!( + normalize_path_lexically(std::path::Path::new("../../dep")), + std::path::PathBuf::from("../../dep") + ); + } + + #[test] + fn lexical_normalization_pops_only_normal_segments() { + assert_eq!( + normalize_path_lexically(std::path::Path::new("app/../../dep")), + std::path::PathBuf::from("../dep") + ); + } +} + #[cfg(test)] mod module_spec_tests { use super::split_module_spec; diff --git a/tests/test_symlinked_entry_imported_class.sh b/tests/test_symlinked_entry_imported_class.sh new file mode 100755 index 000000000..a46de931e --- /dev/null +++ b/tests/test_symlinked_entry_imported_class.sh @@ -0,0 +1,100 @@ +#!/bin/bash +# Regression: resolving static imports from a symlinked entry path must use the +# source-visible importer path before the canonical path. Otherwise a valid +# relative import can be missed, and `new ImportedClass()` falls back to an +# empty class-id-0 placeholder with no fields, methods, or instanceof identity. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +PERRY="${PERRY_BIN:-${PERRY:-$REPO_ROOT/target/release/perry}}" +if [[ ! -x "$PERRY" ]]; then + PERRY="$REPO_ROOT/target/debug/perry" +fi +if [[ ! -x "$PERRY" ]]; then + echo "SKIP: perry binary not found (build with cargo build -p perry)" + exit 0 +fi +if [[ "$PERRY" != /* ]]; then + PERRY="$(cd "$(dirname "$PERRY")" && pwd)/$(basename "$PERRY")" +fi + +TMPDIR="$(mktemp -d)" +trap 'rm -rf "$TMPDIR"' EXIT + +mkdir -p "$TMPDIR/real-parent/app" "$TMPDIR/alias-parent/outside" "$TMPDIR/real-parent/outside" +ln -s "$TMPDIR/real-parent/app" "$TMPDIR/alias-parent/app" + +cat > "$TMPDIR/alias-parent/outside/dep.ts" <<'TS' +export class ExternalCtor { + value: string; + + constructor(value: string) { + this.value = value; + } + + marker(): string { + return this.value; + } +} +TS + +cat > "$TMPDIR/real-parent/outside/dep.ts" <<'TS' +export class ExternalCtor { + value: string; + + constructor(value: string) { + this.value = "decoy:" + value; + } + + marker(): string { + return this.value; + } +} +TS + +cat > "$TMPDIR/alias-parent/app/child.ts" <<'TS' +import { ExternalCtor } from "../outside/dep"; + +export function makeValue(): any { + return new ExternalCtor("ready"); +} +TS + +cat > "$TMPDIR/alias-parent/app/main.ts" <<'TS' +import { ExternalCtor } from "../outside/dep"; +import { makeValue } from "./child"; + +const value: any = makeValue(); +console.log("field", value.value); +console.log("method", typeof value.marker); +console.log("call", value.marker()); +console.log("instanceof", value instanceof ExternalCtor); +TS + +BIN="$TMPDIR/test_bin" +"$PERRY" compile --no-cache --no-auto-optimize "$TMPDIR/alias-parent/app/main.ts" --output "$BIN" >/dev/null +set +e +RUN_OUTPUT="$("$BIN" 2>&1)" +RUN_STATUS=$? +set -e + +EXPECTED="field ready +method function +call ready +instanceof true" + +if [[ "$RUN_STATUS" -eq 0 && "$RUN_OUTPUT" == "$EXPECTED" ]]; then + echo "PASS" + exit 0 +fi + +echo "FAIL: symlinked entry relative import did not preserve imported class behavior" +echo "Exit status: $RUN_STATUS" +echo "Expected:" +echo "$EXPECTED" +echo "" +echo "Got:" +echo "$RUN_OUTPUT" +exit 1