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

libcore: add Debug implementations to most missing types #32054

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Projects
None yet
5 participants
@seanmonstar
Copy link
Contributor

seanmonstar commented Mar 5, 2016

Also adds #![deny(missing_debug_implementations)] to the core crate.

cc #31869

impl<T: ?Sized + Debug> Debug for UnsafeCell<T> {
fn fmt(&self, f: &mut Formatter) -> Result {
f.debug_struct("UnsafeCell")
.field("value", &unsafe { &*self.get() })

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

This is unsafe, I don't think UnsafeCell should access its value in Debug.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

The unsafety is accessing the inner value to print, when another thread could be writing to it?

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

We have no idea what the unsafe cell is being used for. Yes, it may need synchronization.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

True. It seems that perhaps UnsafeCell shouldn't implement Debug, as it isn't safe. Anyone using an UnsafeCell should manually implement Debug for their own type.

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

I think it's fine to implement Debug, but it can't show any fields. Printing the ptr value is pointless, since the pointer is just to the interior of the value.

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

Weak is a precedent, it doesn't show anything about its value either.

@@ -222,7 +222,7 @@ impl<'a> Part<'a> {

/// Formatted result containing one or more parts.
/// This can be written to the byte buffer or converted to the allocated string.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct Formatted<'a> {

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

This struct is not exported, so it doesn't have to impl Debug.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("Unique")
.field("pointer", &self.pointer)
.finish()

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

maybe Unique and Shared should just print like a plain pointer? Somewhere in the guidelines we say that debug output doesn't need to contain type information.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

That's true. These could delegate to fmt::Pointer::fmt, I suppose. Is that objectively better?

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

I think so, the struct around Unique { pointer: 0xfff } adds nothing except the struct name which is statically known already.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

Looking at Box, it prints the actual value in Display and Debug, instead of the pointer. I'm thinking just copy that approach, and also implement fmt::Pointer as well.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

Nevermind, some safety is built in to Box that Unique doesn't provide. Thinking of just killing the Debug impls here, then.

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

Seems to be ok to impl debug like a pointer. This is just a pointer wrapped in some type system properties.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

I didn't see much value, so for now I've marked them allow.

@@ -663,6 +663,15 @@ macro_rules! generate_pattern_iterators {
pub struct $forward_iterator<'a, P: Pattern<'a>>($internal_iterator<'a, P>);

$(#[$common_stability_attribute])*
impl<'a, P: Pattern<'a>> fmt::Debug for $forward_iterator<'a, P> where P::Searcher: fmt::Debug {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_tuple("$forward_iterator")

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

This will use the $forward_iterator string verbatim, right? That's not intended. Same below.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

The macro won't replace it, since it's inside a string?

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

it won't replace it. There is stringify!().

@@ -323,7 +323,7 @@ Section: Iterators
/// Created with the method [`chars()`].
///
/// [`chars()`]: ../primitive.str.html#method.chars
#[derive(Clone)]
#[derive(Clone, Debug)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct Chars<'a> {

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

Chars can have a better impl down the line (that prints a string).

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 5, 2016

Nice!

Providing Debug is our policy ("virtually all types should impl Debug") and it's often requested. We will take a compilation time hit from adding a lot of new code to the libraries, but there isn't really a way around it.

@@ -463,7 +463,7 @@ impl<'a, 'b> Pattern<'a> for &'b [char] {
/////////////////////////////////////////////////////////////////////////////

/// Associated type for `<F as Pattern<'a>>::Searcher`.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CharPredicateSearcher<'a, F>(<CharEqPattern<F> as Pattern<'a>>::Searcher)

This comment has been minimized.

@bluss

bluss Mar 5, 2016

Contributor

This one needs a manual impl too.

This comment has been minimized.

@seanmonstar

seanmonstar Mar 5, 2016

Contributor

Oh right, derp. Dealing with F: Fn fields is unfun.

@seanmonstar seanmonstar force-pushed the seanmonstar:impl-debug-core branch from c1284af to 7422dc8 Mar 5, 2016

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 5, 2016

Updated.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 5, 2016

Looks good to me. This is ready to go (r=me by reviewer), but I'd like to wait a day to see if there are any objections.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 5, 2016

make tidy is unhappy with the stability attributes: https://travis-ci.org/rust-lang/rust/builds/113934157

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 5, 2016

Hm, so do I invent a new feature, rust1_debug or whatever? Or do I lie and say stable since 1.0.0?

Interestingly, it seems the derived implementations don't have stability markers? They are auto stable? And auto-since-whenever?

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 5, 2016

impls don't have checked stability. You can mark them stable (Debug is stable) with an invented new feature name and since 1.9.0.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 6, 2016

This is a pretty strong commitment of libcore, so I'm tagging with T-libs to ensure this comes up during triage.

I'm personally pretty uncomfortable with this. I consider all types candidates for Debug but that doesn't mean that we must indeed implement the trait for all types. Most of these will likely never be used, or they're likely not that useful in the first place. To me these end up just adding more bloat to metadata/distribution that isn't necessarily justified.

@alexcrichton alexcrichton added the T-libs label Mar 6, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 6, 2016

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

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 6, 2016

I understand. Not having these though, means that users have more #[derive(Debug)] frustrations.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 6, 2016

@alexcrichton I understand. My hope is that, if some implementations should be removed, we can mark them as allow, so that we can see the decision was explicit. This way, we can keep deny(missing_debug_implementations), and prevent any accidental omissions on new types that appear. The decision to implement or not should be explicit, not forgotten.

And based on what's decided here, I can go through the other crates and add impls or allow as needed, and in a couple weeks we can be sure no type has been forgotten. Does that sound reasonable?

@alexcrichton alexcrichton self-assigned this Mar 6, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 6, 2016

I personally dislike the idea of adding deny(missing_debug_implementations) to a bunch of our crates. Deny-by-default lints in my opinion should have something actionable beyond adding #[allow]. For example right now the standard library denies missing documentation, which arguably helps improve the breadth of documentation in the standard library (and is also easily actionable if it's missing).

I don't think we have much to benefit from exhaustively specifying Debug everywhere, it ends up just being a maintainability hazard and nightmare to keep up to date.

Another worry of mine is that if this is the "best practice" we shouldn't just unilaterally decide that here. We need a more principled way of approaching this as part of the broader Rust ecosystem.

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 6, 2016

This is not something I made up.

The format docs say

fmt::Debug implementations should be implemented for all public types.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 7, 2016

We can't really cite documentation which is practically ancient and was essentially "defacto stabilized". It's worth reconsidering at least, and in my opinion at the very least rewording the documentation to indicate that it is a candidate for all types, not that it should be implemented for all types.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 7, 2016

I find many of the objections as subjective, not objective, and I disagree with most of them (surprise!). As long as the libs team looks at them as such, I'm fine with whatever the consensus is.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 17, 2016

The libs team discussed this yesterday and the decision was to merge. @seanmonstar can you rebase the PR? I'll r+ after!

@alexcrichton alexcrichton removed the T-libs label Mar 17, 2016

@seanmonstar seanmonstar force-pushed the seanmonstar:impl-debug-core branch from 7422dc8 to 4ef4b12 Mar 18, 2016

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 18, 2016

@alexcrichton rebased and fixed travis tidy complaints.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 18, 2016

@bors: r+ 4ef4b12

Thanks!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 19, 2016

⌛️ Testing commit 4ef4b12 with merge 9e2d976...

bors added a commit that referenced this pull request Mar 19, 2016

Auto merge of #32054 - seanmonstar:impl-debug-core, r=alexcrichton
libcore: add Debug implementations to most missing types

Also adds `#![deny(missing_debug_implementations)]` to the core crate.

cc #31869
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 19, 2016

💔 Test failed - auto-linux-64-x-freebsd

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 19, 2016

@alexcrichton eek, seems theres a duplicate enum in fmt::mod that is also in fmt::rt::v1. Do I just slap the same derives on it (it's also missing PartialEq from the rt mod), or is this enum strictly for docs, as the commit seems to allude, and thus I should just allow the missing impl?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 19, 2016

The types in fmt::rt::v1 are internal and probably don't need any trait implementations, the one in fmt is public and likely wants it (it's unstable today though)

@seanmonstar seanmonstar force-pushed the seanmonstar:impl-debug-core branch from 4ef4b12 to e094593 Mar 20, 2016

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 20, 2016

@alexcrichton ok, I've updated by adding Debug to the public one, and removed Debug from the types in rt::v1.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 21, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 21, 2016

⌛️ Testing commit e094593 with merge 7ec8f5c...

bors added a commit that referenced this pull request Mar 21, 2016

Auto merge of #32054 - seanmonstar:impl-debug-core, r=alexcrichton
libcore: add Debug implementations to most missing types

Also adds `#![deny(missing_debug_implementations)]` to the core crate.

cc #31869

@bors bors merged commit e094593 into rust-lang:master Mar 21, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@bluss bluss added the relnotes label Mar 21, 2016

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Mar 21, 2016

relnote => a nice improvement for rust users that need #[derive(Debug)]

bors pushed a commit that referenced this pull request Jul 12, 2016

Derive Debug on FileType.
Partially fixes #32054

bors added a commit that referenced this pull request Jul 12, 2016

Auto merge of #34757 - sourcefrog:debug-filetype, r=alexcrichton
Derive Debug on FileType.

Partially fixes #32054

badboy added a commit to badboy/rust that referenced this pull request Jul 12, 2016

d-e-s-o added a commit to d-e-s-o/termion that referenced this pull request Apr 20, 2018

Make publicly exported types implement Debug trait
It seems to have become common practice for publicly exported types in a
library to implement the Debug trait. Doing so potentially simplifies
trouble shooting in client code directly but it also is a requirement in
case said client code embeds such objects and wants the wrappers to
implement this trait. For a deeper discussion of this topic please refer
to rust-lang/rust#32054

To that end, this change adjust all publicly exported types to derive
from Debug. It also adds a crate wide lint enforcing this constraint.

d-e-s-o added a commit to d-e-s-o/termion that referenced this pull request Apr 20, 2018

Make publicly exported types implement Debug trait
It seems to have become common practice for publicly exported types in a
library to implement the Debug trait. Doing so potentially simplifies
trouble shooting in client code directly but it also is a requirement in
case said client code embeds such objects and wants the wrappers to
implement this trait. For a deeper discussion of this topic please refer
to rust-lang/rust#32054

To that end, this change adjust all publicly exported types to derive
from Debug. It also adds a crate wide lint enforcing this constraint.

d-e-s-o added a commit to d-e-s-o/termion that referenced this pull request Apr 21, 2018

Make publicly exported types implement Debug trait
It seems to have become common practice for publicly exported types in a
library to implement the Debug trait. Doing so potentially simplifies
trouble shooting in client code directly but it also is a requirement in
case said client code embeds such objects and wants the wrappers to
implement this trait. For a deeper discussion of this topic please refer
to rust-lang/rust#32054

To that end, this change adjust all publicly exported types to derive
from Debug. It also adds a crate wide lint enforcing this constraint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment