Merge pull request #499 from devonhollowood/underscore_binding

Add used_underscore_binding lint

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 83 lints included in this crate:
+There are 84 lints included in this crate:
 
 name                                                                                                     | default | meaning
 ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -86,6 +86,7 @@ name
 [unstable_as_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice)                   | warn    | as_slice is not stable and can be replaced by & v[..]see https://github.com/rust-lang/rust/issues/27729
 [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect)                         | warn    | `collect()`ing an iterator without using the result; this is usually better written as a for loop
 [unused_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes)                     | warn    | unused lifetimes in function definitions
+[used_underscore_binding](https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding)       | warn    | using a binding which is prefixed with an underscore
 [useless_transmute](https://github.com/Manishearth/rust-clippy/wiki#useless_transmute)                   | warn    | transmutes that have the same to and from types
 [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop)                         | warn    | `loop { if let { ... } else break }` can be written as a `while let` loop
 [while_let_on_iterator](https://github.com/Manishearth/rust-clippy/wiki#while_let_on_iterator)           | warn    | using a while-let loop instead of a for loop on an iterator

src/lib.rs

@@ -120,6 +120,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box cyclomatic_complexity::CyclomaticComplexity::new(25));
     reg.register_late_lint_pass(box escape::EscapePass);
     reg.register_early_lint_pass(box misc_early::MiscEarly);
+    reg.register_late_lint_pass(box misc::UsedUnderscoreBinding);
 
     reg.register_lint_group("clippy_pedantic", vec![
         methods::OPTION_UNWRAP_USED,
@@ -184,6 +185,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         misc::MODULO_ONE,
         misc::REDUNDANT_PATTERN,
         misc::TOPLEVEL_REF_ARG,
+        misc::USED_UNDERSCORE_BINDING,
         misc_early::UNNEEDED_FIELD_PATTERN,
         mut_reference::UNNECESSARY_MUT_PASSED,
         mutex_atomic::MUTEX_ATOMIC,

src/misc.rs

@@ -10,7 +10,8 @@ use rustc::middle::const_eval::ConstVal::Float;
 use rustc::middle::const_eval::eval_const_expr_partial;
 use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
 
-use utils::{get_item_name, match_path, snippet, span_lint, walk_ptrs_ty, is_integer_literal};
+use utils::{get_item_name, match_path, snippet, get_parent_expr, span_lint, walk_ptrs_ty,
+            is_integer_literal};
 use utils::span_help_and_lint;
 
 /// **What it does:** This lint checks for function arguments and let bindings denoted as `ref`. It is `Warn` by default.
@@ -316,3 +317,69 @@ impl LateLintPass for PatternPass {
         }
     }
 }
+
+
+/// **What it does:** This lint checks for the use of bindings with a single leading underscore
+///
+/// **Why is this bad?** A single leading underscore is usually used to indicate that a binding
+/// will not be used. Using such a binding breaks this expectation.
+///
+/// **Known problems:** None
+///
+/// **Example**:
+/// ```
+/// let _x = 0;
+/// let y = _x + 1; // Here we are using `_x`, even though it has a leading underscore.
+///                 // We should rename `_x` to `x`
+/// ```
+declare_lint!(pub USED_UNDERSCORE_BINDING, Warn,
+              "using a binding which is prefixed with an underscore");
+
+#[derive(Copy, Clone)]
+pub struct UsedUnderscoreBinding;
+
+impl LintPass for UsedUnderscoreBinding {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(USED_UNDERSCORE_BINDING)
+    }
+}
+
+impl LateLintPass for UsedUnderscoreBinding {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        let needs_lint = match expr.node {
+            ExprPath(_, ref path) => {
+                let ident = path.segments.last()
+                                .expect("path should always have at least one segment")
+                                .identifier;
+                ident.name.as_str().chars().next() == Some('_') //starts with '_'
+                && ident.name.as_str().chars().skip(1).next() != Some('_') //doesn't start with "__"
+                && ident.name != ident.unhygienic_name //not in macro
+                && is_used(cx, expr)
+            },
+            ExprField(_, spanned) => {
+                let name = spanned.node.as_str();
+                name.chars().next() == Some('_')
+                && name.chars().skip(1).next() != Some('_')
+            },
+            _ => false
+        };
+        if needs_lint {
+            cx.span_lint(USED_UNDERSCORE_BINDING, expr.span,
+                         "used binding which is prefixed with an underscore. A leading underscore \
+                          signals that a binding will not be used.");
+        }
+    }
+}
+
+fn is_used(cx: &LateContext, expr: &Expr) -> bool {
+    if let Some(ref parent) = get_parent_expr(cx, expr) {
+        match parent.node {
+            ExprAssign(_, ref rhs) => **rhs == *expr,
+            ExprAssignOp(_, _, ref rhs) => **rhs == *expr,
+            _ => is_used(cx, &parent)
+        }
+    }
+    else {
+        true
+    }
+}

tests/compile-fail/for_loop.rs

@@ -179,8 +179,8 @@ fn main() {
     if false { _index = 0 };
     for _v in &vec { _index += 1 }
 
-    let mut _index = 0;
-    { let mut _x = &mut _index; }
+    let mut index = 0;
+    { let mut _x = &mut index; }
     for _v in &vec { _index += 1 }
 
     let mut index = 0;

tests/compile-fail/range.rs

@@ -22,8 +22,8 @@ fn main() {
     let y = NotARange;
     y.step_by(0);
 
-    let _v1 = vec![1,2,3];
-    let _v2 = vec![4,5];
-    let _x = _v1.iter().zip(0.._v1.len()); //~ERROR It is more idiomatic to use _v1.iter().enumerate()
-    let _y = _v1.iter().zip(0.._v2.len()); // No error
+    let v1 = vec![1,2,3];
+    let v2 = vec![4,5];
+    let _x = v1.iter().zip(0..v1.len()); //~ERROR It is more idiomatic to use v1.iter().enumerate()
+    let _y = v1.iter().zip(0..v2.len()); // No error
 }

tests/compile-fail/used_underscore_binding.rs

@@ -0,0 +1,81 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![deny(clippy)]
+
+/// Test that we lint if we use a binding with a single leading underscore
+fn prefix_underscore(_foo: u32) -> u32 {
+    _foo + 1 //~ ERROR used binding which is prefixed with an underscore
+}
+
+/// Test that we lint even if the use is within a macro expansion
+fn in_macro(_foo: u32) {
+    println!("{}", _foo); //~ ERROR used binding which is prefixed with an underscore
+}
+
+// Struct for testing use of fields prefixed with an underscore
+struct StructFieldTest {
+    _underscore_field: u32,
+}
+
+/// Test that we lint the use of a struct field which is prefixed with an underscore
+fn in_struct_field() {
+    let mut s = StructFieldTest { _underscore_field: 0 };
+    s._underscore_field += 1; //~ Error used binding which is prefixed with an underscore
+}
+
+/// Test that we do not lint if the underscore is not a prefix
+fn non_prefix_underscore(some_foo: u32) -> u32 {
+    some_foo + 1
+}
+
+/// Test that we do not lint if we do not use the binding (simple case)
+fn unused_underscore_simple(_foo: u32) -> u32 {
+    1
+}
+
+/// Test that we do not lint if we do not use the binding (complex case). This checks for
+/// compatibility with the built-in `unused_variables` lint.
+fn unused_underscore_complex(mut _foo: u32) -> u32 {
+    _foo += 1;
+    _foo = 2;
+    1
+}
+
+///Test that we do not lint for multiple underscores
+fn multiple_underscores(__foo: u32) -> u32 {
+    __foo + 1
+}
+
+// Non-variable bindings with preceding underscore
+fn _fn_test() {}
+struct _StructTest;
+enum _EnumTest {
+    _FieldA,
+    _FieldB(_StructTest)
+}
+
+/// Test that we do not lint for non-variable bindings
+fn non_variables() {
+    _fn_test();
+    let _s = _StructTest;
+    let _e = match _EnumTest::_FieldB(_StructTest) {
+        _EnumTest::_FieldA => 0,
+        _EnumTest::_FieldB(_st) => 1,
+    };
+    let f = _fn_test;
+    f();
+}
+
+fn main() {
+    let foo = 0u32;
+    // tests of unused_underscore lint
+    let _ = prefix_underscore(foo);
+    in_macro(foo);
+    in_struct_field();
+    // possible false positives
+    let _ = non_prefix_underscore(foo);
+    let _ = unused_underscore_simple(foo);
+    let _ = unused_underscore_complex(foo);
+    let _ = multiple_underscores(foo);
+    non_variables();
+}