Skip to content

Add a missing fmt::Debug impl lint #21610

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

Merged
merged 1 commit into from
Jan 26, 2015
Merged

Conversation

sfackler
Copy link
Member

Closes #20855

r? @alexcrichton

@alexcrichton
Copy link
Member

Thanks! I think it's a bit too strong to set the default level to warn for this lint though. We've seen real compile time hits as well as code blow up sizes in the past from just adding derive(Debug) to everything. Especially seeing how many crates in the standard distribution are opting out of this lint I think it should be default allow if we're going to have it.

From a library author's perspective it's unfortunate to accidentally leave this out, but it's a backwards compatible change to add one at any time.

@sfackler
Copy link
Member Author

I was under the impression that Debug was supposed to be implemented for all public types: https://github.com/rust-lang/rfcs/blob/master/text/0565-show-string-guidelines.md#debugging-fmtdebug. Should we be revising that?

I disabled the lint for all of the main rustc crates because I didn't feel like adding hundreds of Debug implementations in this PR. I don't see any reason to add a ton of impls in the rustc crates, but we should definitely flesh out the downstream facing ones.

@alexcrichton
Copy link
Member

To me that guideline states that all public types should have a Debug implementation, but forcing it to be so seems somewhat unnecessary. There are many use cases (such as the hundreds of types in this repo) which don't want to have to have this lint turned one (another being *-sys crates for example).

@sfackler
Copy link
Member Author

Updated to be allow by default. We may want to set the missing_copy_implementations to default allow as well.

@alexcrichton
Copy link
Member

I would love to do so for that! It may want to have a separate PR though to prevent entangling the two.

@sfackler
Copy link
Member Author

Yeah, for sure.

@alexcrichton
Copy link
Member

@bors: r+ a7ad08c

@sfackler
Copy link
Member Author

@bors: r=alexcrichton 7021491

bors added a commit that referenced this pull request Jan 26, 2015
@bors
Copy link
Collaborator

bors commented Jan 26, 2015

⌛ Testing commit 7021491 with merge 59dcba5...

@bors
Copy link
Collaborator

bors commented Jan 26, 2015

@bors bors merged commit 7021491 into rust-lang:master Jan 26, 2015
@sfackler sfackler deleted the debug-lint branch November 26, 2016 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lint for public types that don't implement Debug
3 participants