Skip to content

Commit 44bf60f

Browse files
borsbors
committed
Auto merge of #7004 - Jarcho:manual_map_if_then_else, r=camsteffen
Fix `manual_map` at the end of an if chain changelog: Fix `manual_map` suggestion at the end of an if chain
2 parents 0552852 + fa689f8 commit 44bf60f

File tree

5 files changed

+61
-38
lines changed

5 files changed

+61
-38
lines changed

‎clippy_lints/src/manual_map.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF};
22
use clippy_utils::diagnostics::span_lint_and_sugg;
33
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
44
use clippy_utils::ty::{can_partially_move_ty, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable};
5-
use clippy_utils::{is_allowed, is_else_clause_of_if_let_else, match_def_path, match_var, paths, peel_hir_expr_refs};
5+
use clippy_utils::{is_allowed, is_else_clause, match_def_path, match_var, paths, peel_hir_expr_refs};
66
use rustc_ast::util::parser::PREC_POSTFIX;
77
use rustc_errors::Applicability;
88
use rustc_hir::{
@@ -181,8 +181,7 @@ impl LateLintPass<'_> for ManualMap {
181181
expr.span,
182182
"manual implementation of `Option::map`",
183183
"try this",
184-
if matches!(match_kind, MatchSource::IfLetDesugar { .. }) && is_else_clause_of_if_let_else(cx.tcx, expr)
185-
{
184+
if matches!(match_kind, MatchSource::IfLetDesugar { .. }) && is_else_clause(cx.tcx, expr) {
186185
format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str)
187186
} else {
188187
format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str)

‎clippy_utils/src/lib.rs‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -797,22 +797,29 @@ pub fn get_parent_as_impl(tcx: TyCtxt<'_>, id: HirId) -> Option<&Impl<'_>> {
797797
}
798798
}
799799

800-
/// Checks if the given expression is the else clause in the expression `if let .. {} else {}`
801-
pub fn is_else_clause_of_if_let_else(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
800+
/// Checks if the given expression is the else clause of either an `if` or `if let` expression.
801+
pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
802802
let map = tcx.hir();
803803
let mut iter = map.parent_iter(expr.hir_id);
804-
let arm_id = match iter.next() {
805-
Some((id, Node::Arm(..))) => id,
806-
_ => return false,
807-
};
808804
match iter.next() {
805+
Some((arm_id, Node::Arm(..))) => matches!(
806+
iter.next(),
807+
Some((
808+
_,
809+
Node::Expr(Expr {
810+
kind: ExprKind::Match(_, [_, else_arm], MatchSource::IfLetDesugar { .. }),
811+
..
812+
})
813+
))
814+
if else_arm.hir_id == arm_id
815+
),
809816
Some((
810817
_,
811818
Node::Expr(Expr {
812-
kind: ExprKind::Match(_, [_, else_arm], kind),
819+
kind: ExprKind::If(_, _, Some(else_expr)),
813820
..
814821
}),
815-
)) => else_arm.hir_id == arm_id && matches!(kind, MatchSource::IfLetDesugar { .. }),
822+
)) => else_expr.hir_id == expr.hir_id,
816823
_ => false,
817824
}
818825
}

‎tests/ui/manual_map_option.fixed‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
clippy::map_identity,
88
clippy::unit_arg,
99
clippy::match_ref_pats,
10+
clippy::redundant_pattern_matching,
1011
dead_code
1112
)]
1213

@@ -130,7 +131,11 @@ fn main() {
130131
}
131132

132133
// #6847
133-
if Some(0).is_some() {
134+
if let Some(_) = Some(0) {
135+
Some(0)
136+
} else { Some(0).map(|x| x + 1) };
137+
138+
if true {
134139
Some(0)
135140
} else { Some(0).map(|x| x + 1) };
136141
}

‎tests/ui/manual_map_option.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
clippy::map_identity,
88
clippy::unit_arg,
99
clippy::match_ref_pats,
10+
clippy::redundant_pattern_matching,
1011
dead_code
1112
)]
1213

@@ -195,4 +196,12 @@ fn main() {
195196
} else {
196197
None
197198
};
199+
200+
if true {
201+
Some(0)
202+
} else if let Some(x) = Some(0) {
203+
Some(x + 1)
204+
} else {
205+
None
206+
};
198207
}

‎tests/ui/manual_map_option.stderr‎

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: manual implementation of `Option::map`
2-
--> $DIR/manual_map_option.rs:14:5
2+
--> $DIR/manual_map_option.rs:15:5
33
|
44
LL | / match Some(0) {
55
LL | | Some(_) => Some(2),
@@ -10,7 +10,7 @@ LL | | };
1010
= note: `-D clippy::manual-map` implied by `-D warnings`
1111

1212
error: manual implementation of `Option::map`
13-
--> $DIR/manual_map_option.rs:19:5
13+
--> $DIR/manual_map_option.rs:20:5
1414
|
1515
LL | / match Some(0) {
1616
LL | | Some(x) => Some(x + 1),
@@ -19,7 +19,7 @@ LL | | };
1919
| |_____^ help: try this: `Some(0).map(|x| x + 1)`
2020

2121
error: manual implementation of `Option::map`
22-
--> $DIR/manual_map_option.rs:24:5
22+
--> $DIR/manual_map_option.rs:25:5
2323
|
2424
LL | / match Some("") {
2525
LL | | Some(x) => Some(x.is_empty()),
@@ -28,7 +28,7 @@ LL | | };
2828
| |_____^ help: try this: `Some("").map(|x| x.is_empty())`
2929

3030
error: manual implementation of `Option::map`
31-
--> $DIR/manual_map_option.rs:29:5
31+
--> $DIR/manual_map_option.rs:30:5
3232
|
3333
LL | / if let Some(x) = Some(0) {
3434
LL | | Some(!x)
@@ -38,7 +38,7 @@ LL | | };
3838
| |_____^ help: try this: `Some(0).map(|x| !x)`
3939

4040
error: manual implementation of `Option::map`
41-
--> $DIR/manual_map_option.rs:36:5
41+
--> $DIR/manual_map_option.rs:37:5
4242
|
4343
LL | / match Some(0) {
4444
LL | | Some(x) => { Some(std::convert::identity(x)) }
@@ -47,7 +47,7 @@ LL | | };
4747
| |_____^ help: try this: `Some(0).map(std::convert::identity)`
4848

4949
error: manual implementation of `Option::map`
50-
--> $DIR/manual_map_option.rs:41:5
50+
--> $DIR/manual_map_option.rs:42:5
5151
|
5252
LL | / match Some(&String::new()) {
5353
LL | | Some(x) => Some(str::len(x)),
@@ -56,7 +56,7 @@ LL | | };
5656
| |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))`
5757

5858
error: manual implementation of `Option::map`
59-
--> $DIR/manual_map_option.rs:51:5
59+
--> $DIR/manual_map_option.rs:52:5
6060
|
6161
LL | / match &Some([0, 1]) {
6262
LL | | Some(x) => Some(x[0]),
@@ -65,7 +65,7 @@ LL | | };
6565
| |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])`
6666

6767
error: manual implementation of `Option::map`
68-
--> $DIR/manual_map_option.rs:56:5
68+
--> $DIR/manual_map_option.rs:57:5
6969
|
7070
LL | / match &Some(0) {
7171
LL | | &Some(x) => Some(x * 2),
@@ -74,7 +74,7 @@ LL | | };
7474
| |_____^ help: try this: `Some(0).map(|x| x * 2)`
7575

7676
error: manual implementation of `Option::map`
77-
--> $DIR/manual_map_option.rs:61:5
77+
--> $DIR/manual_map_option.rs:62:5
7878
|
7979
LL | / match Some(String::new()) {
8080
LL | | Some(ref x) => Some(x.is_empty()),
@@ -83,7 +83,7 @@ LL | | };
8383
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
8484

8585
error: manual implementation of `Option::map`
86-
--> $DIR/manual_map_option.rs:66:5
86+
--> $DIR/manual_map_option.rs:67:5
8787
|
8888
LL | / match &&Some(String::new()) {
8989
LL | | Some(x) => Some(x.len()),
@@ -92,7 +92,7 @@ LL | | };
9292
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
9393

9494
error: manual implementation of `Option::map`
95-
--> $DIR/manual_map_option.rs:71:5
95+
--> $DIR/manual_map_option.rs:72:5
9696
|
9797
LL | / match &&Some(0) {
9898
LL | | &&Some(x) => Some(x + x),
@@ -101,7 +101,7 @@ LL | | };
101101
| |_____^ help: try this: `Some(0).map(|x| x + x)`
102102

103103
error: manual implementation of `Option::map`
104-
--> $DIR/manual_map_option.rs:84:9
104+
--> $DIR/manual_map_option.rs:85:9
105105
|
106106
LL | / match &mut Some(String::new()) {
107107
LL | | Some(x) => Some(x.push_str("")),
@@ -110,7 +110,7 @@ LL | | };
110110
| |_________^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))`
111111

112112
error: manual implementation of `Option::map`
113-
--> $DIR/manual_map_option.rs:90:5
113+
--> $DIR/manual_map_option.rs:91:5
114114
|
115115
LL | / match &mut Some(String::new()) {
116116
LL | | Some(ref x) => Some(x.len()),
@@ -119,7 +119,7 @@ LL | | };
119119
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
120120

121121
error: manual implementation of `Option::map`
122-
--> $DIR/manual_map_option.rs:95:5
122+
--> $DIR/manual_map_option.rs:96:5
123123
|
124124
LL | / match &mut &Some(String::new()) {
125125
LL | | Some(x) => Some(x.is_empty()),
@@ -128,7 +128,7 @@ LL | | };
128128
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
129129

130130
error: manual implementation of `Option::map`
131-
--> $DIR/manual_map_option.rs:100:5
131+
--> $DIR/manual_map_option.rs:101:5
132132
|
133133
LL | / match Some((0, 1, 2)) {
134134
LL | | Some((x, y, z)) => Some(x + y + z),
@@ -137,7 +137,7 @@ LL | | };
137137
| |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)`
138138

139139
error: manual implementation of `Option::map`
140-
--> $DIR/manual_map_option.rs:105:5
140+
--> $DIR/manual_map_option.rs:106:5
141141
|
142142
LL | / match Some([1, 2, 3]) {
143143
LL | | Some([first, ..]) => Some(first),
@@ -146,7 +146,7 @@ LL | | };
146146
| |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)`
147147

148148
error: manual implementation of `Option::map`
149-
--> $DIR/manual_map_option.rs:110:5
149+
--> $DIR/manual_map_option.rs:111:5
150150
|
151151
LL | / match &Some((String::new(), "test")) {
152152
LL | | Some((x, y)) => Some((y, x)),
@@ -155,7 +155,7 @@ LL | | };
155155
| |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
156156

157157
error: manual implementation of `Option::map`
158-
--> $DIR/manual_map_option.rs:168:5
158+
--> $DIR/manual_map_option.rs:169:5
159159
|
160160
LL | / match Some(0) {
161161
LL | | Some(x) => Some(vec![x]),
@@ -164,24 +164,27 @@ LL | | };
164164
| |_____^ help: try this: `Some(0).map(|x| vec![x])`
165165

166166
error: manual implementation of `Option::map`
167-
--> $DIR/manual_map_option.rs:173:5
167+
--> $DIR/manual_map_option.rs:174:5
168168
|
169169
LL | / match option_env!("") {
170170
LL | | Some(x) => Some(String::from(x)),
171171
LL | | None => None,
172172
LL | | };
173173
| |_____^ help: try this: `option_env!("").map(String::from)`
174174

175-
error: redundant pattern matching, consider using `is_some()`
176-
--> $DIR/manual_map_option.rs:191:12
177-
|
178-
LL | if let Some(_) = Some(0) {
179-
| -------^^^^^^^---------- help: try this: `if Some(0).is_some()`
175+
error: manual implementation of `Option::map`
176+
--> $DIR/manual_map_option.rs:194:12
180177
|
181-
= note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
178+
LL | } else if let Some(x) = Some(0) {
179+
| ____________^
180+
LL | | Some(x + 1)
181+
LL | | } else {
182+
LL | | None
183+
LL | | };
184+
| |_____^ help: try this: `{ Some(0).map(|x| x + 1) }`
182185

183186
error: manual implementation of `Option::map`
184-
--> $DIR/manual_map_option.rs:193:12
187+
--> $DIR/manual_map_option.rs:202:12
185188
|
186189
LL | } else if let Some(x) = Some(0) {
187190
| ____________^

0 commit comments

Comments
 (0)