fix: rework on handling DOS device paths on Windows #84
Conversation
WalkthroughThe changes update the error handling and documentation related to symlink resolution and Windows path prefix stripping. The Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 5bb3228 in 2 minutes and 13 seconds. Click for details.
- Reviewed
147lines of code in3files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/error.rs:41
- Draft comment:
Good addition of a specific error variant for unsupported DOS device paths. The error message is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/file_system.rs:220
- Draft comment:
Accessing p[1] assumes that the stripped path has at least two bytes. It would be safer to check thatp.len() > 1before indexing to avoid potential panics. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The code is handling Windows paths, specifically DOS device paths. After stripping "\?" or "\.", we expect a drive letter followed by ":" (like "C:"). If p is too short to have a drive letter and colon, it's not a valid DOS path anyway. The test cases show all valid paths have the expected format. A path shorter than 2 bytes after stripping would be malformed. The comment raises a valid point about bounds checking. Even if we expect valid paths, defensive programming would suggest checking bounds. However, if p is too short, it's not a valid DOS path and should return None anyway. The current code would panic, but that's acceptable for malformed input that should never occur in practice. The comment suggests a valid safety improvement, but it's not critical since it only affects malformed paths that shouldn't exist in practice. The current behavior of panicking on invalid paths is acceptable.
3. src/file_system.rs:211
- Draft comment:
Nice handling of UNC paths by prepending the "\". The use ofunsafefor converting encoded bytes is acceptable given thatas_encoded_bytesensures validity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/fs_cache.rs:83
- Draft comment:
The logic in canonicalize (Windows branch) that uses try_strip_windows_prefix now properly returns an error for unsupported paths. This ensures that mounted volumes without a drive letter are correctly rejected. The change meets the issue requirements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/error.rs:44
- Draft comment:
Clear documentation for the new PathNotSupported error variant. Looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/file_system.rs:221
- Draft comment:
Potential panic: ensure the slice 'p' has length > 1 before accessing p[1]. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code assumes p[1] exists because it's checking for DOS paths which must have a drive letter followed by ':'. If p is too short, it's not a valid DOS path anyway. The error case (p being too short) would be correctly handled by returning None, which is what we want for invalid paths. A bounds check would just be redundant since we want to return None for both cases. The code could panic if strip_prefix() returns a slice shorter than 2 bytes. Even if we expect valid DOS paths, defensive programming would suggest checking bounds. The function is explicitly documented to handle DOS device paths, and a path shorter than 2 bytes after stripping the prefix is clearly not a valid DOS path. Returning None is the correct behavior for invalid paths, whether they're invalid due to length or content. The bounds check is unnecessary since both a bounds error and an incorrect ':' character indicate an invalid DOS path, which should return None. The current code handles the error case correctly.
7. src/file_system.rs:193
- Draft comment:
Inconsistent handling of unsupported DOS device paths: in read_link the code returns the raw target when try_strip_windows_prefix yields None, while in fs_cache canonicalize such cases yield an error. It would help to document or unify this behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Looking at the code, read_link() does explicitly handle the None case by falling back to the raw target. However, I cannot see the canonicalize() implementation's handling of these paths in the provided code - it uses dunce::canonicalize() which is an external dependency. Without seeing how canonicalize() actually handles these paths, I cannot verify if there is truly an inconsistency. I may be missing some implicit knowledge about how dunce::canonicalize handles these paths. The comment author may have deeper context about the expected behavior. Even if there is an inconsistency, without being able to verify the canonicalize() behavior, I cannot be confident enough that this comment is correct and actionable. The comment should be removed because we cannot verify its correctness without seeing the implementation of dunce::canonicalize. The comment requires more context than what is visible in this file.
8. src/fs_cache.rs:83
- Draft comment:
Note that canonicalize propagates an error (ResolveError::PathNotSupported) when try_strip_windows_prefix returns None. Confirm that this behavior (versus returning the raw target as in read_link) is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. src/error.rs:29
- Draft comment:
Typo: In the comment "Matched alias value not found", there's an extra space between 'value' and 'not'. Consider removing the extra space. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. src/error.rs:37
- Draft comment:
Typo: In the comment "Tsconfig's project reference path points to it self", consider replacing "it self" with "itself". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. src/error.rs:57
- Draft comment:
Typo: In the comment "All of the aliased extension are not found", consider changing "extension" to "extensions" for grammatical correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. src/file_system.rs:222
- Draft comment:
Trivial: Consider capitalizing "nodejs" to "Node.js" for consistency and clarity in the comment. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_lFUmiLWgnWm19eup
You can customize 
|
@chenxinyanc CI/lint is broken. Please confirm to run |
CodSpeed Performance ReportMerging #84 will not alter performanceComparing Summary
|
|
It seems there is still issue with this PR. It seems I'll hold it off until the As it may take some time, I've marked the PR as draft and will ping you when it's ready @JounQin |
|
This is the test case I'm plugging into the I won't include this in the PR so keeping a record here just for reference. #[test]
fn resolve_with_mounted_volume() {
let fixture = PathBuf::from(r#"D:\test\unrs-resolver-mounted-partition\src"#);
let opts = ResolveOptions {
tsconfig: Some(TsconfigOptions {
config_file: fixture.join("tsconfig.json"),
references: TsconfigReferences::Auto,
}),
..ResolveOptions::default()
};
let resolver = Resolver::new(opts);
assert_eq!(
resolver.resolve(&fixture, "./mount-root/foo.ts").map(|r| r.full_path()),
Ok(fixture.join(
"mount-root/foo.ts"
)),
);
} |
|
I'm going to merge upstream changes now. |
Sure. This PR may take some time. I've discovered something tough (see below) and am still debugging through it. Anyway, this would still be a rare & wonderful experience for me even if this PR cannot be merged in the end (conflicts incoming!). btw should the package-level import for the following package.json be accepted by the resolver? {
"name": "@fluentui/react-components",
"version": "9.62.0",
"main": "lib-commonjs/index.js",
"module": "lib/index.js",
"typings": "./dist/index.d.ts",
"sideEffects": false,
// ...
"exports": {
".": {
"types": "./dist/index.d.ts",
"node": "./lib-commonjs/index.js",
"import": "./lib/index.js",
"require": "./lib-commonjs/index.js"
// there is no "default" here.
},
"./package.json": "./package.json",
"./unstable": {
"types": "./dist/unstable.d.ts",
"node": "./lib-commonjs/unstable/index.js",
"import": "./lib/unstable/index.js",
"require": "./lib-commonjs/unstable/index.js"
}
},
}I'm asking because when I tried to resolve its entry point with resolver.resolve(&fixture, "@fluentui/react-components").map(|r| r.full_path())I saw the following ... and it seems Note that this |
So you mean the upstream merged PR won't resolve your original issue, right?
You need to pass the custom |
Thanks! Then my change here is basically done. I've tried the change locally with And I'll check whether it's possible to write unit test for this PR. While it's not trivial to set up a Mounted Volume in CI pipeline, I can test whether we can just set up some plain symlinks pointing to a DOS device path (something starting with
I thought Boshen is using a completely different approach in oxc-project/oxc-resolver#455. Let me try out that change before reaching back. |
|
OK it turns out Boshen has reverted the change with However, there is bug with my prior change made in oxc-project/oxc-resolver#455 , so I'll propose another PR to cherry-pick the change back to the upstream, once we have settled the PR here. |
|
Let me do a merge-upstream first before you can get the diffs/conflicts confidently. |
Thanks! I'll resolve the conflict. And I propose that
Sorry for the troubles 🌚 things should have been simpler if I had stuck to making changes in |
Ah, that was me proposed to change first at here, but I still think that's a better approach personally, feel free to disagree and do things on your own way. Merged upstream at #86 I've done the rebase for you. @chenxinyanc |
153c0b6 to
aa869ca
Compare
|
CI is all green, AIs are also happy now. Please help to confirm is there anything I missed? |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to aa869ca in 2 minutes and 2 seconds. Click for details.
- Reviewed
248lines of code in6files - Skipped
0files when reviewing. - Skipped posting
15draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/windows.rs:10
- Draft comment:
Good use of strip_prefix and handling UNC paths; the use of unsafe seems justified by the preconditions. Ensure code commentary is kept precise if new DOS device formats arise. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. src/file_system.rs:192
- Draft comment:
In the Windows branch of try_read_link(), consider using a simpler mapping like.map(PathBuf::from)rather than explicitly calling to_path_buf in a closure (for clarity). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The suggestion is about changed code, specifically the Windows branch that was modified in this PR. The suggestion is technically correct - PathBuf::from would work equally well here and is arguably more idiomatic. However, the change is very minor and both approaches are valid. The current code is not wrong or problematic. The suggestion is quite nitpicky and doesn't meaningfully improve code quality. The current code is perfectly clear and works correctly. While the suggestion is technically valid, it falls into the category of being too minor and obvious to warrant a comment. This comment should be removed as it suggests a very minor style change that doesn't meaningfully improve the code.
3. src/fs_cache.rs:82
- Draft comment:
Ensure that using unwrap_or_else to fallback to the original path is acceptable when try_strip_windows_prefix returns None. This behavior is consistent with the intent to not follow symlinks with unrepresentable target paths. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. src/tests/memory_fs.rs:75
- Draft comment:
The MemoryFS implementation of try_read_link() correctly returns an error for non-symlinks. Ensure that similar behavior is maintained in other FS implementations. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
5. src/error.rs:41
- Draft comment:
Removed the unusedPathNotSupportedvariant from ResolveError. This simplifies error handling since try_strip_windows_prefix now returns an Option rather than an error. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/file_system.rs:56
- Draft comment:
Renamedread_linktotry_read_linkand changed its return type toio::Result<Option<PathBuf>>to indicate unsupported symlink targets (e.g. DOS device paths). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/file_system.rs:192
- Draft comment:
On Windows, the implementation now calls try_strip_windows_prefix and maps its result. This correctly handles cases where the DOS device path is unsupported by returning None. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/fs_cache.rs:82
- Draft comment:
Using unwrap_or_else on try_strip_windows_prefix, so that if it returns None the original path is used. Verify that this fallback behavior is acceptable for all Windows cases. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/windows.rs:22
- Draft comment:
Refactored try_strip_windows_prefix to return an Option rather than a Result. Consider adding a length check before accessing p[1] to ensure safety. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
10. src/tests/memory_fs.rs:72
- Draft comment:
Updated MemoryFS to implement try_read_link (returning an error for non-symlinks) instead of the old read_link. This aligns with the new API. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. src/options.rs:153
- Draft comment:
Enhanced the documentation for symlink resolution. The new docs clearly explain that symlinks referring to mounted volumes or DOS device paths (without a drive letter) will not be followed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. src/error.rs:29
- Draft comment:
Typographical error: In the comment for the 'MatchedAliasNotFound' variant, there are extra spaces between 'value' and 'not found'. Consider changing "Matched alias value not found" to "Matched alias value not found". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. src/error.rs:37
- Draft comment:
Typographical error: In the documentation for the 'TsconfigSelfReference' variant, the phrase "it self" should be replaced with "itself" for correctness. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. src/error.rs:53
- Draft comment:
Typographical error: The comment for the 'ExtensionAlias' variant uses "All of the aliased extension are not found". It would be clearer to use the plural "extensions" instead of "extension". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. src/file_system.rs:305
- Draft comment:
Typographical error: In line 305, consider changing 'Need to trim the extra \0 introduces by nodejs/uvwasi#262' to 'Need to trim the extra \0 introduced by nodejs/uvwasi#262' for correct grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_cOaWFDYLlnYfyUJ2
You can customize
193b5c5 to
11ca86b
Compare
✅ Actions performedFull review triggered. |
|
Friendly ping @chenxinyanc, please help to confirm that is there anything I missed, so that I'll merge this PR as-is ASAP. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…oject/oxc-resolver#455 e8a11841083164401bca72841d8c9b2d8f04185d strip_windows_prefix: Handle DOS device paths properly. 2862145da2685b3365e0bfaafdd23751abec2858 [autofix.ci] apply automated fixes 70f675e01bfc780c84d443fdf801028b303717fa Fix build break. b6cbd102d0723747a5b3ab5bb1c10664d34249e7 Fix build break.
No need to follow symlink at all in FsCache::canonicalize if there is a link pointing to DOS device path that cannot be imported by NodeJS.
98cc9fc to
cf1eabd
Compare
@JounQin Thanks for the follow-up. I'm afraid I'll have to force push here, as I've already resolved conflict yesterday before waiting for the update in the main branch. I'm following the idea from Boshen in I've also tried with creating symlinks in unit tests, but it's not trivial, and I might need to query for the Volume ID with Win 32 API. Even if it were possible, it won't be in this PR. So, again, consider this iteration as final iteration from me for this PR. I've tested this iteration with eslint locally. |
|
@chenxinyanc That's totally fine and greater actually, but the CI is broken again now. :( |
fae657b to
57591fd
Compare
57591fd to
6a0f6d6
Compare
|
|
I'm not sure how to resolve the last CI failure. |
There was a problem hiding this comment.
Thanks for your efforts! I think it's ready to be merged, WDYT? @chenxinyanc
It's some kind of coding style issue from @codacy, I've bypassed that check, let's not waste time on it. |
LGTM I'll follow up with another PR for the test case later. |
Which test case do you mean? I thought all cases were covered already, but it's also fine to merge this PR as-is first. |
Yes please.
I meant adding a new test case to cover the behavior that "we won't follow symlinks if they are pointing to a DOS device path with Volume GUID ( |
|
Thanks for your great work again! @chenxinyanc |
This PR is cherry-picked from the first several iterations of oxc-project/oxc-resolver#455 and fixes #65
Important
Improves handling of Windows DOS device paths by updating
strip_windows_prefixtotry_strip_windows_prefix, returningNonefor unsupported paths, and updating related functions and tests.strip_windows_prefixtotry_strip_windows_prefixinwindows.rs, changing return type toOption<PathBuf>to returnNonefor unsupported DOS device paths.try_read_linkinfile_system.rsandfs_cache.rsto handleNonereturn value, avoiding symlink resolution for unsupported paths.ResolveOptionsinoptions.rsto document behavior for symlinks pointing to unsupported paths.windows.rsto verifytry_strip_windows_prefixreturnsNonefor unsupported paths and correct paths for supported ones.memory_fs.rsto reflect changes intry_read_linkbehavior.This description was created by
for aa869ca. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit