Make two Paths unequal if they differ in trailing slash

[dtolnay]

It seems problematic to consider two paths equal if one of them can exist when the other does not exist.

In the previous behavior, the following asserts pass:

use std::path::Path;

fn main() {
    let path1 = Path::new("/etc/passwd");
    let path2 = Path::new("/etc/passwd/");

    assert_eq!(path1, path2);

    assert!(path1.metadata().is_ok());

    // Err: Os { code: 20, kind: Other, message: "Not a directory" }
    assert!(path2.metadata().is_err());
}

In this PR path1 == path2 becomes false.

[rust-highfive]

r? @yaahc

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

[the8472]

Couldn't this break some use-cases? E.g. shells autocomplete directories with a trailing slash, if a CLI tool then tries to deduplicate paths or exclude paths by comparing them to another set of paths that's derived in some other way that doesn't append the trailing slash they might get a false negative compared to previous behavior.

At the very least it should be documented and have a release note.

[the8472]

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

[dtolnay]

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.

[the8472]

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?

[dtolnay]

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.

[the8472]

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.

[bors]

☔ The latest upstream changes (presumably #86898) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

merge conflicts

[dtolnay]

• Resolved merge conflict with Add fast path for Path::cmp that skips over long shared prefixes #86898

@rustbot label -S-waiting-on-author +S-waiting-on-review

[yaahc]

Part of me feels like it would be nice to have eq work regardless of the trailing slash if the file being named is a directory, but I feel like we almost certainly don't want to be interacting with filesystem APIs to see if paths exist and are dirs when checking equality. So with all of that said this PR looks good to me.

I'm going to FCP this change since it changes runtime behavior.

[yaahc]

@rfcbot merge

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[yaahc]

I do feel like we should resolve the two open comment threads before merging this

[dtolnay]

Responded above.
@rustbot label -S-waiting-on-author +S-waiting-on-review

[yaahc]

@the8472 are you satisfied by @dtolnay's explanations?

[the8472]

The ordering seems fine now.

I still think the newly introduced semantic inconsistency between Path and Components is unfortunate, but at least components() does mention it is normalizing.

Maybe it just needs to be documented somewhere that Path considers a trailing slash semantically meaningful while Components does not.

[yaahc]

Maybe it just needs to be documented somewhere that Path considers a trailing slash semantically meaningful while Components does not.

Sounds good, lets do this in a follow up PR?

@bors r+

[bors]

📌 Commit b2e3862 has been approved by yaahc

[bors]

⌛ Testing commit b2e3862 with merge 44a95033fa2615ffab1182da151fa3ed531eced3...

[rust-log-analyzer]

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

test lto::cdylib_and_rlib ... ok

failures:

---- fix::fix_in_existing_repo_weird_ignore stdout ----
running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo fix`
thread 'fix::fix_in_existing_repo_weird_ignore' panicked at '
test failed running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo fix`
error: process exited with code 101 (expected 0)

--- stderr
--- stderr
error: no VCS found for this package and `cargo fix` can potentially perform destructive changes; if you'd like to suppress this error pass `--allow-no-vcs`
', src/tools/cargo/tests/testsuite/fix.rs:1403:20


failures:
    fix::fix_in_existing_repo_weird_ignore
---
expected success, got: exit status: 101


Build completed unsuccessfully in 0:24:36
make: *** [check-aux] Error 1
Makefile:44: recipe for target 'check-aux' failed

[bors]

💔 Test failed - checks-actions

[matthiaskrgr]

@bors r- (bors having hiccup and trying to retest failed prs)

[dtolnay]

I think upon reflection I am going to retract this for being more disruptive than I would have wanted.

An example of code that's negatively affected is this snippet from cargo where it's testing whether path happens to be the root of a git repo:

    if let Ok(repo) = git2::Repository::discover(path) {
        if repo.workdir().map_or(false, |workdir| workdir == path) {

It turns out path comes originally from std::env::current_dir with no trailing slash on my machine, while git2's Repository::workdir always comes back with a trailing slash. Thus the == comparison could now never be true.

I guess code that wants the non-equal behavior should use a newtype with PartialEq and PartialOrd which mirror the syntactically_dir logic from this PR.