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] 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 { | ____________^