diff options
| -rw-r--r-- | clippy_lints/src/comparison_chain.rs | 4 | ||||
| -rw-r--r-- | clippy_lints/src/copies.rs | 43 | ||||
| -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 | 50 | ||||
| -rw-r--r-- | 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 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 34ea014dd67..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,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<BlockEqual> { 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 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 97db58ae12b..f0d7f76bea2 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<Box<dyn FnMut(&Expr<'_>, &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 280bde73ed7..c21cef61df4 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)) |