Skip to content

fix(transforms/module): replace canonicalization with clean to not mess up symlinks#11585

Open
perbergland wants to merge 11 commits intoswc-project:mainfrom
perbergland:fix/preserve-symlinks-11584
Open

fix(transforms/module): replace canonicalization with clean to not mess up symlinks#11585
perbergland wants to merge 11 commits intoswc-project:mainfrom
perbergland:fix/preserve-symlinks-11584

Conversation

@perbergland
Copy link
Copy Markdown

@perbergland perbergland commented Feb 24, 2026

Summary

  • Replaces std::fs::canonicalize() with path_clean::PathClean::clean() in both NodeImportResolver::try_resolve_import and ModuleConfig::get_resolver
  • This normalizes . and .. path components without resolving symlinks
  • No new config options needed — this is a pure bugfix

Fixes #11584

Root cause

When jsc.baseUrl is set, SWC creates a NodeImportResolver which called std::fs::canonicalize() on resolved paths (path.rs:261). canonicalize() does three things: makes paths absolute, resolves ./.., and resolves symlinks. Only the first two are needed — the symlink resolution breaks setups where symlinked source files need imports resolved relative to the symlink location.

The path_clean crate (already a dependency) provides .clean() which does the same normalization lexically, without touching the filesystem or resolving symlinks.

Changes

  • crates/swc_ecma_transforms_module/src/path.rs: Replace canonicalize(resolved) with resolved.clean(), remove unused fs::canonicalize import
  • crates/swc/src/config/mod.rs: Replace v.canonicalize() with v.clean() (+ make-absolute for relative paths) in get_resolver() so both base and target use the same non-symlink-resolving normalization — this ensures diff_paths compares paths in the same "path space"
  • crates/swc/Cargo.toml: Add path-clean dependency
  • crates/swc_ecma_transforms_module/tests/path_node.rs: Add test with symlinked directory that verifies import paths are preserved through symlinks

Test plan

  • New test issue_11584_symlink_not_canonicalized creates a symlinked directory structure and verifies ../lib/dep resolves to ../lib/dep.js (not ../../real/lib/dep.js)
  • issue_8667_1 Bazel sandbox test passes (confirmed diff_paths produces correct relative paths with consistent normalization)
  • All 260 swc_ecma_transforms_module tests pass
  • All 9 swc_cli_impl tests pass
  • cargo clippy and cargo fmt clean

🤖 Generated with Claude Code

perbergland and others added 2 commits February 24, 2026 19:32
swc-project#11584)

Add test that demonstrates the issue where `canonicalize()` in
`NodeImportResolver::try_resolve_import` resolves symlinked paths to
their real filesystem location, breaking import resolution when
`preserve_symlinks` is intended.

Also adds the `preserve_symlinks` field to `path::Config` and threads
it through `JscConfig` and `build_resolver`, but does not yet skip
the canonicalization (test is expected to fail).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…et (swc-project#11584)

When `jsc.preserveSymlinks` is true, skip the `canonicalize()` call in
`NodeImportResolver::try_resolve_import` so that symlinked paths are
preserved during module resolution. Also skip base filename
canonicalization in `ModuleConfig::get_resolver`.

This fixes the issue where `jsc.baseUrl` causes symlinked source files
to have their imports resolved from the real file location instead of
the symlink location, breaking bundlers that set `resolve.symlinks: false`.

Closes swc-project#11584

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@perbergland perbergland requested review from a team as code owners February 24, 2026 18:37
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 24, 2026

CLA assistant check
All committers have signed the CLA.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Feb 24, 2026

🦋 Changeset detected

Latest commit: 9e5fc33

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Feb 24, 2026

Merging this PR will not alter performance

✅ 192 untouched benchmarks


Comparing perbergland:fix/preserve-symlinks-11584 (9e5fc33) with main (86815b1)

Open in CodSpeed

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09f010577d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 24, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28745160 bytes)

Commit: dfbb790

…et (swc-project#11584)

Replace `std::fs::canonicalize()` with `path_clean::PathClean::clean()`
in `NodeImportResolver::try_resolve_import`. This normalizes `.` and `..`
path components without resolving symlinks, fixing the issue where
symlinked source files had their imports resolved from the real file
location instead of the symlink location.

`canonicalize()` was originally added for Bazel sandbox support (swc-project#8265),
but its symlink resolution is unnecessary — lexical path cleaning
achieves the same normalization without breaking symlinked source files.

This removes the `preserve_symlinks` config option added in the previous
commit since it is no longer needed.

Closes swc-project#11584

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@socket-security
Copy link
Copy Markdown

socket-security bot commented Feb 25, 2026

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5fb410a47d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@perbergland perbergland changed the title fix(transforms/module): add preserveSymlinks option to skip canonicalization fix(transforms/module): replace canonicalization with clean to not mess up symlinks Feb 25, 2026
…roject#11584)

Also normalize the base filename in `get_resolver()` using
`path_clean::PathClean::clean()` instead of `std::fs::canonicalize()`.
This ensures both base and target paths live in the same "path space"
so that `diff_paths` computes correct relative paths in Bazel sandbox
and symlinked setups.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 152d8171ae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

)

The `path_clean::PathClean` import is only used inside the
`#[cfg(feature = "module")]` impl block. Gate the import to avoid
unused-import errors when building without the module feature.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is what we want. I want to ensure that it works for bazel users before merging/publishing

@kdy1
Copy link
Copy Markdown
Member

kdy1 commented Mar 3, 2026

I'm sorry to bother you all, but I want to ensure this doesn't break rules_swc.
Can you check if this PR looks good?

cc @alexeagle @gregmagolan @jbedard

@jbedard
Copy link
Copy Markdown

jbedard commented Mar 3, 2026

Thanks for the ping @kdy1. I think this would be a positive change for rules_swc/js, normally bazel issues come from tools that do resolve or follow symlinks.

Have you observed any issues with things like pnpm package stores? I assume import '@a/pkg is not effected by this change, but what about import './node_modules/@a/pkg? If that is a symlink into ./node_modules/.pnpm/node_modules/@a/pkg I'm wondering if that is an example that this PR fixes?

@kdy1
Copy link
Copy Markdown
Member

kdy1 commented Mar 3, 2026

@claude Add tests for the use case described in #11585 (comment)

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 3, 2026

Claude Code is working…

I'll analyze this and get back to you.

View job run

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c0467f370

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1630 to +1632
env::current_dir()
.map(|cwd| cwd.join(v).clean())
.unwrap_or_else(|_| v.to_path_buf())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve missing relative base against baseUrl

When base is relative, this new logic always makes it absolute via env::current_dir(). In in-memory/virtual-file compiles (relative filename that does not exist on disk), the previous behavior kept base relative, so NodeImportResolver::try_resolve_import could rebase it through absolute_path(base_dir, ...) and keep rewrites in the jsc.baseUrl path space; with this change, base is pinned to process CWD instead, and diff_paths can emit incorrect long paths (for example ../../project/...) when cwd differs from baseUrl.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18cf53e24b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

FileName::Real(v) if !skip_resolver => {
FileName::Real(v.canonicalize().unwrap_or_else(|_| v.to_path_buf()))
}
FileName::Real(v) if !skip_resolver => FileName::Real(v.clean()),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep Windows base path in UNC form before diffing

Replacing canonicalize() with clean() here leaves base as a drive-letter path on Windows, but build_resolver still canonicalizes jsc.baseUrl to UNC (\\?\...) before creating NodeImportResolver; that means try_resolve_import can compare two absolute paths with different prefixes and skip normalization (it only normalizes when absolute-vs-relative differs), so relative rewriting via diff_paths no longer works and imports can fall back to absolute //?/C:/... specifiers for jsc.baseUrl/paths resolutions.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e5fc339f7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
// https://github.com/swc-project/swc/issues/11584
if let FileName::Real(resolved) = &mut target.filename {
*resolved = resolved.clean();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize Windows target paths before import diffing

On Windows, replacing canonicalization with resolved.clean() leaves absolute targets in drive-letter form (for example C:\...), while resolver setup still canonicalizes jsc.baseUrl to UNC (\\?\C:\...) in build_resolver; TsConfigResolver can also return absolute FileName::Real(tp.into()) directly for exact paths mappings (crates/swc_ecma_loader/src/resolvers/tsc.rs, Pattern::Exact). In that case diff_paths receives two absolute paths with different prefixes, fails to compute a relative path, and the rewriter falls back to absolute specifiers, which is a regression for Windows projects using absolute path mappings.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

jsc.baseUrl causes canonicalization of symlinked source files, breaking bundler symlinks: false

4 participants