fix needless_borrow suggestion

[Jarcho]

fixes: #2610

While I'm working on this, should needless_borrow be split into two? One lint for expressions and another for patterns. In expression it only lints when the compiler inserts a dereference, but for patterns it's whenever a double reference is created. I think at least the case where a double reference is needed should be split into a new lint as it's not 'needless', it can just be done without a ref binding.

For illustration:

fn foo(x: &&str) {}

match Some("test") {
    // ref binding is useless here
    Some(ref x) => *x,
    _ => (),
}

match Some("test") {
    // ref binding is useless here
    Some(ref x) => x.len(),
    _ => (),
}

match Some("test") {
    // double reference is needed, but could be `Some(x) => foo(&x)`
    Some(ref x) => foo(x),
    _ => (),
}

changelog: Improve the suggestion for needless_borrow in patterns to change all usage sites as needed.
changelog: Add lint ref_binding_to_reference

[rust-highfive]

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

[camsteffen]

should needless_borrow be split into two?

I think that would be a good idea since needless_borrow will be a rather noisy lint. And it would be a clean, sensible split to lint patterns separately.

the case where a double reference is needed should be split into a new lint as it's not 'needless', it can just be done without a ref binding

Consider the case where you have a variable var that is a &T and you pass it to a function as fun(&var). This is creating a double reference, but not in a pattern. Also consider the case where you have a ref binding to a value that is Copy. That is a needless ref binding, but not a double reference. So there is a matrix of possibilities. I would call the split lint needless_ref_binding. Both lints may detect a double reference or a reference of a Copy. But one lint applies to ref patterns and the other applies to & borrows.

[Jarcho]

I'm fine with splitting this between patterns and expressions. I don't see it as particularly beneficial, but I also don't see it as particularly harmful to do so either. In both cases a value is borrowed when there was no need to do so. References to other copy types aren't linted at all right now, but would definitely fit the current name.

For the pattern part of this lint, however, it currently triggers on any reference-to-reference created, even when it's actually needed (see the final example). It's this that I believe should definitely be split out as the borrow isn't needless. The choice of where the borrow is created is more stylistic than anything.

The split can be done in a different PR anyways, This is currently in the nursery, and this is behaviour that already existed.

[camsteffen]

For the pattern part of this lint, however, it currently triggers on any reference-to-reference created, even when it's actually needed (see the final example). It's this that I believe should definitely be split out as the borrow isn't needless.

I don't think that case should lint at all. That is, only lint if every usage of the binding has an implicit or explicit deref.

[camsteffen]

Hmm I see now the value in making a separate lint for that. Like double_ref_binding, but only lint cases not already linted by needless_borrow?

[flip1995]

Just a quick comment

[Jarcho]

The case where the reference is needed has been split into it's own lint. Should the pattern part of needless borrow still be separated? Would probably be called something like needless_ref_binding.

[camsteffen]

Should the pattern part of needless borrow still be separated? Would probably be called something like needless_ref_binding.

I'm in favor. But I'd rather split lints in a separate PR.

[Jarcho]

Sounds good.

[giraffate]

ping from triage @Jarcho. There seems to be some fixes left to be done. Can you have any updates on this?

[Jarcho]

Ended up requiring the pattern to not come from a macro. There's a lot of ways for that to be a false positive.

[camsteffen]

Implementation is looking good. Nice work @Jarcho!

I just want to consider the lint categories before merging...

I think this resolves the only major bug for needless_borrow, so it can be moved out of nursery in this PR? I'm a bit worried about how noisy the lint will be, but otherwise it seems to fit style since it strictly simplifies code. So I think it's okay to move it to style if there are no other concerns.

For ref_binding_to_reference, I'm just not sure if it should warn-by-default. For example, the lint will suggest

- ref x => x,
+ x => &x,

The change seems pedantic. But on the other hand, I like the idea of banning double references by default. It gives me peace of mind to know that code doesn't have any double referenced bindings. But maybe that's just me worrying about a non-issue. That is - I am pedantic? Would like to hear other opinions. Are double referenced bindings decidedly bad style?

CC @rust-lang/clippy

[flip1995]

For ref_binding_to_reference I would say that it should be pedantic. Not because it is too pedantic, but because it is too opinionated IMO. I don't think Clippy should suggest that using &x in the body than ref x in the pattern is generally the better way (at least not by default).

About the needless_borrow lint: Did you run the lintcheck tool over it? I think we want to do that before moving it back to style.

[Manishearth]

I agree with @flip1995 on both counts here

[Jarcho]

I thought I did put it in pedantic. Fixed now.

needless_borrow will have to wait to change categories. It has another hundred instances in clippy (I just fixed those a couple of months ago). Lintcheck has 18 for it, not too bad.

[camsteffen]

I thought I did put it in pedantic. Fixed now.

needless_borrow will have to wait to change categories. It has another hundred instances in clippy (I just fixed those a couple of months ago). Lintcheck has 18 for it, not too bad.

Okay. Please squash commits.

[bors]

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

[camsteffen]

Thanks!

@bors r+

[bors]

📌 Commit 6d4dc35 has been approved by camsteffen

[bors]

⌛ Testing commit 6d4dc35 with merge 029c326...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 029c326 to master...