Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add real suggestion to option_map_unwrap_or #4634

Merged
merged 1 commit into from
Dec 30, 2019
Merged

Add real suggestion to option_map_unwrap_or #4634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]),
["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]),
["expect", ..] => lint_expect(cx, expr, arg_lists[0]),
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0], method_spans[1]),
["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]),
["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]),
Expand Down
56 changes: 32 additions & 24 deletions clippy_lints/src/methods/option_map_unwrap_or.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use crate::utils::paths;
use crate::utils::{is_copy, match_type, snippet, span_lint, span_note_and_lint};
use crate::utils::{differing_macro_contexts, paths, snippet_with_applicability, span_lint_and_then};
use crate::utils::{is_copy, match_type};
use rustc::hir::intravisit::{walk_path, NestedVisitorMap, Visitor};
use rustc::hir::{self, *};
use rustc::lint::LateContext;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use syntax::source_map::Span;
use syntax_pos::symbol::Symbol;

use super::OPTION_MAP_UNWRAP_OR;
Expand All @@ -14,6 +16,7 @@ pub(super) fn lint<'a, 'tcx>(
expr: &hir::Expr<'_>,
map_args: &'tcx [hir::Expr<'_>],
unwrap_args: &'tcx [hir::Expr<'_>],
map_span: Span,
) {
// lint if the caller of `map()` is an `Option`
if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) {
Expand All @@ -39,14 +42,19 @@ pub(super) fn lint<'a, 'tcx>(
}
}

// get snippets for args to map() and unwrap_or()
let map_snippet = snippet(cx, map_args[1].span, "..");
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
if differing_macro_contexts(unwrap_args[1].span, map_span) {
return;
}

let mut applicability = Applicability::MachineApplicable;
// get snippet for unwrap_or()
let unwrap_snippet = snippet_with_applicability(cx, unwrap_args[1].span, "..", &mut applicability);
// lint message
// comparing the snippet from source to raw text ("None") below is safe
// because we already have checked the type.
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
let suggest = if unwrap_snippet == "None" {
let unwrap_snippet_none = unwrap_snippet == "None";
let suggest = if unwrap_snippet_none {
"and_then(f)"
} else {
"map_or(a, f)"
Expand All @@ -56,24 +64,24 @@ pub(super) fn lint<'a, 'tcx>(
This can be done more directly by calling `{}` instead",
arg, suggest
);
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or() have the same span
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
if same_span && !multiline {
let suggest = if unwrap_snippet == "None" {
format!("and_then({})", map_snippet)
} else {
format!("map_or({}, {})", unwrap_snippet, map_snippet)
};
m-ober marked this conversation as resolved.
Show resolved Hide resolved
let note = format!(
"replace `map({}).unwrap_or({})` with `{}`",
map_snippet, unwrap_snippet, suggest
);
span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, &note);
} else if same_span && multiline {
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
};

span_lint_and_then(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, |db| {
let map_arg_span = map_args[1].span;

let mut suggestion = vec![
(
map_span,
String::from(if unwrap_snippet_none { "and_then" } else { "map_or" }),
),
(expr.span.with_lo(unwrap_args[0].span.hi()), String::from("")),
];

if !unwrap_snippet_none {
suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{}, ", unwrap_snippet)));
}

db.multipart_suggestion(&format!("use `{}` instead", suggest), suggestion, applicability);
});
}
}

Expand Down
76 changes: 0 additions & 76 deletions tests/ui/methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,81 +164,6 @@ impl Mul<T> for T {
}
}

/// Checks implementation of the following lints:
/// * `OPTION_MAP_UNWRAP_OR`
/// * `OPTION_MAP_UNWRAP_OR_ELSE`
#[rustfmt::skip]
fn option_methods() {
let opt = Some(1);

// Check `OPTION_MAP_UNWRAP_OR`.
// Single line case.
let _ = opt.map(|x| x + 1)
// Should lint even though this call is on a separate line.
.unwrap_or(0);
// Multi-line cases.
let _ = opt.map(|x| {
x + 1
}
).unwrap_or(0);
let _ = opt.map(|x| x + 1)
.unwrap_or({
0
});
// Single line `map(f).unwrap_or(None)` case.
let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
// Multi-line `map(f).unwrap_or(None)` cases.
let _ = opt.map(|x| {
Some(x + 1)
}
).unwrap_or(None);
let _ = opt
.map(|x| Some(x + 1))
.unwrap_or(None);
// macro case
let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint

// Should not lint if not copyable
let id: String = "identifier".to_string();
let _ = Some("prefix").map(|p| format!("{}.{}", p, id)).unwrap_or(id);
// ...but DO lint if the `unwrap_or` argument is not used in the `map`
let id: String = "identifier".to_string();
let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);

// Check OPTION_MAP_UNWRAP_OR_ELSE
// single line case
let _ = opt.map(|x| x + 1)
// Should lint even though this call is on a separate line.
.unwrap_or_else(|| 0);
// Multi-line cases.
let _ = opt.map(|x| {
x + 1
}
).unwrap_or_else(|| 0);
let _ = opt.map(|x| x + 1)
.unwrap_or_else(||
0
);
// Macro case.
// Should not lint.
let _ = opt_map!(opt, |x| x + 1).unwrap_or_else(|| 0);

// Issue #4144
{
let mut frequencies = HashMap::new();
let word = "foo";

frequencies
.get_mut(word)
.map(|count| {
*count += 1;
})
.unwrap_or_else(|| {
frequencies.insert(word.to_owned(), 1);
});
}
}

/// Checks implementation of `FILTER_NEXT` lint.
#[rustfmt::skip]
fn filter_next() {
Expand Down Expand Up @@ -302,7 +227,6 @@ fn search_is_some() {
}

fn main() {
option_methods();
filter_next();
search_is_some();
}
125 changes: 12 additions & 113 deletions tests/ui/methods.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,109 +18,8 @@ LL | | }
|
= note: `-D clippy::new-ret-no-self` implied by `-D warnings`

error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
--> $DIR/methods.rs:176:13
|
LL | let _ = opt.map(|x| x + 1)
| _____________^
LL | | // Should lint even though this call is on a separate line.
LL | | .unwrap_or(0);
| |____________________________^
|
= note: `-D clippy::option-map-unwrap-or` implied by `-D warnings`
= note: replace `map(|x| x + 1).unwrap_or(0)` with `map_or(0, |x| x + 1)`

error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
--> $DIR/methods.rs:180:13
|
LL | let _ = opt.map(|x| {
| _____________^
LL | | x + 1
LL | | }
LL | | ).unwrap_or(0);
| |____________________________^

error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
--> $DIR/methods.rs:184:13
|
LL | let _ = opt.map(|x| x + 1)
| _____________^
LL | | .unwrap_or({
LL | | 0
LL | | });
| |__________________^

error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
--> $DIR/methods.rs:189:13
|
LL | let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))`

error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
--> $DIR/methods.rs:191:13
|
LL | let _ = opt.map(|x| {
| _____________^
LL | | Some(x + 1)
LL | | }
LL | | ).unwrap_or(None);
| |_____________________^

error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
--> $DIR/methods.rs:195:13
|
LL | let _ = opt
| _____________^
LL | | .map(|x| Some(x + 1))
LL | | .unwrap_or(None);
| |________________________^
|
= note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))`

error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
--> $DIR/methods.rs:206:13
|
LL | let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: replace `map(|p| format!("{}.", p)).unwrap_or(id)` with `map_or(id, |p| format!("{}.", p))`

error: 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
--> $DIR/methods.rs:210:13
|
LL | let _ = opt.map(|x| x + 1)
| _____________^
LL | | // Should lint even though this call is on a separate line.
LL | | .unwrap_or_else(|| 0);
| |____________________________________^
|
= note: `-D clippy::option-map-unwrap-or-else` implied by `-D warnings`
= note: replace `map(|x| x + 1).unwrap_or_else(|| 0)` with `map_or_else(|| 0, |x| x + 1)`

error: 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
--> $DIR/methods.rs:214:13
|
LL | let _ = opt.map(|x| {
| _____________^
LL | | x + 1
LL | | }
LL | | ).unwrap_or_else(|| 0);
| |____________________________________^

error: 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
--> $DIR/methods.rs:218:13
|
LL | let _ = opt.map(|x| x + 1)
| _____________^
LL | | .unwrap_or_else(||
LL | | 0
LL | | );
| |_________________^

error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
--> $DIR/methods.rs:248:13
--> $DIR/methods.rs:173:13
|
LL | let _ = v.iter().filter(|&x| *x < 0).next();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -129,7 +28,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next();
= 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:251:13
--> $DIR/methods.rs:176:13
|
LL | let _ = v.iter().filter(|&x| {
| _____________^
Expand All @@ -139,33 +38,33 @@ LL | | ).next();
| |___________________________^

error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
--> $DIR/methods.rs:268:22
--> $DIR/methods.rs:193:22
|
LL | let _ = v.iter().find(|&x| *x < 0).is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)`
|
= note: `-D clippy::search-is-some` implied by `-D warnings`

error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
--> $DIR/methods.rs:269:20
--> $DIR/methods.rs:194:20
|
LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)`

error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
--> $DIR/methods.rs:270:20
--> $DIR/methods.rs:195:20
|
LL | let _ = (0..1).find(|x| *x == 0).is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:271:22
--> $DIR/methods.rs:196:22
|
LL | let _ = v.iter().find(|x| **x == 0).is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:274:13
--> $DIR/methods.rs:199:13
|
LL | let _ = v.iter().find(|&x| {
| _____________^
Expand All @@ -175,13 +74,13 @@ LL | | ).is_some();
| |______________________________^

error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
--> $DIR/methods.rs:280:22
--> $DIR/methods.rs:205:22
|
LL | let _ = v.iter().position(|&x| x < 0).is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:283:13
--> $DIR/methods.rs:208:13
|
LL | let _ = v.iter().position(|&x| {
| _____________^
Expand All @@ -191,13 +90,13 @@ LL | | ).is_some();
| |______________________________^

error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
--> $DIR/methods.rs:289:22
--> $DIR/methods.rs:214:22
|
LL | let _ = v.iter().rposition(|&x| x < 0).is_some();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:292:13
--> $DIR/methods.rs:217:13
|
LL | let _ = v.iter().rposition(|&x| {
| _____________^
Expand All @@ -206,5 +105,5 @@ LL | | }
LL | | ).is_some();
| |______________________________^

error: aborting due to 23 previous errors
error: aborting due to 13 previous errors

Loading