Fixes for branches_sharing_code

* Don't suggest moving modifications to locals used in any of the condition expressions
* Don't suggest moving anything after a local with a significant drop
This commit is contained in:
Jason Newcomb 2022-07-08 15:29:23 -04:00
parent d251bd96e7
commit 55563f9ce1
3 changed files with 113 additions and 7 deletions

View File

@ -1,13 +1,16 @@
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::ty::needs_ordered_drop;
use clippy_utils::visitors::for_each_expr;
use clippy_utils::{
eq_expr_value, get_enclosing_block, hash_expr, hash_stmt, if_sequence, is_else_clause, is_lint_allowed,
search_same, ContainsName, HirEqInterExpr, SpanlessEq,
capture_local_usage, eq_expr_value, get_enclosing_block, hash_expr, hash_stmt, if_sequence, is_else_clause,
is_lint_allowed, path_to_local, search_same, ContainsName, HirEqInterExpr, SpanlessEq,
};
use core::iter;
use core::ops::ControlFlow;
use rustc_errors::Applicability;
use rustc_hir::intravisit;
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Stmt, StmtKind};
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, HirIdSet, Stmt, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::hygiene::walk_chain;
@ -214,7 +217,7 @@ fn lint_if_same_then_else(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[&
fn lint_branches_sharing_code<'tcx>(
cx: &LateContext<'tcx>,
conds: &[&'tcx Expr<'_>],
blocks: &[&Block<'tcx>],
blocks: &[&'tcx Block<'_>],
expr: &'tcx Expr<'_>,
) {
// We only lint ifs with multiple blocks
@ -340,6 +343,21 @@ fn eq_binding_names(s: &Stmt<'_>, names: &[(HirId, Symbol)]) -> bool {
}
}
/// Checks if the statement modifies or moves any of the given locals.
fn modifies_any_local<'tcx>(cx: &LateContext<'tcx>, s: &'tcx Stmt<'_>, locals: &HirIdSet) -> bool {
for_each_expr(s, |e| {
if let Some(id) = path_to_local(e)
&& locals.contains(&id)
&& !capture_local_usage(cx, e).is_imm_ref()
{
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_some()
}
/// Checks if the given statement should be considered equal to the statement in the same position
/// for each block.
fn eq_stmts(
@ -365,18 +383,52 @@ fn eq_stmts(
.all(|b| get_stmt(b).map_or(false, |s| eq.eq_stmt(s, stmt)))
}
fn scan_block_for_eq(cx: &LateContext<'_>, _conds: &[&Expr<'_>], block: &Block<'_>, blocks: &[&Block<'_>]) -> BlockEq {
#[expect(clippy::too_many_lines)]
fn scan_block_for_eq<'tcx>(
cx: &LateContext<'tcx>,
conds: &[&'tcx Expr<'_>],
block: &'tcx Block<'_>,
blocks: &[&'tcx Block<'_>],
) -> BlockEq {
let mut eq = SpanlessEq::new(cx);
let mut eq = eq.inter_expr();
let mut moved_locals = Vec::new();
let mut cond_locals = HirIdSet::default();
for &cond in conds {
let _: Option<!> = for_each_expr(cond, |e| {
if let Some(id) = path_to_local(e) {
cond_locals.insert(id);
}
ControlFlow::Continue(())
});
}
let mut local_needs_ordered_drop = false;
let start_end_eq = block
.stmts
.iter()
.enumerate()
.find(|&(i, stmt)| !eq_stmts(stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals))
.find(|&(i, stmt)| {
if let StmtKind::Local(l) = stmt.kind
&& needs_ordered_drop(cx, cx.typeck_results().node_type(l.hir_id))
{
local_needs_ordered_drop = true;
return true;
}
modifies_any_local(cx, stmt, &cond_locals)
|| !eq_stmts(stmt, blocks, |b| b.stmts.get(i), &mut eq, &mut moved_locals)
})
.map_or(block.stmts.len(), |(i, _)| i);
if local_needs_ordered_drop {
return BlockEq {
start_end_eq,
end_begin_eq: None,
moved_locals,
};
}
// Walk backwards through the final expression/statements so long as their hashes are equal. Note
// `SpanlessHash` treats all local references as equal allowing locals declared earlier in the block
// to match those in other blocks. e.g. If each block ends with the following the hash value will be

View File

@ -890,7 +890,7 @@ pub fn capture_local_usage<'tcx>(cx: &LateContext<'tcx>, e: &Expr<'_>) -> Captur
Node::Expr(e) => match e.kind {
ExprKind::AddrOf(_, mutability, _) => return CaptureKind::Ref(mutability),
ExprKind::Index(..) | ExprKind::Unary(UnOp::Deref, _) => capture = CaptureKind::Ref(Mutability::Not),
ExprKind::Assign(lhs, ..) | ExprKind::Assign(_, lhs, _) if lhs.hir_id == child_id => {
ExprKind::Assign(lhs, ..) | ExprKind::AssignOp(_, lhs, _) if lhs.hir_id == child_id => {
return CaptureKind::Ref(Mutability::Mut);
},
ExprKind::Field(..) => {

View File

@ -1,6 +1,8 @@
#![allow(dead_code)]
#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)]
use std::sync::Mutex;
// ##################################
// # Issue clippy#7369
// ##################################
@ -38,4 +40,56 @@ fn main() {
let (y, x) = x;
foo(x, y)
};
let m = Mutex::new(0u32);
let l = m.lock().unwrap();
let _ = if true {
drop(l);
println!("foo");
m.lock().unwrap();
0
} else if *l == 0 {
drop(l);
println!("foo");
println!("bar");
m.lock().unwrap();
1
} else {
drop(l);
println!("foo");
println!("baz");
m.lock().unwrap();
2
};
if true {
let _guard = m.lock();
println!("foo");
} else {
println!("foo");
}
if true {
let _guard = m.lock();
println!("foo");
println!("bar");
} else {
let _guard = m.lock();
println!("foo");
println!("baz");
}
let mut c = 0;
for _ in 0..5 {
if c == 0 {
c += 1;
println!("0");
} else if c == 1 {
c += 1;
println!("1");
} else {
c += 1;
println!("more");
}
}
}