WIP: Add `not_exhaustive_enough´ lint

[jrqc]

r? @ebroto
Fixes #5557
changelog: new lint not_exhaustive_enough

[Rantanen]

The point of #[non_exhaustive] is that the wildcard match is always required:

Suggested change

	/// E::Third => {}
	/// }
	/// E::Third => {}
	/// _ => {}
	/// }

[ebroto]

Thanks for the effort you've put into this!

I did a first round of review, don't hesitate to ask for clarification if needed.

[ebroto]

Also, my apologies for the delay in reviewing, I did not have much free time lately.

[Rantanen]

I'm slightly concerned that the full use of this lint will be held back by inability to distinguish between:

• I want to only match these two items and don't care about the rest.
• I want to match everything, but need to use wildcard for non_exhaustive.

match io_error.kind() {
  ErrorKind::TimedOut => self.retry(),
  _ => return Err(io_error),  // This is intended to match all known and unknown kinds.
}

if let Some(Foo { x, .. /* don't care for the rest */ }) = maybe_foo {
  ..
}

These issues can always be ignored case-by-case with #[allow(..)] attributes on the statements, but if that has to be done frequently the added noise of said attributes might outweigh the benefits.

I don't really have any good solutions to this problem and I do feel having the lint as it is implemented here is better than not having it at all. :)

[jrqc]

Thank you for the review! I will make the necessary changes as soon as possible

[ebroto]

@Rantanen that's a very good point.

The idea behind the lint is to warn in case new fields/variants are added, so it should target those cases where the user wanted to be as exhaustive as possible. But we don't know how the data structure looked like when the user wrote the code, so we can only approximate.

Some possible heuristics could be (although I'm not happy with any of them TBH):

• Warn only if a majority of the fields/variants are already included (i.e. >= n/2+1).
• Do not warn if only one field/variant is included (without taking wildcard/rest pattern into account).

[ebroto]

If the lint turns out to be more annoying than helpful we can always turn it into a restriction lint.

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[giraffate]

ping from triage @jrqc. There seem to be some small fixes left to be done. Could you have any update on this?

[jrqc]

Sorry about the delayed response @ebroto @giraffate
I will update it today

[jrqc]

@ebroto Made the necessary changes! Hope I didn't miss anything
Again sorry about the late edit

[ebroto]

Sorry about the delayed response

No problem, thanks for sticking with it! I will try to review in the coming days.

[ebroto]

r? @Manishearth (I'm leaving the team, so I'm reassigning my PRs to other active members)

[Manishearth]

I'll review this when I get a chance, but I'm wondering: we're replicating a lot of the exhaustiveness checking logic in rustc, yeah? Can we use it, or make this lint a lint in rustc?

[Manishearth]

@flip1995 what are your opinions on the above^ ?

[flip1995]

I don't really have an opinion on this, since I didn't review any of the exhaustiveness PRs AFAICR. I just can give a general answer: If we can share code with rustc, we should do that. But I don't know how code in Clippy or rustc looks like for exhaustiveness checks.

[Manishearth]

@flip1995 so the thing is that rustc already has exhaustiveness checks, and this is a lint that was proposed in a Rust RFC, so it would both be easy to implement in rustc and make sense. It feels weird to reimplement exhaustiveness checking in a partial way outside of rustc when we can have a perfect version of this lint in rustc.

[flip1995]

Yeah, agreed.

[giraffate]

ping from triage @Manishearth. Do we still need discussion here?

[Manishearth]

Yeah, sorry, I think we've discussed this and we shouldn't be doing this in clippy. It's worth filing a bug on rust-lang/rust to add such an allow-default lint. Sorry @jrqc!

[jplatte]

I can't find an issue about this lint at rust-lang/rust, did anybody end up creating one? Also, given that this won't be implemented in clippy, somebody should close the associated issue (#5557).