PR suggestions and removing utils::parent_node_is_if_expr

This commit is contained in:
xFrednet 2021-04-14 19:40:51 +02:00
parent 2992b19c82
commit 0b4af7257d
6 changed files with 40 additions and 96 deletions

View File

@ -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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}
// 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;
}

View File

@ -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
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`
///
/// 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) {
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(

View File

@ -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 @@ fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'tcx Expr<'_>) {
}
// 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;
}

View File

@ -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 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
snip = snip.make_return();
}
if parent_node_is_if_expr(e, cx) {
if is_else_clause(cx.tcx, e) {
snip = snip.blockify()
}

View File

@ -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 @@ pub fn new(cx: &'a LateContext<'tcx>) -> Self {
maybe_typeck_results: cx.maybe_typeck_results(),
allow_side_effects: true,
expr_fallback: None,
check_inferred_local_types: false,
}
}
@ -82,13 +56,6 @@ pub fn expr_fallback(self, expr_fallback: impl FnMut(&Expr<'_>, &Expr<'_>) -> bo
}
}
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;
}
}

View File

@ -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))