Skip to content

Commit 59a9fcb

Browse files
committed
Added expression check for shared_code_in_if_blocks
1 parent 4637f05 commit 59a9fcb

File tree

3 files changed

+60
-12
lines changed

3 files changed

+60
-12
lines changed

clippy_lints/src/copies.rs

+24-11
Original file line numberDiff line numberDiff line change
@@ -180,11 +180,11 @@ fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_uncondit
180180
}
181181
}
182182

183-
fn lint_duplicate_code(cx: &LateContext<'_>, position: &str, lint_span: Span) {
183+
fn lint_duplicate_code(cx: &LateContext<'_>, position: &str, lint_start: Span, lint_end: Span) {
184184
span_lint_and_help(
185185
cx,
186186
SHARED_CODE_IN_IF_BLOCKS,
187-
lint_span,
187+
lint_start.to(lint_end),
188188
format!("All if blocks contain the same code at the {}", position).as_str(),
189189
None,
190190
"Consider moving the code out of the if statement to prevent code duplication",
@@ -196,9 +196,16 @@ fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_uncondit
196196
return;
197197
}
198198

199+
let eval_expr = if let Some(expr) = blocks[0].expr {
200+
cx.typeck_results().expr_ty(expr).is_unit()
201+
} else {
202+
false
203+
};
204+
199205
// Check if each block has shared code
200206
let mut start_eq = usize::MAX;
201207
let mut end_eq = usize::MAX;
208+
let mut expr_eq = true;
202209
for (index, win) in blocks.windows(2).enumerate() {
203210
let l_stmts = win[0].stmts;
204211
let r_stmts = win[1].stmts;
@@ -208,12 +215,13 @@ fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_uncondit
208215
let current_end_eq = count_eq(&mut l_stmts.iter().rev(), &mut r_stmts.iter().rev(), |l, r| {
209216
evaluator.eq_stmt(l, r)
210217
});
211-
let current_block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
218+
219+
let block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
212220

213221
// IF_SAME_THEN_ELSE
214222
// We only lint the first two blocks (index == 0). Further blocks will be linted when that if
215223
// statement is checked
216-
if index == 0 && current_block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq {
224+
if index == 0 && block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq {
217225
span_lint_and_note(
218226
cx,
219227
IF_SAME_THEN_ELSE,
@@ -228,9 +236,10 @@ fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_uncondit
228236

229237
start_eq = min(start_eq, current_start_eq);
230238
end_eq = min(end_eq, current_end_eq);
239+
expr_eq = expr_eq && block_expr_eq;
231240

232241
// We can return if the eq count is 0 from both sides or if it has no unconditional else case
233-
if !has_unconditional_else || start_eq == 0 && end_eq == 0 {
242+
if !has_unconditional_else || (start_eq == 0 && end_eq == 0 && !(eval_expr && expr_eq)) {
234243
return;
235244
};
236245
}
@@ -245,18 +254,22 @@ fn lint_same_then_else(cx: &LateContext<'_>, blocks: &[&Block<'_>], has_uncondit
245254
if start_eq != 0 {
246255
let start = first_block.span.shrink_to_lo();
247256
let end = get_source_span(first_block.stmts[start_eq - 1].span);
248-
let lint_span = start.to(end);
249257

250-
lint_duplicate_code(cx, "start", lint_span);
258+
lint_duplicate_code(cx, "start", start, end);
251259
}
252260

253-
if end_eq != 0 {
261+
let lint_end = first_block.span.shrink_to_hi();
262+
if end_eq != 0 && (!eval_expr || expr_eq) {
254263
let index = first_block.stmts.len() - end_eq;
255264
let start = get_source_span(first_block.stmts[index].span);
256-
let end = first_block.span.shrink_to_hi();
257-
let lint_span = start.to(end);
265+
266+
lint_duplicate_code(cx, "end", start, lint_end);
267+
} else if eval_expr && expr_eq {
268+
if let Some(expr) = first_block.expr {
269+
let start = get_source_span(expr.span);
258270

259-
lint_duplicate_code(cx, "end", lint_span);
271+
lint_duplicate_code(cx, "end", start, lint_end);
272+
};
260273
}
261274
}
262275

tests/ui/shared_code_in_if_blocks.rs

+26
Original file line numberDiff line numberDiff line change
@@ -169,4 +169,30 @@ fn shared_code_at_end() {
169169
};
170170
}
171171

172+
fn expr_with_unit_type() {
173+
// This lint should also evaluate equality of the expression if it's return type is `()`
174+
let x = 9;
175+
176+
// Don't lint
177+
if x == 9 {
178+
let _a = 17;
179+
let _b = 49;
180+
181+
println!("A message")
182+
} else {
183+
let _b = 49;
184+
185+
println!("A different message message")
186+
}
187+
188+
// lint
189+
if x == 7 {
190+
let _a = 0;
191+
192+
println!("We are doppelgänger")
193+
} else {
194+
println!("We are doppelgänger")
195+
}
196+
}
197+
172198
fn main() {}

tests/ui/shared_code_in_if_blocks.stderr

+10-1
Original file line numberDiff line numberDiff line change
@@ -122,5 +122,14 @@ LL | | } else {
122122
|
123123
= help: Consider moving the code out of the if statement to prevent code duplication
124124

125-
error: aborting due to 10 previous errors
125+
error: All if blocks contain the same code at the end
126+
--> $DIR/shared_code_in_if_blocks.rs:192:9
127+
|
128+
LL | / println!("We are doppelgänger")
129+
LL | | } else {
130+
| |_____^
131+
|
132+
= help: Consider moving the code out of the if statement to prevent code duplication
133+
134+
error: aborting due to 11 previous errors
126135

0 commit comments

Comments
 (0)