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

[aturon]

Thanks @alexcrichton for taking care of the hash issue -- it's an interesting footgun with derive.

Regarding the semantic/normalization issues: there's definitely disagreement with the docs here, which is bad (and indicates insufficient unit testing). The question about trailing / is particularly interesting, since we explored a few different normalization approaches and ended up only being sensitive to a (semantic) extra . in the front of the path (see https://doc.rust-lang.org/std/path/index.html#normalization).

In any case, I agree that the behavior here is counterintuitive and there are cases where you want trailing / sensitivity; I think it was probably a mistake to normalize the trailing slash away.

If the APIs weren't stable, I'd suggest treating foo/ and foo/. as if they have a trailing "current directory" component, which is currently normalized away. That would mean that . is significant when leading or trailing, but not when embedded. It would mean that foo/ and foo/. are equal, and that neither is equal to foo. And it would mean that file_name would return None for these cases.

The behavior of pop on .. just seems like an outright bug to me.

All that said, I'm not sure what's best to do given the stable status of these APIs. We should definitely discuss this at the next libs team meeting.

[aidanhs]

Just focusing on the trailing slash aspect of this issue.

Some notes on using basename/filename, i.e. "get the final component":

• Python 2 and C++ (boost 1.59) treats a trailing slash as implying an empty component
• Posix (the posix basename command, i.e. coreutils and the posix function by the same name i.e. the C api), tcl 8.5, ruby 2.2, nodejs 5, java 1.8, PHP 5.6 and Go all ignore trailing slash.

As much as I hate to admit it, I think treating trailing slashes as meaningful in terms of components is probably outvoted (especially since this behaviour is stable).
This wouldn't be a problem if there was a decent way to work with OsStrings ("get me the last character of this path") - Path::new is not lossy (not sure about PathBuf), so as long as the library (or wherever you get the path from) isn't doing split+join, you'd still be able to check for the trailing slash if you really wanted to. Sadly, I have not found OsString manipulation easy.

So: document trailing slashes being ignored as part of components and (someday) make OsString easier to work with?

---

Python 2:

>>> import os.path
>>> os.path.basename('/a/b/c')
'c'
>>> os.path.basename('/a/b/c/')
''

C++ (boost 1.59):

    boost::filesystem::path p1("/a/b/c");
    boost::filesystem::path p2("/a/b/c/");
    cout << p1.filename() << "\n" << p2.filename();

c
.

---

Tcl 8.5

% llength [file split /a/b/c]
4
% llength [file split /a/b/c/]
4

Ruby 2.2

   Pathname.new("/a/b/c").basename
=> #<Pathname:c>
   Pathname.new("/a/b/c/").basename
=> #<Pathname:c>

Nodejs 5

> path.basename('/a/b/c');
'c'
> path.basename('/a/b/c/');
'c'

Java 1.8

    System.out.println(Paths.get("/a/b/c").getFileName());
    System.out.println(Paths.get("/a/b/c/").getFileName());
[...]
c
c

PHP 5.6

echo basename("/a/b/c") . " " . basename("/a/b/c/");
[...]
c c

Go

Base returns the last element of path. Trailing slashes are removed before extracting the last element. [...]

[alexcrichton]

@aturon is going to send a PR about docs to clarify trailing slash behavior and some of the other issues going on here.

[jminer]

.NET handles it like Python and C++, which does seem like more useful behavior: https://msdn.microsoft.com/en-us/library/system.io.path.getfilename%28v=vs.110%29.aspx

string path = @"C:\mydir\";
Console.WriteLine("GetFileName('{0}') returns '{1}'",
    path, Path.GetFileName(path););
[...]
GetFileName('C:\mydir\') returns ''

[aidanhs]

The stdlib Python 3 pathlib module strips the trailing slash:

>>> from pathlib import Path
>>> print(Path('/a/b/c/').parts[-1] + ' ' + Path('/a/b/c').parts[-1])
c c

[brson]

Just needs to update docs pr @aturon's last comment.