Skip to content

Make two Paths unequal if they differ in trailing slash #87339

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

Closed
dtolnay wants to merge 2 commits into from
Closed

Make two Paths unequal if they differ in trailing slash #87339

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d628f76
Make two Paths unequal if they differ in trailing slash
dtolnay Jul 21, 2021
b2e3862
Take into account trailing /. as referring to directory
dtolnay Oct 26, 2021
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
18 changes: 14 additions & 4 deletions library/std/src/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1738,7 +1738,7 @@ impl ToOwned for Path {
impl cmp::PartialEq for PathBuf {
#[inline]
fn eq(&self, other: &PathBuf) -> bool {
self.components() == other.components()
Path::eq(self, other)
}
}

Expand All @@ -1756,15 +1756,15 @@ impl cmp::Eq for PathBuf {}
impl cmp::PartialOrd for PathBuf {
#[inline]
fn partial_cmp(&self, other: &PathBuf) -> Option<cmp::Ordering> {
Some(compare_components(self.components(), other.components()))
Path::partial_cmp(self, other)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl cmp::Ord for PathBuf {
#[inline]
fn cmp(&self, other: &PathBuf) -> cmp::Ordering {
compare_components(self.components(), other.components())
Path::cmp(self, other)
}
}

Expand Down Expand Up @@ -2718,6 +2718,14 @@ impl Path {
let inner = unsafe { Box::from_raw(rw) };
PathBuf { inner: OsString::from(inner) }
}

fn ends_with_separator(&self) -> bool {
let components = self.components();
match components.path.last() {
None => false,
Some(byte) => components.is_sep_byte(*byte),
}
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -2779,6 +2787,7 @@ impl cmp::PartialEq for Path {
#[inline]
fn eq(&self, other: &Path) -> bool {
self.components() == other.components()
&& self.ends_with_separator() == other.ends_with_separator()
Copy link
Member

@the8472 the8472 Jul 21, 2021

Choose a reason for hiding this comment

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

Shouldn't this be implemented on the Components level instead of Path? Otherwise they'll give different results.

Copy link
Member Author

@dtolnay dtolnay Sep 28, 2021

Choose a reason for hiding this comment

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

Giving different results is correct. The path components are the same i.e. the Components iterator would produce the identical sequence of Component enum values, even if one path ends in separator and the other does not.

Copy link
Member

@the8472 the8472 Sep 28, 2021

Choose a reason for hiding this comment

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

If we say that a trailing slash is semantically different then shouldn't components preserve that semantic difference? E.g. the last component could include the trailing slash. Or there could be a final zero-length component. Ideally this would be expressed through a new enum variant but it's exhaustive so we can't extend it 🙁.

Is assert_eq!(foo, foo.components().as_path()) always true?

Copy link
Member Author

@dtolnay dtolnay Oct 27, 2021

Choose a reason for hiding this comment

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

I don't think it follows that we need components to preserve that semantic difference.

It's often the case that code knows what kind of filesystem object it's expecting to manipulate, whether all files like a tests/ui/*.stderr glob expansion in the trybuild crate, or all dirs like ~/.rustup/toolchains/*. I don't see throwing an "empty" path component onto the end of the Components iterator in the dir case being helpful or desirable in the common case. The directoryness comes through based on what the code does with the paths after examining their components, such as using std::fs::remove_dir on it rather than remove_file.

Copy link
Member

@the8472 the8472 Nov 2, 2021

Choose a reason for hiding this comment

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

I don't think it follows that we need components to preserve that semantic difference.

Well... the thing is that Components implements AsRef<Path>, so if you have a generic function taking it via AsRef then it'll behave differently depending on whether you pass in a Components or something else that implements AsRef<Path>.

It's consistent with existing documentation, it's clearly spelled out that path.components() will strip trailing slashes, but previously that wasn't considered a semantic difference (since Eq considered them equal), now it is.

}
}

Expand All @@ -2798,7 +2807,7 @@ impl cmp::Eq for Path {}
impl cmp::PartialOrd for Path {
#[inline]
fn partial_cmp(&self, other: &Path) -> Option<cmp::Ordering> {
Some(compare_components(self.components(), other.components()))
Some(Ord::cmp(self, other))
}
}

Expand All @@ -2807,6 +2816,7 @@ impl cmp::Ord for Path {
#[inline]
fn cmp(&self, other: &Path) -> cmp::Ordering {
compare_components(self.components(), other.components())
.then_with(|| self.ends_with_separator().cmp(&other.ends_with_separator()))
}
}

Expand Down
38 changes: 33 additions & 5 deletions library/std/src/path/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,7 +1427,11 @@ pub fn test_compare() {
let eq = path1 == path2;
assert!(eq == $eq, "{:?} == {:?}, expected {:?}, got {:?}",
$path1, $path2, $eq, eq);
assert!($eq == (hash(path1) == hash(path2)),

// They have the same hash if the paths are equal or differ only in
// trailing separator.
assert_eq!($eq || $path1.trim_end_matches('/') == $path2.trim_end_matches('/'),
hash(path1) == hash(path2),
"{:?} == {:?}, expected {:?}, got {} and {}",
$path1, $path2, $eq, hash(path1), hash(path2));

Expand Down Expand Up @@ -1480,7 +1484,7 @@ pub fn test_compare() {
);

tc!("foo/", "foo",
eq: true,
eq: false,
starts_with: true,
ends_with: true,
relative_from: Some("")
Expand Down Expand Up @@ -1532,6 +1536,27 @@ pub fn test_compare() {
}
}

#[test]
fn test_trailing_separator() {
let file = Path::new("tmp/f");
let dir = Path::new("tmp/f/");

assert_ne!(file, dir);
assert!(file < dir);
assert_eq!(dir, Path::new("tmp/f//"));
assert_eq!(file.components(), dir.components());

if cfg!(windows) {
assert_eq!(dir, Path::new(r"tmp\f\"));
assert_eq!(dir, Path::new(r"tmp\f\\"));
}

let file = file.to_owned();
let dir = dir.to_owned();
assert_ne!(file, dir);
assert!(file < dir);
}

#[test]
fn test_components_debug() {
let path = Path::new("/tmp");
Expand Down Expand Up @@ -1629,10 +1654,13 @@ fn test_ord() {
ord!(Less, "1", "2");
ord!(Less, "/foo/bar", "/foo./bar");
ord!(Less, "foo/bar", "foo/bar.");
ord!(Equal, "foo/./bar", "foo/bar/");
ord!(Equal, "foo/bar", "foo/bar/");
ord!(Equal, "foo/./bar", "foo/bar");
ord!(Less, "foo/./bar", "foo/bar/");
ord!(Equal, "foo/./bar/", "foo/bar/");
ord!(Less, "foo/bar", "foo/bar/");
ord!(Equal, "foo/bar", "foo/bar/.");
ord!(Equal, "foo/bar", "foo/bar//");
ord!(Less, "foo/bar", "foo/bar//");
ord!(Equal, "foo/bar/", "foo/bar//");
}

#[bench]
Expand Down