fix(runtime): path and url default to win32 semantics on Windows#6663
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds host-aware default path dispatch, pins ChangesPath semantics
URL path compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/url/node_compat.rs (1)
491-521: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winResolve the Win32 path before checking for a UNC prefix.
Node's
pathToFileURLcallspath.win32.resolve()on the input before checking if it's a UNC path. By isolating the resolution step to theelsebranch, UNC inputs bypass path resolution entirely (meaning..segments remain unnormalized). Additionally, inputs like//server/sharemiss the UNC branch because they don't begin with\\\\until after resolution, causing them to fall into theelseblock and get incorrectly formatted as drive-letter paths (e.g.,file:////server/shareinstead offile://server/share).Move the Win32 resolution step outside and above the UNC check so that UNC paths are correctly resolved and identified.
🐛 Proposed fix
- // Win32 (`#2975`). UNC paths (`\\host\share\...`) become - // `file://host/share/...`; everything else is a (drive-letter) path - // with `\` separators rewritten to `/`. - if let Some(unc) = path.strip_prefix("\\\\") { - // First segment after `\\` is the host; the remainder is the path. - let (host, rest) = match unc.find('\\') { - Some(idx) => (&unc[..idx], &unc[idx..]), - None => (unc, ""), - }; - let rest_fwd = rest.replace('\\', "/"); - format!("file://{}{}", host, encode_file_url_path(&rest_fwd)) - } else { - // Node's win32 `pathToFileURL` resolves the input against the - // cwd first (`path.win32.resolve(filepath)`), then preserves a - // trailing separator — mirroring the posix arm below. Absolute - // inputs pass through resolution unchanged (modulo - // normalization). - let preserve_trailing_sep = path.ends_with('\\') || path.ends_with('/'); - let mut resolved = crate::path::resolve_win32_str(&path); - if preserve_trailing_sep && !resolved.ends_with('\\') { - resolved.push('\\'); - } - let fwd = resolved.replace('\\', "/"); - let encoded = encode_file_url_path(&fwd); - if encoded.starts_with('/') { - format!("file://{}", encoded) - } else { - format!("file:///{}", encoded) - } - } + // Node's win32 `pathToFileURL` resolves the input against the + // cwd first (`path.win32.resolve(filepath)`), then preserves a + // trailing separator — mirroring the posix arm below. Absolute + // inputs pass through resolution unchanged (modulo + // normalization). + let preserve_trailing_sep = path.ends_with('\\') || path.ends_with('/'); + let mut resolved = crate::path::resolve_win32_str(&path); + if preserve_trailing_sep && !resolved.ends_with('\\') { + resolved.push('\\'); + } + + // Win32 (`#2975`). UNC paths (`\\host\share\...`) become + // `file://host/share/...`; everything else is a (drive-letter) path + // with `\` separators rewritten to `/`. + if let Some(unc) = resolved.strip_prefix("\\\\") { + // First segment after `\\` is the host; the remainder is the path. + let (host, rest) = match unc.find('\\') { + Some(idx) => (&unc[..idx], &unc[idx..]), + None => (unc, ""), + }; + let rest_fwd = rest.replace('\\', "/"); + format!("file://{}{}", host, encode_file_url_path(&rest_fwd)) + } else { + let fwd = resolved.replace('\\', "/"); + let encoded = encode_file_url_path(&fwd); + if encoded.starts_with('/') { + format!("file://{}", encoded) + } else { + format!("file:///{}", encoded) + } + }🤖 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-runtime/src/url/node_compat.rs` around lines 491 - 521, Update the Win32 URL construction around the `href` branch to call `crate::path::resolve_win32_str` before checking for the UNC prefix. Use the resolved path for both UNC detection and formatting, so `..` segments are normalized and slash-prefixed UNC inputs such as `//server/share` enter the `file://host/share` path; preserve trailing-separator handling for non-UNC paths.crates/perry-runtime/src/path.rs (1)
553-566: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
js_path_posix_extnamelexicalPath::new(...).extension()is host-dependent here;path.posix.extname("a.b\\c")should return.b\\c, but Windows-target builds treat\as a separator and can return"". Use the same lexical/-based split as the other pinned POSIX helpers, and add a backslash-containing test case.🤖 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-runtime/src/path.rs` around lines 553 - 566, Update js_path_posix_extname to determine the extension using POSIX lexical rules with only “/” treated as a separator, rather than Path::new(...).extension(), so backslashes remain part of the filename and “a.b\c” returns “.b\c”. Add a test covering this backslash-containing case alongside the existing pinned POSIX helper tests.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/path.rs`:
- Around line 553-566: Update js_path_posix_extname to determine the extension
using POSIX lexical rules with only “/” treated as a separator, rather than
Path::new(...).extension(), so backslashes remain part of the filename and
“a.b\c” returns “.b\c”. Add a test covering this backslash-containing case
alongside the existing pinned POSIX helper tests.
In `@crates/perry-runtime/src/url/node_compat.rs`:
- Around line 491-521: Update the Win32 URL construction around the `href`
branch to call `crate::path::resolve_win32_str` before checking for the UNC
prefix. Use the resolved path for both UNC detection and formatting, so `..`
segments are normalized and slash-prefixed UNC inputs such as `//server/share`
enter the `file://host/share` path; preserve trailing-separator handling for
non-UNC paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d70c3c1-ba73-40a0-a65d-a04f04cb1cf0
📒 Files selected for processing (5)
crates/perry-hir/src/lower/expr_call/nested_namespace.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_m_p.rscrates/perry-runtime/src/path.rscrates/perry-runtime/src/path/tests.rscrates/perry-runtime/src/url/node_compat.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Node-parity contract
On win32, Node aliases the default namespace to the win32 implementation:
path === path.win32, sopath.sep === '\\',path.delimiter === ';', and everypath.*function uses Windows rules.url.fileURLToPath/url.pathToFileURLdefault theirwindowsoption to the platform (options?.windows ?? isWindows). Perry's runtime pinned all of these to POSIX regardless of host; this PR makes the DEFAULTS platform-correct while keeping the explicitpath.posix.*/path.win32.*namespaces pinned.What dispatches where
Perry has three routes into the path module, and all three now agree:
path.join(...)->Expr::PathJoin-> thejs_path_*extern symbols. These symbols are shared by default calls AND the HIR rewrite ofpath.posix.*member calls, so the platform switch lives INSIDE the default entry points: eachjs_path_Xnow doesif cfg!(windows) { win32 } else { posix }, and the pre-dispatch POSIX bodies are preserved as a newpub(crate) js_path_posix_*family (crates/perry-runtime/src/path.rs).path[k].method(...), named-import value calls):nm_dispatch_pathincrates/perry-runtime/src/object/native_module_dispatch/dispatch_m_p.rs. The("path", ...)arms now select bycfg!(windows); the("path.posix", ...)arms were repointed from the base family (no longer posix-pinned) tojs_path_posix_*.path.sep/path.delimiterlower tojs_path_sep_get/js_path_delimiter_get, now platform-aware. The runtime property-table getter for module"path"(object/native_module/constants.rs) was ALREADYcfg!(windows)-aware — the two read paths now agree instead of contradicting each other.path.posix.sep/path.win32.sepstay compile-time-folded pinned literals.Keeping
path.posix.*pinned required one HIR change (crates/perry-hir/src/lower/expr_call/nested_namespace.rs): the static rewrite ofpath.posix.X(...)produced the sameExpr::PathXnodes as default calls — indistinguishable downstream. On Windows hosts only (cfg!(windows), so non-Windows lowering is byte-identical),dispatch_path_subnamespacenow routes posix member calls toExpr::NativeMethodCall { module: "path.posix", .. }, i.e. the existing #1740 runtime-dispatch path whose arms call the pinned posix family.path.win32.*is untouched (already pinned tojs_path_win32_*).url changes
options_windows_flag(crates/perry-runtime/src/url/node_compat.rs) now defaults tocfg!(windows)when the options argument is missing, not an object, or itswindowsproperty isundefined/null— mirroring Node'soptions?.windows ?? isWindows. An explicit boolean still wins in both directions. This one choke point coversfileURLToPath,fileURLToPathBuffer, andpathToFileURLin both their statically-lowered and runtime-dispatched forms. The win32 arm ofpathToFileURLalso now resolves relative inputs against the cwd first (Node callspath.win32.resolvebefore building the URL) with trailing-separator preservation mirroring the posix arm. Internal machinery that is posix by construction (file_url_to_path_string_posix,module_base_to_path, fs PathLike decoding) is deliberately unchanged.Why cfg!() runtime dispatch
cfg!(windows)inside perry-runtime is evaluated when the runtime is compiled for the target, so linked programs get target-correct semantics (unlike compile-time folding in the compiler, which would bake in the compiler host's platform). It also keeps both the win32 and posix code paths compiled on every host, so neither family can bit-rot behind a#[cfg].Bug fixed along the way
win32_resolve_innergrafted the input's drive onto the cwd unconditionally — correct for driveless POSIX-host cwds, but on a Windows hostpath.resolve("C:foo")producedC:C:\Users\...garbage. Drive-relative inputs now resolve against the cwd when the drives match, fall back to the drive root when they differ, and keep the historical POSIX-host graft byte-for-byte.Also: the pinned posix
isAbsolute/resolveabsoluteness check switched fromstd::path::Path::is_absolute()(host-dependent: applies win32 rules on Windows builds) to the lexical leading-/check. Byte-identical on Unix; posix-correct on Windows.File split
path.rscrossed the CI 2,000-line cap; the#[cfg(test)]modules moved verbatim to a child modulecrates/perry-runtime/src/path/tests.rs(path.rs is now ~1.9k lines).Tests run (Windows 11 x64, MSVC host)
cargo check -p perry-runtime/-p perry-hir— clean (no warnings from changed code; pre-existing crate warnings unchanged).cargo test -p perry-runtime --release -- path::tests url::node_compat::tests— 26 passed, 0 failed. Includes new suites:path::tests::platform_default_tests—#[cfg(windows)]asserts defaults are win32 (sep/delimiter/join/normalize/dirname/basename/extname/isAbsolute/resolve/relative/parse/toNamespacedPath);#[cfg(not(windows))]counterpart asserts posix defaults.path::tests::posix_pinned_tests— platform-independent assertions that thejs_path_posix_*family keeps/semantics on every host.windows_flag_defaults_to_platform,explicit_windows_option_wins_over_platform, cfg-splitfile_url_conversions_default_to_*.rustfmt --edition 2021 --checkon all five changed files — clean (CI'scargo fmt --allcannot run on this Windows host: os error 206, command line too long).bash scripts/check_file_size.sh— my files pass (path.rs1864 after moving its test modules topath/tests.rs); the script currently fails onperry-transform/src/inline/call_inliner.rs(2056) andperry-ui-windows/src/widgets/mod.rs(2118), both byte-identical on pristineorigin/main— inherited, untouched by this PR.python scripts/addr_class_inventory.py— no findings in any file this PR touches; the ratchet currently fails onbun_compat/mod.rs,object/global_this_webassembly.rs,object/collection_proto_thunks.rs,perry-stdlibratelimit.rs/slugify.rs, all verbatim on pristineorigin/main— likewise inherited.Caveats:
cargo test -p perry-runtime --release pathaborts with0xc0000028(STATUS_BAD_STACK) in two tests that merely match the filter by name —dyn_eval::tests::depd_fast_path_still_intactandnative_arena::tests::disposed_native_uint8_views_throw_in_fallback_paths. Verified pre-existing by A/B: both crash identically on a pristineorigin/maincheckout with zero changes from this PR (at the time of the A/B that required the one-lineExitProcessdeclaration fix from fix(runtime): declare ExitProcess as never-returning on Windows (build break) #6609 to compile on Windows; this branch is now based on a main that already includes fix(runtime): declare ExitProcess as never-returning on Windows (build break) #6609, so nothing extra is carried here). They exercise the exception/throw machinery in release on MSVC and are unrelated to this change.resolve_win32_for_namespace's handling of rooted-but-driveless paths is unchanged (toNamespacedPath("/tmp/x")->\tmp\x, where Node on Windows gives\\?\C:\tmp\x) — pre-existing pinned win32-family behavior, out of scope.No version bump/changelog per maintainer instruction.
Summary by CodeRabbit
Bug Fixes
path.posixconsistently uses POSIX behavior, regardless of the host platform.fileURLToPathandpathToFileURLplatform detection, overrides, and Windows path resolution.Tests