From e4fbeb49470818d6528ea9a36f245bd9d80b3297 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Wed, 23 Dec 2015 02:12:08 +0100 Subject: [PATCH 01/20] Don't trigger block_in_if_condition_expr lint if the block is unsafe --- src/block_in_if_condition.rs | 30 +++++++++++---------- tests/compile-fail/block_in_if_condition.rs | 9 +++++++ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/block_in_if_condition.rs b/src/block_in_if_condition.rs index b4e81ac6a..f7c181f5f 100644 --- a/src/block_in_if_condition.rs +++ b/src/block_in_if_condition.rs @@ -74,23 +74,25 @@ impl LateLintPass for BlockInIfCondition { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if let ExprIf(ref check, ref then, _) = expr.node { if let ExprBlock(ref block) = check.node { - if block.stmts.is_empty() { - if let Some(ref ex) = block.expr { - // don't dig into the expression here, just suggest that they remove - // the block + if block.rules == DefaultBlock { + if block.stmts.is_empty() { + if let Some(ref ex) = block.expr { + // don't dig into the expression here, just suggest that they remove + // the block - span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_EXPR, check.span, - BRACED_EXPR_MESSAGE, - &format!("try\nif {} {} ... ", snippet_block(cx, ex.span, ".."), + span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_EXPR, check.span, + BRACED_EXPR_MESSAGE, + &format!("try\nif {} {} ... ", snippet_block(cx, ex.span, ".."), + snippet_block(cx, then.span, ".."))); + } + } else { + // move block higher + span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_STMT, check.span, + COMPLEX_BLOCK_MESSAGE, + &format!("try\nlet res = {};\nif res {} ... ", + snippet_block(cx, block.span, ".."), snippet_block(cx, then.span, ".."))); } - } else { - // move block higher - span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_STMT, check.span, - COMPLEX_BLOCK_MESSAGE, - &format!("try\nlet res = {};\nif res {} ... ", - snippet_block(cx, block.span, ".."), - snippet_block(cx, then.span, ".."))); } } else { let mut visitor = ExVisitor { found_block: None }; diff --git a/tests/compile-fail/block_in_if_condition.rs b/tests/compile-fail/block_in_if_condition.rs index c075d4829..cd95fbd20 100644 --- a/tests/compile-fail/block_in_if_condition.rs +++ b/tests/compile-fail/block_in_if_condition.rs @@ -60,5 +60,14 @@ fn closure_without_block() { } } +fn condition_is_unsafe_block() { + let a: i32 = 1; + + // this should not warn because the condition is an unsafe block + if unsafe { 1u32 == std::mem::transmute(a) } { + println!("1u32 == a"); + } +} + fn main() { } From a2b842dff33abc90258c60cfaffb18443a225362 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 23 Dec 2015 07:53:01 +0530 Subject: [PATCH 02/20] Bump cargo (fixes #517) --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 609f847b4..5c35f9f42 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.0.32" +version = "0.0.33" authors = [ "Manish Goregaokar ", "Andre Bogus ", From 4958878ad219d570388a1866d13761e35122cc0a Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 22:36:37 +0100 Subject: [PATCH 03/20] Fix missing parameter in `panic!` --- src/consts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/consts.rs b/src/consts.rs index f70694765..9d3a9e7d7 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -75,7 +75,7 @@ impl Constant { if let ConstantInt(val, _) = *self { val // TODO we may want to check the sign if any } else { - panic!("Could not convert a {:?} to u64"); + panic!("Could not convert a {:?} to u64", self); } } From 592ca26e902bfa9ed3648db4ef0eeb53a5d598fe Mon Sep 17 00:00:00 2001 From: mcarton Date: Wed, 23 Dec 2015 22:37:52 +0100 Subject: [PATCH 04/20] Fix #518 --- README.md | 3 ++- src/attrs.rs | 4 ++-- src/lib.rs | 3 +++ src/panic.rs | 42 +++++++++++++++++++++++++++++++++++++ src/utils.rs | 1 + tests/compile-fail/panic.rs | 22 +++++++++++++++++++ 6 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 src/panic.rs create mode 100644 tests/compile-fail/panic.rs diff --git a/README.md b/README.md index dc8fd37df..2c8b20f83 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 85 lints included in this crate: +There are 86 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -59,6 +59,7 @@ name [option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`) [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing) | deny | out of bound constant indexing +[panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params) | warn | missing parameters in `panic!` [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | catches operations where precedence may be unclear. See the wiki for a list of cases caught [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | warn | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively [range_step_by_zero](https://github.com/Manishearth/rust-clippy/wiki#range_step_by_zero) | warn | using Range::step_by(0), which produces an infinite iterator diff --git a/src/attrs.rs b/src/attrs.rs index 10db4a551..0882f3af4 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -6,7 +6,7 @@ use reexport::*; use syntax::codemap::Span; use syntax::attr::*; use syntax::ast::{Attribute, MetaList, MetaWord}; -use utils::{in_macro, match_path, span_lint}; +use utils::{in_macro, match_path, span_lint, BEGIN_UNWIND}; /// **What it does:** This lint warns on items annotated with `#[inline(always)]`, unless the annotated function is empty or simply panics. /// @@ -94,7 +94,7 @@ fn is_relevant_expr(expr: &Expr) -> bool { ExprRet(None) | ExprBreak(_) => false, ExprCall(ref path_expr, _) => { if let ExprPath(_, ref path) = path_expr.node { - !match_path(path, &["std", "rt", "begin_unwind"]) + !match_path(path, &BEGIN_UNWIND) } else { true } } _ => true diff --git a/src/lib.rs b/src/lib.rs index 29b911a0c..adc1d402e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -67,6 +67,7 @@ pub mod cyclomatic_complexity; pub mod escape; pub mod misc_early; pub mod array_indexing; +pub mod panic; mod reexport { pub use syntax::ast::{Name, NodeId}; @@ -123,6 +124,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_early_lint_pass(box misc_early::MiscEarly); reg.register_late_lint_pass(box misc::UsedUnderscoreBinding); reg.register_late_lint_pass(box array_indexing::ArrayIndexing); + reg.register_late_lint_pass(box panic::PanicPass); reg.register_lint_group("clippy_pedantic", vec![ methods::OPTION_UNWRAP_USED, @@ -198,6 +200,7 @@ pub fn plugin_registrar(reg: &mut Registry) { needless_update::NEEDLESS_UPDATE, no_effect::NO_EFFECT, open_options::NONSENSICAL_OPEN_OPTIONS, + panic::PANIC_PARAMS, precedence::PRECEDENCE, ptr_arg::PTR_ARG, ranges::RANGE_STEP_BY_ZERO, diff --git a/src/panic.rs b/src/panic.rs new file mode 100644 index 000000000..6f713804e --- /dev/null +++ b/src/panic.rs @@ -0,0 +1,42 @@ +use rustc::lint::*; +use rustc_front::hir::*; +use syntax::ast::Lit_::LitStr; + +use utils::{span_lint, in_external_macro, match_path, BEGIN_UNWIND}; + +/// **What it does:** Warn about missing parameters in `panic!`. +/// +/// **Known problems:** Should you want to use curly brackets in `panic!` without any parameter, +/// this lint will warn. +/// +/// **Example:** +/// ``` +/// panic!("This panic! is probably missing a parameter there: {}"); +/// ``` +declare_lint!(pub PANIC_PARAMS, Warn, "missing parameters in `panic!`"); + +#[allow(missing_copy_implementations)] +pub struct PanicPass; + +impl LintPass for PanicPass { + fn get_lints(&self) -> LintArray { + lint_array!(PANIC_PARAMS) + } +} + +impl LateLintPass for PanicPass { + fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + if_let_chain! {[ + in_external_macro(cx, expr.span), + let ExprCall(ref fun, ref params) = expr.node, + params.len() == 2, + let ExprPath(None, ref path) = fun.node, + match_path(path, &BEGIN_UNWIND), + let ExprLit(ref lit) = params[0].node, + let LitStr(ref string, _) = lit.node, + string.contains('{') + ], { + span_lint(cx, PANIC_PARAMS, expr.span, "You probably are missing some parameter in your `panic!` call"); + }} + } +} diff --git a/src/utils.rs b/src/utils.rs index 92b1cb1cd..1ce97ccea 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -21,6 +21,7 @@ pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "Linke pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"]; pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"]; +pub const BEGIN_UNWIND:[&'static str; 3] = ["std", "rt", "begin_unwind"]; /// Produce a nested chain of if-lets and ifs from the patterns: /// diff --git a/tests/compile-fail/panic.rs b/tests/compile-fail/panic.rs new file mode 100644 index 000000000..36427f433 --- /dev/null +++ b/tests/compile-fail/panic.rs @@ -0,0 +1,22 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(panic_params)] + +fn missing() { + panic!("{}"); //~ERROR: You probably are missing some parameter +} + +fn ok_sigle() { + panic!("foo bar"); +} + +fn ok_multiple() { + panic!("{}", "This is {ok}"); +} + +fn main() { + missing(); + ok_sigle(); + ok_multiple(); +} From dbf1cdf34aa89cdaba1bb9993e6aed221dfceb90 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 24 Dec 2015 15:27:31 +0530 Subject: [PATCH 05/20] Fix panic lint --- src/panic.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/panic.rs b/src/panic.rs index 6f713804e..40d6e7d4d 100644 --- a/src/panic.rs +++ b/src/panic.rs @@ -28,15 +28,22 @@ impl LateLintPass for PanicPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if_let_chain! {[ in_external_macro(cx, expr.span), - let ExprCall(ref fun, ref params) = expr.node, + let ExprBlock(ref block) = expr.node, + let Some(ref ex) = block.expr, + let ExprCall(ref fun, ref params) = ex.node, params.len() == 2, let ExprPath(None, ref path) = fun.node, match_path(path, &BEGIN_UNWIND), let ExprLit(ref lit) = params[0].node, let LitStr(ref string, _) = lit.node, - string.contains('{') + string.contains('{'), + let Some(sp) = cx.sess().codemap() + .with_expn_info(expr.span.expn_id, + |info| info.map(|i| i.call_site)) ], { - span_lint(cx, PANIC_PARAMS, expr.span, "You probably are missing some parameter in your `panic!` call"); + + span_lint(cx, PANIC_PARAMS, sp, + "You probably are missing some parameter in your `panic!` call"); }} } } From f1aac931bdb687c12b77980e52458ae921f90f21 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Sun, 27 Dec 2015 01:22:53 -0800 Subject: [PATCH 06/20] Refactor `check_expr()` impl for `MethodsPass` --- src/methods.rs | 269 ++++++++++++++++++++++++++++++------------------- src/utils.rs | 22 ++++ 2 files changed, 188 insertions(+), 103 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 35207cb74..31e3bfb8f 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -5,8 +5,8 @@ use rustc::middle::subst::{Subst, TypeSpace}; use std::iter; use std::borrow::Cow; -use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, walk_ptrs_ty_depth, - walk_ptrs_ty}; +use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, match_method_chain, + walk_ptrs_ty_depth, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; use self::SelfKind::*; @@ -157,107 +157,21 @@ impl LintPass for MethodsPass { impl LateLintPass for MethodsPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { - - if let ExprMethodCall(ref name, _, ref args) = expr.node { - let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); - match &*name.node.as_str() { - "unwrap" if match_type(cx, obj_ty, &OPTION_PATH) => { - span_lint(cx, OPTION_UNWRAP_USED, expr.span, - "used unwrap() on an Option value. If you don't want \ - to handle the None case gracefully, consider using \ - expect() to provide a better panic message"); - }, - "unwrap" if match_type(cx, obj_ty, &RESULT_PATH) => { - span_lint(cx, RESULT_UNWRAP_USED, expr.span, - "used unwrap() on a Result value. Graceful handling \ - of Err values is preferred"); - }, - "to_string" if obj_ty.sty == ty::TyStr => { - let mut arg_str = snippet(cx, args[0].span, "_"); - if ptr_depth > 1 { - arg_str = Cow::Owned(format!( - "({}{})", - iter::repeat('*').take(ptr_depth - 1).collect::(), - arg_str)); - } - span_lint(cx, STR_TO_STRING, expr.span, &format!( - "`{}.to_owned()` is faster", arg_str)); - }, - "to_string" if match_type(cx, obj_ty, &STRING_PATH) => { - span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \ - `clone()` to make a copy"); - }, - "expect" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { - if inner_name.node.as_str() == "ok" - && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &RESULT_PATH) { - let result_type = cx.tcx.expr_ty(&inner_args[0]); - if let Some(error_type) = get_error_type(cx, result_type) { - if has_debug_impl(error_type, cx) { - span_lint(cx, OK_EXPECT, expr.span, - "called `ok().expect()` on a Result \ - value. You can call `expect` directly \ - on the `Result`"); - } - } - } - }, - // check Option.map(_).unwrap_or(_) - "unwrap_or" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { - if inner_name.node.as_str() == "map" - && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) { - // lint message - let msg = - "called `map(f).unwrap_or(a)` on an Option value. This can be done \ - more directly by calling `map_or(a, f)` instead"; - // get args to map() and unwrap_or() - let map_arg = snippet(cx, inner_args[1].span, ".."); - let unwrap_arg = snippet(cx, args[1].span, ".."); - // lint, with note if neither arg is > 1 line and both map() and - // unwrap_or() have the same span - let multiline = map_arg.lines().count() > 1 - || unwrap_arg.lines().count() > 1; - let same_span = inner_args[1].span.expn_id == args[1].span.expn_id; - if same_span && !multiline { - span_note_and_lint( - cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, - &format!("replace this with map_or({1}, {0})", - map_arg, unwrap_arg) - ); - } - else if same_span && multiline { - span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg); - }; - } - }, - // check Option.map(_).unwrap_or_else(_) - "unwrap_or_else" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node { - if inner_name.node.as_str() == "map" - && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) { - // lint message - let msg = - "called `map(f).unwrap_or_else(g)` on an Option value. This can be \ - done more directly by calling `map_or_else(g, f)` instead"; - // get args to map() and unwrap_or_else() - let map_arg = snippet(cx, inner_args[1].span, ".."); - let unwrap_arg = snippet(cx, args[1].span, ".."); - // lint, with note if neither arg is > 1 line and both map() and - // unwrap_or_else() have the same span - let multiline = map_arg.lines().count() > 1 - || unwrap_arg.lines().count() > 1; - let same_span = inner_args[1].span.expn_id == args[1].span.expn_id; - if same_span && !multiline { - span_note_and_lint( - cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span, - &format!("replace this with map_or_else({1}, {0})", - map_arg, unwrap_arg) - ); - } - else if same_span && multiline { - span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg); - }; - } - }, - _ => {}, + if let ExprMethodCall(_, _, _) = expr.node { + if match_method_chain(expr, &["unwrap"]) { + lint_unwrap(cx, expr); + } + else if match_method_chain(expr, &["to_string"]) { + lint_to_string(cx, expr); + } + else if match_method_chain(expr, &["ok", "expect"]) { + lint_ok_expect(cx, expr); + } + else if match_method_chain(expr, &["map", "unwrap_or"]) { + lint_map_unwrap_or(cx, expr); + } + else if match_method_chain(expr, &["map", "unwrap_or_else"]) { + lint_map_unwrap_or_else(cx, expr); } } } @@ -304,6 +218,155 @@ impl LateLintPass for MethodsPass { } } +/// lint use of `unwrap()` for `Option`s and `Result`s +fn lint_unwrap(cx: &LateContext, expr: &Expr) { + let args = match expr.node { + ExprMethodCall(_, _, ref args) => args, + _ => panic!("clippy methods.rs: should not have called `lint_unwrap()` on a non-matching \ + expression!"), + }; + + let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); + + if match_type(cx, obj_ty, &OPTION_PATH) { + span_lint(cx, OPTION_UNWRAP_USED, expr.span, + "used unwrap() on an Option value. If you don't want to handle the None case \ + gracefully, consider using expect() to provide a better panic message"); + } + else if match_type(cx, obj_ty, &RESULT_PATH) { + span_lint(cx, RESULT_UNWRAP_USED, expr.span, + "used unwrap() on a Result value. Graceful handling of Err values is preferred"); + } +} + +/// lint use of `to_string()` for `&str`s and `String`s +fn lint_to_string(cx: &LateContext, expr: &Expr) { + let args = match expr.node { + ExprMethodCall(_, _, ref args) => args, + _ => panic!("clippy methods.rs: should not have called `lint_to_string()` on a \ + non-matching expression!"), + }; + + let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); + + if obj_ty.sty == ty::TyStr { + let mut arg_str = snippet(cx, args[0].span, "_"); + if ptr_depth > 1 { + arg_str = Cow::Owned(format!( + "({}{})", + iter::repeat('*').take(ptr_depth - 1).collect::(), + arg_str)); + } + span_lint(cx, STR_TO_STRING, expr.span, + &format!("`{}.to_owned()` is faster", arg_str)); + } + else if match_type(cx, obj_ty, &STRING_PATH) { + span_lint(cx, STRING_TO_STRING, expr.span, + "`String.to_string()` is a no-op; use `clone()` to make a copy"); + } +} + +/// lint use of `ok().expect()` for `Result`s +fn lint_ok_expect(cx: &LateContext, expr: &Expr) { + let expect_args = match expr.node { + ExprMethodCall(_, _, ref expect_args) => expect_args, + _ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \ + non-matching expression!") + }; + let ok_args = match expect_args[0].node { + ExprMethodCall(_, _, ref ok_args) => ok_args, + _ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \ + non-matching expression!") + }; + + // lint if the caller of `ok()` is a `Result` + if match_type(cx, cx.tcx.expr_ty(&ok_args[0]), &RESULT_PATH) { + let result_type = cx.tcx.expr_ty(&ok_args[0]); + if let Some(error_type) = get_error_type(cx, result_type) { + if has_debug_impl(error_type, cx) { + span_lint(cx, OK_EXPECT, expr.span, + "called `ok().expect()` on a Result value. You can call `expect` \ + directly on the `Result`"); + } + } + } +} + +/// lint use of `map().unwrap_or()` for `Option`s +fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr) { + let unwrap_args = match expr.node { + ExprMethodCall(_, _, ref unwrap_args) => unwrap_args, + _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \ + non-matching expression!") + }; + let map_args = match unwrap_args[0].node { + ExprMethodCall(_, _, ref map_args) => map_args, + _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \ + non-matching expression!") + }; + + // lint if the caller of `map()` is an `Option` + if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { + // lint message + let msg = "called `map(f).unwrap_or(a)` on an Option value. This can be done more \ + directly by calling `map_or(a, f)` instead"; + // get snippets for args to map() and unwrap_or() + let map_snippet = snippet(cx, map_args[1].span, ".."); + let unwrap_snippet = snippet(cx, unwrap_args[1].span, ".."); + // lint, with note if neither arg is > 1 line and both map() and + // unwrap_or() have the same span + let multiline = map_snippet.lines().count() > 1 + || unwrap_snippet.lines().count() > 1; + let same_span = map_args[1].span.expn_id == unwrap_args[1].span.expn_id; + if same_span && !multiline { + span_note_and_lint( + cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, + &format!("replace this with map_or({1}, {0})", map_snippet, unwrap_snippet) + ); + } + else if same_span && multiline { + span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg); + }; + } +} + +/// lint use of `map().unwrap_or_else()` for `Option`s +fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr) { + let unwrap_args = match expr.node { + ExprMethodCall(_, _, ref unwrap_args) => unwrap_args, + _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \ + non-matching expression!") + }; + let map_args = match unwrap_args[0].node { + ExprMethodCall(_, _, ref map_args) => map_args, + _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \ + non-matching expression!") + }; + + if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { + // lint message + let msg = "called `map(f).unwrap_or_else(g)` on an Option value. This can be done more \ + directly by calling `map_or_else(g, f)` instead"; + // get snippets for args to map() and unwrap_or_else() + let map_snippet = snippet(cx, map_args[1].span, ".."); + let unwrap_snippet = snippet(cx, unwrap_args[1].span, ".."); + // lint, with note if neither arg is > 1 line and both map() and + // unwrap_or_else() have the same span + let multiline = map_snippet.lines().count() > 1 + || unwrap_snippet.lines().count() > 1; + let same_span = map_args[1].span.expn_id == unwrap_args[1].span.expn_id; + if same_span && !multiline { + span_note_and_lint( + cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span, + &format!("replace this with map_or_else({1}, {0})", map_snippet, unwrap_snippet) + ); + } + else if same_span && multiline { + span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg); + }; + } +} + // Given a `Result` type, return its error type (`E`) fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { diff --git a/src/utils.rs b/src/utils.rs index 1ce97ccea..479ee1425 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -136,6 +136,7 @@ pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool { false } } + /// check if method call given in "expr" belongs to given trait pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool { let method_call = ty::MethodCall::expr(expr.id); @@ -163,6 +164,27 @@ pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool { |(a, b)| a.identifier.name.as_str() == *b) } +/// match an Expr against a chain of methods. For example, if `expr` represents the `.baz()` in +/// `foo.bar().baz()`, `matched_method_chain(expr, &["bar", "baz"])` will return true. +pub fn match_method_chain(expr: &Expr, methods: &[&str]) -> bool { + let mut current = &expr.node ; + for method_name in methods.iter().rev() { // method chains are stored last -> first + if let ExprMethodCall(ref name, _, ref args) = *current { + if name.node.as_str() == *method_name { + current = &args[0].node + } + else { + return false; + } + } + else { + return false; + } + } + true +} + + /// get the name of the item the expression is in, if available pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option { let parent_id = cx.tcx.map.get_parent(expr.id); From 29b53d600f63b998eaf3d723ee0da90379b7a8b8 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Sun, 27 Dec 2015 14:15:09 -0800 Subject: [PATCH 07/20] Replace `match_method_chain()` with `method_chain_args()` --- src/methods.rs | 92 +++++++++++++++----------------------------------- src/utils.rs | 25 +++++++++----- 2 files changed, 44 insertions(+), 73 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 31e3bfb8f..d4809d374 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -5,9 +5,10 @@ use rustc::middle::subst::{Subst, TypeSpace}; use std::iter; use std::borrow::Cow; -use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, match_method_chain, +use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args, walk_ptrs_ty_depth, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; +use utils::MethodArgs; use self::SelfKind::*; use self::OutType::*; @@ -158,20 +159,20 @@ impl LintPass for MethodsPass { impl LateLintPass for MethodsPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if let ExprMethodCall(_, _, _) = expr.node { - if match_method_chain(expr, &["unwrap"]) { - lint_unwrap(cx, expr); + if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { + lint_unwrap(cx, expr, arglists[0]); } - else if match_method_chain(expr, &["to_string"]) { - lint_to_string(cx, expr); + else if let Some(arglists) = method_chain_args(expr, &["to_string"]) { + lint_to_string(cx, expr, arglists[0]); } - else if match_method_chain(expr, &["ok", "expect"]) { - lint_ok_expect(cx, expr); + else if let Some(arglists) = method_chain_args(expr, &["ok", "expect"]) { + lint_ok_expect(cx, expr, arglists[0]); } - else if match_method_chain(expr, &["map", "unwrap_or"]) { - lint_map_unwrap_or(cx, expr); + else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or"]) { + lint_map_unwrap_or(cx, expr, arglists[0], arglists[1]); } - else if match_method_chain(expr, &["map", "unwrap_or_else"]) { - lint_map_unwrap_or_else(cx, expr); + else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or_else"]) { + lint_map_unwrap_or_else(cx, expr, arglists[0], arglists[1]); } } } @@ -218,15 +219,10 @@ impl LateLintPass for MethodsPass { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `unwrap()` for `Option`s and `Result`s -fn lint_unwrap(cx: &LateContext, expr: &Expr) { - let args = match expr.node { - ExprMethodCall(_, _, ref args) => args, - _ => panic!("clippy methods.rs: should not have called `lint_unwrap()` on a non-matching \ - expression!"), - }; - - let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); +fn lint_unwrap(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs) { + let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&unwrap_args[0])); if match_type(cx, obj_ty, &OPTION_PATH) { span_lint(cx, OPTION_UNWRAP_USED, expr.span, @@ -239,18 +235,13 @@ fn lint_unwrap(cx: &LateContext, expr: &Expr) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `to_string()` for `&str`s and `String`s -fn lint_to_string(cx: &LateContext, expr: &Expr) { - let args = match expr.node { - ExprMethodCall(_, _, ref args) => args, - _ => panic!("clippy methods.rs: should not have called `lint_to_string()` on a \ - non-matching expression!"), - }; - - let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); +fn lint_to_string(cx: &LateContext, expr: &Expr, to_string_args: &MethodArgs) { + let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&to_string_args[0])); if obj_ty.sty == ty::TyStr { - let mut arg_str = snippet(cx, args[0].span, "_"); + let mut arg_str = snippet(cx, to_string_args[0].span, "_"); if ptr_depth > 1 { arg_str = Cow::Owned(format!( "({}{})", @@ -266,19 +257,9 @@ fn lint_to_string(cx: &LateContext, expr: &Expr) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `ok().expect()` for `Result`s -fn lint_ok_expect(cx: &LateContext, expr: &Expr) { - let expect_args = match expr.node { - ExprMethodCall(_, _, ref expect_args) => expect_args, - _ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \ - non-matching expression!") - }; - let ok_args = match expect_args[0].node { - ExprMethodCall(_, _, ref ok_args) => ok_args, - _ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \ - non-matching expression!") - }; - +fn lint_ok_expect(cx: &LateContext, expr: &Expr, ok_args: &MethodArgs) { // lint if the caller of `ok()` is a `Result` if match_type(cx, cx.tcx.expr_ty(&ok_args[0]), &RESULT_PATH) { let result_type = cx.tcx.expr_ty(&ok_args[0]); @@ -292,19 +273,10 @@ fn lint_ok_expect(cx: &LateContext, expr: &Expr) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or()` for `Option`s -fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr) { - let unwrap_args = match expr.node { - ExprMethodCall(_, _, ref unwrap_args) => unwrap_args, - _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \ - non-matching expression!") - }; - let map_args = match unwrap_args[0].node { - ExprMethodCall(_, _, ref map_args) => map_args, - _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \ - non-matching expression!") - }; - +fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, + map_args: &MethodArgs) { // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message @@ -330,19 +302,11 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or_else()` for `Option`s -fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr) { - let unwrap_args = match expr.node { - ExprMethodCall(_, _, ref unwrap_args) => unwrap_args, - _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \ - non-matching expression!") - }; - let map_args = match unwrap_args[0].node { - ExprMethodCall(_, _, ref map_args) => map_args, - _ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \ - non-matching expression!") - }; - +fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, + map_args: &MethodArgs) { + // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message let msg = "called `map(f).unwrap_or_else(g)` on an Option value. This can be done more \ diff --git a/src/utils.rs b/src/utils.rs index 479ee1425..2ff498b56 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -8,10 +8,13 @@ use rustc::middle::ty; use std::borrow::Cow; use syntax::ast::Lit_::*; use syntax::ast; +use syntax::ptr::P; use rustc::session::Session; use std::str::FromStr; +pub type MethodArgs = HirVec>; + // module DefPaths for certain structs/enums we check for pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"]; pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"]; @@ -164,24 +167,28 @@ pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool { |(a, b)| a.identifier.name.as_str() == *b) } -/// match an Expr against a chain of methods. For example, if `expr` represents the `.baz()` in -/// `foo.bar().baz()`, `matched_method_chain(expr, &["bar", "baz"])` will return true. -pub fn match_method_chain(expr: &Expr, methods: &[&str]) -> bool { - let mut current = &expr.node ; +/// match an Expr against a chain of methods, and return the matched Exprs. For example, if `expr` +/// represents the `.baz()` in `foo.bar().baz()`, `matched_method_chain(expr, &["bar", "baz"])` +/// will return a Vec containing the Exprs for `.bar()` and `.baz()` +pub fn method_chain_args<'a>(expr: &'a Expr, methods: &[&str]) -> Option> { + let mut current = expr; + let mut matched = Vec::with_capacity(methods.len()); for method_name in methods.iter().rev() { // method chains are stored last -> first - if let ExprMethodCall(ref name, _, ref args) = *current { + if let ExprMethodCall(ref name, _, ref args) = current.node { if name.node.as_str() == *method_name { - current = &args[0].node + matched.push(args); // build up `matched` backwards + current = &args[0] // go to parent expression } else { - return false; + return None; } } else { - return false; + return None; } } - true + matched.reverse(); // reverse `matched`, so that it is in the same order as `methods` + Some(matched) } From bbd439ec9ea3ef20edefa319b479cb06739ba52d Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Mon, 28 Dec 2015 16:56:58 -0800 Subject: [PATCH 08/20] Add FILTER_NEXT lint --- README.md | 7 ++++--- src/lib.rs | 1 + src/methods.rs | 37 ++++++++++++++++++++++++++++++++--- tests/compile-fail/methods.rs | 33 +++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 2c8b20f83..9840eda8d 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 86 lints included in this crate: +There are 87 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -28,6 +28,7 @@ name [eq_op](https://github.com/Manishearth/rust-clippy/wiki#eq_op) | warn | equal operands on both sides of a comparison or bitwise combination (e.g. `x == x`) [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do +[filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2` @@ -55,8 +56,8 @@ name [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file [ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result -[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`) -[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`) +[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)` +[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)` [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing) | deny | out of bound constant indexing [panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params) | warn | missing parameters in `panic!` diff --git a/src/lib.rs b/src/lib.rs index adc1d402e..4caf008e3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -176,6 +176,7 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::MATCH_BOOL, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, + methods::FILTER_NEXT, methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, methods::OPTION_MAP_UNWRAP_OR_ELSE, diff --git a/src/methods.rs b/src/methods.rs index d4809d374..f2f7dbdea 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -6,7 +6,7 @@ use std::iter; use std::borrow::Cow; use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args, - walk_ptrs_ty_depth, walk_ptrs_ty}; + match_trait_method, walk_ptrs_ty_depth, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; use utils::MethodArgs; @@ -135,7 +135,7 @@ declare_lint!(pub OK_EXPECT, Warn, /// **Example:** `x.map(|a| a + 1).unwrap_or(0)` declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn, "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as \ - `map_or(a, f)`)"); + `map_or(a, f)`"); /// **What it does:** This lint `Warn`s on `_.map(_).unwrap_or_else(_)`. /// @@ -146,7 +146,17 @@ declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn, /// **Example:** `x.map(|a| a + 1).unwrap_or_else(some_function)` declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn, "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \ - `map_or_else(g, f)`)"); + `map_or_else(g, f)`"); + +/// **What it does:** This lint `Warn`s on `_.filter(_).next()`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as `_.find(_)`. +/// +/// **Known problems:** None. +/// +/// **Example:** `iter.filter(|x| x == 0).next()` +declare_lint!(pub FILTER_NEXT, Warn, + "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`"); impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { @@ -174,6 +184,9 @@ impl LateLintPass for MethodsPass { else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or_else"]) { lint_map_unwrap_or_else(cx, expr, arglists[0], arglists[1]); } + else if let Some(arglists) = method_chain_args(expr, &["filter", "next"]) { + lint_filter_next(cx, expr, arglists[0]); + } } } @@ -331,6 +344,24 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodAr } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec +/// lint use of `filter().next() for Iterators` +fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) { + // lint if caller of `.filter().next()` is an Iterator + if match_trait_method(cx, expr, &["core", "iter", "Iterator"]) { + let msg = "called `filter(p).next()` on an Iterator. This is more succinctly expressed by \ + calling `.find(p)` instead."; + let filter_snippet = snippet(cx, filter_args[1].span, ".."); + if filter_snippet.lines().count() <= 1 { // add note if not multi-line + span_note_and_lint(cx, FILTER_NEXT, expr.span, msg, expr.span, + &format!("replace this with `find({})`)", filter_snippet)); + } + else { + span_lint(cx, FILTER_NEXT, expr.span, msg); + } + } +} + // Given a `Result` type, return its error type (`E`) fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 9078a78d6..d2fed0f90 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -83,6 +83,39 @@ fn option_methods() { } +/// Struct to generate false positive for FILTER_NEXT lint +struct FilterNextTest { + _foo: u32, +} + +impl FilterNextTest { + fn filter(self) -> FilterNextTest { + self + } + fn next(self) -> FilterNextTest { + self + } +} + +/// Checks implementation of FILTER_NEXT lint +fn filter_next() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + + // check single-line case + let _ = v.iter().filter(|&x| *x < 0).next(); //~ERROR called `filter(p).next()` on an Iterator. + //~| NOTE replace this + + // check multi-line case + let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an Iterator. + *x < 0 + } + ).next(); + + // check that we don't lint if the caller is not an Iterator + let foo = FilterNextTest { _foo: 0 }; + let _ = foo.filter().next(); +} + fn main() { use std::io; From e7f3fa6713c24882c5a8affe506e2c17dc50f1fa Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 29 Dec 2015 10:25:53 +0530 Subject: [PATCH 09/20] Remove * dep --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 5c35f9f42..4bae9f5aa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ name = "clippy" plugin = true [dependencies] -unicode-normalization = "*" +unicode-normalization = "0.1" [dev-dependencies] compiletest_rs = "0.0.11" From a6bd2d06227fa315c7a404d61dee9ded4a1e45bc Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Wed, 30 Dec 2015 00:38:03 -0800 Subject: [PATCH 10/20] Add SEARCH_IS_SOME lint --- README.md | 3 +- src/lib.rs | 1 + src/methods.rs | 40 +++++++++++++++++++++ tests/compile-fail/methods.rs | 66 +++++++++++++++++++++++++++++++---- 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 9840eda8d..9fe59d4fd 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 87 lints included in this crate: +There are 88 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -69,6 +69,7 @@ name [redundant_pattern](https://github.com/Manishearth/rust-clippy/wiki#redundant_pattern) | warn | using `name @ _` in a pattern [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used) | allow | using `Result.unwrap()`, which might be better handled [reverse_range_loop](https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop) | warn | Iterating over an empty range, such as `10..0` or `5..5` +[search_is_some](https://github.com/Manishearth/rust-clippy/wiki#search_is_some) | warn | using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()` [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse) | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value diff --git a/src/lib.rs b/src/lib.rs index 4caf008e3..6d39cad20 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -180,6 +180,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, methods::OPTION_MAP_UNWRAP_OR_ELSE, + methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, diff --git a/src/methods.rs b/src/methods.rs index f2f7dbdea..081e64b53 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -158,6 +158,18 @@ declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn, declare_lint!(pub FILTER_NEXT, Warn, "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`"); +/// **What it does:** This lint `Warn`s on an iterator search (such as `find()`, `position()`, or +/// `rposition()`) followed by a call to `is_some()`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as `_.any(_)`. +/// +/// **Known problems:** None. +/// +/// **Example:** `iter.find(|x| x == 0).is_some()` +declare_lint!(pub SEARCH_IS_SOME, Warn, + "using an iterator search followed by `is_some()`, which is more succinctly \ + expressed as a call to `any()`"); + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, @@ -187,6 +199,15 @@ impl LateLintPass for MethodsPass { else if let Some(arglists) = method_chain_args(expr, &["filter", "next"]) { lint_filter_next(cx, expr, arglists[0]); } + else if let Some(arglists) = method_chain_args(expr, &["find", "is_some"]) { + lint_search_is_some(cx, expr, "find", arglists[0], arglists[1]); + } + else if let Some(arglists) = method_chain_args(expr, &["position", "is_some"]) { + lint_search_is_some(cx, expr, "position", arglists[0], arglists[1]); + } + else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) { + lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]); + } } } @@ -362,6 +383,25 @@ fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) { } } +#[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec +/// lint searching an Iterator followed by `is_some()` +fn lint_search_is_some(cx: &LateContext, expr: &Expr, search_method: &str, search_args: &MethodArgs, + is_some_args: &MethodArgs) { + // lint if caller of search is an Iterator + if match_trait_method(cx, &*is_some_args[0], &["core", "iter", "Iterator"]) { + let msg = format!("called `is_some()` after searching an iterator with {}. This is more \ + succinctly expressed by calling `any()`.", search_method); + let search_snippet = snippet(cx, search_args[1].span, ".."); + if search_snippet.lines().count() <= 1 { // add note if not multi-line + span_note_and_lint(cx, SEARCH_IS_SOME, expr.span, &msg, expr.span, + &format!("replace this with `any({})`)", search_snippet)); + } + else { + span_lint(cx, SEARCH_IS_SOME, expr.span, &msg); + } + } +} + // Given a `Result` type, return its error type (`E`) fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index d2fed0f90..1878ae15b 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -83,18 +83,32 @@ fn option_methods() { } -/// Struct to generate false positive for FILTER_NEXT lint -struct FilterNextTest { - _foo: u32, +/// Struct to generate false positive for Iterator-based lints +#[derive(Copy, Clone)] +struct IteratorFalsePositives { + foo: u32, } -impl FilterNextTest { - fn filter(self) -> FilterNextTest { +impl IteratorFalsePositives { + fn filter(self) -> IteratorFalsePositives { self } - fn next(self) -> FilterNextTest { + + fn next(self) -> IteratorFalsePositives { self } + + fn find(self) -> Option { + Some(self.foo) + } + + fn position(self) -> Option { + Some(self.foo) + } + + fn rposition(self) -> Option { + Some(self.foo) + } } /// Checks implementation of FILTER_NEXT lint @@ -112,10 +126,48 @@ fn filter_next() { ).next(); // check that we don't lint if the caller is not an Iterator - let foo = FilterNextTest { _foo: 0 }; + let foo = IteratorFalsePositives { foo: 0 }; let _ = foo.filter().next(); } +/// Checks implementation of SEARCH_IS_SOME lint +fn search_is_some() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + + // check `find().is_some()`, single-line + let _ = v.iter().find(|&x| *x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `find().is_some()`, multi-line + let _ = v.iter().find(|&x| { //~ERROR called `is_some()` after searching + *x < 0 + } + ).is_some(); + + // check `position().is_some()`, single-line + let _ = v.iter().position(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `position().is_some()`, multi-line + let _ = v.iter().position(|&x| { //~ERROR called `is_some()` after searching + x < 0 + } + ).is_some(); + + // check `rposition().is_some()`, single-line + let _ = v.iter().rposition(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching + //~| NOTE replace this + // check `rposition().is_some()`, multi-line + let _ = v.iter().rposition(|&x| { //~ERROR called `is_some()` after searching + x < 0 + } + ).is_some(); + + // check that we don't lint if the caller is not an Iterator + let foo = IteratorFalsePositives { foo: 0 }; + let _ = foo.find().is_some(); + let _ = foo.position().is_some(); + let _ = foo.rposition().is_some(); +} + fn main() { use std::io; From 2c42d46468e7a047841c89d1ebdd517917bb12a2 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Wed, 30 Dec 2015 00:55:38 -0800 Subject: [PATCH 11/20] Bug fix --- src/methods.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 081e64b53..5515ba17b 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -309,8 +309,8 @@ fn lint_ok_expect(cx: &LateContext, expr: &Expr, ok_args: &MethodArgs) { #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or()` for `Option`s -fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, - map_args: &MethodArgs) { +fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, + unwrap_args: &MethodArgs) { // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message @@ -338,8 +338,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `map().unwrap_or_else()` for `Option`s -fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, unwrap_args: &MethodArgs, - map_args: &MethodArgs) { +fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, + unwrap_args: &MethodArgs) { // lint if the caller of `map()` is an `Option` if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) { // lint message From 093582c102b4ca983e9b6ef620860a75f6a1d812 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Wed, 30 Dec 2015 01:07:40 -0800 Subject: [PATCH 12/20] Make MethodsPass lint notes clearer --- src/methods.rs | 11 +++++++---- tests/compile-fail/methods.rs | 27 +++++++++++++++++---------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 5515ba17b..a6200534e 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -327,7 +327,8 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, if same_span && !multiline { span_note_and_lint( cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, - &format!("replace this with map_or({1}, {0})", map_snippet, unwrap_snippet) + &format!("replace `map({0}).unwrap_or({1})` with `map_or({1}, {0})`", map_snippet, + unwrap_snippet) ); } else if same_span && multiline { @@ -356,7 +357,8 @@ fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr, map_args: &MethodArgs, if same_span && !multiline { span_note_and_lint( cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span, - &format!("replace this with map_or_else({1}, {0})", map_snippet, unwrap_snippet) + &format!("replace `map({0}).unwrap_or_else({1})` with `with map_or_else({1}, {0})`", + map_snippet, unwrap_snippet) ); } else if same_span && multiline { @@ -375,7 +377,7 @@ fn lint_filter_next(cx: &LateContext, expr: &Expr, filter_args: &MethodArgs) { let filter_snippet = snippet(cx, filter_args[1].span, ".."); if filter_snippet.lines().count() <= 1 { // add note if not multi-line span_note_and_lint(cx, FILTER_NEXT, expr.span, msg, expr.span, - &format!("replace this with `find({})`)", filter_snippet)); + &format!("replace `filter({0}).next()` with `find({0})`", filter_snippet)); } else { span_lint(cx, FILTER_NEXT, expr.span, msg); @@ -394,7 +396,8 @@ fn lint_search_is_some(cx: &LateContext, expr: &Expr, search_method: &str, searc let search_snippet = snippet(cx, search_args[1].span, ".."); if search_snippet.lines().count() <= 1 { // add note if not multi-line span_note_and_lint(cx, SEARCH_IS_SOME, expr.span, &msg, expr.span, - &format!("replace this with `any({})`)", search_snippet)); + &format!("replace `{0}({1}).is_some()` with `any({1})`", search_method, + search_snippet)); } else { span_lint(cx, SEARCH_IS_SOME, expr.span, &msg); diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 1878ae15b..b41b28dc1 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -50,7 +50,7 @@ fn option_methods() { // Check OPTION_MAP_UNWRAP_OR // single line case let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)` - //~| NOTE replace this + //~| NOTE replace `map(|x| x + 1).unwrap_or(0)` .unwrap_or(0); // should lint even though this call is on a separate line // multi line cases let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or(a)` @@ -67,7 +67,7 @@ fn option_methods() { // Check OPTION_MAP_UNWRAP_OR_ELSE // single line case let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or_else(g)` - //~| NOTE replace this + //~| NOTE replace `map(|x| x + 1).unwrap_or_else(|| 0)` .unwrap_or_else(|| 0); // should lint even though this call is on a separate line // multi line cases let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or_else(g)` @@ -116,8 +116,9 @@ fn filter_next() { let v = vec![3, 2, 1, 0, -1, -2, -3]; // check single-line case - let _ = v.iter().filter(|&x| *x < 0).next(); //~ERROR called `filter(p).next()` on an Iterator. - //~| NOTE replace this + let _ = v.iter().filter(|&x| *x < 0).next(); + //~^ ERROR called `filter(p).next()` on an Iterator. + //~| NOTE replace `filter(|&x| *x < 0).next()` // check multi-line case let _ = v.iter().filter(|&x| { //~ERROR called `filter(p).next()` on an Iterator. @@ -135,8 +136,10 @@ fn search_is_some() { let v = vec![3, 2, 1, 0, -1, -2, -3]; // check `find().is_some()`, single-line - let _ = v.iter().find(|&x| *x < 0).is_some(); //~ERROR called `is_some()` after searching - //~| NOTE replace this + let _ = v.iter().find(|&x| *x < 0).is_some(); + //~^ ERROR called `is_some()` after searching + //~| NOTE replace `find(|&x| *x < 0).is_some()` + // check `find().is_some()`, multi-line let _ = v.iter().find(|&x| { //~ERROR called `is_some()` after searching *x < 0 @@ -144,8 +147,10 @@ fn search_is_some() { ).is_some(); // check `position().is_some()`, single-line - let _ = v.iter().position(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching - //~| NOTE replace this + let _ = v.iter().position(|&x| x < 0).is_some(); + //~^ ERROR called `is_some()` after searching + //~| NOTE replace `position(|&x| x < 0).is_some()` + // check `position().is_some()`, multi-line let _ = v.iter().position(|&x| { //~ERROR called `is_some()` after searching x < 0 @@ -153,8 +158,10 @@ fn search_is_some() { ).is_some(); // check `rposition().is_some()`, single-line - let _ = v.iter().rposition(|&x| x < 0).is_some(); //~ERROR called `is_some()` after searching - //~| NOTE replace this + let _ = v.iter().rposition(|&x| x < 0).is_some(); + //~^ ERROR called `is_some()` after searching + //~| NOTE replace `rposition(|&x| x < 0).is_some()` + // check `rposition().is_some()`, multi-line let _ = v.iter().rposition(|&x| { //~ERROR called `is_some()` after searching x < 0 From 06f30a61dd2a948c58907ef9ebb80b040e930450 Mon Sep 17 00:00:00 2001 From: Johannes Linke Date: Fri, 1 Jan 2016 17:48:19 +0100 Subject: [PATCH 13/20] Add "warn/allow by default" to lint descriptions where it was missing. --- src/attrs.rs | 2 +- src/block_in_if_condition.rs | 4 ++-- src/escape.rs | 2 +- src/loops.rs | 4 ++-- src/minmax.rs | 2 +- src/misc.rs | 2 +- src/mut_reference.rs | 2 +- src/strings.rs | 2 +- src/types.rs | 5 +++-- src/zero_div_zero.rs | 2 +- 10 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/attrs.rs b/src/attrs.rs index 0882f3af4..f0dbf390e 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -8,7 +8,7 @@ use syntax::attr::*; use syntax::ast::{Attribute, MetaList, MetaWord}; use utils::{in_macro, match_path, span_lint, BEGIN_UNWIND}; -/// **What it does:** This lint warns on items annotated with `#[inline(always)]`, unless the annotated function is empty or simply panics. +/// **What it does:** This lint `Warn`s on items annotated with `#[inline(always)]`, unless the annotated function is empty or simply panics. /// /// **Why is this bad?** While there are valid uses of this annotation (and once you know when to use it, by all means `allow` this lint), it's a common newbie-mistake to pepper one's code with it. /// diff --git a/src/block_in_if_condition.rs b/src/block_in_if_condition.rs index f7c181f5f..03265635b 100644 --- a/src/block_in_if_condition.rs +++ b/src/block_in_if_condition.rs @@ -3,7 +3,7 @@ use rustc::lint::{LateLintPass, LateContext, LintArray, LintPass}; use rustc_front::intravisit::{Visitor, walk_expr}; use utils::*; -/// **What it does:** This lint checks for `if` conditions that use blocks to contain an expression. +/// **What it does:** This lint checks for `if` conditions that use blocks to contain an expression. It is `Warn` by default. /// /// **Why is this bad?** It isn't really rust style, same as using parentheses to contain expressions. /// @@ -15,7 +15,7 @@ declare_lint! { "braces can be eliminated in conditions that are expressions, e.g `if { true } ...`" } -/// **What it does:** This lint checks for `if` conditions that use blocks containing statements, or conditions that use closures with blocks. +/// **What it does:** This lint checks for `if` conditions that use blocks containing statements, or conditions that use closures with blocks. It is `Warn` by default. /// /// **Why is this bad?** Using blocks in the condition makes it hard to read. /// diff --git a/src/escape.rs b/src/escape.rs index 63894cda9..21bf30e13 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -14,7 +14,7 @@ use utils::span_lint; pub struct EscapePass; -/// **What it does:** This lint checks for usage of `Box` where an unboxed `T` would work fine +/// **What it does:** This lint checks for usage of `Box` where an unboxed `T` would work fine. It is `Warn` by default. /// /// **Why is this bad?** This is an unnecessary allocation, and bad for performance /// diff --git a/src/loops.rs b/src/loops.rs index 5c552d2ce..9f103f4e7 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -50,7 +50,7 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn, declare_lint!{ pub ITER_NEXT_LOOP, Warn, "for-looping over `_.next()` which is probably not intended" } -/// **What it does:** This lint detects `loop + match` combinations that are easier written as a `while let` loop. +/// **What it does:** This lint detects `loop + match` combinations that are easier written as a `while let` loop. It is `Warn` by default. /// /// **Why is this bad?** The `while let` loop is usually shorter and more readable /// @@ -85,7 +85,7 @@ declare_lint!{ pub UNUSED_COLLECT, Warn, "`collect()`ing an iterator without using the result; this is usually better \ written as a for loop" } -/// **What it does:** This lint checks for loops over ranges `x..y` where both `x` and `y` are constant and `x` is greater or equal to `y`, unless the range is reversed or has a negative `.step_by(_)`. +/// **What it does:** This lint checks for loops over ranges `x..y` where both `x` and `y` are constant and `x` is greater or equal to `y`, unless the range is reversed or has a negative `.step_by(_)`. It is `Warn` by default. /// /// **Why is it bad?** Such loops will either be skipped or loop until wrap-around (in debug code, this may `panic!()`). Both options are probably not intended. /// diff --git a/src/minmax.rs b/src/minmax.rs index ac8d6f052..b63e83961 100644 --- a/src/minmax.rs +++ b/src/minmax.rs @@ -8,7 +8,7 @@ use consts::{Constant, constant_simple}; use utils::{match_def_path, span_lint}; use self::MinMax::{Min, Max}; -/// **What it does:** This lint checks for expressions where `std::cmp::min` and `max` are used to clamp values, but switched so that the result is constant. +/// **What it does:** This lint checks for expressions where `std::cmp::min` and `max` are used to clamp values, but switched so that the result is constant. It is `Warn` by default. /// /// **Why is this bad?** This is in all probability not the intended outcome. At the least it hurts readability of the code. /// diff --git a/src/misc.rs b/src/misc.rs index 139dfde36..92276961d 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -281,7 +281,7 @@ impl LateLintPass for ModuloOne { } } -/// **What it does:** This lint checks for patterns in the form `name @ _`. +/// **What it does:** This lint checks for patterns in the form `name @ _`. It is `Warn` by default. /// /// **Why is this bad?** It's almost always more readable to just use direct bindings. /// diff --git a/src/mut_reference.rs b/src/mut_reference.rs index 9ba978233..133462071 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -4,7 +4,7 @@ use utils::span_lint; use rustc::middle::ty::{TypeAndMut, TypeVariants, MethodCall, TyS}; use syntax::ptr::P; -/// **What it does:** This lint detects giving a mutable reference to a function that only requires an immutable reference. +/// **What it does:** This lint detects giving a mutable reference to a function that only requires an immutable reference. It is `Warn` by default. /// /// **Why is this bad?** The immutable reference rules out all other references to the value. Also the code misleads about the intent of the call site. /// diff --git a/src/strings.rs b/src/strings.rs index b567d9493..d60a045aa 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -11,7 +11,7 @@ use eq_op::is_exp_equal; use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr}; use utils::STRING_PATH; -/// **What it does:** This lint matches code of the form `x = x + y` (without `let`!) +/// **What it does:** This lint matches code of the form `x = x + y` (without `let`!). It is `Allow` by default. /// /// **Why is this bad?** Because this expression needs another copy as opposed to `x.push_str(y)` (in practice LLVM will usually elide it, though). Despite [llogiq](https://github.com/llogiq)'s reservations, this lint also is `allow` by default, as some people opine that it's more readable. /// diff --git a/src/types.rs b/src/types.rs index 9bc506432..f33265918 100644 --- a/src/types.rs +++ b/src/types.rs @@ -17,7 +17,7 @@ use utils::{LL_PATH, VEC_PATH}; #[allow(missing_copy_implementations)] pub struct TypePass; -/// **What it does:** This lint checks for use of `Box>` anywhere in the code. +/// **What it does:** This lint checks for use of `Box>` anywhere in the code. It is `Warn` by default. /// /// **Why is this bad?** `Vec` already keeps its contents in a separate area on the heap. So if you `Box` it, you just add another level of indirection without any benefit whatsoever. /// @@ -26,7 +26,8 @@ pub struct TypePass; /// **Example:** `struct X { values: Box> }` declare_lint!(pub BOX_VEC, Warn, "usage of `Box>`, vector elements are already on the heap"); -/// **What it does:** This lint checks for usage of any `LinkedList`, suggesting to use a `Vec` or a `VecDeque` (formerly called `RingBuf`). + +/// **What it does:** This lint checks for usage of any `LinkedList`, suggesting to use a `Vec` or a `VecDeque` (formerly called `RingBuf`). It is `Warn` by default. /// /// **Why is this bad?** Gankro says: /// diff --git a/src/zero_div_zero.rs b/src/zero_div_zero.rs index c4d7cf4a5..5a4d39316 100644 --- a/src/zero_div_zero.rs +++ b/src/zero_div_zero.rs @@ -9,7 +9,7 @@ use consts::{Constant, constant_simple, FloatWidth}; /// 0.0/0.0 with std::f32::NaN or std::f64::NaN, depending on the precision. pub struct ZeroDivZeroPass; -/// **What it does:** This lint checks for `0.0 / 0.0` +/// **What it does:** This lint checks for `0.0 / 0.0`. It is `Warn` by default. /// /// **Why is this bad?** It's less readable than `std::f32::NAN` or `std::f64::NAN` /// From b287739c0b2e4d6202d94eb2a20a385a69c74d63 Mon Sep 17 00:00:00 2001 From: Johannes Linke Date: Fri, 1 Jan 2016 17:48:46 +0100 Subject: [PATCH 14/20] Remove reference to a fixed issue --- src/map_clone.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/map_clone.rs b/src/map_clone.rs index b1ba47a9b..ef992ad08 100644 --- a/src/map_clone.rs +++ b/src/map_clone.rs @@ -8,7 +8,7 @@ use utils::{walk_ptrs_ty, walk_ptrs_ty_depth}; /// /// **Why is this bad?** It makes the code less readable. /// -/// **Known problems:** False negative: The lint currently misses mapping `Clone::clone` directly. Issue #436 is tracking this. +/// **Known problems:** None /// /// **Example:** `x.map(|e| e.clone());` declare_lint!(pub MAP_CLONE, Warn, From f89e4005784abfe0b71a2c24fbd2fa007e57a61a Mon Sep 17 00:00:00 2001 From: Johannes Linke Date: Fri, 1 Jan 2016 17:49:01 +0100 Subject: [PATCH 15/20] Minor documentation cleanups --- src/escape.rs | 4 ++-- src/mut_reference.rs | 8 ++++---- src/precedence.rs | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/escape.rs b/src/escape.rs index 21bf30e13..4b6a8258d 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -16,9 +16,9 @@ pub struct EscapePass; /// **What it does:** This lint checks for usage of `Box` where an unboxed `T` would work fine. It is `Warn` by default. /// -/// **Why is this bad?** This is an unnecessary allocation, and bad for performance +/// **Why is this bad?** This is an unnecessary allocation, and bad for performance. It is only necessary to allocate if you wish to move the box into something. /// -/// It is only necessary to allocate if you wish to move the box into something. +/// **Known problems:** None /// /// **Example:** /// diff --git a/src/mut_reference.rs b/src/mut_reference.rs index 133462071..ddfd9ddcc 100644 --- a/src/mut_reference.rs +++ b/src/mut_reference.rs @@ -36,7 +36,7 @@ impl LateLintPass for UnnecessaryMutPassed { match borrowed_table.node_types.get(&fn_expr.id) { Some(function_type) => { if let ExprPath(_, ref path) = fn_expr.node { - check_arguments(cx, &arguments, function_type, + check_arguments(cx, &arguments, function_type, &format!("{}", path)); } } @@ -50,7 +50,7 @@ impl LateLintPass for UnnecessaryMutPassed { ExprMethodCall(ref name, _, ref arguments) => { let method_call = MethodCall::expr(e.id); match borrowed_table.method_map.get(&method_call) { - Some(method_type) => check_arguments(cx, &arguments, method_type.ty, + Some(method_type) => check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str())), None => unreachable!(), // Just like above, this should never happen. }; @@ -68,9 +68,9 @@ fn check_arguments(cx: &LateContext, arguments: &[P], type_definition: &Ty TypeVariants::TyRef(_, TypeAndMut {mutbl: MutImmutable, ..}) | TypeVariants::TyRawPtr(TypeAndMut {mutbl: MutImmutable, ..}) => { if let ExprAddrOf(MutMutable, _) = argument.node { - span_lint(cx, UNNECESSARY_MUT_PASSED, + span_lint(cx, UNNECESSARY_MUT_PASSED, argument.span, &format!("The function/method \"{}\" \ - doesn't need a mutable reference", + doesn't need a mutable reference", name)); } } diff --git a/src/precedence.rs b/src/precedence.rs index 39a3e9e56..3aae9fd0d 100644 --- a/src/precedence.rs +++ b/src/precedence.rs @@ -4,7 +4,7 @@ use syntax::ast::*; use utils::{span_lint, snippet}; -/// **What it does:** This lint checks for operations where precedence may be unclear and `Warn`'s about them by default, suggesting to add parentheses. Currently it catches the following: +/// **What it does:** This lint checks for operations where precedence may be unclear and `Warn`s about them by default, suggesting to add parentheses. Currently it catches the following: /// * mixed usage of arithmetic and bit shifting/combining operators without parentheses /// * a "negative" numeric literal (which is really a unary `-` followed by a numeric literal) followed by a method call /// @@ -33,17 +33,17 @@ impl EarlyLintPass for Precedence { if let ExprBinary(Spanned { node: op, ..}, ref left, ref right) = expr.node { if !is_bit_op(op) { return; } match (is_arith_expr(left), is_arith_expr(right)) { - (true, true) => span_lint(cx, PRECEDENCE, expr.span, + (true, true) => span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `({}) {} ({})`", snippet(cx, left.span, ".."), op.to_string(), snippet(cx, right.span, ".."))), - (true, false) => span_lint(cx, PRECEDENCE, expr.span, + (true, false) => span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `({}) {} {}`", snippet(cx, left.span, ".."), op.to_string(), snippet(cx, right.span, ".."))), - (false, true) => span_lint(cx, PRECEDENCE, expr.span, + (false, true) => span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `{} {} ({})`", snippet(cx, left.span, ".."), From 1605ef6ed48fded84b7b8556c9b65809c5aba1ad Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Fri, 1 Jan 2016 02:09:03 +0530 Subject: [PATCH 16/20] Rustup to syntax::errors changes --- Cargo.toml | 2 +- src/cyclomatic_complexity.rs | 13 +-- src/eta_reduction.rs | 4 +- src/len_zero.rs | 2 +- src/minmax.rs | 2 +- src/mut_mut.rs | 8 +- src/mutex_atomic.rs | 2 +- src/precedence.rs | 23 ++++-- src/returns.rs | 8 +- src/shadow.rs | 22 +++--- src/strings.rs | 4 +- src/utils.rs | 88 ++++++++++++++------- tests/compile-fail/cyclomatic_complexity.rs | 3 +- 13 files changed, 110 insertions(+), 71 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4bae9f5aa..3ab2c9ef6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.0.33" +version = "0.0.34" authors = [ "Manish Goregaokar ", "Andre Bogus ", diff --git a/src/cyclomatic_complexity.rs b/src/cyclomatic_complexity.rs index c2cc6a8c4..7beb3296a 100644 --- a/src/cyclomatic_complexity.rs +++ b/src/cyclomatic_complexity.rs @@ -9,7 +9,7 @@ use syntax::attr::*; use syntax::ast::Attribute; use rustc_front::intravisit::{Visitor, walk_expr}; -use utils::{in_macro, LimitStack}; +use utils::{in_macro, LimitStack, span_help_and_lint}; /// **What it does:** It `Warn`s on methods with high cyclomatic complexity /// @@ -59,8 +59,8 @@ impl CyclomaticComplexity { } else { let rust_cc = cc + divergence - narms; if rust_cc > self.limit.limit() { - cx.span_lint_help(CYCLOMATIC_COMPLEXITY, span, - &format!("The function has a cyclomatic complexity of {}.", rust_cc), + span_help_and_lint(cx, CYCLOMATIC_COMPLEXITY, span, + &format!("The function has a cyclomatic complexity of {}", rust_cc), "You could split it up into multiple smaller functions"); } } @@ -140,8 +140,9 @@ fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, div: u64, span: Span) { #[cfg(not(feature="debugging"))] fn report_cc_bug(cx: &LateContext, cc: u64, narms: u64, div: u64, span: Span) { if cx.current_level(CYCLOMATIC_COMPLEXITY) != Level::Allow { - cx.sess().span_note(span, &format!("Clippy encountered a bug calculating cyclomatic complexity \ - (hide this message with `#[allow(cyclomatic_complexity)]`): \ - cc = {}, arms = {}, div = {}. Please file a bug report.", cc, narms, div)); + cx.sess().span_note_without_error(span, + &format!("Clippy encountered a bug calculating cyclomatic complexity \ + (hide this message with `#[allow(cyclomatic_complexity)]`): \ + cc = {}, arms = {}, div = {}. Please file a bug report.", cc, narms, div)); } } diff --git a/src/eta_reduction.rs b/src/eta_reduction.rs index c25228793..6b561ff7a 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -84,9 +84,9 @@ fn check_closure(cx: &LateContext, expr: &Expr) { } span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure found", - || { + |db| { if let Some(snippet) = snippet_opt(cx, caller.span) { - cx.sess().span_suggestion(expr.span, + db.span_suggestion(expr.span, "remove closure as shown:", snippet); } diff --git a/src/len_zero.rs b/src/len_zero.rs index 589b4f6eb..d574fa2c3 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -135,7 +135,7 @@ fn check_len_zero(cx: &LateContext, span: Span, name: &Name, has_is_empty(cx, &args[0]) { span_lint(cx, LEN_ZERO, span, &format!( "consider replacing the len comparison with `{}{}.is_empty()`", - op, snippet(cx, args[0].span, "_"))) + op, snippet(cx, args[0].span, "_"))); } } } diff --git a/src/minmax.rs b/src/minmax.rs index b63e83961..2a8e064f9 100644 --- a/src/minmax.rs +++ b/src/minmax.rs @@ -37,7 +37,7 @@ impl LateLintPass for MinMaxPass { (_, None) | (Max, Some(Less)) | (Min, Some(Greater)) => (), _ => { span_lint(cx, MIN_MAX, expr.span, - "this min/max combination leads to constant result") + "this min/max combination leads to constant result"); } } } diff --git a/src/mut_mut.rs b/src/mut_mut.rs index ade688d37..db6f0e032 100644 --- a/src/mut_mut.rs +++ b/src/mut_mut.rs @@ -30,8 +30,8 @@ impl LateLintPass for MutMut { } fn check_ty(&mut self, cx: &LateContext, ty: &Ty) { - unwrap_mut(ty).and_then(unwrap_mut).map_or((), |_| span_lint(cx, MUT_MUT, - ty.span, "generally you want to avoid `&mut &mut _` if possible")) + unwrap_mut(ty).and_then(unwrap_mut).map_or((), |_| { span_lint(cx, MUT_MUT, + ty.span, "generally you want to avoid `&mut &mut _` if possible"); }); } } @@ -52,12 +52,12 @@ fn check_expr_mut(cx: &LateContext, expr: &Expr) { cx.tcx.expr_ty(e).sty { span_lint(cx, MUT_MUT, expr.span, "this expression mutably borrows a mutable reference. \ - Consider reborrowing") + Consider reborrowing"); } }, |_| { span_lint(cx, MUT_MUT, expr.span, - "generally you want to avoid `&mut &mut _` if possible") + "generally you want to avoid `&mut &mut _` if possible"); } ) }) diff --git a/src/mutex_atomic.rs b/src/mutex_atomic.rs index 40f7d21f4..8899eb56d 100644 --- a/src/mutex_atomic.rs +++ b/src/mutex_atomic.rs @@ -63,7 +63,7 @@ impl LateLintPass for MutexAtomic { ty::TyInt(t) if t != ast::TyIs => span_lint(cx, MUTEX_INTEGER, expr.span, &msg), _ => span_lint(cx, MUTEX_ATOMIC, expr.span, &msg) - } + }; } } } diff --git a/src/precedence.rs b/src/precedence.rs index 3aae9fd0d..91ff0680b 100644 --- a/src/precedence.rs +++ b/src/precedence.rs @@ -33,21 +33,27 @@ impl EarlyLintPass for Precedence { if let ExprBinary(Spanned { node: op, ..}, ref left, ref right) = expr.node { if !is_bit_op(op) { return; } match (is_arith_expr(left), is_arith_expr(right)) { - (true, true) => span_lint(cx, PRECEDENCE, expr.span, + (true, true) => { + span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `({}) {} ({})`", snippet(cx, left.span, ".."), - op.to_string(), snippet(cx, right.span, ".."))), - (true, false) => span_lint(cx, PRECEDENCE, expr.span, + op.to_string(), snippet(cx, right.span, ".."))); + }, + (true, false) => { + span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `({}) {} {}`", snippet(cx, left.span, ".."), - op.to_string(), snippet(cx, right.span, ".."))), - (false, true) => span_lint(cx, PRECEDENCE, expr.span, + op.to_string(), snippet(cx, right.span, ".."))); + }, + (false, true) => { + span_lint(cx, PRECEDENCE, expr.span, &format!("operator precedence can trip the unwary. \ Consider parenthesizing your expression:\ `{} {} ({})`", snippet(cx, left.span, ".."), - op.to_string(), snippet(cx, right.span, ".."))), + op.to_string(), snippet(cx, right.span, ".."))); + }, _ => (), } } @@ -57,12 +63,13 @@ impl EarlyLintPass for Precedence { if let Some(slf) = args.first() { if let ExprLit(ref lit) = slf.node { match lit.node { - LitInt(..) | LitFloat(..) | LitFloatUnsuffixed(..) => + LitInt(..) | LitFloat(..) | LitFloatUnsuffixed(..) => { span_lint(cx, PRECEDENCE, expr.span, &format!( "unary minus has lower precedence than \ method call. Consider adding parentheses \ to clarify your intent: -({})", - snippet(cx, rhs.span, ".."))), + snippet(cx, rhs.span, ".."))); + } _ => () } } diff --git a/src/returns.rs b/src/returns.rs index 3ef94a4c9..15f9bb80d 100644 --- a/src/returns.rs +++ b/src/returns.rs @@ -75,9 +75,9 @@ impl ReturnPass { if in_external_macro(cx, spans.1) {return;} span_lint_and_then(cx, NEEDLESS_RETURN, spans.0, "unneeded return statement", - || { + |db| { if let Some(snippet) = snippet_opt(cx, spans.1) { - cx.sess().span_suggestion(spans.0, + db.span_suggestion(spans.0, "remove `return` as shown:", snippet); } @@ -105,11 +105,11 @@ impl ReturnPass { fn emit_let_lint(&mut self, cx: &EarlyContext, lint_span: Span, note_span: Span) { if in_external_macro(cx, note_span) {return;} - span_lint(cx, LET_AND_RETURN, lint_span, + let mut db = span_lint(cx, LET_AND_RETURN, lint_span, "returning the result of a let binding from a block. \ Consider returning the expression directly."); if cx.current_level(LET_AND_RETURN) != Level::Allow { - cx.sess().span_note(note_span, + db.span_note(note_span, "this expression can be directly returned"); } } diff --git a/src/shadow.rs b/src/shadow.rs index 27bed50c2..1210590bb 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -7,7 +7,7 @@ use rustc_front::intravisit::{Visitor, FnKind}; use rustc::lint::*; use rustc::middle::def::Def::{DefVariant, DefStruct}; -use utils::{is_from_for_desugar, in_external_macro, snippet, span_lint, span_note_and_lint}; +use utils::{is_from_for_desugar, in_external_macro, snippet, span_lint, span_note_and_lint, DiagnosticWrapper}; /// **What it does:** This lint checks for bindings that shadow other bindings already in scope, while just changing reference level or mutability. It is `Allow` by default. /// @@ -180,39 +180,39 @@ fn check_pat(cx: &LateContext, pat: &Pat, init: &Option<&Expr>, span: Span, fn lint_shadow(cx: &LateContext, name: Name, span: Span, lspan: Span, init: &Option, prev_span: Span) where T: Deref { - fn note_orig(cx: &LateContext, lint: &'static Lint, span: Span) { + fn note_orig(cx: &LateContext, mut db: DiagnosticWrapper, lint: &'static Lint, span: Span) { if cx.current_level(lint) != Level::Allow { - cx.sess().span_note(span, "previous binding is here"); + db.span_note(span, "previous binding is here"); } } if let Some(ref expr) = *init { if is_self_shadow(name, expr) { - span_lint(cx, SHADOW_SAME, span, &format!( + let db = span_lint(cx, SHADOW_SAME, span, &format!( "{} is shadowed by itself in {}", snippet(cx, lspan, "_"), snippet(cx, expr.span, ".."))); - note_orig(cx, SHADOW_SAME, prev_span); + note_orig(cx, db, SHADOW_SAME, prev_span); } else { if contains_self(name, expr) { - span_note_and_lint(cx, SHADOW_REUSE, lspan, &format!( + let db = span_note_and_lint(cx, SHADOW_REUSE, lspan, &format!( "{} is shadowed by {} which reuses the original value", snippet(cx, lspan, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); - note_orig(cx, SHADOW_REUSE, prev_span); + note_orig(cx, db, SHADOW_REUSE, prev_span); } else { - span_note_and_lint(cx, SHADOW_UNRELATED, lspan, &format!( + let db = span_note_and_lint(cx, SHADOW_UNRELATED, lspan, &format!( "{} is shadowed by {}", snippet(cx, lspan, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); - note_orig(cx, SHADOW_UNRELATED, prev_span); + note_orig(cx, db, SHADOW_UNRELATED, prev_span); } } } else { - span_lint(cx, SHADOW_UNRELATED, span, &format!( + let db = span_lint(cx, SHADOW_UNRELATED, span, &format!( "{} shadows a previous declaration", snippet(cx, lspan, "_"))); - note_orig(cx, SHADOW_UNRELATED, prev_span); + note_orig(cx, db, SHADOW_UNRELATED, prev_span); } } diff --git a/src/strings.rs b/src/strings.rs index d60a045aa..861ae0bb0 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -75,13 +75,13 @@ impl LateLintPass for StringAdd { } span_lint(cx, STRING_ADD, e.span, "you added something to a string. \ - Consider using `String::push_str()` instead") + Consider using `String::push_str()` instead"); } } else if let ExprAssign(ref target, ref src) = e.node { if is_string(cx, target) && is_add(cx, src, target) { span_lint(cx, STRING_ADD_ASSIGN, e.span, "you assigned the result of adding something to this string. \ - Consider using `String::push_str()` instead") + Consider using `String::push_str()` instead"); } } } diff --git a/src/utils.rs b/src/utils.rs index 2ff498b56..7c0ee09b3 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -8,10 +8,12 @@ use rustc::middle::ty; use std::borrow::Cow; use syntax::ast::Lit_::*; use syntax::ast; +use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; use rustc::session::Session; use std::str::FromStr; +use std::ops::{Deref, DerefMut}; pub type MethodArgs = HirVec>; @@ -307,63 +309,91 @@ pub fn get_enclosing_block<'c>(cx: &'c LateContext, node: NodeId) -> Option<&'c } else { None } } -#[cfg(not(feature="structured_logging"))] -pub fn span_lint(cx: &T, lint: &'static Lint, sp: Span, msg: &str) { - cx.span_lint(lint, sp, msg); - if cx.current_level(lint) != Level::Allow { - cx.sess().fileline_help(sp, &format!("for further information visit \ - https://github.com/Manishearth/rust-clippy/wiki#{}", - lint.name_lower())) +pub struct DiagnosticWrapper<'a>(pub DiagnosticBuilder<'a>); + +impl<'a> Drop for DiagnosticWrapper<'a> { + fn drop(&mut self) { + self.0.emit(); } } +impl<'a> DerefMut for DiagnosticWrapper<'a> { + fn deref_mut(&mut self) -> &mut DiagnosticBuilder<'a> { + &mut self.0 + } +} + +impl<'a> Deref for DiagnosticWrapper<'a> { + type Target = DiagnosticBuilder<'a>; + fn deref(&self) -> &DiagnosticBuilder<'a> { + &self.0 + } +} + +#[cfg(not(feature="structured_logging"))] +pub fn span_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, + sp: Span, msg: &str) -> DiagnosticWrapper<'a> { + let mut db = cx.struct_span_lint(lint, sp, msg); + if cx.current_level(lint) != Level::Allow { + db.fileline_help(sp, &format!("for further information visit \ + https://github.com/Manishearth/rust-clippy/wiki#{}", + lint.name_lower())); + } + DiagnosticWrapper(db) +} + #[cfg(feature="structured_logging")] -pub fn span_lint(cx: &T, lint: &'static Lint, sp: Span, msg: &str) { +pub fn span_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, + sp: Span, msg: &str) -> DiagnosticWrapper<'a> { // lint.name / lint.desc is can give details of the lint // cx.sess().codemap() has all these nice functions for line/column/snippet details // http://doc.rust-lang.org/syntax/codemap/struct.CodeMap.html#method.span_to_string - cx.span_lint(lint, sp, msg); + let mut db = cx.struct_span_lint(lint, sp, msg); if cx.current_level(lint) != Level::Allow { - cx.sess().fileline_help(sp, &format!("for further information visit \ + db.fileline_help(sp, &format!("for further information visit \ https://github.com/Manishearth/rust-clippy/wiki#{}", - lint.name_lower())) + lint.name_lower())); } + DiagnosticWrapper(db) } -pub fn span_help_and_lint(cx: &T, lint: &'static Lint, span: Span, - msg: &str, help: &str) { - cx.span_lint(lint, span, msg); +pub fn span_help_and_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, span: Span, + msg: &str, help: &str) -> DiagnosticWrapper<'a> { + let mut db = cx.struct_span_lint(lint, span, msg); if cx.current_level(lint) != Level::Allow { - cx.sess().fileline_help(span, &format!("{}\nfor further information \ + db.fileline_help(span, &format!("{}\nfor further information \ visit https://github.com/Manishearth/rust-clippy/wiki#{}", - help, lint.name_lower())) + help, lint.name_lower())); } + DiagnosticWrapper(db) } -pub fn span_note_and_lint(cx: &T, lint: &'static Lint, span: Span, - msg: &str, note_span: Span, note: &str) { - cx.span_lint(lint, span, msg); +pub fn span_note_and_lint<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, span: Span, + msg: &str, note_span: Span, note: &str) -> DiagnosticWrapper<'a> { + let mut db = cx.struct_span_lint(lint, span, msg); if cx.current_level(lint) != Level::Allow { if note_span == span { - cx.sess().fileline_note(note_span, note) + db.fileline_note(note_span, note); } else { - cx.sess().span_note(note_span, note) + db.span_note(note_span, note); } - cx.sess().fileline_help(span, &format!("for further information visit \ + db.fileline_help(span, &format!("for further information visit \ https://github.com/Manishearth/rust-clippy/wiki#{}", - lint.name_lower())) + lint.name_lower())); } + DiagnosticWrapper(db) } -pub fn span_lint_and_then(cx: &T, lint: &'static Lint, sp: Span, - msg: &str, f: F) where F: Fn() { - cx.span_lint(lint, sp, msg); +pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, + msg: &str, f: F) -> DiagnosticWrapper<'a> where F: Fn(&mut DiagnosticWrapper) { + let mut db = DiagnosticWrapper(cx.struct_span_lint(lint, sp, msg)); if cx.current_level(lint) != Level::Allow { - f(); - cx.sess().fileline_help(sp, &format!("for further information visit \ + f(&mut db); + db.fileline_help(sp, &format!("for further information visit \ https://github.com/Manishearth/rust-clippy/wiki#{}", - lint.name_lower())) + lint.name_lower())); } + db } /// return the base type for references and raw pointers diff --git a/tests/compile-fail/cyclomatic_complexity.rs b/tests/compile-fail/cyclomatic_complexity.rs index 1a6dfd287..f79440af1 100644 --- a/tests/compile-fail/cyclomatic_complexity.rs +++ b/tests/compile-fail/cyclomatic_complexity.rs @@ -4,7 +4,8 @@ #![deny(cyclomatic_complexity)] #![allow(unused)] -fn main() { //~ ERROR: The function has a cyclomatic complexity of 28. + +fn main() { //~ERROR The function has a cyclomatic complexity of 28 if true { println!("a"); } From c11d140ebf1f4027868a52aa5d67e28e567ef844 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sat, 2 Jan 2016 16:10:31 +0530 Subject: [PATCH 17/20] Bump to 35 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 3ab2c9ef6..2b203e8b7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.0.34" +version = "0.0.35" authors = [ "Manish Goregaokar ", "Andre Bogus ", From 32cf6e32f66980cea7630edacaa66d1bd0497b2e Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sat, 2 Jan 2016 16:33:44 +0530 Subject: [PATCH 18/20] Improve documentation on match_ref_pats (fixes #532) --- README.md | 2 +- src/matches.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 9fe59d4fd..0e0a60537 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ name [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist) | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque [map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone) | warn | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead) [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool) | warn | a match on boolean expression; recommends `if..else` block instead -[match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead +[match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match or `if let` has all arms prefixed with `&`; the match expression can be dereferenced instead [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | warn | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | allow | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) diff --git a/src/matches.rs b/src/matches.rs index 460893d93..aaf41a391 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -22,7 +22,7 @@ use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_bloc declare_lint!(pub SINGLE_MATCH, Warn, "a match statement with a single nontrivial arm (i.e, where the other arm \ is `_ => {}`) is used; recommends `if let` instead"); -/// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It is `Warn` by default. +/// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It also checks for `if let &foo = bar` blocks. It is `Warn` by default. /// /// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code. /// @@ -38,7 +38,7 @@ declare_lint!(pub SINGLE_MATCH, Warn, /// } /// ``` declare_lint!(pub MATCH_REF_PATS, Warn, - "a match has all arms prefixed with `&`; the match expression can be \ + "a match or `if let` has all arms prefixed with `&`; the match expression can be \ dereferenced instead"); /// **What it does:** This lint checks for matches where match expression is a `bool`. It suggests to replace the expression with an `if...else` block. It is `Warn` by default. /// From a745efd5665b1c9a0d7e6af3f34493093896b701 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sat, 2 Jan 2016 21:41:48 +0530 Subject: [PATCH 19/20] Add smarter macro check for block_in_if (fixes #528) --- src/block_in_if_condition.rs | 7 ++++++- src/utils.rs | 4 ++++ tests/compile-fail/block_in_if_condition.rs | 9 +++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/block_in_if_condition.rs b/src/block_in_if_condition.rs index 03265635b..ce01f591c 100644 --- a/src/block_in_if_condition.rs +++ b/src/block_in_if_condition.rs @@ -79,13 +79,18 @@ impl LateLintPass for BlockInIfCondition { if let Some(ref ex) = block.expr { // don't dig into the expression here, just suggest that they remove // the block - + if differing_macro_contexts(expr.span, ex.span) { + return; + } span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_EXPR, check.span, BRACED_EXPR_MESSAGE, &format!("try\nif {} {} ... ", snippet_block(cx, ex.span, ".."), snippet_block(cx, then.span, ".."))); } } else { + if differing_macro_contexts(expr.span, block.stmts[0].span) { + return; + } // move block higher span_help_and_lint(cx, BLOCK_IN_IF_CONDITION_STMT, check.span, COMPLEX_BLOCK_MESSAGE, diff --git a/src/utils.rs b/src/utils.rs index 7c0ee09b3..90e8e27b4 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -74,6 +74,10 @@ macro_rules! if_let_chain { }; } +/// Returns true if the two spans come from differing expansions (i.e. one is from a macro and one isn't) +pub fn differing_macro_contexts(sp1: Span, sp2: Span) -> bool { + sp1.expn_id != sp2.expn_id +} /// returns true if this expn_info was expanded by any macro pub fn in_macro(cx: &T, span: Span) -> bool { cx.sess().codemap().with_expn_info(span.expn_id, diff --git a/tests/compile-fail/block_in_if_condition.rs b/tests/compile-fail/block_in_if_condition.rs index cd95fbd20..0a68d80c3 100644 --- a/tests/compile-fail/block_in_if_condition.rs +++ b/tests/compile-fail/block_in_if_condition.rs @@ -5,6 +5,15 @@ #![deny(block_in_if_condition_stmt)] #![allow(unused)] + +macro_rules! blocky { + () => {{true}} +} + +fn macro_if() { + if blocky!() { + } +} fn condition_has_block() -> i32 { if { //~ERROR in an 'if' condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a 'let' From d8d3ee907bafc690f466e34ab790f568dbeeea36 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Sat, 2 Jan 2016 21:49:53 +0530 Subject: [PATCH 20/20] Add macro check for box vec (fixes #529) --- src/types.rs | 7 ++++--- tests/compile-fail/box_vec.rs | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/types.rs b/src/types.rs index f33265918..e119ef634 100644 --- a/src/types.rs +++ b/src/types.rs @@ -9,9 +9,7 @@ use syntax::ast::IntTy::*; use syntax::ast::UintTy::*; use syntax::ast::FloatTy::*; -use utils::{match_type, snippet, span_lint, span_help_and_lint}; -use utils::{is_from_for_desugar, in_macro, in_external_macro}; -use utils::{LL_PATH, VEC_PATH}; +use utils::*; /// Handles all the linting of funky types #[allow(missing_copy_implementations)] @@ -50,6 +48,9 @@ impl LintPass for TypePass { impl LateLintPass for TypePass { fn check_ty(&mut self, cx: &LateContext, ast_ty: &Ty) { + if in_macro(cx, ast_ty.span) { + return + } if let Some(ty) = cx.tcx.ast_ty_to_ty_cache.borrow().get(&ast_ty.id) { if let ty::TyBox(ref inner) = ty.sty { if match_type(cx, inner, &VEC_PATH) { diff --git a/tests/compile-fail/box_vec.rs b/tests/compile-fail/box_vec.rs index 58e780f19..4fd98cd52 100644 --- a/tests/compile-fail/box_vec.rs +++ b/tests/compile-fail/box_vec.rs @@ -3,6 +3,15 @@ #![plugin(clippy)] #![deny(clippy)] +macro_rules! boxit { + ($init:expr, $x:ty) => { + let _: Box<$x> = Box::new($init); + } +} + +fn test_macro() { + boxit!(Vec::new(), Vec); +} pub fn test(foo: Box>) { //~ ERROR you seem to be trying to use `Box>` println!("{:?}", foo.get(0)) } @@ -14,4 +23,5 @@ pub fn test2(foo: Box)>) { // pass if #31 is fixed fn main(){ test(Box::new(Vec::new())); test2(Box::new(|v| println!("{:?}", v))); + test_macro(); }