Use `with_native_path` for Windows #139683

ChrisDenton · 2025-04-11T16:20:55Z

Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to std::fs::File::open and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts.

As with the previous Unix PR (#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes.

rustbot · 2025-04-11T16:21:01Z

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

joboet · 2025-04-11T16:27:37Z

This introduces a number of safe functions that take a &[u16] string but expect that slice to be NUL-terminated. While it's not public API, that makes me a bit worried – it's now so easy to miss that precondition.

Would it be possible to introduce a WCStr in this PR that ensures NUL-termination?

ChrisDenton · 2025-04-11T16:28:00Z

Yeah, that's fair.

ChrisDenton · 2025-04-11T17:17:57Z

OK I've add a WCStr type that's currently just a thin wrapper around a slice with an unsafe constructor to assert the safety requirements.

joboet

Great, that looks safer, thank you.
r=me once CI is green.

library/std/src/sys/fs/mod.rs

library/std/src/sys/path/windows.rs

tgross35 · 2025-04-11T17:26:51Z

library/std/src/sys/path/windows.rs

+}
+
+#[inline]
+pub fn with_native_path<T>(path: &Path, f: &dyn Fn(&WCStr) -> io::Result<T>) -> io::Result<T> {


(Curiosity) why do these use &dyn rather than impl?

That was the optimisation made in #121101. I've not personally looked into the trade-offs though.

Binary size, see #121101.

Ah thanks, I didn't realize this existed before #138832

That was the optimisation made in #121101. I've not personally looked into the trade-offs though.

Jinxed 😄. It might be possible to improve things by splitting the string validation and copying to a separate, non-generic function but keep the stack allocation and inner function call in the generic part. Maybe I'll pursue that once this PR is merged...

tgross35 · 2025-04-11T17:54:18Z

LGTM too with a squash. Might as well do a windows sanity check

@bors try

…try> Use `with_native_path` for Windows Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts. As with the previous Unix PR (rust-lang#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes. try-job: x86_64-msvc-1

bors · 2025-04-11T17:55:30Z

⌛ Trying commit b217479 with merge 6298e6e...

Also add a WCStr type

ChrisDenton · 2025-04-11T20:48:19Z

@bors r=tgross35,joboet

bors · 2025-04-11T20:48:22Z

📌 Commit b613e97 has been approved by tgross35,joboet

It is now in the queue for this repository.

…=tgross35,joboet Use `with_native_path` for Windows Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts. As with the previous Unix PR (rust-lang#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes.

Rollup of 5 pull requests Successful merges: - rust-lang#139107 (std: make `cmath` functions safe) - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral) - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib) - rust-lang#139683 (Use `with_native_path` for Windows) - rust-lang#139691 (Document that `opt-dist` requires metrics to be enabled) r? `@ghost` `@rustbot` modify labels: rollup

fbstj · 2025-04-12T09:48:10Z

library/std/src/sys/fs/mod.rs

+    #[cfg(windows)]
+    return imp::symlink(original, link);
+    #[cfg(not(windows))]


just to double check, this has the oposite logic to the other cfg's

Yep, that's intentional. Because this is the one case where we can't (yet) simply pass through a native path to the Windows function but we can for Unix. And fortunately if I get the logic wrong here it'll be a loud error due to the signatures not matching.

thaliaarchi · 2025-04-12T13:59:11Z

library/std/src/sys/path/windows.rs

+    /// # Safety
+    ///
+    /// The slice must end in a null.
+    pub unsafe fn from_wchars_with_null_unchecked(s: &[u16]) -> &Self {


Btw, the convention in CStr is to write nul to refer to the ASCII byte and null to refer to a pointer.

It’s not worth bumping this out of the queue, but something to possibly keep in mind for your next PR in the series.

…=tgross35,joboet Use `with_native_path` for Windows Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts. As with the previous Unix PR (rust-lang#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes.

…enton Rollup of 11 pull requests Successful merges: - rust-lang#138972 (std: Fix build for NuttX targets) - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test) - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result) - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8) - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral) - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib) - rust-lang#139683 (Use `with_native_path` for Windows) - rust-lang#139695 (compiletest: consistently use `camino::{Utf8Path,Utf8PathBuf}` throughout) - rust-lang#139710 (Move `args` into `std::sys`) - rust-lang#139721 (End all lines in src/stage0 with trailing newline) - rust-lang#139726 (Move `select_unpredictable` to the `hint` module) r? `@ghost` `@rustbot` modify labels: rollup

…enton Rollup of 10 pull requests Successful merges: - rust-lang#138972 (std: Fix build for NuttX targets) - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test) - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result) - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8) - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral) - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib) - rust-lang#139683 (Use `with_native_path` for Windows) - rust-lang#139710 (Move `args` into `std::sys`) - rust-lang#139721 (End all lines in src/stage0 with trailing newline) - rust-lang#139726 (Move `select_unpredictable` to the `hint` module) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#139683 - ChrisDenton:windows-with-native, r=tgross35,joboet Use `with_native_path` for Windows Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts. As with the previous Unix PR (rust-lang#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes.

…=tgross35,joboet Use `with_native_path` for Windows Ideally, each platform should use their own native path type internally. This will, for example, allow passing a UTF-16 string directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated wide string. However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this just does some of the Windows parts. As with the previous Unix PR (rust-lang#138832) this is intended to be merely a refactoring so I've avoided anything that may require more substantial changes.

…enton Rollup of 10 pull requests Successful merges: - rust-lang#138972 (std: Fix build for NuttX targets) - rust-lang#139177 (Use -C target-cpu=z13 on s390x vector test) - rust-lang#139511 (libtest: Pass the test's panic payload as Option instead of Result) - rust-lang#139605 (update ```miniz_oxide``` to 0.8.8) - rust-lang#139618 (compiletest: Make `SUGGESTION` annotations viral) - rust-lang#139677 (Fix profiler_builtins build script to handle full path to profiler lib) - rust-lang#139683 (Use `with_native_path` for Windows) - rust-lang#139710 (Move `args` into `std::sys`) - rust-lang#139721 (End all lines in src/stage0 with trailing newline) - rust-lang#139726 (Move `select_unpredictable` to the `hint` module) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned tgross35 Apr 11, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 11, 2025

joboet approved these changes Apr 11, 2025

View reviewed changes

joboet assigned joboet and unassigned tgross35 Apr 11, 2025

tgross35 reviewed Apr 11, 2025

View reviewed changes

ChrisDenton force-pushed the windows-with-native branch from a211185 to b217479 Compare April 11, 2025 17:49

tgross35 approved these changes Apr 11, 2025

View reviewed changes

Use with_native_path for Windows

b613e97

Also add a WCStr type

ChrisDenton force-pushed the windows-with-native branch from b217479 to b613e97 Compare April 11, 2025 18:02

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 Apr 11, 2025

jieyouxu mentioned this pull request Apr 11, 2025

Rollup of 5 pull requests #139694

Closed

fbstj reviewed Apr 12, 2025

View reviewed changes

thaliaarchi reviewed Apr 12, 2025

View reviewed changes

ChrisDenton mentioned this pull request Apr 13, 2025

Rollup of 11 pull requests #139742

Closed

ChrisDenton mentioned this pull request Apr 13, 2025

Rollup of 10 pull requests #139746

Merged

bors merged commit 8a6d6f5 into rust-lang:master Apr 13, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 13, 2025

ChrisDenton deleted the windows-with-native branch April 13, 2025 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `with_native_path` for Windows #139683

Use `with_native_path` for Windows #139683

ChrisDenton commented Apr 11, 2025 •

edited

Loading

rustbot commented Apr 11, 2025

joboet commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

joboet left a comment

tgross35 Apr 11, 2025

ChrisDenton Apr 11, 2025

joboet Apr 11, 2025

tgross35 Apr 11, 2025

joboet Apr 11, 2025

tgross35 commented Apr 11, 2025

bors commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

bors commented Apr 11, 2025

fbstj Apr 12, 2025

ChrisDenton Apr 12, 2025

thaliaarchi Apr 12, 2025

Use with_native_path for Windows #139683

Use with_native_path for Windows #139683

Conversation

ChrisDenton commented Apr 11, 2025 • edited Loading

rustbot commented Apr 11, 2025

joboet commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

joboet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 commented Apr 11, 2025

bors commented Apr 11, 2025

ChrisDenton commented Apr 11, 2025

bors commented Apr 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `with_native_path` for Windows #139683

Use `with_native_path` for Windows #139683

ChrisDenton commented Apr 11, 2025 •

edited

Loading