stdlib Path has inconsistent normalisation behaviour

[aidanhs]

Given:

use std::path::{Path, PathBuf};

fn main() {
    let mut p = PathBuf::from("/a/b/c");
    println!("{:?} {:?}", p, p.file_name());
    p.push("");
    println!("{:?} {:?}", p, p.file_name());
    p.push("");
    println!("{:?} {:?}", p, p.file_name());
    p.pop();
    println!("{:?} {:?}", p, p.file_name());

    p = PathBuf::from("/");
    println!("{:?} {:?}", p, p.file_name());
    p.push("");
    println!("{:?} {:?}", p, p.file_name());
    p.push("");
    println!("{:?} {:?}", p, p.file_name());
    p.pop();
    println!("{:?} {:?}", p, p.file_name());
}

You get:

"/a/b/c" Some("c")
"/a/b/c/" Some("c")
"/a/b/c/" Some("c")
"/a/b" Some("b")
"/" None
"/" None
"/" None
"/" None

http://is.gd/XbyUdj

First, the documentation is misleading (wrong?). file_name (https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.file_name) is documented as "The final component of the path, if it is a normal file.", but it's returning the directory when there's a trailing slash, which is not (by my book) a normal file?
Looking at the implementation of components it will return the final component of the path unless it is the root directory or a relative component. Maybe it just needs clarifying.

There's a stranger issue with normalisation of trailing slashes. Initially I thought it should be documented (https://doc.rust-lang.org/std/path/index.html#normalization), because trailing slashes are ignored by components(). However, unlike any other normalisation, it is possible to reconstruct the original path (by doing .push(""))!

I'm unsure what the intended behaviour is here and what needs fixing (implementation or docs).
Given stabilisation, I'm going to assume that implemented behaviour wins at this point? It's slightly irritating because being able to detect a trailing slash can be useful (as rsync does). Failing that I might be tempted to make .push("") do nothing - this uncertain state with trailing slashes is strange. Is there an RFC I can look at maybe?

[aidanhs]

use std::path::PathBuf;
fn main() {
    let mut p4 = PathBuf::from("/a/b/c/..");
    println!("{:?} {:?}", p4, p4.file_name());
    println!("pop: {}", p4.pop());
    println!("{:?} {:?}", p4, p4.file_name());
}

Result:

"/a/b/c/.." None
pop: true
"/a/b/c" Some("c")

http://is.gd/E1YHHm

This one contradicts the docs for pop https://doc.rust-lang.org/std/path/struct.PathBuf.html#method.pop

Truncate self to self.parent().

Returns false and does nothing if self.file_name() is None. Otherwise, returns true.

file_name was None but it did something. Could do with looking at the most recent path RFC or whatever then I can start fixing.

[alexcrichton]

cc @aturon

[aidanhs]

This one surprised me a lot (and wasted a fair amount of time)!

use std::path::Path;
use std::collections::HashSet;
fn main() {
    let p1 = Path::new("/a/b/");
    let p2 = Path::new("/a/b/");
    let p3 = Path::new("/a/b");
    println!("{} {} {}", p1 == p2, p2 == p3, p1 == p3);
    let mut hs = HashSet::new();
    hs.insert(p1);
    hs.insert(p2);
    hs.insert(p3);
    println!("{}", hs.len());
}

http://is.gd/yli8ls
Result:

true true true
2

[retep998]

I'm pretty sure it's a bug if the implementation of Hash doesn't match the behavior of Eq (note that floating point numbers don't implement Eq, only PartialEq). At the same time changing this behavior will likely break existing code, so it's a lose-lose situation either way.

[aidanhs]

I confess, I've not found the edge-case handling of the Path library entirely intuitive (as you can probably tell from this issue).
Unfortunately, a fix for any of these problems will probably cause subtle bugs, arguably worse than the memcpy (or whatever it was) argument reordering because it'll just cause different results rather than crashes.

I'm secretly a bit pleased about the Hash vs Eq thing because it seems like the kind of thing that absolutely requires some change to be made, at which point some of the other backwards-incompatible complaints could be addressed.

[alexcrichton]

triage: I-nominated

[alexcrichton]

triage: P-high

[alexcrichton]

seems like we should fix the Hash/Eq situation at the very least