diff options
| author | bors <bors@rust-lang.org> | 2021-04-16 18:33:45 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2021-04-16 18:33:45 +0000 |
| commit | eaf0f3df1509e8d0312895afbda94cf821b5f4c2 (patch) | |
| tree | f7aaea2c3b5d7de559bfe6e8628554e33f546124 | |
| parent | 28dbcd85c8a956a1d1e39066668fcf1484080949 (diff) | |
| parent | 0b4af7257ddea5cf508305635fbdc4c0d1e6bdd5 (diff) | |
| download | rust-eaf0f3df1509e8d0312895afbda94cf821b5f4c2.tar.gz rust-eaf0f3df1509e8d0312895afbda94cf821b5f4c2.zip | |
Auto merge of #7075 - xFrednet:7054-fp-branches-sharing-code, r=camsteffen,flip1995
Fixing FPs for the `branches_sharing_code` lint Fixes #7053 Fixes #7054 And an additional CSS adjustment to support dark mode for every inline code. It currently only works in paragraphs, which was an oversight on my part :sweat_smile:. [Current Example](https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name) This also includes ~50 lines of doc comments and is therefor not as big as the changes would indicate. :penguin: --- changelog: none All of these bugs were introduced in this dev version and are therefor not worth a change log entry. r? `@phansch` cc: `@camsteffen` since you have a pretty good overview of the `SpanlessEq` implementation :upside_down_face:
| -rw-r--r-- | clippy_lints/src/comparison_chain.rs | 4 | ||||
| -rw-r--r-- | clippy_lints/src/copies.rs | 33 | ||||
| -rw-r--r-- | clippy_lints/src/if_then_some_else_none.rs | 4 | ||||
| -rw-r--r-- | clippy_lints/src/needless_bool.rs | 4 | ||||
| -rw-r--r-- | clippy_utils/src/hir_utils.rs | 10 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 15 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_bottom.rs | 14 | ||||
| -rw-r--r-- | tests/ui/branches_sharing_code/shared_at_top.rs | 11 | ||||
| -rw-r--r-- | util/gh-pages/index.html | 2 |
9 files changed, 69 insertions, 28 deletions
diff --git a/clippy_lints/src/comparison_chain.rs b/clippy_lints/src/comparison_chain.rs index 31ae63b5184..42e153909ce 100644 --- a/clippy_lints/src/comparison_chain.rs +++ b/clippy_lints/src/comparison_chain.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::ty::implements_trait; -use clippy_utils::{get_trait_def_id, if_sequence, parent_node_is_if_expr, paths, SpanlessEq}; +use clippy_utils::{get_trait_def_id, if_sequence, is_else_clause, paths, SpanlessEq}; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -60,7 +60,7 @@ impl<'tcx> LateLintPass<'tcx> for ComparisonChain { } // We only care about the top-most `if` in the chain - if parent_node_is_if_expr(expr, cx) { + if is_else_clause(cx.tcx, expr) { return; } diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 8b503c9a030..f956d171bfb 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then}; use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt}; use clippy_utils::{ - both, count_eq, eq_expr_value, get_enclosing_block, get_parent_expr, if_sequence, in_macro, parent_node_is_if_expr, + both, count_eq, eq_expr_value, get_enclosing_block, get_parent_expr, if_sequence, in_macro, is_else_clause, run_lints, search_same, ContainsName, SpanlessEq, SpanlessHash, }; use if_chain::if_chain; @@ -188,13 +188,18 @@ fn lint_same_then_else<'tcx>( expr: &'tcx Expr<'_>, ) { // We only lint ifs with multiple blocks - if blocks.len() < 2 || parent_node_is_if_expr(expr, cx) { + if blocks.len() < 2 || is_else_clause(cx.tcx, expr) { return; } // Check if each block has shared code let has_expr = blocks[0].expr.is_some(); - let (start_eq, mut end_eq, expr_eq) = scan_block_for_eq(cx, blocks); + + let (start_eq, mut end_eq, expr_eq) = if let Some(block_eq) = scan_block_for_eq(cx, blocks) { + (block_eq.start_eq, block_eq.end_eq, block_eq.expr_eq) + } else { + return; + }; // BRANCHES_SHARING_CODE prerequisites if has_conditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) { @@ -290,7 +295,19 @@ fn lint_same_then_else<'tcx>( } } -fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, usize, bool) { +struct BlockEqual { + /// The amount statements that are equal from the start + start_eq: usize, + /// The amount statements that are equal from the end + end_eq: usize, + /// An indication if the block expressions are the same. This will also be true if both are + /// `None` + expr_eq: bool, +} + +/// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `None` to +/// abort any further processing and avoid duplicate lint triggers. +fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> Option<BlockEqual> { let mut start_eq = usize::MAX; let mut end_eq = usize::MAX; let mut expr_eq = true; @@ -332,7 +349,7 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, "same as this", ); - return (0, 0, false); + return None; } } @@ -352,7 +369,11 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, end_eq = min_block_size - start_eq; } - (start_eq, end_eq, expr_eq) + Some(BlockEqual { + start_eq, + end_eq, + expr_eq, + }) } fn check_for_warn_of_moved_symbol( diff --git a/clippy_lints/src/if_then_some_else_none.rs b/clippy_lints/src/if_then_some_else_none.rs index f73b3a1fe67..85c95f1151f 100644 --- a/clippy_lints/src/if_then_some_else_none.rs +++ b/clippy_lints/src/if_then_some_else_none.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::source::snippet_with_macro_callsite; -use clippy_utils::{is_lang_ctor, meets_msrv, parent_node_is_if_expr}; +use clippy_utils::{is_else_clause, is_lang_ctor, meets_msrv}; use if_chain::if_chain; use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind}; @@ -68,7 +68,7 @@ impl LateLintPass<'_> for IfThenSomeElseNone { } // We only care about the top-most `if` in the chain - if parent_node_is_if_expr(expr, cx) { + if is_else_clause(cx.tcx, expr) { return; } diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 96a58d1410f..dd458198637 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -5,7 +5,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg}; use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; -use clippy_utils::{is_expn_of, parent_node_is_if_expr}; +use clippy_utils::{is_else_clause, is_expn_of}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; @@ -81,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBool { snip = snip.make_return(); } - if parent_node_is_if_expr(e, cx) { + if is_else_clause(cx.tcx, e) { snip = snip.blockify() } diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index 3ad1ac75e56..07ae6e924e2 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -96,6 +96,16 @@ impl HirEqInterExpr<'_, '_, '_> { pub fn eq_stmt(&mut self, left: &Stmt<'_>, right: &Stmt<'_>) -> bool { match (&left.kind, &right.kind) { (&StmtKind::Local(ref l), &StmtKind::Local(ref r)) => { + // This additional check ensures that the type of the locals are equivalent even if the init + // expression or type have some inferred parts. + if let Some(typeck) = self.inner.maybe_typeck_results { + let l_ty = typeck.pat_ty(&l.pat); + let r_ty = typeck.pat_ty(&r.pat); + if !rustc_middle::ty::TyS::same_type(l_ty, r_ty) { + return false; + } + } + // eq_pat adds the HirIds to the locals map. We therefor call it last to make sure that // these only get added if the init and type is equal. both(&l.init, &r.init, |l, r| self.eq_expr(l, r)) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index a5986ed00da..b7017411927 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1300,21 +1300,6 @@ pub fn if_sequence<'tcx>(mut expr: &'tcx Expr<'tcx>) -> (Vec<&'tcx Expr<'tcx>>, (conds, blocks) } -/// This function returns true if the given expression is the `else` or `if else` part of an if -/// statement -pub fn parent_node_is_if_expr(expr: &Expr<'_>, cx: &LateContext<'_>) -> bool { - let map = cx.tcx.hir(); - let parent_id = map.get_parent_node(expr.hir_id); - let parent_node = map.get(parent_id); - matches!( - parent_node, - Node::Expr(Expr { - kind: ExprKind::If(_, _, _), - .. - }) - ) -} - // Finds the `#[must_use]` attribute, if any pub fn must_use_attr(attrs: &[Attribute]) -> Option<&Attribute> { attrs.iter().find(|a| a.has_name(sym::must_use)) diff --git a/tests/ui/branches_sharing_code/shared_at_bottom.rs b/tests/ui/branches_sharing_code/shared_at_bottom.rs index c389c243d44..ce2040bdeb8 100644 --- a/tests/ui/branches_sharing_code/shared_at_bottom.rs +++ b/tests/ui/branches_sharing_code/shared_at_bottom.rs @@ -206,4 +206,18 @@ fn fp_test() { } } +fn fp_if_let_issue7054() { + // This shouldn't trigger the lint + let string; + let _x = if let true = true { + "" + } else if true { + string = "x".to_owned(); + &string + } else { + string = "y".to_owned(); + &string + }; +} + fn main() {} diff --git a/tests/ui/branches_sharing_code/shared_at_top.rs b/tests/ui/branches_sharing_code/shared_at_top.rs index e65bcfd7873..51a46481399 100644 --- a/tests/ui/branches_sharing_code/shared_at_top.rs +++ b/tests/ui/branches_sharing_code/shared_at_top.rs @@ -100,4 +100,15 @@ fn check_if_same_than_else_mask() { } } +#[allow(clippy::vec_init_then_push)] +fn pf_local_with_inferred_type_issue7053() { + if true { + let mut v = Vec::new(); + v.push(0); + } else { + let mut v = Vec::new(); + v.push(""); + }; +} + fn main() {} diff --git a/util/gh-pages/index.html b/util/gh-pages/index.html index 082cb35c2e0..27ecb532dd0 100644 --- a/util/gh-pages/index.html +++ b/util/gh-pages/index.html @@ -133,7 +133,7 @@ opacity: 30%; } - p > code { + :not(pre) > code { color: var(--inline-code-color); background-color: var(--inline-code-bg); } |
