From 9306e9a4df2e6f8ed577fff3c82006c532ca51d2 Mon Sep 17 00:00:00 2001 From: Dany Marcoux Date: Wed, 18 May 2022 20:28:11 +0200 Subject: [PATCH 01/10] Ignore bodies containing `todo!()` in `clippy::if_same_then_else` --- clippy_lints/src/copies.rs | 18 +++++++++++++++++- tests/ui/if_same_then_else.rs | 3 +++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 1deff9684a1..6f92caf7391 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -12,7 +12,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; -use rustc_span::{BytePos, Span, Symbol}; +use rustc_span::{sym, BytePos, Span, Symbol}; use std::borrow::Cow; declare_clippy_lint! { @@ -365,6 +365,21 @@ fn eq_stmts( .all(|b| get_stmt(b).map_or(false, |s| eq.eq_stmt(s, stmt))) } +fn block_contains_todo_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { + dbg!(block); + if let Some(macro_def_id) = block.span.ctxt().outer_expn_data().macro_def_id { + dbg!(macro_def_id); + if let Some(diagnostic_name) = cx.tcx.get_diagnostic_name(macro_def_id) { + dbg!(diagnostic_name); + diagnostic_name == sym::todo_macro + } else { + false + } + } else { + false + } +} + fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<'_>, blocks: &[&Block<'_>]) -> BlockEq { let mut eq = SpanlessEq::new(cx); let mut eq = eq.inter_expr(); @@ -398,6 +413,7 @@ fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<' moved_locals, }; } + let end_search_start = block.stmts[start_end_eq..] .iter() .rev() diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index ef956745500..767185c207b 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -126,6 +126,9 @@ fn if_same_then_else() { _ => 4, }; } + + // Issue #8836 + if true { todo!() } else { todo!() } } // Issue #2423. This was causing an ICE. From 7cb4cef123a53534aaaa3956d4edf079d8482c6b Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Thu, 16 Jun 2022 21:29:43 +0900 Subject: [PATCH 02/10] feat(fix): ignore todo! and unimplemented! in if_same_then_else --- clippy_lints/src/copies.rs | 51 ++++++++++++++++-------- tests/ui/if_same_then_else.rs | 64 +++++++++++++++++++++++++++++-- tests/ui/if_same_then_else.stderr | 20 +++++----- 3 files changed, 106 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 6f92caf7391..a771656c20f 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,4 +1,5 @@ use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then}; +use clippy_utils::macros::macro_backtrace; use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt}; use clippy_utils::{ eq_expr_value, get_enclosing_block, hash_expr, hash_stmt, if_sequence, is_else_clause, is_lint_allowed, @@ -7,14 +8,16 @@ use clippy_utils::{ use core::iter; use rustc_errors::Applicability; use rustc_hir::intravisit; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Stmt, StmtKind}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; -use rustc_span::{sym, BytePos, Span, Symbol}; +use rustc_span::{BytePos, Span, Symbol}; use std::borrow::Cow; +const ACCEPTABLE_MACRO: [&str; 2] = ["todo", "unimplemented"]; + declare_clippy_lint! { /// ### What it does /// Checks for consecutive `if`s with the same condition. @@ -195,7 +198,12 @@ fn lint_if_same_then_else(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[& .array_windows::<2>() .enumerate() .fold(true, |all_eq, (i, &[lhs, rhs])| { - if eq.eq_block(lhs, rhs) && !contains_let(conds[i]) && conds.get(i + 1).map_or(true, |e| !contains_let(e)) { + if eq.eq_block(lhs, rhs) + && !contains_acceptable_macro(cx, lhs) + && !contains_acceptable_macro(cx, rhs) + && !contains_let(conds[i]) + && conds.get(i + 1).map_or(true, |e| !contains_let(e)) + { span_lint_and_note( cx, IF_SAME_THEN_ELSE, @@ -365,19 +373,33 @@ fn eq_stmts( .all(|b| get_stmt(b).map_or(false, |s| eq.eq_stmt(s, stmt))) } -fn block_contains_todo_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { - dbg!(block); - if let Some(macro_def_id) = block.span.ctxt().outer_expn_data().macro_def_id { - dbg!(macro_def_id); - if let Some(diagnostic_name) = cx.tcx.get_diagnostic_name(macro_def_id) { - dbg!(diagnostic_name); - diagnostic_name == sym::todo_macro - } else { - false +fn contains_acceptable_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { + for stmt in block.stmts { + match stmt.kind { + StmtKind::Semi(semi_expr) if acceptable_macro(cx, semi_expr) => return true, + _ => {}, } - } else { - false } + + if let Some(block_expr) = block.expr + && acceptable_macro(cx, block_expr) + { + return true + } + + false +} + +fn acceptable_macro(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let ExprKind::Call(call_expr, _) = expr.kind + && let ExprKind::Path(QPath::Resolved(None, path)) = call_expr.kind + && macro_backtrace(path.span).any(|macro_call| { + ACCEPTABLE_MACRO.contains(&cx.tcx.item_name(macro_call.def_id).as_str()) + }) { + return true; + } + + false } fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<'_>, blocks: &[&Block<'_>]) -> BlockEq { @@ -413,7 +435,6 @@ fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<' moved_locals, }; } - let end_search_start = block.stmts[start_end_eq..] .iter() .rev() diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 767185c207b..2598c2ab426 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -6,7 +6,9 @@ clippy::no_effect, clippy::unused_unit, clippy::zero_divided_by_zero, - clippy::branches_sharing_code + clippy::branches_sharing_code, + dead_code, + unreachable_code )] struct Foo { @@ -126,9 +128,6 @@ fn if_same_then_else() { _ => 4, }; } - - // Issue #8836 - if true { todo!() } else { todo!() } } // Issue #2423. This was causing an ICE. @@ -158,4 +157,61 @@ mod issue_5698 { } } +mod issue_8836 { + fn do_not_lint() { + if true { + todo!() + } else { + todo!() + } + if true { + todo!(); + } else { + todo!(); + } + if true { + unimplemented!() + } else { + unimplemented!() + } + if true { + unimplemented!(); + } else { + unimplemented!(); + } + + if true { + println!("FOO"); + todo!(); + } else { + println!("FOO"); + todo!(); + } + + if true { + println!("FOO"); + unimplemented!(); + } else { + println!("FOO"); + unimplemented!(); + } + + if true { + println!("FOO"); + todo!() + } else { + println!("FOO"); + todo!() + } + + if true { + println!("FOO"); + unimplemented!() + } else { + println!("FOO"); + unimplemented!() + } + } +} + fn main() {} diff --git a/tests/ui/if_same_then_else.stderr b/tests/ui/if_same_then_else.stderr index 2f38052fc20..2cdf442486a 100644 --- a/tests/ui/if_same_then_else.stderr +++ b/tests/ui/if_same_then_else.stderr @@ -1,5 +1,5 @@ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:21:13 + --> $DIR/if_same_then_else.rs:23:13 | LL | if true { | _____________^ @@ -13,7 +13,7 @@ LL | | } else { | = note: `-D clippy::if-same-then-else` implied by `-D warnings` note: same as this - --> $DIR/if_same_then_else.rs:29:12 + --> $DIR/if_same_then_else.rs:31:12 | LL | } else { | ____________^ @@ -26,7 +26,7 @@ LL | | } | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:65:21 + --> $DIR/if_same_then_else.rs:67:21 | LL | let _ = if true { | _____________________^ @@ -35,7 +35,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:67:12 + --> $DIR/if_same_then_else.rs:69:12 | LL | } else { | ____________^ @@ -45,7 +45,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:72:21 + --> $DIR/if_same_then_else.rs:74:21 | LL | let _ = if true { | _____________________^ @@ -54,7 +54,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:74:12 + --> $DIR/if_same_then_else.rs:76:12 | LL | } else { | ____________^ @@ -64,7 +64,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:88:21 + --> $DIR/if_same_then_else.rs:90:21 | LL | let _ = if true { | _____________________^ @@ -73,7 +73,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:90:12 + --> $DIR/if_same_then_else.rs:92:12 | LL | } else { | ____________^ @@ -83,7 +83,7 @@ LL | | }; | |_____^ error: this `if` has identical blocks - --> $DIR/if_same_then_else.rs:95:13 + --> $DIR/if_same_then_else.rs:97:13 | LL | if true { | _____________^ @@ -96,7 +96,7 @@ LL | | } else { | |_____^ | note: same as this - --> $DIR/if_same_then_else.rs:102:12 + --> $DIR/if_same_then_else.rs:104:12 | LL | } else { | ____________^ From a5b6d25ca4f1600a158ffb68301dd816c8eda3cf Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Fri, 17 Jun 2022 10:10:40 +0900 Subject: [PATCH 03/10] use get_diagnostic_name for checking macro_call --- clippy_lints/src/copies.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index a771656c20f..9d1ca3588b3 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -13,11 +13,9 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; -use rustc_span::{BytePos, Span, Symbol}; +use rustc_span::{sym, BytePos, Span, Symbol}; use std::borrow::Cow; -const ACCEPTABLE_MACRO: [&str; 2] = ["todo", "unimplemented"]; - declare_clippy_lint! { /// ### What it does /// Checks for consecutive `if`s with the same condition. @@ -394,7 +392,10 @@ fn acceptable_macro(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { if let ExprKind::Call(call_expr, _) = expr.kind && let ExprKind::Path(QPath::Resolved(None, path)) = call_expr.kind && macro_backtrace(path.span).any(|macro_call| { - ACCEPTABLE_MACRO.contains(&cx.tcx.item_name(macro_call.def_id).as_str()) + matches!( + &cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::todo_macro | sym::unimplemented_macro) + ) }) { return true; } From 697c75ef4b39f1560bf087841aebc8799b5c64d6 Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 18 Jun 2022 00:19:30 +0900 Subject: [PATCH 04/10] check only the end --- clippy_lints/src/copies.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 9d1ca3588b3..f0f2c742034 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -391,12 +391,12 @@ fn contains_acceptable_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { fn acceptable_macro(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { if let ExprKind::Call(call_expr, _) = expr.kind && let ExprKind::Path(QPath::Resolved(None, path)) = call_expr.kind - && macro_backtrace(path.span).any(|macro_call| { + && macro_backtrace(path.span).last().map_or(false, |macro_call| matches!( &cx.tcx.get_diagnostic_name(macro_call.def_id), Some(sym::todo_macro | sym::unimplemented_macro) ) - }) { + ) { return true; } From f411c18a734a0c4d020ac578ccc3b5b8a7814f8f Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 18 Jun 2022 17:41:26 +0900 Subject: [PATCH 05/10] check macro_backtrace only --- clippy_lints/src/copies.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index f0f2c742034..673744d90b8 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -8,7 +8,7 @@ use clippy_utils::{ use core::iter; use rustc_errors::Applicability; use rustc_hir::intravisit; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, QPath, Stmt, StmtKind}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::walk_chain; @@ -389,13 +389,11 @@ fn contains_acceptable_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { } fn acceptable_macro(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let ExprKind::Call(call_expr, _) = expr.kind - && let ExprKind::Path(QPath::Resolved(None, path)) = call_expr.kind - && macro_backtrace(path.span).last().map_or(false, |macro_call| - matches!( - &cx.tcx.get_diagnostic_name(macro_call.def_id), - Some(sym::todo_macro | sym::unimplemented_macro) - ) + if macro_backtrace(expr.span).last().map_or(false, |macro_call| + matches!( + &cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::todo_macro | sym::unimplemented_macro) + ) ) { return true; } From 7a83809c8c63b135bf5d2d1c9e42d0a94ea5c5ad Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 18 Jun 2022 17:49:03 +0900 Subject: [PATCH 06/10] check only first statement --- clippy_lints/src/copies.rs | 12 +++++++----- tests/ui/if_same_then_else.rs | 32 -------------------------------- 2 files changed, 7 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 673744d90b8..9b64d1631bd 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -372,11 +372,13 @@ fn eq_stmts( } fn contains_acceptable_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { - for stmt in block.stmts { - match stmt.kind { - StmtKind::Semi(semi_expr) if acceptable_macro(cx, semi_expr) => return true, - _ => {}, - } + if block.stmts.first().map_or(false, |stmt| + matches!( + stmt.kind, + StmtKind::Semi(semi_expr) if acceptable_macro(cx, semi_expr) + ) + ) { + return true; } if let Some(block_expr) = block.expr diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 2598c2ab426..4110d1a9c01 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -179,38 +179,6 @@ mod issue_8836 { } else { unimplemented!(); } - - if true { - println!("FOO"); - todo!(); - } else { - println!("FOO"); - todo!(); - } - - if true { - println!("FOO"); - unimplemented!(); - } else { - println!("FOO"); - unimplemented!(); - } - - if true { - println!("FOO"); - todo!() - } else { - println!("FOO"); - todo!() - } - - if true { - println!("FOO"); - unimplemented!() - } else { - println!("FOO"); - unimplemented!() - } } } From 040d45e41253aeb6f8efbf3f07f737ea1fb961df Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 18 Jun 2022 18:24:39 +0900 Subject: [PATCH 07/10] check macro in eq_block --- clippy_lints/src/copies.rs | 33 +---------------------------- clippy_utils/src/hir_utils.rs | 39 +++++++++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 9b64d1631bd..f6e4d9a4f5c 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,5 +1,4 @@ use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then}; -use clippy_utils::macros::macro_backtrace; use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt}; use clippy_utils::{ eq_expr_value, get_enclosing_block, hash_expr, hash_stmt, if_sequence, is_else_clause, is_lint_allowed, @@ -13,7 +12,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; -use rustc_span::{sym, BytePos, Span, Symbol}; +use rustc_span::{BytePos, Span, Symbol}; use std::borrow::Cow; declare_clippy_lint! { @@ -197,8 +196,6 @@ fn lint_if_same_then_else(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[& .enumerate() .fold(true, |all_eq, (i, &[lhs, rhs])| { if eq.eq_block(lhs, rhs) - && !contains_acceptable_macro(cx, lhs) - && !contains_acceptable_macro(cx, rhs) && !contains_let(conds[i]) && conds.get(i + 1).map_or(true, |e| !contains_let(e)) { @@ -371,37 +368,9 @@ fn eq_stmts( .all(|b| get_stmt(b).map_or(false, |s| eq.eq_stmt(s, stmt))) } -fn contains_acceptable_macro(cx: &LateContext<'_>, block: &Block<'_>) -> bool { - if block.stmts.first().map_or(false, |stmt| - matches!( - stmt.kind, - StmtKind::Semi(semi_expr) if acceptable_macro(cx, semi_expr) - ) - ) { - return true; - } - if let Some(block_expr) = block.expr - && acceptable_macro(cx, block_expr) - { - return true - } - false -} -fn acceptable_macro(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if macro_backtrace(expr.span).last().map_or(false, |macro_call| - matches!( - &cx.tcx.get_diagnostic_name(macro_call.def_id), - Some(sym::todo_macro | sym::unimplemented_macro) - ) - ) { - return true; - } - - false -} fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<'_>, blocks: &[&Block<'_>]) -> BlockEq { let mut eq = SpanlessEq::new(cx); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 39f970dc420..f21098f95f7 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -1,5 +1,6 @@ use crate::consts::constant_simple; use crate::source::snippet_opt; +use crate::macros::macro_backtrace; use rustc_ast::ast::InlineAsmTemplatePiece; use rustc_data_structures::fx::FxHasher; use rustc_hir::def::Res; @@ -12,7 +13,7 @@ use rustc_hir::{ use rustc_lexer::{tokenize, TokenKind}; use rustc_lint::LateContext; use rustc_middle::ty::TypeckResults; -use rustc_span::Symbol; +use rustc_span::{sym, Symbol}; use std::hash::{Hash, Hasher}; /// Type used to check whether two ast are the same. This is different from the @@ -65,7 +66,9 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } pub fn eq_block(&mut self, left: &Block<'_>, right: &Block<'_>) -> bool { - self.inter_expr().eq_block(left, right) + !self.cannot_be_compared_block(left) + && !self.cannot_be_compared_block(right) + && self.inter_expr().eq_block(left, right) } pub fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool { @@ -83,6 +86,38 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { pub fn eq_path_segments(&mut self, left: &[PathSegment<'_>], right: &[PathSegment<'_>]) -> bool { self.inter_expr().eq_path_segments(left, right) } + + fn cannot_be_compared_block(&mut self, block: &Block<'_>) -> bool { + if block.stmts.first().map_or(false, |stmt| + matches!( + stmt.kind, + StmtKind::Semi(semi_expr) if self.should_ignore(semi_expr) + ) + ) { + return true; + } + + if let Some(block_expr) = block.expr + && self.should_ignore(block_expr) + { + return true + } + + false + } + + fn should_ignore(&mut self, expr: &Expr<'_>) -> bool { + if macro_backtrace(expr.span).last().map_or(false, |macro_call| + matches!( + &self.cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::todo_macro | sym::unimplemented_macro) + ) + ) { + return true; + } + + false + } } pub struct HirEqInterExpr<'a, 'b, 'tcx> { From 4a02ae9636d57dd4b5b93e5d2688f9eec5de39e9 Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 18 Jun 2022 18:29:39 +0900 Subject: [PATCH 08/10] cargo dev fmt --- clippy_lints/src/copies.rs | 9 +-------- clippy_utils/src/hir_utils.rs | 10 +++++----- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index f6e4d9a4f5c..1deff9684a1 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -195,10 +195,7 @@ fn lint_if_same_then_else(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[& .array_windows::<2>() .enumerate() .fold(true, |all_eq, (i, &[lhs, rhs])| { - if eq.eq_block(lhs, rhs) - && !contains_let(conds[i]) - && conds.get(i + 1).map_or(true, |e| !contains_let(e)) - { + if eq.eq_block(lhs, rhs) && !contains_let(conds[i]) && conds.get(i + 1).map_or(true, |e| !contains_let(e)) { span_lint_and_note( cx, IF_SAME_THEN_ELSE, @@ -368,10 +365,6 @@ fn eq_stmts( .all(|b| get_stmt(b).map_or(false, |s| eq.eq_stmt(s, stmt))) } - - - - fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<'_>, blocks: &[&Block<'_>]) -> BlockEq { let mut eq = SpanlessEq::new(cx); let mut eq = eq.inter_expr(); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index f21098f95f7..97a15108d0c 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -1,6 +1,6 @@ use crate::consts::constant_simple; -use crate::source::snippet_opt; use crate::macros::macro_backtrace; +use crate::source::snippet_opt; use rustc_ast::ast::InlineAsmTemplatePiece; use rustc_data_structures::fx::FxHasher; use rustc_hir::def::Res; @@ -88,12 +88,12 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } fn cannot_be_compared_block(&mut self, block: &Block<'_>) -> bool { - if block.stmts.first().map_or(false, |stmt| + if block.stmts.first().map_or(false, |stmt| { matches!( stmt.kind, StmtKind::Semi(semi_expr) if self.should_ignore(semi_expr) ) - ) { + }) { return true; } @@ -107,12 +107,12 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } fn should_ignore(&mut self, expr: &Expr<'_>) -> bool { - if macro_backtrace(expr.span).last().map_or(false, |macro_call| + if macro_backtrace(expr.span).last().map_or(false, |macro_call| { matches!( &self.cx.tcx.get_diagnostic_name(macro_call.def_id), Some(sym::todo_macro | sym::unimplemented_macro) ) - ) { + }) { return true; } From 46d056e2eb7c5cab767c91383cf7ef1828809fd9 Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Mon, 20 Jun 2022 11:05:40 +0900 Subject: [PATCH 09/10] check last statement --- clippy_utils/src/hir_utils.rs | 2 +- tests/ui/if_same_then_else.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 97a15108d0c..3eb01bf75a6 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -88,7 +88,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } fn cannot_be_compared_block(&mut self, block: &Block<'_>) -> bool { - if block.stmts.first().map_or(false, |stmt| { + if block.stmts.last().map_or(false, |stmt| { matches!( stmt.kind, StmtKind::Semi(semi_expr) if self.should_ignore(semi_expr) diff --git a/tests/ui/if_same_then_else.rs b/tests/ui/if_same_then_else.rs index 4110d1a9c01..2598c2ab426 100644 --- a/tests/ui/if_same_then_else.rs +++ b/tests/ui/if_same_then_else.rs @@ -179,6 +179,38 @@ mod issue_8836 { } else { unimplemented!(); } + + if true { + println!("FOO"); + todo!(); + } else { + println!("FOO"); + todo!(); + } + + if true { + println!("FOO"); + unimplemented!(); + } else { + println!("FOO"); + unimplemented!(); + } + + if true { + println!("FOO"); + todo!() + } else { + println!("FOO"); + todo!() + } + + if true { + println!("FOO"); + unimplemented!() + } else { + println!("FOO"); + unimplemented!() + } } } From 39ffda014debd9d1d4126a8996d39af934fe8d94 Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Mon, 20 Jun 2022 11:14:52 +0900 Subject: [PATCH 10/10] check macro in HitEqInterExpr --- clippy_utils/src/hir_utils.rs | 71 ++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 3eb01bf75a6..46b615c04f3 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -66,9 +66,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } pub fn eq_block(&mut self, left: &Block<'_>, right: &Block<'_>) -> bool { - !self.cannot_be_compared_block(left) - && !self.cannot_be_compared_block(right) - && self.inter_expr().eq_block(left, right) + self.inter_expr().eq_block(left, right) } pub fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool { @@ -86,38 +84,6 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { pub fn eq_path_segments(&mut self, left: &[PathSegment<'_>], right: &[PathSegment<'_>]) -> bool { self.inter_expr().eq_path_segments(left, right) } - - fn cannot_be_compared_block(&mut self, block: &Block<'_>) -> bool { - if block.stmts.last().map_or(false, |stmt| { - matches!( - stmt.kind, - StmtKind::Semi(semi_expr) if self.should_ignore(semi_expr) - ) - }) { - return true; - } - - if let Some(block_expr) = block.expr - && self.should_ignore(block_expr) - { - return true - } - - false - } - - fn should_ignore(&mut self, expr: &Expr<'_>) -> bool { - if macro_backtrace(expr.span).last().map_or(false, |macro_call| { - matches!( - &self.cx.tcx.get_diagnostic_name(macro_call.def_id), - Some(sym::todo_macro | sym::unimplemented_macro) - ) - }) { - return true; - } - - false - } } pub struct HirEqInterExpr<'a, 'b, 'tcx> { @@ -156,6 +122,9 @@ impl HirEqInterExpr<'_, '_, '_> { /// Checks whether two blocks are the same. fn eq_block(&mut self, left: &Block<'_>, right: &Block<'_>) -> bool { + if self.cannot_be_compared_block(left) || self.cannot_be_compared_block(right) { + return false; + } match (left.stmts, left.expr, right.stmts, right.expr) { ([], None, [], None) => { // For empty blocks, check to see if the tokens are equal. This will catch the case where a macro @@ -206,6 +175,38 @@ impl HirEqInterExpr<'_, '_, '_> { } } + fn cannot_be_compared_block(&mut self, block: &Block<'_>) -> bool { + if block.stmts.last().map_or(false, |stmt| { + matches!( + stmt.kind, + StmtKind::Semi(semi_expr) if self.should_ignore(semi_expr) + ) + }) { + return true; + } + + if let Some(block_expr) = block.expr + && self.should_ignore(block_expr) + { + return true + } + + false + } + + fn should_ignore(&mut self, expr: &Expr<'_>) -> bool { + if macro_backtrace(expr.span).last().map_or(false, |macro_call| { + matches!( + &self.inner.cx.tcx.get_diagnostic_name(macro_call.def_id), + Some(sym::todo_macro | sym::unimplemented_macro) + ) + }) { + return true; + } + + false + } + pub fn eq_array_length(&mut self, left: ArrayLen, right: ArrayLen) -> bool { match (left, right) { (ArrayLen::Infer(..), ArrayLen::Infer(..)) => true,