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

new lint: missing_fields_in_debug #10616

Merged
merged 2 commits into from
May 31, 2023
Merged

Conversation

y21
Copy link
Member

@y21 y21 commented Apr 9, 2023

Fixes #10429

This PR adds a new lint that looks for manual Debug implementations that do not "use" all of the fields.
This often happens when adding a new field to a struct.
It also acts as a style lint in case leaving out a field was intentional. In that case, it's preferred to use DebugStruct::finish_non_exhaustive, which indicates that there are more fields that were explicitly not shown.

changelog: [`missing_fields_in_debug`]: missing fields in manual `Debug` implementation

@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2023

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 9, 2023
tests/ui/missing_field_in_debug.stderr Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
tests/ui/missing_field_in_debug.rs Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Apr 23, 2023

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

@y21 y21 force-pushed the missing_field_in_debug branch from 003ffb3 to fc86d55 Compare April 27, 2023 23:24
@bors
Copy link
Contributor

bors commented Apr 29, 2023

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

@y21
Copy link
Member Author

y21 commented May 27, 2023

Is there anything else or is it just waiting on a rebase?

clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
clippy_lints/src/missing_fields_in_debug.rs Outdated Show resolved Hide resolved
@Alexendoo
Copy link
Member

Sorry for the delay, can't remember if I had anything else in mind so I just read it over again this weekend 😄

A thought that came to me just now is maybe we don't actually need to lint enums, unlike structs where it's very easy to add a new field and forget to update the Debug impl using a match already has you consider enum variants fully. The author would have to explicitly ignore the fields with ../_ in order to miss them

@y21
Copy link
Member Author

y21 commented May 31, 2023

A thought that came to me just now is maybe we don't actually need to lint enums, unlike structs where it's very easy to add a new field and forget to update the Debug impl using a match already has you consider enum variants fully.

That's a very good point, I agree with this. I went ahead and removed the enum checking (and also addressed the other comments). That made me realize that one can do something similar with structs by destructuring self (you get a compiler error when a new field is added):

struct F { a: i32, b: i32 }

impl fmt::Debug for F {
  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
    let Self { a } = self; // error, b not handled
    // ...
  }
}

Although a lint for this would still be nice 😅

@Alexendoo
Copy link
Member

Thanks! Yeah that's a good way of catching it

With a rebase for the conflicts I'd say this is ready to merge

y21 added 2 commits May 31, 2023 23:52
move some strings into consts, more tests

s/missing_field_in_debug/missing_fields_in_debug

dont trigger in macro expansions

make dogfood tests happy

minor cleanups

replace HashSet with FxHashSet

replace match_def_path with match_type

if_chain -> let chains, fix markdown, allow newtype pattern

fmt

consider string literal in `.field()` calls as used

don't intern defined symbol, remove mentions of 'debug_tuple'

special-case PD, account for field access through `Deref`
@y21 y21 force-pushed the missing_field_in_debug branch from bfb696b to a859b0e Compare May 31, 2023 21:53
@Alexendoo
Copy link
Member

👍

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2023

📌 Commit a859b0e has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 31, 2023

⌛ Testing commit a859b0e with merge 652b4c7...

@bors
Copy link
Contributor

bors commented May 31, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 652b4c7 to master...

@bors bors merged commit 652b4c7 into rust-lang:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint missing fields in manual Debug implementations
5 participants