[Pre-RFC] Deprecate `Path::display()`

Summary: Deprecate Path::display() in favor of using Path::to_str() and Path::to_string_lossy() when one wants to print a path using fmt::Display.

If you want to print a path using println!(), trying to print the path directly:

println!("{}", my_path);

won’t work. Instead one must use the Path::display() method to get a display-able version of the path:

println!("{}", my_path.display());

This is a minor difference, but it’s inconsistent with conventions in the standard library and winds up being a pain point for new users (at least it was for me).

According Huon Wilson, Path doesn’t impl fmt::Display directly because the conversion is lossy. In theory forcing the user to call Path::display() helps to make this cost clearer, but I’d argue that it fails at this purpose. Worse yet, Path::to_string_lossy() already does the exact same thing in a way that makes both the purpose and the costs clearer.

I think the right solution is to get rid of Path::display(). It’s functionally equivalent to Path::to_string_lossy(), but fails to obviate the associated costs of the UTF-8 conversion the way that to_string_lossy() does. Additionally, if the user wants to guarantee they only print UTF-8 paths, then they can use to_str() to perform a cheap, lossless conversion and handle any failures as they see fit. This addresses all concerns except for making Path easy to use for new developers; You still can’t print a Path directly. The situation is at least improved, though, because it’s clearer what to_string_lossy() is doing to create a display-able path.

Impact

While we can’t completely remove Path::display(), deprecating it is likely to introduce warnings into a large amount of existing code. While I don’t have any numbers, it’s bound to have a pretty widespread impact. Fortunately, most usage of Path::display() should still work as intended if replaced directly with Path::to_string_lossy(). The only case that would break is if someone were referring to path::Display by name directly, e.g. if they were storing it in a struct. I can’t imagine that’s a common use-case, though, as there’s no advantage to holding onto a path::Display object.

Interestingly, this change does have the potential to improve performance in a select few use-cases. Consider the following:

let display = my_path.display();
for _ in 0..100 {
    println!("My path is: {}", display);
}

Each time the path is printed it will perform the UTF-8 conversion, which will allocate if my_path contained non-utf8 characters. If we replace my_path.display() with my_path.to_string_lossy(), the code still compiles but only performs the allocation and UTF-8 conversion once.

Alternatives

  • Do nothing. Valid, doesn’t break any code, but leaves this pain point unaddressed.
  • Remove Path::display(), have Path impl fmt::Display. This makes Path easier to use for new Rust developers, but it still hides the lossy-ness/cost of the UTF-8 conversion. It also makes Path inconsistent with OsStr which doesn’t impl fmt::Display.

Side Note: Comparison to OsStr

Functionally, Path is just a wrapper around OsStr that adds utility methods like exists() and is_absolute(). As such, I’d expect that printing a Path and printing an OsStr should be the same, but OsStr has no display() method, it only has to_str() and to_string_lossy(). Deprecating Path::display() brings Path in line with OsStr, which I think is an improvement.

1 Like

Path::to_string_lossy allocates additional memory. Path::display is there to let you print Paths without additional allocation.

Edit: I know see the current implementation calls to_string_lossy internally. This is an implementation detail and shouldn’t happen. It is technically possible to display a Path with a lossy conversion without allocation additional memory.

7 Likes

@jethrogb I don’t see how a separate struct would be the thing that allows this to happen without allocating. What logic would happen in path::Display's Display impl that couldn’t happen in Path's Display impl?

1 Like

fmt::Display implementations don’t need to construct a string, they only need to push the right bytes in the right order into the fmt::Write object passed in. So it can write the valid UTF-8 fragments and the replacement sequences without having to allocate a whole new string to hold the valid UTF-8 intermingled with the replacements for invalid sequences.

2 Likes

I don’t think the issue was that Path couldn’t impl fmt::Display, rather the Rust devs didn’t want people to be surprised by the fact that it performs a lossy UTF-8 conversion. If the community thinks that it’s okay for Path (and by extension OsStr) to lose some characters when being printed then I think we should definitely impl fmt::Display for Path (and OsStr) directly. Either way, keeping Path::display() seems like a bad idea.

Oh wow, sorry, I misread this as proposing to implement Display directly.

That was the original idea, but I think that silently doing a lossy conversion violates some of Rust’s design principles, so at this point I think the right thing to do is remove display() which feels redundant.

I’m a bit ambivalent about a direct impl - I could probably be convinced in either direction there.

I do think we want something with a Display impl - even if it’s allocating right now, that’s just a matter of finding someone motivated enough to fix the implementation. I do kind of think that display() is a bad name for the function, since it doesn’t indicate why we’ve split it out. display_lossy() would maybe be a better name IMO. Probably not worth the downstream pain of actually doing and deprecating display() though.

I like the idea of display_lossy() returning a struct that performs an allocation-free version lossy conversion, but I agree that it wouldn’t be worth the issues for just a rename, especially when the existing display() could be made allocation-free.

That brings up an alternate proposal: Fix display() to be allocation-free, and add a display() method to OsStr to provide the same functionality. I had interpreted OsStr not having display() to mean that Path shouldn’t have it either, but maybe they should both have display(). Would an RFC be necessary for adding that? At the least fixing Path::display() to not allocate should be able to change without one.

At that point, though, I’d rather have Path and OsStr impl fmt::Display directly. The only option for displaying them is to do a lossy conversion, so without display_lossy() to make the lossy-ness obvious I think removing the bit of friction that display() introduces would be better.

Hmmmm, now I feel like I want to go back to the original proposal of adding an fmt::Display impl for Path.

¯\_(ツ)_/¯

ToString hasn’t been mentioned.

Paths implementing Display would be fine. Unfortunately ToString is implemented automatically too (blanket impl for all Display), and it’s the existence of path.to_string() that would be entirely misleading since it’s lossy. The display being lossy is not a problem.

5 Likes

Darn, that is a nasty wrinkle. I didn’t realize that blanket impl existed. I don’t think there’s a reasonable way of getting around that unless there’s some way to prevent ToString from being implemented on Path (maybe negative trait bounds at some point it the future?).

In light of that, I think my ideal version would be to change display() to display_lossy() and have it not allocate, though it doesn’t seem like that rename would be worth the pain it would case.

I definitely disagree about removing or renaming Path::display. IMO the way it’s implemented makes sense, and most of the confusion really just comes from people not knowing that paths aren’t necessary UTF-8. While in general there was originally the idea that fmt::Display would only be used for things designed to be user facing, in practice people seem to be pretty flexible about what that really means, so in this case it’s good to be explicit about the lossiness of path printing. However, I don’t think it’s necessary to be even more explicit by renaming display to display_lossy since it seems redundant without any other ways of displaying paths.

However, I do think the current implementation of Path::display is actually incorrect/inefficient since it does appear that Display just calls to_string_lossy when being formatted, which could allocate memory. Most people would certainly expect Display to not allocate memory when writing. So I think you may have actually uncovered a bug here.

1 Like

There is a related wrinkle on macOS — it has a concept of a display version of a path, which is much more advanced than Rust's, but also incompatible with what Rust does currently (using it in Path::display() would break Cargo's println-based API).

On macOS POSIX paths are not supposed to be displayed to users at all, and paths have their "display" versions which can be localized and have : and / swapped, because macOS tries to create an illusion for users that macOS supports / in filenames.

So if the current loss-string-converting display() is eventually removed, this would leave space to add the os-x like "pretty-print" display instead.

@kornel I think that’s an interesting issue but at this point I’m not proposing to change the behavior of Path's display behavior. I’m glad you brought it up, though, because it’s worth keeping in mind in the case that I wind up proposing a larger overhaul of how Path and friends are displayed.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.