From 3609a2211a1cdb27c77f139108b25f9c9d3613f9 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 22 Sep 2015 12:38:42 +0530 Subject: [PATCH] Handle let ref in toplevel_ref_arg as well --- README.md | 4 ++-- src/misc.rs | 29 ++++++++++++++++++++++++-- tests/compile-fail/matches.rs | 4 ++-- tests/compile-fail/toplevel_ref_arg.rs | 8 +++++++ 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 011078c5407..902667f31c5 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. There are 58 lints included in this crate: name | default | meaning --------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- +-------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ [approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant) | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask) | warn | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have) [box_vec](https://github.com/Manishearth/rust-clippy/wiki#box_vec) | warn | usage of `Box>`, vector elements are already on the heap @@ -59,7 +59,7 @@ name [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add) | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign) | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead [string_to_string](https://github.com/Manishearth/rust-clippy/wiki#string_to_string) | warn | calling `String.to_string()` which is a no-op -[toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`) +[toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take references with `&`. [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc) | allow | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information) [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) diff --git a/src/misc.rs b/src/misc.rs index 20f5d16bbc1..c18c483bcba 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -8,11 +8,13 @@ use rustc_front::visit::FnKind; use rustc::middle::ty; use utils::{get_item_name, match_path, snippet, span_lint, walk_ptrs_ty, is_integer_literal}; +use utils::span_help_and_lint; use consts::constant; declare_lint!(pub TOPLEVEL_REF_ARG, Warn, - "a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not \ - `fn foo((ref x, ref y): (u8, u8))`)"); + "An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), \ + or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take \ + references with `&`."); #[allow(missing_copy_implementations)] pub struct TopLevelRefPass; @@ -39,6 +41,29 @@ impl LateLintPass for TopLevelRefPass { } } } + fn check_stmt(&mut self, cx: &LateContext, s: &Stmt) { + if_let_chain! { + [ + let StmtDecl(ref d, _) = s.node, + let DeclLocal(ref l) = d.node, + let PatIdent(BindByRef(_), i, None) = l.pat.node, + let Some(ref init) = l.init + ], { + let tyopt = if let Some(ref ty) = l.ty { + format!(": {:?} ", ty) + } else { + "".to_owned() + }; + span_help_and_lint(cx, + TOPLEVEL_REF_ARG, + l.pat.span, + "`ref` on an entire `let` pattern is discouraged, take a reference with & instead", + &format!("try `let {} {}= &{};`", snippet(cx, i.span, "_"), + tyopt, snippet(cx, init.span, "_")) + ); + } + }; + } } declare_lint!(pub CMP_NAN, Deny, diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 2fd9df33ef5..f25a5fa3fa4 100755 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -40,7 +40,7 @@ fn single_match(){ fn ref_pats() { { - let ref v = Some(0); + let v = &Some(0); match v { //~ERROR instead of prefixing all patterns with `&` &Some(v) => println!("{:?}", v), &None => println!("none"), @@ -50,7 +50,7 @@ fn ref_pats() { other => println!("other"), } } - let ref tup = (1, 2); + let tup =& (1, 2); match tup { //~ERROR instead of prefixing all patterns with `&` &(v, 1) => println!("{}", v), _ => println!("none"), diff --git a/tests/compile-fail/toplevel_ref_arg.rs b/tests/compile-fail/toplevel_ref_arg.rs index ea69a8cfa15..05ad1af0034 100644 --- a/tests/compile-fail/toplevel_ref_arg.rs +++ b/tests/compile-fail/toplevel_ref_arg.rs @@ -14,5 +14,13 @@ fn main() { // Closures should not warn let y = |ref x| { println!("{:?}", x) }; y(1u8); + + let ref x = 1; //~ ERROR `ref` on an entire `let` pattern is discouraged + //~^ HELP try `let x = &1;` + + let ref y = (&1, 2); //~ ERROR `ref` on an entire `let` pattern is discouraged + //~^ HELP try `let y = &(&1, 2);` + + let (ref x, _) = (1,2); // okay, not top level println!("The answer is {}.", x); }