From cbde4f2c67667c717adecf3fac9df84954154fe0 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sat, 10 Apr 2021 14:45:51 +0200 Subject: [PATCH 1/4] parent_node_is_if_expr now also recognizes if let as parent if --- clippy_lints/src/copies.rs | 7 +++++ clippy_utils/src/lib.rs | 30 ++++++++++++++----- .../branches_sharing_code/shared_at_bottom.rs | 14 +++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 8b503c9a0306..dfc79ac365bf 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -290,6 +290,13 @@ fn lint_same_then_else<'tcx>( } } +/// The return tuple is structured as follows: +/// 1. The amount of equal statements from the start +/// 2. The amount of equal statements from the end +/// 3. An indication if the block expressions are the same. This will also be true if both are `None` +/// +/// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `(0, 0, false)` +/// to aboard any further processing and avoid duplicate lint triggers. fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, usize, bool) { let mut start_eq = usize::MAX; let mut end_eq = usize::MAX; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 8e1a2105b961..1ef5d9d32c5d 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1212,13 +1212,29 @@ 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(_, _, _), - .. - }) - ) + + // Check for `if` + if_chain! { + if let Node::Expr(expr) = parent_node; + if let ExprKind::If(_, _, _) = expr.kind; + then { + return true; + } + } + + // Check for `if let` + if_chain! { + if let Node::Arm(arm) = parent_node; + let arm_parent_id = map.get_parent_node(arm.hir_id); + let arm_parent_node = map.get(arm_parent_id); + if let Node::Expr(expr) = arm_parent_node; + if let ExprKind::Match(_, _, MatchSource::IfLetDesugar { .. }) = expr.kind; + then { + return true; + } + } + + false } // Finds the `#[must_use]` attribute, if any diff --git a/tests/ui/branches_sharing_code/shared_at_bottom.rs b/tests/ui/branches_sharing_code/shared_at_bottom.rs index c389c243d447..ce2040bdeb82 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() {} From 7e90bb671a73a5b9de08133605ef3db93b62b50c Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 13 Apr 2021 20:38:07 +0200 Subject: [PATCH 2/4] Fixed website inline code background --- util/gh-pages/index.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/gh-pages/index.html b/util/gh-pages/index.html index 082cb35c2e03..27ecb532dd00 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); } From 2992b19c82c0c5f96327b3be037777bf42862230 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 13 Apr 2021 22:55:47 +0200 Subject: [PATCH 3/4] Added inferred local type comparion to SpanlessEq --- clippy_lints/src/copies.rs | 9 ++-- clippy_utils/src/hir_utils.rs | 48 +++++++++++++++++++ clippy_utils/src/lib.rs | 2 +- .../ui/branches_sharing_code/shared_at_top.rs | 11 +++++ 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index dfc79ac365bf..34ea014dd678 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -293,10 +293,11 @@ fn lint_same_then_else<'tcx>( /// The return tuple is structured as follows: /// 1. The amount of equal statements from the start /// 2. The amount of equal statements from the end -/// 3. An indication if the block expressions are the same. This will also be true if both are `None` +/// 3. An indication if the block expressions are the same. This will also be true if both are +/// `None` /// -/// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `(0, 0, false)` -/// to aboard any further processing and avoid duplicate lint triggers. +/// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `(0, 0, +/// false)` to aboard any further processing and avoid duplicate lint triggers. fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, usize, bool) { let mut start_eq = usize::MAX; let mut end_eq = usize::MAX; @@ -307,7 +308,7 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, // `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. // The comparison therefore needs to be done in a way that builds the correct context. - let mut evaluator = SpanlessEq::new(cx); + let mut evaluator = SpanlessEq::new(cx).enable_check_inferred_local_types(); let mut evaluator = evaluator.inter_expr(); let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r)); diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index f695f1a61e71..97db58ae12b9 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -1,6 +1,7 @@ use crate::consts::{constant_context, constant_simple}; use crate::differing_macro_contexts; use crate::source::snippet_opt; +use if_chain::if_chain; use rustc_ast::ast::InlineAsmTemplatePiece; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::def::Res; @@ -29,6 +30,30 @@ pub struct SpanlessEq<'a, 'tcx> { maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, allow_side_effects: bool, expr_fallback: Option, &Expr<'_>) -> bool + 'a>>, + /// This adds an additional type comparison to locals that insures that even the + /// inferred of the value is the same. + /// + /// **Example** + /// * Context 1 + /// ```ignore + /// let vec = Vec::new(); + /// vec.push("A string"); + /// ``` + /// + /// * Context 2 + /// ```ignore + /// let vec = Vec::new(); + /// vec.push(0); // An integer + /// ``` + /// + /// Only comparing the first local definition would usually return that they are + /// equal, since they are identical. However, they are different due to the context + /// as they have different inferred types. + /// + /// This option enables or disables the specific check of the inferred type. + /// + /// Note: This check will only be done if `self.maybe_typeck_results` is `Some()`. + check_inferred_local_types: bool, } impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { @@ -38,6 +63,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { maybe_typeck_results: cx.maybe_typeck_results(), allow_side_effects: true, expr_fallback: None, + check_inferred_local_types: false, } } @@ -56,6 +82,13 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } } + pub fn enable_check_inferred_local_types(self) -> Self { + Self { + check_inferred_local_types: true, + ..self + } + } + /// Use this method to wrap comparisons that may involve inter-expression context. /// See `self.locals`. pub fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> { @@ -96,6 +129,21 @@ 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)) => { + // See `SpanlessEq::check_inferred_local_types` for an explication of this check + if_chain! { + if l.ty.is_none() && r.ty.is_none(); + if self.inner.check_inferred_local_types; + if let Some(tcx) = self.inner.maybe_typeck_results; + + // Check the inferred types + let l_ty = tcx.pat_ty(&l.pat); + let r_ty = tcx.pat_ty(&r.pat); + if l_ty != r_ty; + then { + 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 1ef5d9d32c5d..280bde73ed71 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1212,7 +1212,7 @@ 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); - + // Check for `if` if_chain! { if let Node::Expr(expr) = parent_node; diff --git a/tests/ui/branches_sharing_code/shared_at_top.rs b/tests/ui/branches_sharing_code/shared_at_top.rs index e65bcfd78737..51a46481399b 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() {} From 0b4af7257ddea5cf508305635fbdc4c0d1e6bdd5 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Wed, 14 Apr 2021 19:40:51 +0200 Subject: [PATCH 4/4] PR suggestions and removing utils::parent_node_is_if_expr --- clippy_lints/src/comparison_chain.rs | 4 +- clippy_lints/src/copies.rs | 43 ++++++++++++------- clippy_lints/src/if_then_some_else_none.rs | 4 +- clippy_lints/src/needless_bool.rs | 4 +- clippy_utils/src/hir_utils.rs | 50 +++------------------- clippy_utils/src/lib.rs | 31 -------------- 6 files changed, 40 insertions(+), 96 deletions(-) diff --git a/clippy_lints/src/comparison_chain.rs b/clippy_lints/src/comparison_chain.rs index 31ae63b51849..42e153909ce7 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 34ea014dd678..f956d171bfbe 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,15 +295,19 @@ fn lint_same_then_else<'tcx>( } } -/// The return tuple is structured as follows: -/// 1. The amount of equal statements from the start -/// 2. The amount of equal statements from the end -/// 3. An indication if the block expressions are the same. This will also be true if both are -/// `None` -/// -/// This function can also trigger the `IF_SAME_THEN_ELSE` in which case it'll return `(0, 0, -/// false)` to aboard any further processing and avoid duplicate lint triggers. -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 { let mut start_eq = usize::MAX; let mut end_eq = usize::MAX; let mut expr_eq = true; @@ -308,7 +317,7 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, // `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752. // The comparison therefore needs to be done in a way that builds the correct context. - let mut evaluator = SpanlessEq::new(cx).enable_check_inferred_local_types(); + let mut evaluator = SpanlessEq::new(cx); let mut evaluator = evaluator.inter_expr(); let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r)); @@ -340,7 +349,7 @@ fn scan_block_for_eq(cx: &LateContext<'tcx>, blocks: &[&Block<'tcx>]) -> (usize, "same as this", ); - return (0, 0, false); + return None; } } @@ -360,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 f73b3a1fe67c..85c95f1151f8 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 96a58d1410f2..dd4581986377 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 97db58ae12b9..f0d7f76bea20 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -1,7 +1,6 @@ use crate::consts::{constant_context, constant_simple}; use crate::differing_macro_contexts; use crate::source::snippet_opt; -use if_chain::if_chain; use rustc_ast::ast::InlineAsmTemplatePiece; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::def::Res; @@ -30,30 +29,6 @@ pub struct SpanlessEq<'a, 'tcx> { maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>, allow_side_effects: bool, expr_fallback: Option, &Expr<'_>) -> bool + 'a>>, - /// This adds an additional type comparison to locals that insures that even the - /// inferred of the value is the same. - /// - /// **Example** - /// * Context 1 - /// ```ignore - /// let vec = Vec::new(); - /// vec.push("A string"); - /// ``` - /// - /// * Context 2 - /// ```ignore - /// let vec = Vec::new(); - /// vec.push(0); // An integer - /// ``` - /// - /// Only comparing the first local definition would usually return that they are - /// equal, since they are identical. However, they are different due to the context - /// as they have different inferred types. - /// - /// This option enables or disables the specific check of the inferred type. - /// - /// Note: This check will only be done if `self.maybe_typeck_results` is `Some()`. - check_inferred_local_types: bool, } impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { @@ -63,7 +38,6 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { maybe_typeck_results: cx.maybe_typeck_results(), allow_side_effects: true, expr_fallback: None, - check_inferred_local_types: false, } } @@ -82,13 +56,6 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { } } - pub fn enable_check_inferred_local_types(self) -> Self { - Self { - check_inferred_local_types: true, - ..self - } - } - /// Use this method to wrap comparisons that may involve inter-expression context. /// See `self.locals`. pub fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> { @@ -129,17 +96,12 @@ 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)) => { - // See `SpanlessEq::check_inferred_local_types` for an explication of this check - if_chain! { - if l.ty.is_none() && r.ty.is_none(); - if self.inner.check_inferred_local_types; - if let Some(tcx) = self.inner.maybe_typeck_results; - - // Check the inferred types - let l_ty = tcx.pat_ty(&l.pat); - let r_ty = tcx.pat_ty(&r.pat); - if l_ty != r_ty; - then { + // 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; } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 280bde73ed71..c21cef61df4b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1206,37 +1206,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); - - // Check for `if` - if_chain! { - if let Node::Expr(expr) = parent_node; - if let ExprKind::If(_, _, _) = expr.kind; - then { - return true; - } - } - - // Check for `if let` - if_chain! { - if let Node::Arm(arm) = parent_node; - let arm_parent_id = map.get_parent_node(arm.hir_id); - let arm_parent_node = map.get(arm_parent_id); - if let Node::Expr(expr) = arm_parent_node; - if let ExprKind::Match(_, _, MatchSource::IfLetDesugar { .. }) = expr.kind; - then { - return true; - } - } - - false -} - // 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))