[codex] Fix symlinked relative import resolution#5210
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds symlink-safe relative import resolution to perry's compiler by introducing lexical path normalization and dual-path (source and canonical) resolution infrastructure. ChangesSymlink-safe import resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/collect_modules/tests.rs (1)
155-180: ⚡ Quick winExtend the regression to cover canonical-sibling collisions and transitive imports.
This fixture only proves the fallback works when the canonical sibling is missing. Add a decoy under
real-parent/outside/dep.tsand route throughapp/child.ts -> ../outside/depso the test catches both wrong “normal-first” resolution and loss of lexical base after the first queued child.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/collect_modules/tests.rs` around lines 155 - 180, The test fixture currently only verifies the fallback when the canonical sibling is missing. Extend it to also test canonical-sibling collisions and transitive imports by adding: a decoy module at real-parent/outside/dep.ts (with similar or conflicting content), an intermediate file app/child.ts that imports from ../outside/dep, and then update the entry.ts file to import through app/child.ts instead of directly importing from ../outside/dep. This will ensure the test catches both wrong "normal-first" resolution behavior and any loss of lexical base tracking after the first queued child module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Around line 157-173: The function cached_resolve_import_with_lexical_base
resolves imports correctly using the lexical path but only returns the canonical
path, causing downstream modules queued from that canonical path to resolve
their own imports incorrectly when symlinks are involved. Modify the function to
return both the canonical path and the lexical path (for example, as a tuple or
structured pair), where the canonical path is used for tracking visited modules
and the lexical path is used when queuing child modules for processing via
WorkFrame::Enter. Update all callers of cached_resolve_import_with_lexical_base
to handle the returned pair appropriately. Additionally, add a regression test
that verifies symlinked modules correctly resolve transitive imports from the
lexical path, such as a scenario where alias-parent/app/child.ts imports
../outside/dep and a decoy real-parent/outside/dep.ts exists, ensuring the
traversal uses the lexical path and not the canonical path too early.
- Around line 138-147: The guard condition at the start of this block (checking
if spec starts_with "./", "../", or "/") is incomplete and doesn't account for
bare relative specifiers like "." and ".." that the new resolver can handle.
These patterns are being skipped when they should be processed for transitive
dependencies. Replace the manual string-matching guard with a shared
relative-specifier predicate function that properly identifies all relative
import patterns including ".", "..", "./", "../", and absolute paths, ensuring
that specs like "." and ".." are processed by the resolve_relative_import_path
or resolve_with_extensions calls rather than being skipped.
In `@crates/perry/src/commands/compile/resolve.rs`:
- Around line 902-913: The import resolution order is backwards in the fallback
logic. Currently, resolve_with_extensions is called first on the
filesystem-resolved path, then falls back to the lexically normalized path. This
causes symlinked paths to be resolved before considering the source-visible path
written by the programmer. Swap the order of resolution attempts: first try
resolve_with_extensions on the lexical path (created by
normalize_path_lexically), and only if that returns None should it fall back to
resolve_with_extensions on the original resolved path.
- Around line 922-925: The Component::ParentDir handling in the
normalize_path_lexically function (or similar path normalization logic)
incorrectly pops all components including leading `..` segments. Fix this by
checking whether the last component in the normalized path is an actual
directory segment before popping it. Only pop if the last component is a normal
directory (not ParentDir or CurDir); otherwise, push the ParentDir component to
preserve leading `..` sequences. Additionally, add the two provided test cases
to verify that `../../dep` normalizes to `../../dep` and `app/../../dep`
normalizes to `../dep`.
---
Nitpick comments:
In `@crates/perry/src/commands/compile/collect_modules/tests.rs`:
- Around line 155-180: The test fixture currently only verifies the fallback
when the canonical sibling is missing. Extend it to also test canonical-sibling
collisions and transitive imports by adding: a decoy module at
real-parent/outside/dep.ts (with similar or conflicting content), an
intermediate file app/child.ts that imports from ../outside/dep, and then update
the entry.ts file to import through app/child.ts instead of directly importing
from ../outside/dep. This will ensure the test catches both wrong "normal-first"
resolution behavior and any loss of lexical base tracking after the first queued
child module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3949f47-4f34-4bf9-b38e-3de27a1ad678
📒 Files selected for processing (4)
crates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/tests.rscrates/perry/src/commands/compile/resolve.rstests/test_symlinked_entry_imported_class.sh
…ive-import-resolution # Conflicts: # docs/src/api/reference.md
Summary
./..normalization as a fallback for relative specifiers whose importer path contains a symlinked component.instanceofthrough a symlinked entry path.Root Cause
Perry stores and reads collected modules by canonical path, but source relative import specifiers are written against the path passed to the compiler. When that path contains a symlinked component, resolving
..after filesystem canonicalization can probe a different sibling directory. The import can then be missed, leaving imported class metadata absent and allowing construction to fall back to the empty placeholder shape.Verification
cargo fmt --checkcargo test -p perry symlinked_entry_resolves_relative_imports_from_lexical_pathcargo build -p perry -p perry-runtimePERRY_BIN=target/debug/perry tests/test_symlinked_entry_imported_class.shPERRY_BIN=target/debug/perry tests/test_xmod_imported_zero_arg_super.shPERRY_BIN=target/debug/perry tests/test_xmod_empty_imported_derived_arg_forward.shSummary by CodeRabbit
commander.argsas an instance method.