From 4e054ad32007142b7e9a501a5f5c29013ea5d1c8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 31 Oct 2018 06:26:29 +0200 Subject: [PATCH 1/2] Replace big if/else expression with match --- clippy_lints/src/methods/mod.rs | 92 +++++++++++++-------------------- clippy_lints/src/utils/mod.rs | 25 ++++++++- 2 files changed, 60 insertions(+), 57 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8d0cd32e23b..dacdefd2265 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -16,12 +16,13 @@ use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc_errors::Applicability; use crate::syntax::ast; use crate::syntax::source_map::{BytePos, Span}; +use crate::syntax::symbol::LocalInternedString; use crate::utils::paths; use crate::utils::sugg; use crate::utils::{ get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type, - match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_macro_callsite, span_lint, + match_var, method_calls, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_macro_callsite, span_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq, }; use if_chain::if_chain; @@ -783,63 +784,42 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { return; } + let (method_names, arg_lists) = method_calls(expr, 2); + let method_names: Vec = method_names.iter().map(|s| s.as_str()).collect(); + let mut method_names = method_names.iter().map(|s| s.as_ref()).chain(iter::repeat("")); + + match [method_names.next().unwrap(), method_names.next().unwrap()] { + ["unwrap", "get"] => lint_get_unwrap(cx, expr, arg_lists[1], false), + ["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true), + ["unwrap", _] => lint_unwrap(cx, expr, arg_lists[0]), + ["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]), + ["unwrap_or", "map"] => lint_map_unwrap_or(cx, expr, arg_lists[1], arg_lists[0]), + ["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]), + ["map_or", _] => lint_map_or_none(cx, expr, arg_lists[0]), + ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]), + ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]), + ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]), + ["flat_map", "filter"] => lint_filter_flat_map(cx, expr, arg_lists[1], arg_lists[0]), + ["flat_map", "filter_map"] => lint_filter_map_flat_map(cx, expr, arg_lists[1], arg_lists[0]), + ["flatten", "map"] => lint_map_flatten(cx, expr, arg_lists[1]), + ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0]), + ["is_some", "position"] => lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0]), + ["is_some", "rposition"] => lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0]), + ["extend", _] => lint_extend(cx, expr, arg_lists[0]), + ["as_ptr", "unwrap"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]), + ["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false), + ["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true), + ["next", "skip"] => lint_iter_skip_next(cx, expr), + ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]), + ["as_ref", _] => lint_asref(cx, expr, "as_ref", arg_lists[0]), + ["as_mut", _] => lint_asref(cx, expr, "as_mut", arg_lists[0]), + ["fold", _] => lint_unnecessary_fold(cx, expr, arg_lists[0]), + ["filter_map", _] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]), + _ => {} + } + match expr.node { hir::ExprKind::MethodCall(ref method_call, ref method_span, ref args) => { - // Chain calls - // GET_UNWRAP needs to be checked before general `UNWRAP` lints - if let Some(arglists) = method_chain_args(expr, &["get", "unwrap"]) { - lint_get_unwrap(cx, expr, arglists[0], false); - } else if let Some(arglists) = method_chain_args(expr, &["get_mut", "unwrap"]) { - lint_get_unwrap(cx, expr, arglists[0], true); - } else if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { - lint_unwrap(cx, expr, arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["ok", "expect"]) { - lint_ok_expect(cx, expr, arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["map", "unwrap_or"]) { - lint_map_unwrap_or(cx, expr, arglists[0], arglists[1]); - } 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, &["map_or"]) { - lint_map_or_none(cx, expr, arglists[0]); - } 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, &["filter", "map"]) { - lint_filter_map(cx, expr, arglists[0], arglists[1]); - } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "map"]) { - lint_filter_map_map(cx, expr, arglists[0], arglists[1]); - } else if let Some(arglists) = method_chain_args(expr, &["filter", "flat_map"]) { - lint_filter_flat_map(cx, expr, arglists[0], arglists[1]); - } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "flat_map"]) { - lint_filter_map_flat_map(cx, expr, arglists[0], arglists[1]); - } else if let Some(arglists) = method_chain_args(expr, &["map", "flatten"]) { - lint_map_flatten(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]); - } else if let Some(arglists) = method_chain_args(expr, &["extend"]) { - lint_extend(cx, expr, arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["unwrap", "as_ptr"]) { - lint_cstring_as_ptr(cx, expr, &arglists[0][0], &arglists[1][0]); - } else if let Some(arglists) = method_chain_args(expr, &["iter", "nth"]) { - lint_iter_nth(cx, expr, arglists[0], false); - } else if let Some(arglists) = method_chain_args(expr, &["iter_mut", "nth"]) { - lint_iter_nth(cx, expr, arglists[0], true); - } else if method_chain_args(expr, &["skip", "next"]).is_some() { - lint_iter_skip_next(cx, expr); - } else if let Some(arglists) = method_chain_args(expr, &["cloned", "collect"]) { - lint_iter_cloned_collect(cx, expr, arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["as_ref"]) { - lint_asref(cx, expr, "as_ref", arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["as_mut"]) { - lint_asref(cx, expr, "as_mut", arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["fold"]) { - lint_unnecessary_fold(cx, expr, arglists[0]); - } else if let Some(arglists) = method_chain_args(expr, &["filter_map"]) { - unnecessary_filter_map::lint(cx, expr, arglists[0]); - } lint_or_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args); lint_expect_fun_call(cx, expr, *method_span, &method_call.ident.as_str(), args); diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 9d11950dd73..1cd20b68421 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -31,7 +31,7 @@ use crate::syntax::ast::{self, LitKind}; use crate::syntax::attr; use crate::syntax::source_map::{Span, DUMMY_SP}; use crate::syntax::errors::DiagnosticBuilder; -use crate::syntax::symbol::keywords; +use crate::syntax::symbol::{keywords, Symbol}; pub mod camel_case; @@ -274,6 +274,29 @@ pub fn resolve_node(cx: &LateContext<'_, '_>, qpath: &QPath, id: HirId) -> def:: cx.tables.qpath_def(qpath, id) } +/// Return the method names and argument list of nested method call expressions that make up +/// `expr`. +pub fn method_calls<'a>(expr: &'a Expr, max_depth: usize) -> (Vec, Vec<&'a [Expr]>) { + let mut method_names = Vec::with_capacity(max_depth); + let mut arg_lists = Vec::with_capacity(max_depth); + + let mut current = expr; + for _ in 0..max_depth { + if let ExprKind::MethodCall(path, _, args) = ¤t.node { + if args.iter().any(|e| in_macro(e.span)) { + break; + } + method_names.push(path.ident.name); + arg_lists.push(&**args); + current = &args[0]; + } else { + break; + } + } + + (method_names, arg_lists) +} + /// Match an `Expr` against a chain of methods, and return the matched `Expr`s. /// /// For example, if `expr` represents the `.baz()` in `foo.bar().baz()`, From 0a41dfd94630057713e0e55b28accb9455ae2183 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 1 Nov 2018 07:06:47 +0200 Subject: [PATCH 2/2] Use slice patterns instead of padding --- clippy_lints/src/methods/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index dacdefd2265..abf97def50f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -786,16 +786,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { let (method_names, arg_lists) = method_calls(expr, 2); let method_names: Vec = method_names.iter().map(|s| s.as_str()).collect(); - let mut method_names = method_names.iter().map(|s| s.as_ref()).chain(iter::repeat("")); + let method_names: Vec<&str> = method_names.iter().map(|s| s.as_ref()).collect(); - match [method_names.next().unwrap(), method_names.next().unwrap()] { + match method_names.as_slice() { ["unwrap", "get"] => lint_get_unwrap(cx, expr, arg_lists[1], false), ["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true), - ["unwrap", _] => lint_unwrap(cx, expr, arg_lists[0]), + ["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]), ["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]), ["unwrap_or", "map"] => lint_map_unwrap_or(cx, expr, arg_lists[1], arg_lists[0]), ["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]), - ["map_or", _] => lint_map_or_none(cx, expr, arg_lists[0]), + ["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]), ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]), ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]), ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]), @@ -805,16 +805,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0]), ["is_some", "position"] => lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0]), ["is_some", "rposition"] => lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0]), - ["extend", _] => lint_extend(cx, expr, arg_lists[0]), + ["extend", ..] => lint_extend(cx, expr, arg_lists[0]), ["as_ptr", "unwrap"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]), ["nth", "iter"] => lint_iter_nth(cx, expr, arg_lists[1], false), ["nth", "iter_mut"] => lint_iter_nth(cx, expr, arg_lists[1], true), ["next", "skip"] => lint_iter_skip_next(cx, expr), ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]), - ["as_ref", _] => lint_asref(cx, expr, "as_ref", arg_lists[0]), - ["as_mut", _] => lint_asref(cx, expr, "as_mut", arg_lists[0]), - ["fold", _] => lint_unnecessary_fold(cx, expr, arg_lists[0]), - ["filter_map", _] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]), + ["as_ref", ..] => lint_asref(cx, expr, "as_ref", arg_lists[0]), + ["as_mut", ..] => lint_asref(cx, expr, "as_mut", arg_lists[0]), + ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0]), + ["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]), _ => {} }