Skip to content

Commit cecaca3

Browse files
borsbors
committed
Auto merge of #4634 - m-ober:feature/2320-map-or, r=llogiq
Add real suggestion to option_map_unwrap_or changelog: Add real suggestion to `option_map_unwrap_or` Fixes #2320
2 parents 5b25588 + c5046fd commit cecaca3

File tree

6 files changed

+271
-214
lines changed

6 files changed

+271
-214
lines changed

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1167,7 +1167,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
11671167
["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]),
11681168
["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]),
11691169
["expect", ..] => lint_expect(cx, expr, arg_lists[0]),
1170-
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
1170+
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0], method_spans[1]),
11711171
["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]),
11721172
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
11731173
["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]),

‎clippy_lints/src/methods/option_map_unwrap_or.rs‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
use crate::utils::paths;
2-
use crate::utils::{is_copy, match_type, snippet, span_lint, span_note_and_lint};
1+
use crate::utils::{differing_macro_contexts, paths, snippet_with_applicability, span_lint_and_then};
2+
use crate::utils::{is_copy, match_type};
33
use rustc::hir::intravisit::{walk_path, NestedVisitorMap, Visitor};
44
use rustc::hir::{self, *};
55
use rustc::lint::LateContext;
66
use rustc_data_structures::fx::FxHashSet;
7+
use rustc_errors::Applicability;
8+
use syntax::source_map::Span;
79
use syntax_pos::symbol::Symbol;
810

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

42-
// get snippets for args to map() and unwrap_or()
43-
let map_snippet = snippet(cx, map_args[1].span, "..");
44-
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
45+
if differing_macro_contexts(unwrap_args[1].span, map_span) {
46+
return;
47+
}
48+
49+
let mut applicability = Applicability::MachineApplicable;
50+
// get snippet for unwrap_or()
51+
let unwrap_snippet = snippet_with_applicability(cx, unwrap_args[1].span, "..", &mut applicability);
4552
// lint message
4653
// comparing the snippet from source to raw text ("None") below is safe
4754
// because we already have checked the type.
4855
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
49-
let suggest = if unwrap_snippet == "None" {
56+
let unwrap_snippet_none = unwrap_snippet == "None";
57+
let suggest = if unwrap_snippet_none {
5058
"and_then(f)"
5159
} else {
5260
"map_or(a, f)"
@@ -56,24 +64,24 @@ pub(super) fn lint<'a, 'tcx>(
5664
This can be done more directly by calling `{}` instead",
5765
arg, suggest
5866
);
59-
// lint, with note if neither arg is > 1 line and both map() and
60-
// unwrap_or() have the same span
61-
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
62-
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
63-
if same_span && !multiline {
64-
let suggest = if unwrap_snippet == "None" {
65-
format!("and_then({})", map_snippet)
66-
} else {
67-
format!("map_or({}, {})", unwrap_snippet, map_snippet)
68-
};
69-
let note = format!(
70-
"replace `map({}).unwrap_or({})` with `{}`",
71-
map_snippet, unwrap_snippet, suggest
72-
);
73-
span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, &note);
74-
} else if same_span && multiline {
75-
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
76-
};
67+
68+
span_lint_and_then(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, |db| {
69+
let map_arg_span = map_args[1].span;
70+
71+
let mut suggestion = vec![
72+
(
73+
map_span,
74+
String::from(if unwrap_snippet_none { "and_then" } else { "map_or" }),
75+
),
76+
(expr.span.with_lo(unwrap_args[0].span.hi()), String::from("")),
77+
];
78+
79+
if !unwrap_snippet_none {
80+
suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{}, ", unwrap_snippet)));
81+
}
82+
83+
db.multipart_suggestion(&format!("use `{}` instead", suggest), suggestion, applicability);
84+
});
7785
}
7886
}
7987

‎tests/ui/methods.rs‎

Lines changed: 0 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -164,81 +164,6 @@ impl Mul<T> for T {
164164
}
165165
}
166166

167-
/// Checks implementation of the following lints:
168-
/// * `OPTION_MAP_UNWRAP_OR`
169-
/// * `OPTION_MAP_UNWRAP_OR_ELSE`
170-
#[rustfmt::skip]
171-
fn option_methods() {
172-
let opt = Some(1);
173-
174-
// Check `OPTION_MAP_UNWRAP_OR`.
175-
// Single line case.
176-
let _ = opt.map(|x| x + 1)
177-
// Should lint even though this call is on a separate line.
178-
.unwrap_or(0);
179-
// Multi-line cases.
180-
let _ = opt.map(|x| {
181-
x + 1
182-
}
183-
).unwrap_or(0);
184-
let _ = opt.map(|x| x + 1)
185-
.unwrap_or({
186-
0
187-
});
188-
// Single line `map(f).unwrap_or(None)` case.
189-
let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
190-
// Multi-line `map(f).unwrap_or(None)` cases.
191-
let _ = opt.map(|x| {
192-
Some(x + 1)
193-
}
194-
).unwrap_or(None);
195-
let _ = opt
196-
.map(|x| Some(x + 1))
197-
.unwrap_or(None);
198-
// macro case
199-
let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint
200-
201-
// Should not lint if not copyable
202-
let id: String = "identifier".to_string();
203-
let _ = Some("prefix").map(|p| format!("{}.{}", p, id)).unwrap_or(id);
204-
// ...but DO lint if the `unwrap_or` argument is not used in the `map`
205-
let id: String = "identifier".to_string();
206-
let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);
207-
208-
// Check OPTION_MAP_UNWRAP_OR_ELSE
209-
// single line case
210-
let _ = opt.map(|x| x + 1)
211-
// Should lint even though this call is on a separate line.
212-
.unwrap_or_else(|| 0);
213-
// Multi-line cases.
214-
let _ = opt.map(|x| {
215-
x + 1
216-
}
217-
).unwrap_or_else(|| 0);
218-
let _ = opt.map(|x| x + 1)
219-
.unwrap_or_else(||
220-
0
221-
);
222-
// Macro case.
223-
// Should not lint.
224-
let _ = opt_map!(opt, |x| x + 1).unwrap_or_else(|| 0);
225-
226-
// Issue #4144
227-
{
228-
let mut frequencies = HashMap::new();
229-
let word = "foo";
230-
231-
frequencies
232-
.get_mut(word)
233-
.map(|count| {
234-
*count += 1;
235-
})
236-
.unwrap_or_else(|| {
237-
frequencies.insert(word.to_owned(), 1);
238-
});
239-
}
240-
}
241-
242167
/// Checks implementation of `FILTER_NEXT` lint.
243168
#[rustfmt::skip]
244169
fn filter_next() {
@@ -302,7 +227,6 @@ fn search_is_some() {
302227
}
303228

304229
fn main() {
305-
option_methods();
306230
filter_next();
307231
search_is_some();
308232
}

‎tests/ui/methods.stderr‎

Lines changed: 12 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -18,109 +18,8 @@ LL | | }
1818
|
1919
= note: `-D clippy::new-ret-no-self` implied by `-D warnings`
2020

21-
error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
22-
--> $DIR/methods.rs:176:13
23-
|
24-
LL | let _ = opt.map(|x| x + 1)
25-
| _____________^
26-
LL | | // Should lint even though this call is on a separate line.
27-
LL | | .unwrap_or(0);
28-
| |____________________________^
29-
|
30-
= note: `-D clippy::option-map-unwrap-or` implied by `-D warnings`
31-
= note: replace `map(|x| x + 1).unwrap_or(0)` with `map_or(0, |x| x + 1)`
32-
33-
error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
34-
--> $DIR/methods.rs:180:13
35-
|
36-
LL | let _ = opt.map(|x| {
37-
| _____________^
38-
LL | | x + 1
39-
LL | | }
40-
LL | | ).unwrap_or(0);
41-
| |____________________________^
42-
43-
error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
44-
--> $DIR/methods.rs:184:13
45-
|
46-
LL | let _ = opt.map(|x| x + 1)
47-
| _____________^
48-
LL | | .unwrap_or({
49-
LL | | 0
50-
LL | | });
51-
| |__________________^
52-
53-
error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
54-
--> $DIR/methods.rs:189:13
55-
|
56-
LL | let _ = opt.map(|x| Some(x + 1)).unwrap_or(None);
57-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
58-
|
59-
= note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))`
60-
61-
error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
62-
--> $DIR/methods.rs:191:13
63-
|
64-
LL | let _ = opt.map(|x| {
65-
| _____________^
66-
LL | | Some(x + 1)
67-
LL | | }
68-
LL | | ).unwrap_or(None);
69-
| |_____________________^
70-
71-
error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead
72-
--> $DIR/methods.rs:195:13
73-
|
74-
LL | let _ = opt
75-
| _____________^
76-
LL | | .map(|x| Some(x + 1))
77-
LL | | .unwrap_or(None);
78-
| |________________________^
79-
|
80-
= note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))`
81-
82-
error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead
83-
--> $DIR/methods.rs:206:13
84-
|
85-
LL | let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);
86-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
87-
|
88-
= note: replace `map(|p| format!("{}.", p)).unwrap_or(id)` with `map_or(id, |p| format!("{}.", p))`
89-
90-
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
91-
--> $DIR/methods.rs:210:13
92-
|
93-
LL | let _ = opt.map(|x| x + 1)
94-
| _____________^
95-
LL | | // Should lint even though this call is on a separate line.
96-
LL | | .unwrap_or_else(|| 0);
97-
| |____________________________________^
98-
|
99-
= note: `-D clippy::option-map-unwrap-or-else` implied by `-D warnings`
100-
= note: replace `map(|x| x + 1).unwrap_or_else(|| 0)` with `map_or_else(|| 0, |x| x + 1)`
101-
102-
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
103-
--> $DIR/methods.rs:214:13
104-
|
105-
LL | let _ = opt.map(|x| {
106-
| _____________^
107-
LL | | x + 1
108-
LL | | }
109-
LL | | ).unwrap_or_else(|| 0);
110-
| |____________________________________^
111-
112-
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
113-
--> $DIR/methods.rs:218:13
114-
|
115-
LL | let _ = opt.map(|x| x + 1)
116-
| _____________^
117-
LL | | .unwrap_or_else(||
118-
LL | | 0
119-
LL | | );
120-
| |_________________^
121-
12221
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
123-
--> $DIR/methods.rs:248:13
22+
--> $DIR/methods.rs:173:13
12423
|
12524
LL | let _ = v.iter().filter(|&x| *x < 0).next();
12625
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -129,7 +28,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next();
12928
= note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)`
13029

13130
error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead.
132-
--> $DIR/methods.rs:251:13
31+
--> $DIR/methods.rs:176:13
13332
|
13433
LL | let _ = v.iter().filter(|&x| {
13534
| _____________^
@@ -139,33 +38,33 @@ LL | | ).next();
13938
| |___________________________^
14039

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

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

15554
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
156-
--> $DIR/methods.rs:270:20
55+
--> $DIR/methods.rs:195:20
15756
|
15857
LL | let _ = (0..1).find(|x| *x == 0).is_some();
15958
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`
16059

16160
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
162-
--> $DIR/methods.rs:271:22
61+
--> $DIR/methods.rs:196:22
16362
|
16463
LL | let _ = v.iter().find(|x| **x == 0).is_some();
16564
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)`
16665

16766
error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
168-
--> $DIR/methods.rs:274:13
67+
--> $DIR/methods.rs:199:13
16968
|
17069
LL | let _ = v.iter().find(|&x| {
17170
| _____________^
@@ -175,13 +74,13 @@ LL | | ).is_some();
17574
| |______________________________^
17675

17776
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
178-
--> $DIR/methods.rs:280:22
77+
--> $DIR/methods.rs:205:22
17978
|
18079
LL | let _ = v.iter().position(|&x| x < 0).is_some();
18180
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
18281

18382
error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
184-
--> $DIR/methods.rs:283:13
83+
--> $DIR/methods.rs:208:13
18584
|
18685
LL | let _ = v.iter().position(|&x| {
18786
| _____________^
@@ -191,13 +90,13 @@ LL | | ).is_some();
19190
| |______________________________^
19291

19392
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
194-
--> $DIR/methods.rs:289:22
93+
--> $DIR/methods.rs:214:22
19594
|
19695
LL | let _ = v.iter().rposition(|&x| x < 0).is_some();
19796
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
19897

19998
error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
200-
--> $DIR/methods.rs:292:13
99+
--> $DIR/methods.rs:217:13
201100
|
202101
LL | let _ = v.iter().rposition(|&x| {
203102
| _____________^
@@ -206,5 +105,5 @@ LL | | }
206105
LL | | ).is_some();
207106
| |______________________________^
208107

209-
error: aborting due to 23 previous errors
108+
error: aborting due to 13 previous errors
210109

0 commit comments

Comments
 (0)