Optimize `PathBuf::push` for verbatim paths by ChrisDenton · Pull Request #162490 · rust-lang/rust

ChrisDenton · 2026-09-08T16:46:57Z

Collecting all the parts is unnecessary.

The added tests are there to test that existing behaviour is kept. This PR does not change the behaviour of push,

rustbot · 2026-09-08T16:47:02Z

rustbot has assigned @Darksonn.
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

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: @ChrisDenton, libs
@ChrisDenton, libs expanded to 13 candidates
Random selection from Darksonn, JohnTitor, Mark-Simulacrum

Darksonn

@rustbot author

View changes since this review

Darksonn · 2026-09-08T21:04:54Z

+        tp!(r"\\?\C:\foo\bar", "../baz", r"\\?\C:\foo\baz");
+        tp!(r"\\?\C:\foo\bar\", "../baz", r"\\?\C:\foo\baz");
+        tp!(r"\\?\C:\foo\bar\", "..", r"\\?\C:\foo");


What about pushing .. to \\?\C:\foo, going all the way to the root? Or even pushing .. to \\?\C:\? What about pushing .. to \\?\C:\foo\..?

What about the UNC share case? What if you push .. to \\?\UNC\server\share\foo or \\?\UNC\server\share\?

What about pushing .. to \\?\C:\foo, going all the way to the root? Or even pushing .. to \\?\C:\? What about pushing .. to \\?\C:\foo\..?

Aren't some these cases not handled above and below? I can add some more though.

What about the UNC share case? What if you push .. to \\?\UNC\server\share\foo or \\?\UNC\server\share\?

Hm, looking through the tests it seems we don't have as good coverage of \\?\UNC paths as I thought. I'll add some.

Done. There were basically two cases where the old and new version differed:

When a root ended with a \ (e.g. \\?\C:\) the old version always preserved the \ but the new version didn't (e.g. \\?\C:). I think either is arguable for verbatim paths but I'll side with keep the existing behaviour.

In the old version a verbatim path ending with .. was not removed at all. So pushing .. to \\?\C:\foo\.. resulted in \\?\C:\foo\... I cannot see how that could be right, especially as it didn't have a test case. So I think fixing it is the right thing to do.

In any case I've split the commits into two so you can see what changed more easily.

rustbot · 2026-09-08T23:22:10Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Darksonn

Are we really sure that this optimization is a good idea? It's pretty error-prone.

@rustbot author

View changes since this review

Darksonn · 2026-09-09T06:33:53Z

+        tp!(r"\\?\C:\foo\bar\", "..", r"\\?\C:\foo");
+        tp!(r"\\?\C:\foo", "..", r"\\?\C:\");
+        tp!(r"\\?\C:\", "..", r"\\?\C:\");
+        tp!(r"\\?\C:\foo\..", "..", r"\\?\C:\foo");


This doesn't make sense. Resolving the paths, you're starting at the root \\?\C:\ (except written slightly funny as \\?\C:\foo\..), then you push .., and now you are in a sub-directory of the root \\?\C:\foo.

Surely .. should never navigate to a sub-directory.

This is confusing but for verbatim paths, the .. in \\?\C:\foo\.. is a literal directory name so it's equivalent to something like \\?\C:\foo\bar. It is intended that there's a special case for pushing .. so that it behaves like a pop for verbatim paths.

I'm not sure, but I think we should stick to keeping existing behavior in this PR. If you think this behavior is wrong, can we fix that in a separate and more targeted PR?

I just seems buggy to me that PathBuf::from(r"\\?\C:\foo\..").push(r"..\..\..\bar") gives the path \\?\C:\foo\..\bar. But I can try to recreate it.

ChrisDenton · 2026-09-09T09:03:46Z

Are we really sure that this optimization is a good idea? It's pretty error-prone.

I think the previous implementation was not quite working as intended either but that aside, it's very slow. Every single component you push required collecting a new Vec of all the components in the base path and the base path would grow on every iteration. When pushing long paths this could get very expensive.

Darksonn · 2026-09-09T09:05:02Z

If the generic implementation is O(n^2), could we fix that rather than add a special case that skips it?

ChrisDenton · 2026-09-09T09:22:32Z

I mean it was already a 40 line special case, no? It was using OsString::push directly and not the generic PathBuf::push. I'm not sure that can be rescued without rewriting it into something else because we want to avoid the initial Vec and we want to reuse the underlying PathBuf allocation instead of making a new OsString.

Darksonn · 2026-09-09T09:30:03Z

Ok, I didn't catch this was entirely replacing the O(n^2) logic

Darksonn · 2026-09-09T09:38:23Z

Could we potentially treat the path as non-verbatim when it contains an .. component?

ChrisDenton · 2026-09-09T12:17:59Z

Could we potentially treat the path as non-verbatim when it contains an .. component?

I like the idea that how the path is handled is decided by its prefix alone. And I'm not too keen on needing to scan the entire path on every push.

In any case I've now updated it so there's no behaviour change in this PR.

Darksonn

Since Path::components doesn't allocate, could we make use of it here? For instance, invoke self.components().next_back() to check whether the trailing component is Component::Normal(_), instead of the current ad-hoc check.

@rustbot author

View changes since this review

Darksonn · 2026-09-09T12:40:21Z

-                        if let Some(Component::Normal(_)) = buf.last() {
-                            buf.pop();
+                        // Preserve pre-existing behaviour of not going above any literal `..` in the base path.
+                        if self.inner.as_encoded_bytes().trim_suffix(b"\\").ends_with(b"\\..") {


This doesn't handle repeated slashes, so pushing .. to a path like \\?\C:\foo\..\\ still changes behavior.

Hm, empty path components in verbatim paths are not valid. I'm not sure that we particularly need to preserve the behaviour in such cases? Or at least I worry about specifying it.

Darksonn · 2026-09-09T12:42:29Z

+                        // Preserve pre-existing behaviour of not going above any literal `..` in the base path.
+                        if self.inner.as_encoded_bytes().trim_suffix(b"\\").ends_with(b"\\..") {


The pre-existing behavior is to only go up for normal components, and .. is not the only non-normal component. So for example, \\?\C:\foo\. will remain unchanged when pushing .. on main, but this PR changes it to remove the trailing CurDir component.

Darksonn · 2026-09-09T12:44:51Z

-                            buf.pop();
+                        // Preserve pre-existing behaviour of not going above any literal `..` in the base path.
+                        if self.inner.as_encoded_bytes().trim_suffix(b"\\").ends_with(b"\\..") {
+                            continue;


When you hit this continue, no changes are made to the existing path, but the code on main will remove trailing slashes. So for example, you changed the behavior of pushing .. to \\?\C:\foo\..\.

Darksonn · 2026-09-09T12:48:54Z

+                        // Manually implement a simplified version of pop optimized for verbatim paths.
+                        let mut iter = self.inner.as_encoded_bytes()[prefix_len..].iter();
+                        iter.rfind(|&&b| b != sep_byte);
+                        let pos = iter.rposition(|&b| b == sep_byte).unwrap_or(0);


This use of rposition means that pushing .. to \\?\C:\foo\\\\bar will return \\?\C:\foo\\\, whereas on main this will return \\?\C:\foo on main.

ChrisDenton · 2026-09-09T15:14:59Z

I rewrote the Component::ParentDir branch to be in terms of self.components. Thus it should preserve existing behaviour.

ChrisDenton · 2026-09-10T00:58:52Z

Ok, I've rewritten it to use pre-existing functions instead of hand coding it. I've also changed it to match the existing behaviour, though I do find that behaviour to be questionable.

Darksonn

I do agree that the current behavior is somewhat questionale, but if we're changing behavior I think we need to libs-nominate it to check that the libs team is ok with the change.

Process-wise, it's easier to do that by making no behavior changes here, and then changing it in a follow-up PR, because that follow-up PR can then show a diff where the behaviors that are changed show up as modifications to the tests.

View changes since this review

Darksonn · 2026-09-10T08:46:44Z

We're still changing the behavior when there are internal double slashes. For example:

PathBuf::from(r"\\?\C:\foo\\").push("bar")

PathBuf::from(r"\\?\C:\foo\\\\bar").push("baz")

I've added back iterating over the full base path to fixup consecutive \ but without allocating, which does require a way to get a mutable slice from an OsStr.

I think you can use mem::take on the PathBuf, then convert it to a vector, then use retain, then convert it back.

ChrisDenton · 2026-09-11T15:14:40Z

I created #162643 to fix issues with the trailing sep methods when used with verbatim paths.

Darksonn

I now agree that this does not change behavior. A few nits below, but otherwise LGTM.

View changes since this review

Darksonn · 2026-09-12T08:15:46Z

+            // collapse multiple verbatim separators in the base path.
+            let mut i = prefix_len;
+            while i < self.inner.len() {
+                // SAFETY: we're only removing the platform's separator characters
+                let bytes = &mut unsafe { self.inner.as_bytes_mut() }[i..];
+                let pos = bytes.iter().position(|&b| !is_verbatim_sep(b)).unwrap_or(bytes.len());
+                if pos > 1 {
+                    bytes.copy_within(pos.., 1);
+                    let new_len = i + (bytes.len() - pos) + 1;
+                    self.inner.truncate(new_len);
+                }
+                i += 1;
+            }


Might be slightly simpler if you convert the path into a vector and use retain.

let mut skip = prefix_len; let mut prev_sep = false; vector.retain_mut(|&mut v| { if skip > 0 { skip -= 1; return true; } let cur_sep = is_verbatim_sep(v); let is_duplicate = prev_sep && cur_sep; prev_sep = cur_sep; !is_duplicate });

Darksonn · 2026-09-12T08:16:53Z

        self.inner.eq_ignore_ascii_case(&other.inner)
    }
+
+    pub unsafe fn as_mut_slice(&self) -> &[u8] {


Only used on a tier 3 target, but this does not compile.

Suggested change

pub unsafe fn as_mut_slice(&self) -> &[u8] {

pub unsafe fn as_bytes_mut(&self) -> &[u8] {

Darksonn · 2026-09-12T08:22:26Z

I think the test coverage is missing cases starting with Verbatim("") such as \\?\\foo (as opposed to \\?\foo) where the double \\ right before foo must not be deleted by our loop.

rustbot assigned Darksonn Sep 8, 2026

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 Sep 8, 2026

Darksonn reviewed Sep 8, 2026

View reviewed changes

rustbot 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 Sep 8, 2026

ChrisDenton force-pushed the optimize-path-push branch from 3ea9190 to 7407b98 Compare September 8, 2026 23:22

Darksonn reviewed Sep 9, 2026

View reviewed changes

ChrisDenton force-pushed the optimize-path-push branch from 7407b98 to 7f6df80 Compare September 9, 2026 09:00

ChrisDenton force-pushed the optimize-path-push branch from 7f6df80 to 4223d10 Compare September 9, 2026 12:13

Darksonn reviewed Sep 9, 2026

View reviewed changes

ChrisDenton force-pushed the optimize-path-push branch from 4223d10 to 6306019 Compare September 9, 2026 15:13

ChrisDenton force-pushed the optimize-path-push branch 3 times, most recently from 6691859 to 42ee54e Compare September 10, 2026 00:55

Darksonn reviewed Sep 10, 2026

View reviewed changes

Add more PathBuf::push tests for verbatim paths

cc08cc7

ChrisDenton force-pushed the optimize-path-push branch from 42ee54e to 059b100 Compare September 10, 2026 21:49

This comment has been minimized.

Sign in to view

Optimize PathBuf::push for verbatim paths

2d4df66

ChrisDenton force-pushed the optimize-path-push branch from 059b100 to 2d4df66 Compare September 10, 2026 22:01

Darksonn reviewed Sep 12, 2026

View reviewed changes

		// Preserve pre-existing behaviour of not going above any literal `..` in the base path.
		if self.inner.as_encoded_bytes().trim_suffix(b"\\").ends_with(b"\\..") {

	pub unsafe fn as_mut_slice(&self) -> &[u8] {
	pub unsafe fn as_bytes_mut(&self) -> &[u8] {

Uh oh!

Uh oh!

Conversation

ChrisDenton commented Sep 8, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 8, 2026

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 8, 2026

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Darksonn Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisDenton commented Sep 9, 2026

Uh oh!

Darksonn commented Sep 9, 2026

Uh oh!

ChrisDenton commented Sep 9, 2026

Uh oh!

Darksonn commented Sep 9, 2026

Uh oh!

Darksonn commented Sep 9, 2026

Uh oh!

ChrisDenton commented Sep 9, 2026

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisDenton commented Sep 9, 2026

Uh oh!

ChrisDenton commented Sep 10, 2026

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisDenton Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

ChrisDenton commented Sep 8, 2026 •

edited by rustbot

Loading

Darksonn left a comment •

edited by rustbot

Loading

Darksonn left a comment •

edited by rustbot

Loading

Darksonn Sep 9, 2026 •

edited

Loading

Darksonn left a comment •

edited by rustbot

Loading

Darksonn left a comment •

edited by rustbot

Loading

ChrisDenton Sep 10, 2026 •

edited

Loading

Darksonn left a comment •

edited by rustbot

Loading