Skip to content

Optimize PathBuf::push for verbatim paths #162490

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.

Sign up for GitHub

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

Open
ChrisDenton wants to merge 2 commits into
base: main
Choose a base branch
from
+102 −29
Open

Optimize PathBuf::push for verbatim paths #162490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab1d5e4
Add more PathBuf::push tests for verbatim paths
ChrisDenton Sep 8, 2026
f782871
Optimize PathBuf::push for verbatim paths
ChrisDenton Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 55 additions & 29 deletions library/std/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
#![deny(unsafe_op_in_unsafe_fn)]

use core::clone::CloneToUninit;
use core::mem;

use crate::borrow::{Borrow, Cow};
use crate::collections::TryReserveError;
Expand Down Expand Up @@ -1344,7 +1345,7 @@ impl PathBuf {
let mut need_sep = buf.last().map(|c| !is_sep_byte(*c)).unwrap_or(false);

// in the special case of `C:` on Windows, do *not* add a separator
let comps = self.components();
let mut comps = self.components();

if comps.prefix_len() > 0
&& comps.prefix_len() == comps.path.len()
Expand All @@ -1367,45 +1368,70 @@ impl PathBuf {
self.inner.clear();

// verbatim paths need . and .. removed
} else if comps.prefix_verbatim() && !path.inner.is_empty() {
let mut buf: Vec<_> = comps.collect();
} else if comps.prefix_verbatim() && !path.is_empty() {
let mut prefix_len = comps.prefix_len();
let has_root_dir = comps.nth(1) == Some(Component::RootDir);

// collapse multiple verbatim separators in the base path.
let mut temp = mem::take(self);
let mut vector = temp.into_os_string().into_encoded_bytes();
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
});
// SAFETY: we've only removed the platform's separator characters
temp = unsafe { OsString::from_encoded_bytes_unchecked(vector) }.into();
*self = mem::take(&mut temp);

if has_root_dir {
prefix_len += 1;
}
for c in path.components() {
match c {
// RootDir can only ever appear once.
Component::RootDir => {
buf.truncate(1);
buf.push(c);
self.inner.truncate(prefix_len);
if !has_root_dir {
self.inner.push(MAIN_SEPARATOR_STR);
prefix_len += 1;
}
}
Component::CurDir => (),
Component::CurDir => self.pop_trailing_sep(),
Component::ParentDir => {
if let Some(Component::Normal(_)) = buf.last() {
buf.pop();
let mut components = self.components();
// Preserve pre-existing behaviour of stopping at any non-normal component in the base path.
match components.next_back() {
Some(Component::Normal(_)) => {
self.pop();
if self.components().next_back() == Some(Component::RootDir) {
self.inner.truncate(prefix_len);
}
}
Some(Component::CurDir | Component::ParentDir) => {
self.pop_trailing_sep()
}
_ => {}
}
}
_ => buf.push(c),
}
}

let mut res = OsString::new();
let mut need_sep = false;

for c in buf {
if need_sep && c != Component::RootDir {
res.push(MAIN_SEPARATOR_STR);
}
res.push(c.as_os_str());

need_sep = match c {
Component::RootDir => false,
Component::Prefix(prefix) => {
!prefix.parsed.is_drive() && prefix.parsed.len() > 0
_ => {
// FIXME: We can't use `push_trailing_sep` here due to a bug
// that treats `/` as a separator when pushing.
if !self.has_trailing_sep() || self == r"\\?\" {
self.inner.push(MAIN_SEPARATOR_STR);
}
self.inner.push(c);
}
_ => true,
}
}

self.inner = res;
return;

// `path` has a root but no prefix, e.g., `\windows` (Windows only)
} else if path.has_root() {
let prefix_len = self.components().prefix_remaining();
Expand Down
47 changes: 47 additions & 0 deletions library/std/tests/path.rs
View file
Open in desktop

@Darksonn Darksonn Sep 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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")

@ChrisDenton ChrisDenton Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@Darksonn Darksonn Sep 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread
ChrisDenton marked this conversation as resolved.

@Darksonn Darksonn Sep 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -1759,13 +1759,60 @@ pub fn test_push() {

tp!(r"\\?\C:\bar", "../foo", r"\\?\C:\foo");
tp!(r"\\?\C:\bar", "../../foo", r"\\?\C:\foo");
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");
Comment on lines +1762 to +1764

@Darksonn Darksonn Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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\?

@ChrisDenton ChrisDenton Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@ChrisDenton ChrisDenton Sep 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

tp!(r"\\?\C:\foo", "..", r"\\?\C:\");
tp!(r"\\?\C:\", "..", r"\\?\C:\");
tp!(r"\\?\C:\foo\..", "..", r"\\?\C:\foo\..");
tp!(r"\\?\C:\", "../foo", r"\\?\C:\foo");
tp!(r"\\?\C:", r"D:\foo/./", r"D:\foo/./");
tp!(r"\\?\C:", r"\\?\D:\foo\.\", r"\\?\D:\foo\.\");
tp!(r"\\?\C:", r"\..", r"\\?\C:\");
tp!(r"\\?\C:", r"\\foo\\\..", r"\\?\C:\");
tp!(r"\\?\C:", r"foo\foo\..\..", r"\\?\C:");
tp!(r"\\?\C:\foo\..\..\bar", r"..\..\..\", r"\\?\C:\foo\..\..");
tp!(r"\\?\C:\foo\..", r"..\bar", r"\\?\C:\foo\..\bar");
tp!(r"\\?\C:\foo\..\..", r"..\bar", r"\\?\C:\foo\..\..\bar");
tp!(r"\\?\C:\foo\..", r"..\..\..\bar", r"\\?\C:\foo\..\bar");
tp!(r"\\?\C:\foo\..\", r"..\..\..\bar", r"\\?\C:\foo\..\bar");
tp!(r"\\?\C:\foo\..\\", r"..", r"\\?\C:\foo\..");
tp!(r"\\?\C:\foo\.", r"..", r"\\?\C:\foo\.");
tp!(r"\\?\C:\foo\..\", r"..", r"\\?\C:\foo\..");
tp!(r"\\?\C:\foo\\\\bar", r"..", r"\\?\C:\foo");
tp!(r"\\?\C:\foo\", r".", r"\\?\C:\foo");
tp!(r"\\?\C:\foo\\", r"bar", r"\\?\C:\foo\bar");
tp!(r"\\?\C:\foo\\\\bar", r"baz", r"\\?\C:\foo\bar\baz");
tp!(r"\\?\A:\x\y", "/foo", r"\\?\A:\foo");
tp!(r"\\?\A:", r"..\foo\.", r"\\?\A:\foo");
tp!(r"\\?\A:\x\y", r".\foo\.", r"\\?\A:\x\y\foo");
tp!(r"\\?\A:\x\y", r"", r"\\?\A:\x\y\");

tp!(r"\\?\UNC\server\share\foo", r"..", r"\\?\UNC\server\share\");
tp!(r"\\?\UNC\server\share\", r"..", r"\\?\UNC\server\share\");
tp!(r"\\?\UNC\server\share\foo\bar", "../baz", r"\\?\UNC\server\share\foo\baz");
tp!(r"\\?\UNC\server\share\foo\bar\", "../baz", r"\\?\UNC\server\share\foo\baz");
tp!(r"\\?\UNC\server\share\foo\bar\", "..", r"\\?\UNC\server\share\foo");
tp!(r"\\?\UNC\server\share\foo", "..", r"\\?\UNC\server\share\");
tp!(r"\\?\UNC\server\share\", "..", r"\\?\UNC\server\share\");
tp!(r"\\?\UNC\server\share\foo\..", "..", r"\\?\UNC\server\share\foo\..");
tp!(r"\\?\UNC\server\share\", "../foo", r"\\?\UNC\server\share\foo");
tp!(r"\\?\UNC\server\share", r"D:\foo/./", r"D:\foo/./");
tp!(r"\\?\UNC\server\share", r"\\?\D:\foo\.\", r"\\?\D:\foo\.\");
tp!(r"\\?\UNC\server\share", r"\..", r"\\?\UNC\server\share\");

tp!(r"\\?\PIPE\foo", r"\", r"\\?\PIPE\");

// Empty verbatim prefix.
tp!(r"\\?\\foo", r"..", r"\\?\\");
tp!(r"\\?\\foo", r"../..", r"\\?\\");
tp!(r"\\?\\foo", r"\", r"\\?\\");
tp!(r"\\?\\foo", r"../bar", r"\\?\\bar");

tp!(r"\\?\C:\foo\foo\/", r"foo", r"\\?\C:\foo\foo\/\foo");
tp!(r"\\?\/", r"/", r"\\?\/\");
tp!(r"\\?\foo/", r"/", r"\\?\foo/\");
tp!(r"\\?\", r"foo\foo\..\..", r"\\?\");
}
}

Expand Down
Loading