From 4438c41d145813e61fcb91ef4c4b5d24f7fd6ddd Mon Sep 17 00:00:00 2001 From: Lukas Stevens Date: Tue, 10 Oct 2017 15:35:24 +0200 Subject: [PATCH] Make suggested changes - Fix copy-paste error - Check for opt.map_or argument after ensuring that opt is an Option - Use span_lint_and_then and span_suggestion - Update reference --- clippy_lints/src/methods.rs | 36 +++++++++++++++++------------------- tests/ui/methods.stderr | 10 ++++++++-- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 08c94be27c55..d986a548576b 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -208,7 +208,7 @@ declare_lint! { pub OPTION_MAP_OR_NONE, Warn, "using `Option.map_or(None, f)`, which is more succinctly expressed as \ - `map_or_else(g, f)`" + `and_then(f)`" } /// **What it does:** Checks for usage of `_.filter(_).next()`. @@ -1243,30 +1243,28 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir /// lint use of `_.map_or(None, _)` for `Option`s fn lint_map_or_none<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, map_or_args: &'tcx [hir::Expr]) { - // check if the first non-self argument to map_or() is None - let map_or_arg_is_none = if let hir::Expr_::ExprPath(ref qpath) = map_or_args[1].node { - match_qpath(qpath, &paths::OPTION_NONE) - } else { - false - }; - if match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION) && map_or_arg_is_none { - // lint message - let msg = "called `map_or(None, f)` on an Option value. This can be done more directly by calling \ - `and_then(f)` instead"; - let map_or_none_snippet = snippet(cx, map_or_args[1].span, ".."); - let map_or_func_snippet = snippet(cx, map_or_args[2].span, ".."); - let multiline = map_or_func_snippet.lines().count() > 1 || map_or_none_snippet.lines().count() > 1; - if multiline { - span_lint(cx, OPTION_MAP_OR_NONE, expr.span, msg); + if match_type(cx, cx.tables.expr_ty(&map_or_args[0]), &paths::OPTION) { + // check if the first non-self argument to map_or() is None + let map_or_arg_is_none = if let hir::Expr_::ExprPath(ref qpath) = map_or_args[1].node { + match_qpath(qpath, &paths::OPTION_NONE) } else { - span_note_and_lint( + false + }; + + if map_or_arg_is_none { + // lint message + let msg = "called `map_or(None, f)` on an Option value. This can be done more directly by calling \ + `and_then(f)` instead"; + let map_or_self_snippet = snippet(cx, map_or_args[0].span, ".."); + let map_or_func_snippet = snippet(cx, map_or_args[2].span, ".."); + let hint = format!("{0}.and_then({1})", map_or_self_snippet, map_or_func_snippet); + span_lint_and_then( cx, OPTION_MAP_OR_NONE, expr.span, msg, - expr.span, - &format!("replace `map_or({0}, {1})` with `and_then({1})`", map_or_none_snippet, map_or_func_snippet) + |db| { db.span_suggestion(expr.span, "try using and_then instead", hint); }, ); } } diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index a2f310037f53..97e8c25ad758 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -199,10 +199,9 @@ error: called `map_or(None, f)` on an Option value. This can be done more direct --> $DIR/methods.rs:144:13 | 144 | let _ = opt.map_or(None, |x| Some(x + 1)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using and_then instead: `opt.and_then(|x| Some(x + 1))` | = note: `-D option-map-or-none` implied by `-D warnings` - = note: replace `map_or(None, |x| Some(x + 1))` with `and_then(|x| Some(x + 1))` error: called `map_or(None, f)` on an Option value. This can be done more directly by calling `and_then(f)` instead --> $DIR/methods.rs:146:13 @@ -213,6 +212,13 @@ error: called `map_or(None, f)` on an Option value. This can be done more direct 148 | | } 149 | | ); | |_________________^ + | +help: try using and_then instead + | +146 | let _ = opt.and_then(|x| { +147 | Some(x + 1) +148 | }); + | error: unnecessary structure name repetition --> $DIR/methods.rs:173:24