diff --git a/README.md b/README.md index 4e4100028529..015a7d9adf26 100644 --- a/README.md +++ b/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 diff --git a/src/lib.rs b/src/lib.rs index 03e0cbea19c1..05181e69b2ce 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/misc.rs b/src/misc.rs index b5c3d0c514f1..44d044a4384c 100644 --- a/src/misc.rs +++ b/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 + } +} diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 7a18c210d0c7..f1c1adf6cc89 100644 --- a/tests/compile-fail/for_loop.rs +++ b/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; diff --git a/tests/compile-fail/range.rs b/tests/compile-fail/range.rs index 2d731670cbe3..064d9f551732 100644 --- a/tests/compile-fail/range.rs +++ b/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 } diff --git a/tests/compile-fail/used_underscore_binding.rs b/tests/compile-fail/used_underscore_binding.rs new file mode 100644 index 000000000000..39a33c96876e --- /dev/null +++ b/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(); +}