New lint: single_field_patterns

[peter-shen-dev]

changelog: Add [`single_field_patterns`] lint

fixes #7122

Comments:

1. I wasn't sure how to handle Lint for useless tuple binding. #2852 in a relevant way since it seems to require context outside of the expression/statement - which could make the code more complex - if I'm missing something, or there's a smart way of approaching it, I'm all ears.
2. I don't lint [.., x] or (.., x) because those communicate "get the last element", where array[800] or tuple.4 would feel a bit magical (less relevant in the tuple case imo, that's a bit iffy)
3. I allow if let 1 = x[0] to be suggested because handling it would require effectively re-implementing equatable_if_let, and it deals with the same issues. So I think dealing with that should be equatable_if_let's responsibility, and if people don't like it, they should enable that lint and do a second pass.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

[ghost]

This lint generates too many warnings to be warn by default. I tested it on around 1000 popular crates. I got 541 warnings. The 10 worst crates were:

Number	Crate
25	chrono-0.4.19
24	petgraph-0.6.0
21	hdrhistogram-7.4.0
16	mockall_derive-0.11.0
16	html5ever-0.25.1
15	num-bigint-0.4.3
13	rust-crypto-0.2.36
12	crossbeam-epoch-0.9.5
11	rayon-1.5.1
11	image-0.23.14

Personally, I strongly prefer the pattern matching to the tuple indexing. I'd like to hear other people's opinions on this.

---

From the implementation side, the only problem I found is that the suggestions don't handle macros correctly.

#![allow(unused_variables)]

macro_rules! m {
    () => { (1, 2, 30) }
}

fn main() {
    let (_, x, _) = m!();
}

outputs

warning: this single-variant pattern only matches one field
 --> src/main.rs:8:5
  |
8 |     let (_, x, _) = m!();
  |     ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(clippy::single_field_patterns)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_field_patterns
help: try this
  |
4 ~     () => { (1, 2, 30).1 }
5 | }
6 | 
7 | fn main() {
8 ~     let x = m!();
  |

[peter-shen-dev]

Personally, I strongly prefer the pattern matching to the tuple indexing. I'd like to hear other people's opinions on this.

I think you're in the majority (or there are enough people that prefer it that it should be opt-out). Breaking down 34 lints from cargo lintcheck, I got:

• 28 plain tuples
• 3 tuple structs
• 3 structs with named fields

The frequency of plain tuple matching suggests that people are doing it intentionally.

Also, of the struct lints, 5/6 were newtypes, which may also be something some people do intentionally.

I tested it on around 1000 popular crates.

Is there a pre-existing way to do that?

This lint generates too many warnings to be warn by default.

Would it make more sense if I made it configurable, and ignored plain tuples + newtypes by default? Depending on further tests on the 1000 crates.

From the implementation side, the only problem I found is that the suggestions don't handle macros correctly.

Is there any way to make this do the expected thing, or should I just try to ignore code that has macros for either the pattern or scrutinee?

[matthiaskrgr]

Hmm, my two cents;
I would not see this as an improvement in my own code. :/

If for example we do some code changes and it turns out we need the second return type of if_sequence() as well, we'd have to revert back to (conds, ..) which is more work than just replacing _ with a variable binding.

[peter-shen-dev]

Considering the feedback, I've opted to only handle structs with named fields. I could add tuples/arrays/etc. back as a pedantic lint if anyone was interested, and assuming that's an appropriate category for them (they seem controversial).

I don't think there's any getting around the loss of flexibility, but I think in some of these cases the decrease in verbosity is a fair trade-off. Here it seem reasonable to me - the type is no longer specified, and it's less verbose overall:

-if let Spanned {
-    node: LitKind::Bool(lit),
-    ..
-} = *span
-{
+if let LitKind::Bool(lit) = span.node {

Does that seem like a reasonable correction?

Also, there are currently cases where it requires repetition, which isn't good - I'm planning on making changes to not lint these, but wanted to make sure I'm not wasting time with a bad design:

-let Range { start, .. } = self.pixel_indices_unchecked(sx, sy);
+let start = self.pixel_indices_unchecked(sx, sy).start;

[flip1995]

Sorry for the way too late review, $day_job and christmas vacation got in the way..

This lint and its impl looks really good overall. I agree that tuples should not be linted.

Also, there are currently cases where it requires repetition, which isn't good - I'm planning on making changes to not lint these, but wanted to make sure I'm not wasting time with a bad design:

What do you mean by "repetition"?

[peter-shen-dev]

Sorry for the way too late review, $day_job and christmas vacation got in the way..

No worries, I was also preoccupied with work and some other stuff for a while.

What do you mean by "repetition"?

By repetition, I meant it would lint something like let Struct { field, .. } = s and suggest let field = s.field - though I suppose whether or not that's bad depends on whether you would rather repeat the struct name or the field name. Should I avoid that case?

I've either replied to comments, or made the relevant changes - not sure if I should resolve conversations myself or wait for you to.

Also, I'm not sure why CI is failing - my local machine is building/passing tests with rust 1.60.0 nightly, and the failing code doesn't seem problematic to me. Any ideas why that might be happening?

[flip1995]

not sure if I should resolve conversations myself or wait for you to.

You can resolve them yourself.

By repetition, I meant it would lint something like let Struct { field, .. } = s and suggest let field = s.field

I would lint those too. If the user would care about the name, they would have named it in the Struct match already.

Also, I'm not sure why CI is failing - my local machine is building/passing tests with rust 1.60.0 nightly,

You probably need to rebase on the latest master. With that rebase you'll also get the same nightly that CI uses (the one specified in rust-toolchain.toml on the master branch)

[flip1995]

I have one comment left. Not sure how hard it would be to implement this. If you can't find an easy way to do that, feel free to keep the current behavior. This can also be done in a future PR.

How to move forward:

Please rebase on top of the latest master. Also, please squash (some of) your commits. If you need help with that, let me know!

After that, I'll give it a final review and this should be ready to merge!

[flip1995]

I think a better suggestion would be for the ref and ref mut pattern to borrow the field, instead of keeping it in the pattern. This is just more idiomatic Rust IMO.

[flip1995]

This should be // run-rustfix, since this lint is auto applicable in theory. With that comment on top of the test file, the UI tests will make sure that the suggestions are applied cleanly and don't break anything.

[bors]

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

[flip1995]

Closing because of inactivity. @hahohihu if you want to continue to work on this, let me know!

[peter-shen-dev]

@flip1995 Sorry for abandoning this - life got in the way. Do you think I could pick it back up?