libstd: impl Show for Path

[richo]

No description provided.

[sfackler]

I believe that the current situation is working as intended. http://static.rust-lang.org/doc/master/std/path/trait.GenericPath.html#method.display

[richo]

I'm not really sure. I've been trying to instrument a quirk in rustc, and dumping structures with println! fell over when I tried this.

Is this a common thing? I've not run across a struct with a display method for when you're trying to print it before. I guess what I'm saying is that I found this surprising.

[huonw]

Not all paths can be fully represented as a string (e.g. the byte 255 is generally a valid path name on Linux, but it is not a valid byte in UTF-8 at all), so an impl of Show would be a lossy conversion.

The documentation of this can definitely be improved.

[lilyball]

As @huonw said, not all Paths are valid utf-8 strings. This is precisely why it has a .display() method, to make it Show-compatible while using str::from_utf8_lossy(). It does not have a Show method itself because it does not want e.g. a .to_str() method, as this conversion loses information and is purely meant for displaying to the user.

If you feel the documentation could be improved, I would be happy to accept a PR on that front. But I am going to close this one as working as intended.

[richo]

Would a lint/warning at compile time when you try to Show a struct that has a .display that implements Show, be excessive/too aggressive in codifying this convention?

My goal with this patch was to make seemingly reasonable inputs have reasonable outputs.

EDIT: If that seems sane I'm very happy to implement (which seems like it would have carry on wins for all of rust, but if there isn't one already I guess maybe I should put forth an RFC for the .display convention?)

[lilyball]

That seems too ad-hoc. Path is the only standard-library-provided type I know of that has a .display method, and it's fairly unique in that paths are often represented as strings by people (who don't know any better), an so being able to Show a Path makes it too likely that the coder will do something wrong (e.g. call .to_str() to try and get a stringly-typed path). This scenario doesn't hold for any other type I can think of.

Theoretically, one could implement Show for Path but mark it as #[deprecated] with the appropriate suggestion to use .display(). But this suggestion has two serious flaws:

1. It provides functionality that is strictly wrong. Yes, it's deprecated (although deprecating new API as it's introduced is quite weird), but it's still there.
2. I don't think deprecation warnings will actually work on trait methods like this. Certainly not if the value is passed to a generic function accepting any Show. The warning would have to be on the trait impl itself, and I don't know if that's supported.

Ideally, we'd have some way of providing a custom error message when attempting to use Path as a Show. Basically like deprecation, except forbidding the use of the API (while still providing it as something to hang the error message off of). I don't know how this would actually work, though, and it seems like perhaps overkill when it would only ever be used once in the standard libraries.

Instead, I think we should just verify that the documentation is sufficient. Glancing at the docs for std::path I do see that it explains how display() works. That's only going to help people who actually read the module documentation, though. I don't know if we can do any better.

[richo]

Thanks for the thorough explanation. It definitely appears that I was just Doing It Wrong.