Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::path::absolute #91673

Merged
merged 2 commits into from
Feb 13, 2022
Merged

std::path::absolute #91673

merged 2 commits into from
Feb 13, 2022

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Dec 8, 2021

Implements #59117 by adding a std::path::absolute function that creates an absolute path without reading the filesystem. This is intended to be a drop-in replacement for std::fs::canonicalize in cases where it isn't necessary to resolve symlinks. It can be used on paths that don't exist or where resolving symlinks is unwanted. It can also be used to avoid circumstances where canonicalize might otherwise fail.

On Windows this is a wrapper around GetFullPathNameW. On Unix it partially implements the POSIX pathname resolution specification, stopping just short of actually resolving symlinks.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2021
@the8472
Copy link
Member

the8472 commented Dec 8, 2021

absolute(a.join(b)) will give resolve()-like behavior which some other languages have, right? If so then the comment could mention that combination as a way to resolve relative paths to something other than the current working directory.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Dec 8, 2021

Good point! But I'm slightly wary of the Windows behaviour here. I'm thinking of legacy paths such as D:foo which will resolve to the D drive's current directory if the application is run from the command prompt (or the root of D otherwise).

E.g.

let path = Path::new(r"C:\path").join("D:foo");
// path is "D:foo"
let absolute = absolute(path);
// absolute is like "D:\something\foo"
// actual value depends on the `D` drive's current directory.

My point being that the given path is relative but the base directory is still ignored.

@the8472
Copy link
Member

the8472 commented Dec 8, 2021

Is that an issue with join or with absolute? Or with using them to achieve resolve?

@ChrisDenton
Copy link
Member Author

Hm, probably with using them to implement resolve. The command prompt can have a different current directory for each letter from A to Z (though it may not have them all). So to capture this you'd need to pass in mappings of (optional) current directories for each drive.

join could work differently. E.g. it could treat D:foo as either .\D:foo or D:\foo. But it doesn't and if it did it may be unexpected. I guess absolute could error on such paths. However, I don't think it should do any thing different from the OS as the goal here is to be as close to canonicalize as possible.

To be clear, I'm uncertain how much of an issue this is but it does make me wary of suggesting it.

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 9, 2021
// SAFETY: `fill_utf16_buf` ensures the `buffer` and `size` are valid.
// `lpfilename` is a pointer to a null terminated string that is not
// invalidated until after `GetFullPathNameW` returns successfully.
|buffer, size| unsafe { c::GetFullPathNameW(lpfilename, size, buffer, ptr::null_mut()) },
Copy link
Member

Choose a reason for hiding this comment

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

Does this work for non \\?\ paths bigger than MAX_PATH? It would be nice to be able to do std::path::absolute() and then prefix \\?\ if necessary to handle long paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed it does! The std already does exactly that. Unfortunately the current docs for GetFullPathNameW suggests otherwise but this was an oversight that's going to be corrected when the online docs are next rebuilt (see the sdk-api source).

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 10, 2021
@Mark-Simulacrum
Copy link
Member

In general this seems "fine", though I don't know if it's quite the API we want. Adding it as unstable to start though seems quite reasonable; we can discuss prior to stabilization whether this hits the right point on the API spectrum. Some initial questions I might think about:

  • Should we prepend the current directory, or skip that step? The user can typically join with the current directory afterwards, right?
  • Is the avoidance of stripping .. actually the right call? It does seem consistent with the docs linked, but may not be the most intuitive behavior (particularly given that Windows doesn't do that).
  • Probably some other questions based on the thread here so far, you probably have a better sense than me.

Can you file a tracking issue, and then I can r+?

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2022
@ChrisDenton
Copy link
Member Author

@Mark-Simulacrum I've filed a tracking issue. I've added your points to the unresolved questions and I'll try to give my thoughts on them soon.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

Thanks! Going to go ahead and approve this PR.

@bors
Copy link
Contributor

bors commented Jan 10, 2022

📌 Commit 0660732 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 11, 2022
…imulacrum

`std::path::absolute`

Implements rust-lang#59117 by adding a `std::path::absolute` function that creates an absolute path without reading the filesystem. This is intended to be a drop-in replacement for [`std::fs::canonicalize`](https://doc.rust-lang.org/std/fs/fn.canonicalize.html) in cases where it isn't necessary to resolve symlinks. It can be used on paths that don't exist or where resolving symlinks is unwanted. It can also be used to avoid circumstances where `canonicalize` might otherwise fail.

On Windows this is a wrapper around [`GetFullPathNameW`](https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew). On Unix it partially implements the POSIX [pathname resolution](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13) specification, stopping just short of actually resolving symlinks.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 11, 2022
…imulacrum

`std::path::absolute`

Implements rust-lang#59117 by adding a `std::path::absolute` function that creates an absolute path without reading the filesystem. This is intended to be a drop-in replacement for [`std::fs::canonicalize`](https://doc.rust-lang.org/std/fs/fn.canonicalize.html) in cases where it isn't necessary to resolve symlinks. It can be used on paths that don't exist or where resolving symlinks is unwanted. It can also be used to avoid circumstances where `canonicalize` might otherwise fail.

On Windows this is a wrapper around [`GetFullPathNameW`](https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew). On Unix it partially implements the POSIX [pathname resolution](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13) specification, stopping just short of actually resolving symlinks.
@ehuss
Copy link
Contributor

ehuss commented Jan 12, 2022

@bors r-

Failed in #92785 (comment)

The wasm32-unknown-unknown target reuses unix/path.rs but itself is not unix.

Let me know if you have any questions about that or how to test it.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 12, 2022
@ChrisDenton
Copy link
Member Author

Thanks! I got wasm32-unknown-unknown to build locally. The fix was fortunately simple. I also added the tracking issue number and fixed a typo while I was there.

@bors
Copy link
Contributor

bors commented Feb 8, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 8, 2022
@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member Author

#87869 changed the way of creating a const io::Error. So I've rebased this PR and switch to using io::const_io_error! in library/std/src/path.rs. Nothing else has changed.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 8, 2022

📌 Commit 81cc3af has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2022
@bors
Copy link
Contributor

bors commented Feb 13, 2022

⌛ Testing commit 81cc3af with merge 1f4681a...

@bors
Copy link
Contributor

bors commented Feb 13, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 1f4681a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 13, 2022
@bors bors merged commit 1f4681a into rust-lang:master Feb 13, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 13, 2022
@bors bors mentioned this pull request Feb 13, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1f4681a): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

assert_eq!(absolute("//a/b/c").unwrap(), Path::new("//a/b/c"));
assert_eq!(absolute("///a/b/c").unwrap(), Path::new("/a/b/c"));
assert_eq!(absolute("/a/b/c/").unwrap(), Path::new("/a/b/c/"));
assert_eq!(absolute("/a/./b/../c/.././..").unwrap(), Path::new("/a/b/../c/../.."));
Copy link

@axetroy axetroy Mar 5, 2022

Choose a reason for hiding this comment

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

Unix test without dot prefix.

assert_eq!(absolute("./a").unwrap(), Path::new("/pwd/a")); // return /pwd/./a
assert_eq!(absolute("../a").unwrap(), Path::new("/parent_pwd/a")); // return /pwd/../a

I have tried in darwin. it is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for reporting this. I'll add more tests and fix the dot prefix case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.