Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document that Display entails ToString and should be lossless and infallible #94488

Closed
wants to merge 2 commits into from

Conversation

JKAnderson409
Copy link

#92941
Taking what was said here and in the issue itself, I tried to be concise and explicit. I kept my lines to 80 characters according to the guide.

I want to be able to write docs like the ones in std so any feedback is appreciated. This is also my first contribution so I apologize if I made mistakes in the submission process.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2022
@the8472
Copy link
Member

the8472 commented Mar 1, 2022

I don't think this is universally true. In the Path case it's because it is very tempting to treat paths as strings so not doing a lossy to_string avoids people running into that footgun.

But for complex aggregate types which don't implement FromStr or similar one doesn't necessarily expect the display to be a lossless representation of its internal state.

@JKAnderson409
Copy link
Author

From the thread:

A weak heuristic: If you're not going to bother making a `FromStr` too, just skip `Display` .
...
`Debug` and `serde` support are the important ones, most of the time.

This implies that the String returned by to_string should losslessly represent the state of the instance. I guess this is exactly what the issue is trying to clear up.

@the8472
Copy link
Member

the8472 commented Mar 1, 2022

That kind of seems backwards to me. If it is user-facing (as the documentation says) then there should be no assumption about FromStr which is more of a serialization or user-input thing. Not everything that can be shown to a user must be parsable.
So where does that heuristic come from?

Again, for simple types representing just one thing I kind of can see it. But for complex types I would apply different heuristics.

I guess we can discuss this somewhere else, I just want to register my general disagreement.

@JKAnderson409
Copy link
Author

I agree that it's a bit confusing. For me, it clears things up to think of this as applying only with the context of the standard library's formatter. The std::fmt module is defined as, "Utilities for formatting and printing Strings." When we do things like print!("{}", thing) we rely on Display but you could define a Print or Show trait that applies to a different use case.

Comment on lines +700 to +701
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output,
/// and so cannot be derived. When `Display` is implemented, the [`ToString`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output,
/// and so cannot be derived. When `Display` is implemented, the [`ToString`]
/// `Display` is similar to [`Debug`], but `Display` is for user-facing output.
/// When `Display` is implemented, the [`ToString`]

As long as we're fixing this documentation: this conclusion doesn't actually automatically follow, and might not always be true.

@joshtriplett
Copy link
Member

But for complex aggregate types which don't implement FromStr or similar one doesn't necessarily expect the display to be a lossless representation of its internal state.

Agreed; in particular, the "lossless" phrasing might encourage people to produce Display impls that don't do as good a job at being human-readable, because they over-focus on round-tripping.

@m-ou-se m-ou-se added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2022
@apiraino apiraino added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2022
@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Apr 6, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@JKAnderson409 Can you please post your status on this PR? Thanks.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@JKAnderson409
Copy link
Author

@rustbot ready

Sorry I've been out for a long time but I'm back in the office today.

FYI, the Book gives the impression that Display can be lossy and that it cannot be derived. If this change means that it may become derivable, then that suggested change by @joshtriplett should go in.

An example of a trait that can’t be derived is Display, which handles formatting for end users. You should always consider the appropriate way to display a type to an end user. What parts of the type should an end user be allowed to see? What parts would they find relevant? What format of the data would be most relevant to them? The Rust compiler doesn’t have this insight, so it can’t provide appropriate default behavior for you.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2022
@m-ou-se m-ou-se assigned joshtriplett and unassigned m-ou-se Jun 20, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
/// trait is implemented automatically, adding the `to_string` method to all
/// `Display` types.
///
/// A `Display` implementation should be lossless and infallible, otherwise it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you define lossless here? Any definition I can think of feels way too strict..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"doesn't lose information from its internal data" and then we can use Path as an example?

/// A `Display` implementation should be lossless and infallible, otherwise it
/// does not fit the API. For example, converting a path to a [`String`] is
/// potentially lossy or fallible, so [`Path`] doesn’t implement `Display`
/// directly (but offers a .display() method).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// directly (but offers a .display() method).
/// directly (but offers a `.display()` method).

@bors
Copy link
Contributor

bors commented Sep 27, 2022

☔ The latest upstream changes (presumably #102331) made this pull request unmergeable. Please resolve the merge conflicts.

@KittyBorgX
Copy link
Member

@rustbot author
@JKAnderson409 Hey! It looks like there are some merge conflicts, can you sort them out?
Also, any update on the status of the PR? There are pending review changes that some reviewers have suggested so it'd be great if you can sort all of these out.

Once you're done doing everything, type in @rustbot review here to change the tag to S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2023
@Dylan-DPC
Copy link
Member

Closing this pr due to inactivity. If you wish you can reöpen this PR or create a new one when you have the time for it and we can take it forward from there

@Dylan-DPC Dylan-DPC closed this May 12, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.