From c6604bb281d6f8ca77c33f15e67a26e0ceeb95a3 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 16 Jan 2016 18:47:45 +0100 Subject: [PATCH] Add a lint to warn about call to `.*or(foo(..))` --- README.md | 3 +- src/lib.rs | 1 + src/methods.rs | 61 +++++++++++++++++++++++++++++++++-- src/utils.rs | 2 +- tests/compile-fail/methods.rs | 27 ++++++++++++++++ 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 631c1d7f1d2..29644a14fc4 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 92 lints included in this crate: +There are 93 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -63,6 +63,7 @@ name [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()` +[or_fun_call](https://github.com/Manishearth/rust-clippy/wiki#or_fun_call) | warn | using any `*or` method when the `*or_else` would do [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 diff --git a/src/lib.rs b/src/lib.rs index 9bb59693795..4e13ba5c458 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -194,6 +194,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, methods::OPTION_MAP_UNWRAP_OR_ELSE, + methods::OR_FUN_CALL, methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, diff --git a/src/methods.rs b/src/methods.rs index 83b9e60f4be..ff91759be23 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -4,6 +4,7 @@ use rustc::middle::ty; use rustc::middle::subst::{Subst, TypeSpace}; use std::iter; use std::borrow::Cow; +use syntax::ptr::P; use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args, match_trait_method, walk_ptrs_ty_depth, walk_ptrs_ty}; @@ -170,6 +171,25 @@ 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()`"); +/// **What it does:** This lint checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`, etc., and +/// suggests to use `or_else`, `unwrap_or_else`, etc., instead. +/// +/// **Why is this bad?** The function will always be called and potentially allocate an object +/// in expressions such as: +/// ```rust +/// foo.unwrap_or(String::new()) +/// ``` +/// this can instead be written: +/// ```rust +/// foo.unwrap_or_else(String::new) +/// ``` +/// +/// **Known problems:** If the function as side-effects, not calling it will change the semantic of +/// the program, but you shouldn't rely on that anyway. The will won't catch +/// `foo.unwrap_or(vec![])`. +declare_lint!(pub OR_FUN_CALL, Warn, + "using any `*or` method when the `*or_else` would do"); + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, @@ -181,13 +201,15 @@ impl LintPass for MethodsPass { WRONG_PUB_SELF_CONVENTION, OK_EXPECT, OPTION_MAP_UNWRAP_OR, - OPTION_MAP_UNWRAP_OR_ELSE) + OPTION_MAP_UNWRAP_OR_ELSE, + OR_FUN_CALL) } } impl LateLintPass for MethodsPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { - if let ExprMethodCall(_, _, _) = expr.node { + if let ExprMethodCall(name, _, ref args) = expr.node { + // Chain calls if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { lint_unwrap(cx, expr, arglists[0]); } else if let Some(arglists) = method_chain_args(expr, &["to_string"]) { @@ -207,6 +229,8 @@ impl LateLintPass for MethodsPass { } else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) { lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]); } + + lint_or_fun_call(cx, expr, &name.node.as_str(), &args); } } @@ -258,6 +282,39 @@ impl LateLintPass for MethodsPass { } } +/// Checks for the `OR_FUN_CALL` lint. +fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P]) { + if args.len() == 2 && ["map_or", "ok_or", "or", "unwrap_or"].contains(&name) { + let self_ty = cx.tcx.expr_ty(&args[0]); + + let is_result = if match_type(cx, self_ty, &RESULT_PATH) { + true + } + else if match_type(cx, self_ty, &OPTION_PATH) { + false + } + else { + return; + }; + + if let ExprCall(ref fun, ref or_args) = args[1].node { + let sugg = match (is_result, or_args.is_empty()) { + (true, _) => format!("|_| {}", snippet(cx, args[1].span, "..")), + (false, false) => format!("|| {}", snippet(cx, args[1].span, "..")), + (false, true) => format!("{}", snippet(cx, fun.span, "..")), + }; + + span_lint(cx, OR_FUN_CALL, expr.span, + &format!("use of `{}` followed by a function call", name)) + .span_suggestion(expr.span, "try this", + format!("{}.{}_else({})", + snippet(cx, args[0].span, "_"), + name, + sugg)); + } + } +} + #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `unwrap()` for `Option`s and `Result`s diff --git a/src/utils.rs b/src/utils.rs index 312cf77d068..4b144452063 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -244,7 +244,7 @@ pub fn is_from_for_desugar(decl: &Decl) -> bool { /// snippet(cx, expr.span, "..") /// ``` pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> { - cx.sess().codemap().span_to_snippet(span).map(From::from).unwrap_or(Cow::Borrowed(default)) + cx.sess().codemap().span_to_snippet(span).map(From::from).unwrap_or_else(|_| Cow::Borrowed(default)) } /// Convert a span to a code snippet. Returns `None` if not available. diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index b41b28dc11e..570715db6af 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -175,6 +175,33 @@ fn search_is_some() { let _ = foo.rposition().is_some(); } +/// Checks implementation of the OR_FUN_CALL lint +fn or_fun_call() { + let foo = Some(vec![1]); + foo.unwrap_or(Vec::new()); + //~^ERROR use of `unwrap_or` + //~|HELP try this + //~|SUGGESTION foo.unwrap_or_else(Vec::new); + + let bar = Some(vec![1]); + bar.unwrap_or(Vec::with_capacity(12)); + //~^ERROR use of `unwrap_or` + //~|HELP try this + //~|SUGGESTION bar.unwrap_or_else(|| Vec::with_capacity(12)); + + let baz : Result<_, ()> = Ok(vec![1]); + baz.unwrap_or(Vec::new()); + //~^ERROR use of `unwrap_or` + //~|HELP try this + //~|SUGGESTION baz.unwrap_or_else(|_| Vec::new()); + + let qux : Result<_, ()> = Ok(vec![1]); + qux.unwrap_or(Vec::with_capacity(12)); + //~^ERROR use of `unwrap_or` + //~|HELP try this + //~|SUGGESTION qux.unwrap_or_else(|_| Vec::with_capacity(12)); +} + fn main() { use std::io;