Add lint &ref x patterns that could be just x

[CBenoit]

Pull request for https://github.com/Manishearth/rust-clippy/issues/1434

This is an unfinished PR to discuss, I need to complete doc comment and investigate for false positives.
I will perform a rebase before the actual merge.

Currently, with the following code:

#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    let mut v = Vec::<String>::new();
    let _ = v.iter_mut().filter(|&ref a| a.is_empty());
    /* could just be |a| a.is_empty() */
}

I get the following warning:

warning: this pattern takes a needless borrowed reference
  --> src/main.rs:11:35
   |
11 |     let _ = v.iter_mut().filter(|&ref a| a.is_empty());
   |                                   ^^^^^
   |
   = note: #[warn(needless_borrowed_reference)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrowed_reference

I put comments here Manishearth/rust-clippy@master...CBenoit:master#diff-c4b7ab8d347ac2a35bfd4dd00d14d6a2R40 to check my understanding of the code.

By the way, as I tried to understand the code of needless_borrow lint I came across three side questions:

• what the S of rustc::ty::TyS stands for?
• why is there two "PatKind" (one in hir and one in ast)? (https://manishearth.github.io/rust-internals-docs/rustc/hir/enum.PatKind.html and https://manishearth.github.io/rust-internals-docs/syntax/ast/enum.PatKind.html)
• what tcx stands for? (lifetime parameter in https://manishearth.github.io/rust-internals-docs/rustc/lint/trait.LateLintPass.html)

Thanks!

[oli-obk]

what the S of rustc::ty::TyS stands for?

Maybe "struct"? not sure, you should mostly be using ty::Ty everywhere, since that's what the compiler works with (it's just a type Ty = &'tcx TyS<'tcx>);

why is there two "PatKind" (one in hir and one in ast)? (https://manishearth.github.io/rust-internals-docs/rustc/hir/enum.PatKind.html and https://manishearth.github.io/rust-internals-docs/syntax/ast/enum.PatKind.html)

the one is for EarlyLintPass, the other for LateLintPass. As you can see, the hir-variant has no macro variant anymore, since they don't exist in the HIR anymore.

what tcx stands for? (lifetime parameter in https://manishearth.github.io/rust-internals-docs/rustc/lint/trait.LateLintPass.html)

"Type ConteXt", if you have a lifetime of that name, it usually means that you can feed it into many functions expecting a 'tcx lifetime bound object. It is used internally in the compiler to ensure that nothing is allocated multiple times (to save memory and probably for fast comparison, too).

[mcarton]

why is there two "PatKind" (one in hir and one in ast)?

It's like the second time in a week someone has asked that. Maybe we should update the CONTRIBUTE.md file (or rustc's doc itself?)

[llogiq]

I'd update the latter and link from the former.

[Manishearth]

IIRC TyS stands for "Type (Safe)", since it replaced the older unsafe ty::t_box and ty::t stuff.

[CBenoit]

Thank you very much for explanations! I'm sorry I couldn't finish this long time ago. I started working on it again, but I haven't been able to build clippy with Rust Nightly for some time even if I grab the latest version. I'm a little bit confused since I get a lot of errors not related to my modifications : https://pastebin.com/uU4H3vhm
Would you happen to know what could be the cause? Thanks.

[oli-obk]

yea, clippy needs to be fixed to work on the latest nightly, see #1646

[oli-obk]

Note that you can use rustup update nightly-2017-03-28 to get a working nightly and continue from there until we get clippy fixed.

[CBenoit]

Ahw, I see. Thank you, I will use that.

[oli-obk]

clippy now works with the latest nightly again.

[oli-obk]

Just a reminder that you have a PR open @CBenoit . There's no hurry whatsoever!

[CBenoit]

Thank you for reminding me!

[CBenoit]

Hello!

I'm trying to finish this lint.

I tried running util/dogfood.sh, but could not do it because of the following error :

error[E0463]: can't find crate for `clippy`

I don't know what can be done about that…

Furthermore, I don't know why but the lint in its current state doen't works anymore even though it previously gave the result presented in the first post…
And when documenting the lint, I realized that I didn't understand very well the meaning of a &ref ... In the past, I only saw ref in pattern matching and & otherwise. But never &ref. I don't see the point of writing |&ref a| a.is_empty() but I can't tell why it is bad to do so.

Also, I wanted to try checking the pattern is in a closure's |…| but didn't find how. Can I do it only with check_pat method?

[oli-obk]

There's no need to run doogfood.sh, you can simply run cargo test dogfood.

It's not bad to write &ref except in the sense of it being completely useless and therefor making the code look more complex than it actually is.

check_pat should be triggered for every single pattern, no matter whether it's in a closure or a match.

What do you mean by "not working anymore"?

[CBenoit]

Ok! It works now.

But at the end, what meaning does &ref has? Is it never useful?

Yes, but there is no way of checking if it is specifically in a clojure? I have to use another check_* ?

It doesn't lint my test code anymore. Maybe the reason is that my fork doesn't provide the lint, but I ran the "update_lints.py" beforehand.

[oli-obk]

I think &mut ref might be useful to turn a mutable reference into an immutable one in the pattern. Other than that I don't see any use for that pattern

There's an issue (https://github.com/Manishearth/rust-clippy/issues/1846) with our test suite. I'm fixing it right now. @llogiq already did that. Just need to wait until it's through...

[oli-obk]

r=me when we get travis back in line

[mcarton]

Where were the tests for this new lint?

[CBenoit]

I didn't had the time to write it yet (and don't know yet how to do it). Furthermore, it seems that the lint doesn't works anymore as mentioned four days ago... I didn't figure out why yet.

[killercup]

Can we add a suggestion to remove the &ref part of the binding?

[oli-obk]

@CBenoit I was planning on adding the suggestion after the merge, but if you still want to do the tests, I can hold off and do that later.

To add a new test simply add a file in clippy_tests/examples and run cargo run --example yourexample 2> examples/yourexample.stderr in the clippy_tests folder. cargo test should then work inside the main folder.

[CBenoit]

OK! Thank you, I'll open a new pull request soon.