From f0a1eff1c47b57815bda774a7e16fcc2a39eaa65 Mon Sep 17 00:00:00 2001 From: "G. Endignoux" Date: Mon, 30 Oct 2017 13:04:26 +0100 Subject: [PATCH 1/2] Start working on #1590 --- clippy_lints/src/methods.rs | 63 ++++++++++++-- tests/ui/methods.rs | 23 +++++ tests/ui/methods.stderr | 168 ++++++++++++++++++------------------ 3 files changed, 161 insertions(+), 93 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index caccfccdda2..baa1b852fe1 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -193,6 +193,24 @@ declare_lint! { `map_or_else(g, f)`" } +/// **What it does:** Checks for usage of `result.map(_).unwrap_or_else(_)`. +/// +/// **Why is this bad?** Readability, this can be written more concisely as +/// `result.ok().map_or_else(_, _)`. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// x.map(|a| a + 1).unwrap_or_else(some_function) +/// ``` +declare_lint! { + pub RESULT_MAP_UNWRAP_OR_ELSE, + Allow, + "using `Result.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \ + `.ok().map_or_else(g, f)`" +} + /// **What it does:** Checks for usage of `_.map_or(None, _)`. /// /// **Why is this bad?** Readability, this can be written more concisely as @@ -615,6 +633,7 @@ impl LintPass for Pass { OK_EXPECT, OPTION_MAP_UNWRAP_OR, OPTION_MAP_UNWRAP_OR_ELSE, + RESULT_MAP_UNWRAP_OR_ELSE, OPTION_MAP_OR_NONE, OR_FUN_CALL, CHARS_NEXT_CMP, @@ -1241,13 +1260,25 @@ fn lint_map_unwrap_or(cx: &LateContext, expr: &hir::Expr, map_args: &[hir::Expr] } } -/// lint use of `map().unwrap_or_else()` for `Option`s -fn lint_map_unwrap_or_else<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, map_args: &'tcx [hir::Expr], unwrap_args: &'tcx [hir::Expr]) { +/// lint use of `map().unwrap_or_else()` for `Option`s and `Result`s +fn lint_map_unwrap_or_else<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &'tcx hir::Expr, + map_args: &'tcx [hir::Expr], + unwrap_args: &'tcx [hir::Expr], +) { // lint if the caller of `map()` is an `Option` - if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) { + let is_option = match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION); + let is_result = match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::RESULT); + if is_option || is_result { // 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"; + let msg = if is_option { + "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" + } else { + "called `map(f).unwrap_or_else(g)` on a Result value. This can be done more directly by calling \ + `ok().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, ".."); @@ -1258,18 +1289,32 @@ fn lint_map_unwrap_or_else<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir if same_span && !multiline { span_note_and_lint( cx, - OPTION_MAP_UNWRAP_OR_ELSE, + if is_option { + OPTION_MAP_UNWRAP_OR_ELSE + } else { + RESULT_MAP_UNWRAP_OR_ELSE + }, expr.span, msg, expr.span, &format!( - "replace `map({0}).unwrap_or_else({1})` with `map_or_else({1}, {0})`", + "replace `map({0}).unwrap_or_else({1})` with `{2}map_or_else({1}, {0})`", map_snippet, - unwrap_snippet + unwrap_snippet, + if is_result { "ok()." } else { "" } ), ); } else if same_span && multiline { - span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg); + span_lint( + cx, + if is_option { + OPTION_MAP_UNWRAP_OR_ELSE + } else { + RESULT_MAP_UNWRAP_OR_ELSE + }, + expr.span, + msg, + ); }; } } diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 24adbe943e1..9e37c894001 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -149,6 +149,29 @@ fn option_methods() { ); } +/// Checks implementation of the following lints: +/// * `RESULT_MAP_UNWRAP_OR_ELSE` +fn result_methods() { + let res: Result = Ok(1); + + // Check RESULT_MAP_UNWRAP_OR_ELSE + // single line case + let _ = res.map(|x| x + 1) + + .unwrap_or_else(|e| 0); // should lint even though this call is on a separate line + // multi line cases + let _ = res.map(|x| { + x + 1 + } + ).unwrap_or_else(|e| 0); + let _ = res.map(|x| x + 1) + .unwrap_or_else(|e| + 0 + ); + // macro case + let _ = opt_map!(res, |x| x + 1).unwrap_or_else(|e| 0); // should not lint +} + /// Struct to generate false positives for things with .iter() #[derive(Copy, Clone)] struct HasIter; diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 97e8c25ad75..a34b80823b9 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -221,251 +221,251 @@ help: try using and_then instead | error: unnecessary structure name repetition - --> $DIR/methods.rs:173:24 + --> $DIR/methods.rs:196:24 | -173 | fn filter(self) -> IteratorFalsePositives { +196 | fn filter(self) -> IteratorFalsePositives { | ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/methods.rs:177:22 + --> $DIR/methods.rs:200:22 | -177 | fn next(self) -> IteratorFalsePositives { +200 | fn next(self) -> IteratorFalsePositives { | ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/methods.rs:197:32 + --> $DIR/methods.rs:220:32 | -197 | fn skip(self, _: usize) -> IteratorFalsePositives { +220 | fn skip(self, _: usize) -> IteratorFalsePositives { | ^^^^^^^^^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:207:13 + --> $DIR/methods.rs:230:13 | -207 | let _ = v.iter().filter(|&x| *x < 0).next(); +230 | let _ = v.iter().filter(|&x| *x < 0).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D filter-next` implied by `-D warnings` = note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)` error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:210:13 + --> $DIR/methods.rs:233:13 | -210 | let _ = v.iter().filter(|&x| { +233 | let _ = v.iter().filter(|&x| { | _____________^ -211 | | *x < 0 -212 | | } -213 | | ).next(); +234 | | *x < 0 +235 | | } +236 | | ).next(); | |___________________________^ error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:225:13 + --> $DIR/methods.rs:248:13 | -225 | let _ = v.iter().find(|&x| *x < 0).is_some(); +248 | let _ = v.iter().find(|&x| *x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D search-is-some` implied by `-D warnings` = note: replace `find(|&x| *x < 0).is_some()` with `any(|&x| *x < 0)` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:228:13 + --> $DIR/methods.rs:251:13 | -228 | let _ = v.iter().find(|&x| { +251 | let _ = v.iter().find(|&x| { | _____________^ -229 | | *x < 0 -230 | | } -231 | | ).is_some(); +252 | | *x < 0 +253 | | } +254 | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:234:13 + --> $DIR/methods.rs:257:13 | -234 | let _ = v.iter().position(|&x| x < 0).is_some(); +257 | let _ = v.iter().position(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: replace `position(|&x| x < 0).is_some()` with `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:237:13 + --> $DIR/methods.rs:260:13 | -237 | let _ = v.iter().position(|&x| { +260 | let _ = v.iter().position(|&x| { | _____________^ -238 | | x < 0 -239 | | } -240 | | ).is_some(); +261 | | x < 0 +262 | | } +263 | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:243:13 + --> $DIR/methods.rs:266:13 | -243 | let _ = v.iter().rposition(|&x| x < 0).is_some(); +266 | let _ = v.iter().rposition(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: replace `rposition(|&x| x < 0).is_some()` with `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:246:13 + --> $DIR/methods.rs:269:13 | -246 | let _ = v.iter().rposition(|&x| { +269 | let _ = v.iter().rposition(|&x| { | _____________^ -247 | | x < 0 -248 | | } -249 | | ).is_some(); +270 | | x < 0 +271 | | } +272 | | ).is_some(); | |______________________________^ error: unnecessary structure name repetition - --> $DIR/methods.rs:263:21 + --> $DIR/methods.rs:286:21 | -263 | fn new() -> Foo { Foo } +286 | fn new() -> Foo { Foo } | ^^^ help: use the applicable keyword: `Self` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:281:5 + --> $DIR/methods.rs:304:5 | -281 | with_constructor.unwrap_or(make()); +304 | with_constructor.unwrap_or(make()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_constructor.unwrap_or_else(make)` | = note: `-D or-fun-call` implied by `-D warnings` error: use of `unwrap_or` followed by a call to `new` - --> $DIR/methods.rs:284:5 + --> $DIR/methods.rs:307:5 | -284 | with_new.unwrap_or(Vec::new()); +307 | with_new.unwrap_or(Vec::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:287:5 + --> $DIR/methods.rs:310:5 | -287 | with_const_args.unwrap_or(Vec::with_capacity(12)); +310 | with_const_args.unwrap_or(Vec::with_capacity(12)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_const_args.unwrap_or_else(|| Vec::with_capacity(12))` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:290:5 + --> $DIR/methods.rs:313:5 | -290 | with_err.unwrap_or(make()); +313 | with_err.unwrap_or(make()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err.unwrap_or_else(|_| make())` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:293:5 + --> $DIR/methods.rs:316:5 | -293 | with_err_args.unwrap_or(Vec::with_capacity(12)); +316 | with_err_args.unwrap_or(Vec::with_capacity(12)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err_args.unwrap_or_else(|_| Vec::with_capacity(12))` error: use of `unwrap_or` followed by a call to `default` - --> $DIR/methods.rs:296:5 + --> $DIR/methods.rs:319:5 | -296 | with_default_trait.unwrap_or(Default::default()); +319 | with_default_trait.unwrap_or(Default::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_trait.unwrap_or_default()` error: use of `unwrap_or` followed by a call to `default` - --> $DIR/methods.rs:299:5 + --> $DIR/methods.rs:322:5 | -299 | with_default_type.unwrap_or(u64::default()); +322 | with_default_type.unwrap_or(u64::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:302:5 + --> $DIR/methods.rs:325:5 | -302 | with_vec.unwrap_or(vec![]); +325 | with_vec.unwrap_or(vec![]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_vec.unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:307:5 + --> $DIR/methods.rs:330:5 | -307 | without_default.unwrap_or(Foo::new()); +330 | without_default.unwrap_or(Foo::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `without_default.unwrap_or_else(Foo::new)` error: use of `or_insert` followed by a function call - --> $DIR/methods.rs:310:5 + --> $DIR/methods.rs:333:5 | -310 | map.entry(42).or_insert(String::new()); +333 | map.entry(42).or_insert(String::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `map.entry(42).or_insert_with(String::new)` error: use of `or_insert` followed by a function call - --> $DIR/methods.rs:313:5 + --> $DIR/methods.rs:336:5 | -313 | btree.entry(42).or_insert(String::new()); +336 | btree.entry(42).or_insert(String::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `btree.entry(42).or_insert_with(String::new)` error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:316:13 + --> $DIR/methods.rs:339:13 | -316 | let _ = stringy.unwrap_or("".to_owned()); +339 | let _ = stringy.unwrap_or("".to_owned()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `stringy.unwrap_or_else(|| "".to_owned())` error: called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable - --> $DIR/methods.rs:327:23 + --> $DIR/methods.rs:350:23 | -327 | let bad_vec = some_vec.iter().nth(3); +350 | let bad_vec = some_vec.iter().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D iter-nth` implied by `-D warnings` error: called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable - --> $DIR/methods.rs:328:26 + --> $DIR/methods.rs:351:26 | -328 | let bad_slice = &some_vec[..].iter().nth(3); +351 | let bad_slice = &some_vec[..].iter().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable - --> $DIR/methods.rs:329:31 + --> $DIR/methods.rs:352:31 | -329 | let bad_boxed_slice = boxed_slice.iter().nth(3); +352 | let bad_boxed_slice = boxed_slice.iter().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `.iter().nth()` on a VecDeque. Calling `.get()` is both faster and more readable - --> $DIR/methods.rs:330:29 + --> $DIR/methods.rs:353:29 | -330 | let bad_vec_deque = some_vec_deque.iter().nth(3); +353 | let bad_vec_deque = some_vec_deque.iter().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `.iter_mut().nth()` on a Vec. Calling `.get_mut()` is both faster and more readable - --> $DIR/methods.rs:335:23 + --> $DIR/methods.rs:358:23 | -335 | let bad_vec = some_vec.iter_mut().nth(3); +358 | let bad_vec = some_vec.iter_mut().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `.iter_mut().nth()` on a slice. Calling `.get_mut()` is both faster and more readable - --> $DIR/methods.rs:338:26 + --> $DIR/methods.rs:361:26 | -338 | let bad_slice = &some_vec[..].iter_mut().nth(3); +361 | let bad_slice = &some_vec[..].iter_mut().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `.iter_mut().nth()` on a VecDeque. Calling `.get_mut()` is both faster and more readable - --> $DIR/methods.rs:341:29 + --> $DIR/methods.rs:364:29 | -341 | let bad_vec_deque = some_vec_deque.iter_mut().nth(3); +364 | let bad_vec_deque = some_vec_deque.iter_mut().nth(3); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` - --> $DIR/methods.rs:353:13 + --> $DIR/methods.rs:376:13 | -353 | let _ = some_vec.iter().skip(42).next(); +376 | let _ = some_vec.iter().skip(42).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D iter-skip-next` implied by `-D warnings` error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` - --> $DIR/methods.rs:354:13 + --> $DIR/methods.rs:377:13 | -354 | let _ = some_vec.iter().cycle().skip(42).next(); +377 | let _ = some_vec.iter().cycle().skip(42).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` - --> $DIR/methods.rs:355:13 + --> $DIR/methods.rs:378:13 | -355 | let _ = (1..10).skip(10).next(); +378 | let _ = (1..10).skip(10).next(); | ^^^^^^^^^^^^^^^^^^^^^^^ error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)` - --> $DIR/methods.rs:356:14 + --> $DIR/methods.rs:379:14 | -356 | let _ = &some_vec[..].iter().skip(3).next(); +379 | let _ = &some_vec[..].iter().skip(3).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: 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 - --> $DIR/methods.rs:365:13 + --> $DIR/methods.rs:388:13 | -365 | let _ = opt.unwrap(); +388 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From 87fd68731d5f61f11f5780a4a6fb6f5e1b1acc3f Mon Sep 17 00:00:00 2001 From: "G. Endignoux" Date: Mon, 30 Oct 2017 14:10:38 +0100 Subject: [PATCH 2/2] Update UI tests. --- clippy_lints/src/lib.rs | 1 + tests/ui/methods.stderr | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 18361f54d64..69885dec1a9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -374,6 +374,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { methods::FILTER_MAP, methods::OPTION_MAP_UNWRAP_OR, methods::OPTION_MAP_UNWRAP_OR_ELSE, + methods::RESULT_MAP_UNWRAP_OR_ELSE, methods::OPTION_UNWRAP_USED, methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index a34b80823b9..b8bac95c2d5 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -220,6 +220,38 @@ help: try using and_then instead 148 | }); | +error: called `map(f).unwrap_or_else(g)` on a Result value. This can be done more directly by calling `ok().map_or_else(g, f)` instead + --> $DIR/methods.rs:159:13 + | +159 | let _ = res.map(|x| x + 1) + | _____________^ +160 | | +161 | | .unwrap_or_else(|e| 0); // should lint even though this call is on a separate line + | |_____________________________________^ + | + = note: `-D result-map-unwrap-or-else` implied by `-D warnings` + = note: replace `map(|x| x + 1).unwrap_or_else(|e| 0)` with `ok().map_or_else(|e| 0, |x| x + 1)` + +error: called `map(f).unwrap_or_else(g)` on a Result value. This can be done more directly by calling `ok().map_or_else(g, f)` instead + --> $DIR/methods.rs:163:13 + | +163 | let _ = res.map(|x| { + | _____________^ +164 | | x + 1 +165 | | } +166 | | ).unwrap_or_else(|e| 0); + | |_____________________________________^ + +error: called `map(f).unwrap_or_else(g)` on a Result value. This can be done more directly by calling `ok().map_or_else(g, f)` instead + --> $DIR/methods.rs:167:13 + | +167 | let _ = res.map(|x| x + 1) + | _____________^ +168 | | .unwrap_or_else(|e| +169 | | 0 +170 | | ); + | |_________________^ + error: unnecessary structure name repetition --> $DIR/methods.rs:196:24 |