-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Implement fmt::Debug
for all structures in libstd.
#38006
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There is room for improvement for some of these |
@rkruppe Asked me on IRC to report the diff in size for libstd.rlib |
a7996c9
to
89f2bea
Compare
Via: Before: 8810254 |
@@ -370,6 +371,13 @@ impl ExactSizeIterator for EscapeDefault {} | |||
#[unstable(feature = "fused", issue = "35602")] | |||
impl FusedIterator for EscapeDefault {} | |||
|
|||
#[stable(feature = "std-debug", since = "")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other feature names use underscore to separate
#[stable(feature = "std-debug", since = "")] | ||
impl<T, U> fmt::Debug for Chain<T, U> { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
f.pad("Chain {{ .. }}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the IO adaptors should print their inner reader/writers. They have nice debug output too. This must be addressed now, because the bounds on T, U
need to change and so on.
#[stable(feature = "std-debug", since = "")] | ||
impl<'a, K, V> fmt::Debug for Iter<'a, K, V> { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
f.pad("Iter {{ .. }}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same: Shouldn't this show the elements? Must be addressed now because that changes the K, V
bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I didn't think about the breaking nature of these impl
s in the future to more restrictive bounds. Thanks for pointing that out!
With regards to what should be displayed here for Debug
, I agree that we would ideally display the keys+values here, but do you have any opinions how it should look? Alternatively, would it be bad to (at least initially) have K: Debug
bounds even if we don't display the values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think redundant bounds are bad, there's good reason for it.
Displaying the elements is simple using .entries()
https://doc.rust-lang.org/nightly/std/fmt/struct.DebugMap.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I managed to forget that this is an iterator and maybe it should have a new or different style of display.. I don't know.
@@ -1577,6 +1605,13 @@ impl<K, V> ExactSizeIterator for IntoIter<K, V> { | |||
#[unstable(feature = "fused", issue = "35602")] | |||
impl<K, V> FusedIterator for IntoIter<K, V> {} | |||
|
|||
#[stable(feature = "std-debug", since = "")] | |||
impl<K, V> fmt::Debug for IntoIter<K, V> { | |||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even the IntoIter might want to show the elements, even if it can not iterate itself, it can maybe borrow itself as a by-reference iterator (now or in the future). Which means K, V
need to impl debug.
#[stable(feature = "std_debug", since = "")] | ||
impl fmt::Debug for EscapeDefault { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
f.pad("EscapeDefault {{ .. }}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why {{
and }}
? These aren't format strings.
@@ -1282,7 +1291,7 @@ pub struct IterMut<'a, K: 'a, V: 'a> { | |||
/// HashMap move iterator. | |||
#[stable(feature = "rust1", since = "1.0.0")] | |||
pub struct IntoIter<K, V> { | |||
inner: table::IntoIter<K, V>, | |||
pub(super) inner: table::IntoIter<K, V>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary because HashSet
uses HashMap
internally and it needs access to this inner
field to get the underlying entries without mutating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't HashSet
just delegate to the implementations of Debug
in here rather than accessing the fields directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of 0aaa778, the Debug
impl
s for the HashSet
items will print lists of entries. If we relied on the 'Debug
impl
for HashMap
' for HashSet
, it would print lists of tuple key/value pairs, which will print ()
for the values, which is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm for now instead of pub(super)
could this just be a public function in this module to get access to the table? We could then avoid exporting that as part of the hash_map
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm for now instead of pub(super) could this just be a public function in this module to get access to the table? We could then avoid exporting that as part of the hash_map module.
@alexcrichton Would this be accomplished by changing this line to explicitly export every thing but your new proposed function? Otherwise, wouldn't that function also get exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that'd be the change, but I guess yeah that'd be pretty tough to change. Let's just leave this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Could the since
fields for the stability tags also get filled in here for 1.15.0?
@@ -1282,7 +1291,7 @@ pub struct IterMut<'a, K: 'a, V: 'a> { | |||
/// HashMap move iterator. | |||
#[stable(feature = "rust1", since = "1.0.0")] | |||
pub struct IntoIter<K, V> { | |||
inner: table::IntoIter<K, V>, | |||
pub(super) inner: table::IntoIter<K, V>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't HashSet
just delegate to the implementations of Debug
in here rather than accessing the fields directly?
@@ -47,7 +47,7 @@ mod arch { | |||
#[stable(feature = "raw_ext", since = "1.1.0")] pub type time_t = i64; | |||
|
|||
#[repr(C)] | |||
#[derive(Clone)] | |||
#[derive(Clone, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types are long since deprecated and otherwise this Debug
is sort of just a portability hazard (got to remember to change everything in this folder). Perhaps these could be omitted?
@alexcrichton Your comments have been addressed. |
@rfcbot fcp merge |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, 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. |
☔ The latest upstream changes (presumably #37800) made this pull request unmergeable. Please resolve the merge conflicts. |
4087edf
to
95fc36c
Compare
☔ The latest upstream changes (presumably #38214) made this pull request unmergeable. Please resolve the merge conflicts. |
95fc36c
to
a6dfd24
Compare
Rebased. Merged conflicts have been addressed. |
@brson Have any thoughts here? |
a6dfd24
to
ba950dd
Compare
☔ The latest upstream changes (presumably #38369) made this pull request unmergeable. Please resolve the merge conflicts. |
Part of rust-lang#31869. Also turn on the `missing_debug_implementations` lint at the crate level.
ba950dd
to
86fc63e
Compare
Rebased. |
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @alexcrichton, I wasn't able to add the |
📌 Commit 86fc63e has been approved by |
⌛ Testing commit 86fc63e with merge d2dc2b4... |
💔 Test failed - auto-mac-32-opt |
@bors retry Looks intermittent? |
@bors retry |
⌛ Testing commit 86fc63e with merge d946ed1... |
💔 Test failed - auto-mac-32-opt |
@bors: retry
…On Tue, Dec 20, 2016 at 9:44 AM, bors ***@***.***> wrote:
💔 Test failed - auto-mac-32-opt
<https://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/11398>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#38006 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD95EfUo0vtaCan4ky2G_Sp2PlPu3ZHks5rKBPmgaJpZM4K8qag>
.
|
Implement `fmt::Debug` for all structures in libstd. Part of rust-lang#31869. Also turn on the `missing_debug_implementations` lint at the crate level.
Rollup of 29 pull requests - Successful merges: #37761, #38006, #38131, #38150, #38158, #38171, #38208, #38215, #38236, #38245, #38289, #38302, #38315, #38346, #38388, #38395, #38398, #38418, #38432, #38451, #38463, #38468, #38470, #38471, #38472, #38478, #38486, #38493, #38498 - Failed merges: #38271, #38483
This wasn't actually in 1.15.0, so it seems like the stability attributes are incorrect? |
Version 1.16.0 (2017-03-16) =========================== Language -------- * Lifetimes in statics and consts default to `'static`. [RFC 1623] * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * [Clean up semantics of `self` in an import list][38313] * [`Self` may appear in `impl` headers][38920] * [`Self` may appear in struct expressions][39282] Compiler -------- * [`rustc` now supports `--emit=metadata`, which causes rustc to emit a `.rmeta` file containing only crate metadata][38571]. This can be used by tools like the Rust Language Service to perform metadata-only builds. * [Levenshtein based typo suggestions now work in most places, while previously they worked only for fields and sometimes for local variables][38927]. Together with the overhaul of "no resolution"/"unexpected resolution" errors (#[38154]) they result in large and systematic improvement in resolution diagnostics. * [Fix `transmute::<T, U>` where `T` requires a bigger alignment than `U`][38670] * [rustc: use -Xlinker when specifying an rpath with ',' in it][38798] * [`rustc` no longer attempts to provide "consider using an explicit lifetime" suggestions][37057]. They were inaccurate. Stabilized APIs --------------- * [`VecDeque::truncate`] * [`VecDeque::resize`] * [`String::insert_str`] * [`Duration::checked_add`] * [`Duration::checked_sub`] * [`Duration::checked_div`] * [`Duration::checked_mul`] * [`str::replacen`] * [`str::repeat`] * [`SocketAddr::is_ipv4`] * [`SocketAddr::is_ipv6`] * [`IpAddr::is_ipv4`] * [`IpAddr::is_ipv6`] * [`Vec::dedup_by`] * [`Vec::dedup_by_key`] * [`Result::unwrap_or_default`] * [`<*const T>::wrapping_offset`] * [`<*mut T>::wrapping_offset`] * `CommandExt::creation_flags` * [`File::set_permissions`] * [`String::split_off`] Libraries --------- * [`[T]::binary_search` and `[T]::binary_search_by_key` now take their argument by `Borrow` parameter][37761] * [All public types in std implement `Debug`][38006] * [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327] * [`Ipv6Addr` implements `From<[u16; 8]>`][38131] * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [std: Fix partial writes in `LineWriter`][38062] * [std: Clamp max read/write sizes on Unix][38062] * [Use more specific panic message for `&str` slicing errors][38066] * [`TcpListener::set_only_v6` is deprecated][38304]. This functionality cannot be achieved in std currently. * [`writeln!`, like `println!`, now accepts a form with no string or formatting arguments, to just print a newline][38469] * [Implement `iter::Sum` and `iter::Product` for `Result`][38580] * [Reduce the size of static data in `std_unicode::tables`][38781] * [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`, `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement `Display`][38909] * [`Duration` implements `Sum`][38712] * [`String` implements `ToSocketAddrs`][39048] Cargo ----- * [The `cargo check` command does a type check of a project without building it][cargo/3296] * [crates.io will display CI badges from Travis and AppVeyor, if specified in Cargo.toml][cargo/3546] * [crates.io will display categories listed in Cargo.toml][cargo/3301] * [Compilation profiles accept integer values for `debug`, in addition to `true` and `false`. These are passed to `rustc` as the value to `-C debuginfo`][cargo/3534] * [Implement `cargo --version --verbose`][cargo/3604] * [All builds now output 'dep-info' build dependencies compatible with make and ninja][cargo/3557] * [Build all workspace members with `build --all`][cargo/3511] * [Document all workspace members with `doc --all`][cargo/3515] * [Path deps outside workspace are not members][cargo/3443] Misc ---- * [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies the path to the Rust implementation][38589] * [The `armv7-linux-androideabi` target no longer enables NEON extensions, per Google's ABI guide][38413] * [The stock standard library can be compiled for Redox OS][38401] * [Rust has initial SPARC support][38726]. Tier 3. No builds available. * [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No builds available. * [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379] Compatibility Notes ------------------- * [Uninhabitable enums (those without any variants) no longer permit wildcard match patterns][38069] * In this release, references to uninhabited types can not be pattern-matched. This was accidentally allowed in 1.15. * [The compiler's `dead_code` lint now accounts for type aliases][38051]. * [Ctrl-Z returns from `Stdin.read()` when reading from the console on Windows][38274] * [Clean up semantics of `self` in an import list][38313] [37057]: rust-lang/rust#37057 [37761]: rust-lang/rust#37761 [38006]: rust-lang/rust#38006 [38051]: rust-lang/rust#38051 [38062]: rust-lang/rust#38062 [38062]: rust-lang/rust#38622 [38066]: rust-lang/rust#38066 [38069]: rust-lang/rust#38069 [38131]: rust-lang/rust#38131 [38154]: rust-lang/rust#38154 [38274]: rust-lang/rust#38274 [38304]: rust-lang/rust#38304 [38313]: rust-lang/rust#38313 [38314]: rust-lang/rust#38314 [38327]: rust-lang/rust#38327 [38401]: rust-lang/rust#38401 [38413]: rust-lang/rust#38413 [38469]: rust-lang/rust#38469 [38559]: rust-lang/rust#38559 [38571]: rust-lang/rust#38571 [38580]: rust-lang/rust#38580 [38589]: rust-lang/rust#38589 [38670]: rust-lang/rust#38670 [38712]: rust-lang/rust#38712 [38726]: rust-lang/rust#38726 [38781]: rust-lang/rust#38781 [38798]: rust-lang/rust#38798 [38909]: rust-lang/rust#38909 [38920]: rust-lang/rust#38920 [38927]: rust-lang/rust#38927 [39048]: rust-lang/rust#39048 [39282]: rust-lang/rust#39282 [39379]: rust-lang/rust#39379 [`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset [`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add [`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div [`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul [`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub [`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions [`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4 [`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6 [`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default [`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4 [`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6 [`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str [`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off [`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key [`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by [`VecDeque::resize`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize [`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate [`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat [`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen [cargo/3296]: rust-lang/cargo#3296 [cargo/3301]: rust-lang/cargo#3301 [cargo/3443]: rust-lang/cargo#3443 [cargo/3511]: rust-lang/cargo#3511 [cargo/3515]: rust-lang/cargo#3515 [cargo/3534]: rust-lang/cargo#3534 [cargo/3546]: rust-lang/cargo#3546 [cargo/3557]: rust-lang/cargo#3557 [cargo/3604]: rust-lang/cargo#3604 [RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md
…r=dtolnay Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api
Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang/rust#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang/rust#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api
Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang/rust#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang/rust#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api
Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls. For context, `MutexGuard` has `+ ?Sized` on its `Debug` impl, and all three have `+ ?Sized` on their `Display` impls. It looks like the `?Sized` was just missed when the impls were added (the impl for `MutexGuard` was added in the same PR (rust-lang/rust#38006) with support for `T: Debug + ?Sized`, and `RwLock*Guard`s did allow `T: ?Sized` types already); the `Display` impls were added later (rust-lang/rust#42822) with support for `T: Debug + ?Sized` types. I think this needs a T-libs-api FCP? I'm not sure if this also needs an ACP. If so I can make one. These are changes to (stable) trait impls on stable types so will be insta-stable. `@rustbot` label +T-libs-api
Part of #31869.
Also turn on the
missing_debug_implementations
lint at the cratelevel.