From 443e4556c2fec853cea4fb93f1c3e309ff5c98a4 Mon Sep 17 00:00:00 2001 From: Devon Hollowood Date: Sun, 22 Nov 2015 21:48:19 -0800 Subject: [PATCH] Add lints suggesting map_or() and map_or_else() In accordance with the latter lint, replace map().unwrap_or_else() in src/mut_mut.rs with map_or_else() --- README.md | 4 +- src/lib.rs | 2 + src/methods.rs | 72 +++++++++++++++++++++++++++++++++-- src/mut_mut.rs | 23 ++++++----- tests/compile-fail/methods.rs | 49 ++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index ae19f6ff9d7..0a10c849ca7 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 77 lints included in this crate: +There are 79 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -53,6 +53,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_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()` [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 diff --git a/src/lib.rs b/src/lib.rs index 3664803d022..12cb14f7355 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -156,6 +156,8 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::OK_EXPECT, + methods::OPTION_MAP_UNWRAP_OR, + methods::OPTION_MAP_UNWRAP_OR_ELSE, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, diff --git a/src/methods.rs b/src/methods.rs index b8c6402544d..f1b610c6211 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -5,7 +5,7 @@ use rustc::middle::subst::{Subst, TypeSpace}; use std::iter; use std::borrow::Cow; -use utils::{snippet, span_lint, match_path, match_type, walk_ptrs_ty_depth, +use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, walk_ptrs_ty_depth, walk_ptrs_ty}; use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; @@ -34,12 +34,18 @@ declare_lint!(pub WRONG_PUB_SELF_CONVENTION, Allow, declare_lint!(pub OK_EXPECT, Warn, "using `ok().expect()`, which gives worse error messages than \ calling `expect` directly on the Result"); - +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)`)"); +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)`)"); impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, - SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT) + SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT, OPTION_MAP_UNWRAP_OR, + OPTION_MAP_UNWRAP_OR_ELSE) } } @@ -92,6 +98,66 @@ impl LateLintPass for MethodsPass { } } } + // check Option.map(_).unwrap_or(_) + else if name.node.as_str() == "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(_) + else if name.node.as_str() == "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); + }; + } + } + } } } diff --git a/src/mut_mut.rs b/src/mut_mut.rs index a92338165dd..09ba7d781a2 100644 --- a/src/mut_mut.rs +++ b/src/mut_mut.rs @@ -39,17 +39,20 @@ fn check_expr_mut(cx: &LateContext, expr: &Expr) { } unwrap_addr(expr).map_or((), |e| { - unwrap_addr(e).map(|_| { - span_lint(cx, MUT_MUT, expr.span, - "generally you want to avoid `&mut &mut _` if possible") - }).unwrap_or_else(|| { - if let TyRef(_, TypeAndMut{ty: _, mutbl: MutMutable}) = - cx.tcx.expr_ty(e).sty { - span_lint(cx, MUT_MUT, expr.span, - "this expression mutably borrows a mutable reference. \ - Consider reborrowing") + unwrap_addr(e).map_or_else( + || { + if let TyRef(_, TypeAndMut{ty: _, mutbl: MutMutable}) = + cx.tcx.expr_ty(e).sty { + span_lint(cx, MUT_MUT, expr.span, + "this expression mutably borrows a mutable reference. \ + Consider reborrowing") } - }) + }, + |_| { + span_lint(cx, MUT_MUT, expr.span, + "generally you want to avoid `&mut &mut _` if possible") + } + ) }) } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 6d543596cf5..9078a78d6fe 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -34,6 +34,55 @@ impl Mul for T { fn mul(self, other: T) -> T { self } // no error, obviously } +/// Utility macro to test linting behavior in `option_methods()` +/// The lints included in `option_methods()` should not lint if the call to map is partially +/// within a macro +macro_rules! opt_map { + ($opt:expr, $map:expr) => {($opt).map($map)}; +} + +/// Checks implementation of the following lints: +/// OPTION_MAP_UNWRAP_OR +/// OPTION_MAP_UNWRAP_OR_ELSE +fn option_methods() { + let opt = Some(1); + + // Check OPTION_MAP_UNWRAP_OR + // single line case + let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)` + //~| NOTE replace this + .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)` + x + 1 + } + ).unwrap_or(0); + let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)` + .unwrap_or({ + 0 + }); + // macro case + let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint + + // 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 + .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)` + x + 1 + } + ).unwrap_or_else(|| 0); + let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or_else(g)` + .unwrap_or_else(|| + 0 + ); + // macro case + let _ = opt_map!(opt, |x| x + 1).unwrap_or_else(|| 0); // should not lint + +} + fn main() { use std::io;