Skip to content

Fixing FPs for the branches_sharing_code lint #7075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clippy_lints/src/comparison_chain.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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;
}

Expand Down
33 changes: 27 additions & 6 deletions clippy_lints/src/copies.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/if_then_some_else_none.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/needless_bool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
}

Expand Down
10 changes: 10 additions & 0 deletions clippy_utils/src/hir_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
15 changes: 0 additions & 15 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1206,21 +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);
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))
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/branches_sharing_code/shared_at_bottom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
11 changes: 11 additions & 0 deletions tests/ui/branches_sharing_code/shared_at_top.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
2 changes: 1 addition & 1 deletion util/gh-pages/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
opacity: 30%;
}

p > code {
:not(pre) > code {
color: var(--inline-code-color);
background-color: var(--inline-code-bg);
}
Expand Down