-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Improve the documentation of Display
and FromStr
, and their interactions
#136687
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
Improve the documentation of Display
and FromStr
, and their interactions
#136687
Conversation
I think I generally agree here, but I do think making |
@BurntSushi wrote:
I'm not sure if even a description of "good practice" matches widespread community practice. "Where possible" excludes a large number of things, including things like Path/PathBuf, OsStr/OsString, BStr/BString, and many types whose In addition to that, a That said, I can attempt to tweak the language here, without making any new guarantees. |
I think there might be a misunderstanding? I'm saying that if your type provides both I agree about the delimiters and what not. But I think that's a different can of worms. :-) |
Aside: Lacking only
Personally I don't love the tight coupling between |
I added a note that "Having both |
Thanks! I'm happy with that. |
(Is that an r= or would you prefer someone else to review? :) ) |
I did a bit more of a careful review. I think the phrasing here is still a bit too pessimistic for my liking. |
5192570
to
5f82fb7
Compare
Apparently my attempt to push the change I mentioned in #136687 (comment) failed. Hopefully that helps. |
@BurntSushi I think I've addressed your concern here; please let me know if the current version doesn't address your concern. |
One example in the standard library where a type implements both Display and FromStr but doesn't roundtrip is SocketAddrV6. The flowinfo and possibly the scopeinfo aren't preserved. |
Reading through this thread, it seems that the discussion fundamentally boils down to 2 questions:
Obviously a type may "override" these guarantees with documentation. We only need to specify the default expectations in the absence of specific documentation to the contrary. I believe that the answer to the first question is yes: the default expectation should be that |
Having Maybe in the 2027 edition we can have edition-specific traits (I'm looking at you, Or we could rename Both of these would mean a semantic difference between |
So err maybe we could add some docs to |
I don't see how. The guidelines don't say or recommend that
My 2c is that |
This comment has been minimized.
This comment has been minimized.
…actions In particular: - `Display` is not necessarily lossless - The output of `Display` might not be parseable by `FromStr`, and might not produce the same value if it is. - Calling `.parse()` on the output of `Display` is usually a mistake unless a type's documented output and input formats match. - The input formats accepted by `FromStr` depend on the type.
- Drop the phrasing "usually a mistake". - Mention that `Display` may not be lossless. - Drop a misplaced parenthetical about round-tripping that didn't fit the paragraph it was in.
- Drop "usually a mistake" - Add phrasing from `FromStr` about round-tripping, and about how the inability to round-trip may surprise users.
dcfb1ec
to
742014e
Compare
Reworked based on discussion with @Amanieu to tone down the "usually a mistake" phrasing, and focus on caveats and guidance about potential user confusion. |
The new wording is much more neutral and is a strict improvement over what was there before. @bors r+ rollup |
Rollup of 8 pull requests Successful merges: - #136687 (Improve the documentation of `Display` and `FromStr`, and their interactions) - #137306 (Remove `i128` and `u128` from `improper_ctypes_definitions`) - #138699 (build dist for x86_64-pc-solaris and sparcv9-sun-solaris) - #141250 (add s390x z17 target features) - #141467 (make `OsString::new` and `PathBuf::new` unstably const) - #141871 (index: add method for checking range on DenseBitSet) - #141888 (Use non-2015 edition paths in tests that do not test for their resolution) - #142000 (bootstrap: don't symlink source dir into stage0 sysroot) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #136687 - joshtriplett:improve-display-and-fromstr-docs, r=Amanieu Improve the documentation of `Display` and `FromStr`, and their interactions In particular: - `Display` is not necessarily lossless - The output of `Display` might not be parseable by `FromStr`, and might not produce the same value if it is. - Calling `.parse()` on the output of `Display` is usually a mistake unless a type's documented output and input formats match. - The input formats accepted by `FromStr` depend on the type. This documentation adds no API surface area and makes no guarantees about stability. To the best of my knowledge, everything it says is already established to be true. As such, I don't think it needs an FCP.
Rollup of 8 pull requests Successful merges: - rust-lang/rust#136687 (Improve the documentation of `Display` and `FromStr`, and their interactions) - rust-lang/rust#137306 (Remove `i128` and `u128` from `improper_ctypes_definitions`) - rust-lang/rust#138699 (build dist for x86_64-pc-solaris and sparcv9-sun-solaris) - rust-lang/rust#141250 (add s390x z17 target features) - rust-lang/rust#141467 (make `OsString::new` and `PathBuf::new` unstably const) - rust-lang/rust#141871 (index: add method for checking range on DenseBitSet) - rust-lang/rust#141888 (Use non-2015 edition paths in tests that do not test for their resolution) - rust-lang/rust#142000 (bootstrap: don't symlink source dir into stage0 sysroot) r? `@ghost` `@rustbot` modify labels: rollup
In particular:
Display
is not necessarily losslessDisplay
might not be parseable byFromStr
, and mightnot produce the same value if it is.
.parse()
on the output ofDisplay
is usually a mistakeunless a type's documented output and input formats match.
FromStr
depend on the type.This documentation adds no API surface area and makes no guarantees about stability. To the best of my knowledge, everything it says is already established to be true. As such, I don't think it needs an FCP.