Tracking Issue for os_str_display

[riverbl]

Feature gate: #![feature(os_str_display)]

This is a tracking issue for the OsStr::display method.

Public API

// std::ffi

pub mod os_str {
    pub struct Display<'a> { /* private fields */ }
}

impl OsStr {
    pub fn display(&self) -> os_str::Display<'_>;
}

Steps / History

• ACP: ACP: Add OsStr::display (similar to Path::display) libs-team#326
• Implementation: Add display method to OsStr #120051
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• None yet.

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[ijackson]

I'm concerned about the name. I think OsStr::to_string_lossy is the better precedent. Path has just display and I think this was a mistake.

These methods are really only suitable for use in diagnostics. Places where the stringlike thing is transformed for further use must necessarily either give up on handling invalid unicode (so should use a fallible method) or do something more complicated and awkward.

[ijackson]

(Worst case behaviour from inappropriate use of these methods is modifying the wrong file.)

[riverbl]

I'm concerned about the name. I think OsStr::to_string_lossy is the better precedent. Path has just display and I think this was a mistake.

Path has both to_string_lossy and display. OsStr already has to_string_lossy, which works in the same way as Path::to_string_lossy. This issue is for adding OsStr::display, which would work in the same way as Path::display.

These methods are really only suitable for use in diagnostics. Places where the stringlike thing is transformed for further use must necessarily either give up on handling invalid unicode (so should use a fallible method) or do something more complicated and awkward.

I've encountered situations where OsStr::display would be useful for displaying a potentially non Unicode file name to the user (example). In these cases, the lossy Unicode representation of the OsStr is not used beyond being displayed.

[clarfonthey]

Are there any objections to stabilising this? It's been close to a year.

[dtolnay]

@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[ijackson]

Apparently my earlier comment #120048 (comment) was unclear.

I object to stabilising this with this name. It should be called .display_lossy().

That the analogous method on Path is called .display() is a mistake. Indeed I have seen bugs in real code where Path::display was used inappropriately, where the author probably wouldn't have called a method with lossy in its name. That Path::display has a name encouraging erroneous use, doesn't mean we should make the same mistake again with OsStr.

I've encountered situations where OsStr::display would be useful for displaying a potentially non Unicode file name to the user (example). In these cases, the lossy Unicode representation of the OsStr is not used beyond being displayed.

For the avoidance of doubt, yes, I agree that this method is useful and ought to exist.

All I mean is that it should have lossy in its name. If people agree with me, I would be happy to make an MR to change the name here.

(I would also love to try to deprecate Path::display and replace it with Path::display_lossy. Would that be an ACP?)

[bjoernager]

I don't think that there is anything inherent that says display must equate to lossless. The Display implementations for f16, f32, etc. are all lossy in the sense that many raw representations yield the same pretty output. I don't see why std::os_str::Display (and std::path::Display) can't also discard some data as long as it's done predictably (unless it's the platform-dependence that is the issue). The display functions themselves already add a layer of indirection.

_{Additionally, I don't see why Path and OsStr can't just directly implement Display as well, given that Debug exists.}

[joshtriplett]

Display is not in general expected to be lossless. The fact that you have to call .display() is already enough of a roadblock. (Personally, I would advocate implementing Display directly.) I don't think there's value in making the method even more cumbersome.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[BurntSushi]

I agree with @joshtriplett here, and to add, I also put some value on making this consistent in name with Path::display.

[ijackson]

Display is not in general expected to be lossless.

Where is this currently stated? I think it needs to be a lot more prominent. For example, it's not stated in the trait docs for Display.

[bjoernager]

Where is this currently stated? I think it needs to be a lot more prominent. For example, it's not stated in the trait docs for Display.

It's not necessary (or possible) to list every allowed behaviour of a trait. One can usually assume that a specific implementation is allowed as long as the opposite isn't explicitely stated.

[ijackson]

It's not necessary (or possible) to list every allowed behaviour of a trait. One can usually assume that a specific implementation is allowed as long as the opposite isn't explicitely stated.

I'm sorry to say that this is not the attitude I normally expect within the Rust community. We should be trying to help Rust programmers write correct software. That means absolutely not accepting "the implementation is allowed to do anything that's not explicitly forbidden". That way lies C.

I might be persuaded if this were some obscure detail. But it's not. It's core to the semantics of the whole trait (and of ToString::to_string). That .to_string().parse().unwrap() is buggy is not merely non-obvious; I think it will be surprising to many people.

As I say, trying to use something that came from Display (perhaps via .to_string()) as the value, elsewhere in the program (or the whole system) is a foreseeable mistake. One I have come across people actually making.

At the very least we should explain that this is not the intended use of Display. And if we're going for consistency then why do we have .to_string_lossy()?

[joshtriplett]

That .to_string().parse().unwrap() is buggy is not merely non-obvious; I think it will be surprising to many people.

As I say, trying to use something that came from Display (perhaps via .to_string()) as the value, elsewhere in the program (or the whole system) is a foreseeable mistake.

That might make a good clippy lint to add.

[joshtriplett]

I submitted PR #136687 to expand the documentation and document the established practice that Display need not be a full representation of a type, and that FromStr need not accept the format that Display emits.