-use crate::utils::{both, count_eq, eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
-use crate::utils::{
- first_line_of_span, get_parent_expr, if_sequence, indent_of, parent_node_is_if_expr, reindent_multiline, snippet,
- snippet_opt, span_lint_and_note, span_lint_and_sugg, span_lint_and_then,
+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, is_else_clause,
+ is_lint_allowed, search_same, ContainsName, SpanlessEq, SpanlessHash,
};
+use if_chain::if_chain;
use rustc_data_structures::fx::FxHashSet;
-use rustc_errors::Applicability;
+use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_hir::{Block, Expr, ExprKind, HirId};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::map::Map;
use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{source_map::Span, BytePos};
+use rustc_span::{source_map::Span, symbol::Symbol, BytePos};
use std::borrow::Cow;
declare_clippy_lint! {
///
/// **Why is this bad?** Duplicate code is less maintainable.
///
- /// **Known problems:** Hopefully none.
+ /// **Known problems:**
+ /// * The lint doesn't check if the moved expressions modify values that are beeing used in
+ /// the if condition. The suggestion can in that case modify the behavior of the program.
+ /// See [rust-clippy#7452](https://github.com/rust-lang/rust-clippy/issues/7452)
///
/// **Example:**
/// ```ignore
/// 42
/// };
/// ```
- pub SHARED_CODE_IN_IF_BLOCKS,
- nursery,
+ pub BRANCHES_SHARING_CODE,
+ complexity,
"`if` statement with shared code in all blocks"
}
IFS_SAME_COND,
SAME_FUNCTIONS_IN_IF_CONDITION,
IF_SAME_THEN_ELSE,
- SHARED_CODE_IN_IF_BLOCKS
+ BRANCHES_SHARING_CODE
]);
impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if !expr.span.from_expansion() {
- // skip ifs directly in else, it will be checked in the parent if
- if let Some(&Expr {
- kind: ExprKind::If(_, _, Some(ref else_expr)),
- ..
- }) = get_parent_expr(cx, expr)
- {
- if else_expr.hir_id == expr.hir_id {
- return;
+ if let ExprKind::If(_, _, _) = expr.kind {
+ // skip ifs directly in else, it will be checked in the parent if
+ if let Some(&Expr {
+ kind: ExprKind::If(_, _, Some(else_expr)),
+ ..
+ }) = get_parent_expr(cx, expr)
+ {
+ if else_expr.hir_id == expr.hir_id {
+ return;
+ }
}
- }
- let (conds, blocks) = if_sequence(expr);
- // Conditions
- lint_same_cond(cx, &conds);
- lint_same_fns_in_if_cond(cx, &conds);
- // Block duplication
- lint_same_then_else(cx, &blocks, conds.len() != blocks.len(), expr);
+ let (conds, blocks) = if_sequence(expr);
+ // Conditions
+ lint_same_cond(cx, &conds);
+ lint_same_fns_in_if_cond(cx, &conds);
+ // Block duplication
+ lint_same_then_else(cx, &blocks, conds.len() == blocks.len(), expr);
+ }
}
}
}
-/// Implementation of `SHARED_CODE_IN_IF_BLOCKS` and `IF_SAME_THEN_ELSE` if the blocks are equal.
+/// Implementation of `BRANCHES_SHARING_CODE` and `IF_SAME_THEN_ELSE` if the blocks are equal.
fn lint_same_then_else<'tcx>(
cx: &LateContext<'tcx>,
blocks: &[&Block<'tcx>],
- has_unconditional_else: bool,
+ has_conditional_else: bool,
expr: &'tcx Expr<'_>,
) {
// We only lint ifs with multiple blocks
- // TODO xFrednet 2021-01-01: Check if it's an else if block
- if blocks.len() < 2 || parent_node_is_if_expr(expr, cx) {
+ if blocks.len() < 2 || is_else_clause(cx.tcx, expr) {
return;
}
- let has_expr = blocks[0].expr.is_some();
-
// Check if each block has shared code
- let mut start_eq = usize::MAX;
- let mut end_eq = usize::MAX;
- let mut expr_eq = true;
- for win in blocks.windows(2) {
- let l_stmts = win[0].stmts;
- let r_stmts = win[1].stmts;
-
- let mut evaluator = SpanlessEq::new(cx);
- let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r));
- let current_end_eq = count_eq(&mut l_stmts.iter().rev(), &mut r_stmts.iter().rev(), |l, r| {
- evaluator.eq_stmt(l, r)
- });
- let block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
-
- // IF_SAME_THEN_ELSE
- if block_expr_eq && l_stmts.len() == r_stmts.len() && l_stmts.len() == current_start_eq {
- span_lint_and_note(
- cx,
- IF_SAME_THEN_ELSE,
- win[0].span,
- "this `if` has identical blocks",
- Some(win[1].span),
- "same as this",
- );
-
- return;
- }
-
- start_eq = start_eq.min(current_start_eq);
- end_eq = end_eq.min(current_end_eq);
- expr_eq &= block_expr_eq;
- }
+ let has_expr = blocks[0].expr.is_some();
- // SHARED_CODE_IN_IF_BLOCKS prerequisites
- if !has_unconditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) {
+ 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;
- }
-
- if has_expr && !expr_eq {
- end_eq = 0;
- }
+ };
- // Check if the regions are overlapping. Set `end_eq` to prevent the overlap
- let min_block_size = blocks.iter().map(|x| x.stmts.len()).min().unwrap();
- if (start_eq + end_eq) > min_block_size {
- end_eq = min_block_size - start_eq;
+ // BRANCHES_SHARING_CODE prerequisites
+ if has_conditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) {
+ return;
}
// Only the start is the same
if start_eq != 0 && end_eq == 0 && (!has_expr || !expr_eq) {
- emit_shared_code_in_if_blocks_lint(cx, start_eq, 0, false, blocks, expr);
- } else if end_eq != 0 && (!has_expr || !expr_eq) {
+ let block = blocks[0];
+ let start_stmts = block.stmts.split_at(start_eq).0;
+
+ let mut start_walker = UsedValueFinderVisitor::new(cx);
+ for stmt in start_stmts {
+ intravisit::walk_stmt(&mut start_walker, stmt);
+ }
+
+ emit_branches_sharing_code_lint(
+ cx,
+ start_eq,
+ 0,
+ false,
+ check_for_warn_of_moved_symbol(cx, &start_walker.def_symbols, expr),
+ blocks,
+ expr,
+ );
+ } else if end_eq != 0 || (has_expr && expr_eq) {
let block = blocks[blocks.len() - 1];
- let stmts = block.stmts.split_at(start_eq).1;
- let (block_stmts, moved_stmts) = stmts.split_at(stmts.len() - end_eq);
+ let (start_stmts, block_stmts) = block.stmts.split_at(start_eq);
+ let (block_stmts, end_stmts) = block_stmts.split_at(block_stmts.len() - end_eq);
+
+ // Scan start
+ let mut start_walker = UsedValueFinderVisitor::new(cx);
+ for stmt in start_stmts {
+ intravisit::walk_stmt(&mut start_walker, stmt);
+ }
+ let mut moved_syms = start_walker.def_symbols;
// Scan block
- let mut walker = SymbolFinderVisitor::new(cx);
+ let mut block_walker = UsedValueFinderVisitor::new(cx);
for stmt in block_stmts {
- intravisit::walk_stmt(&mut walker, stmt);
+ intravisit::walk_stmt(&mut block_walker, stmt);
}
- let mut block_defs = walker.defs;
+ let mut block_defs = block_walker.defs;
// Scan moved stmts
let mut moved_start: Option<usize> = None;
- let mut walker = SymbolFinderVisitor::new(cx);
- for (index, stmt) in moved_stmts.iter().enumerate() {
- intravisit::walk_stmt(&mut walker, stmt);
+ let mut end_walker = UsedValueFinderVisitor::new(cx);
+ for (index, stmt) in end_stmts.iter().enumerate() {
+ intravisit::walk_stmt(&mut end_walker, stmt);
- for value in &walker.uses {
+ for value in &end_walker.uses {
// Well we can't move this and all prev statements. So reset
- if block_defs.contains(&value) {
+ if block_defs.contains(value) {
moved_start = Some(index + 1);
- walker.defs.drain().for_each(|x| {
+ end_walker.defs.drain().for_each(|x| {
block_defs.insert(x);
});
+
+ end_walker.def_symbols.clear();
}
}
- walker.uses.clear();
+ end_walker.uses.clear();
}
if let Some(moved_start) = moved_start {
end_eq -= moved_start;
}
- let end_linable = if let Some(expr) = block.expr {
- intravisit::walk_expr(&mut walker, expr);
- walker.uses.iter().any(|x| !block_defs.contains(x))
- } else if end_eq == 0 {
- false
- } else {
- true
+ let end_linable = block.expr.map_or_else(
+ || end_eq != 0,
+ |expr| {
+ intravisit::walk_expr(&mut end_walker, expr);
+ end_walker.uses.iter().any(|x| !block_defs.contains(x))
+ },
+ );
+
+ if end_linable {
+ end_walker.def_symbols.drain().for_each(|x| {
+ moved_syms.insert(x);
+ });
+ }
+
+ emit_branches_sharing_code_lint(
+ cx,
+ start_eq,
+ end_eq,
+ end_linable,
+ check_for_warn_of_moved_symbol(cx, &moved_syms, expr),
+ blocks,
+ expr,
+ );
+ }
+}
+
+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;
+ for win in blocks.windows(2) {
+ let l_stmts = win[0].stmts;
+ let r_stmts = win[1].stmts;
+
+ // `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 = evaluator.inter_expr();
+
+ let current_start_eq = count_eq(&mut l_stmts.iter(), &mut r_stmts.iter(), |l, r| evaluator.eq_stmt(l, r));
+
+ let current_end_eq = {
+ // We skip the middle statements which can't be equal
+ let end_comparison_count = l_stmts.len().min(r_stmts.len()) - current_start_eq;
+ let it1 = l_stmts.iter().skip(l_stmts.len() - end_comparison_count);
+ let it2 = r_stmts.iter().skip(r_stmts.len() - end_comparison_count);
+ it1.zip(it2)
+ .fold(0, |acc, (l, r)| if evaluator.eq_stmt(l, r) { acc + 1 } else { 0 })
};
+ let block_expr_eq = both(&win[0].expr, &win[1].expr, |l, r| evaluator.eq_expr(l, r));
+
+ // IF_SAME_THEN_ELSE
+ if_chain! {
+ if block_expr_eq;
+ if l_stmts.len() == r_stmts.len();
+ if l_stmts.len() == current_start_eq;
+ if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win[0].hir_id);
+ if !is_lint_allowed(cx, IF_SAME_THEN_ELSE, win[1].hir_id);
+ then {
+ span_lint_and_note(
+ cx,
+ IF_SAME_THEN_ELSE,
+ win[0].span,
+ "this `if` has identical blocks",
+ Some(win[1].span),
+ "same as this",
+ );
+
+ return None;
+ }
+ }
+
+ start_eq = start_eq.min(current_start_eq);
+ end_eq = end_eq.min(current_end_eq);
+ expr_eq &= block_expr_eq;
+ }
- emit_shared_code_in_if_blocks_lint(cx, start_eq, end_eq, end_linable, blocks, expr);
+ if !expr_eq {
+ end_eq = 0;
+ }
+
+ // Check if the regions are overlapping. Set `end_eq` to prevent the overlap
+ let min_block_size = blocks.iter().map(|x| x.stmts.len()).min().unwrap();
+ if (start_eq + end_eq) > min_block_size {
+ end_eq = min_block_size - start_eq;
}
+
+ Some(BlockEqual {
+ start_eq,
+ end_eq,
+ expr_eq,
+ })
+}
+
+fn check_for_warn_of_moved_symbol(
+ cx: &LateContext<'tcx>,
+ symbols: &FxHashSet<Symbol>,
+ if_expr: &'tcx Expr<'_>,
+) -> bool {
+ get_enclosing_block(cx, if_expr.hir_id).map_or(false, |block| {
+ let ignore_span = block.span.shrink_to_lo().to(if_expr.span);
+
+ symbols
+ .iter()
+ .filter(|sym| !sym.as_str().starts_with('_'))
+ .any(move |sym| {
+ let mut walker = ContainsName {
+ name: *sym,
+ result: false,
+ };
+
+ // Scan block
+ block
+ .stmts
+ .iter()
+ .filter(|stmt| !ignore_span.overlaps(stmt.span))
+ .for_each(|stmt| intravisit::walk_stmt(&mut walker, stmt));
+
+ if let Some(expr) = block.expr {
+ intravisit::walk_expr(&mut walker, expr);
+ }
+
+ walker.result
+ })
+ })
}
-fn emit_shared_code_in_if_blocks_lint(
+fn emit_branches_sharing_code_lint(
cx: &LateContext<'tcx>,
start_stmts: usize,
end_stmts: usize,
lint_end: bool,
+ warn_about_moved_symbol: bool,
blocks: &[&Block<'tcx>],
if_expr: &'tcx Expr<'_>,
) {
// (help, span, suggestion)
let mut suggestions: Vec<(&str, Span, String)> = vec![];
+ let mut add_expr_note = false;
+ // Construct suggestions
if start_stmts > 0 {
let block = blocks[0];
let span_start = first_line_of_span(cx, if_expr.span).shrink_to_lo();
block.stmts[block.stmts.len() - end_stmts].span
}
.source_callsite();
- let moved_end = if let Some(expr) = block.expr {
- expr.span
- } else {
- block.stmts[block.stmts.len() - 1].span
- }
- .source_callsite();
+ let moved_end = block
+ .expr
+ .map_or_else(|| block.stmts[block.stmts.len() - 1].span, |expr| expr.span)
+ .source_callsite();
let moved_span = moved_start.to(moved_end);
let moved_snipped = reindent_multiline(snippet(cx, moved_span, "_"), true, None);
}
suggestions.push(("end", span, suggestion.to_string()));
+ add_expr_note = !cx.typeck_results().expr_ty(if_expr).is_unit();
}
+ let add_optional_msgs = |diag: &mut DiagnosticBuilder<'_>| {
+ if add_expr_note {
+ diag.note("The end suggestion probably needs some adjustments to use the expression result correctly");
+ }
+
+ if warn_about_moved_symbol {
+ diag.warn("Some moved values might need to be renamed to avoid wrong references");
+ }
+ };
+
+ // Emit lint
if suggestions.len() == 1 {
let (place_str, span, sugg) = suggestions.pop().unwrap();
- let msg = format!("All if blocks contain the same code at the {}", place_str);
- let help = format!("Consider moving the {} statements out like this", place_str);
- span_lint_and_sugg(
- cx,
- SHARED_CODE_IN_IF_BLOCKS,
- span,
- msg.as_str(),
- help.as_str(),
- sugg,
- Applicability::Unspecified,
- );
+ let msg = format!("all if blocks contain the same code at the {}", place_str);
+ let help = format!("consider moving the {} statements out like this", place_str);
+ span_lint_and_then(cx, BRANCHES_SHARING_CODE, span, msg.as_str(), |diag| {
+ diag.span_suggestion(span, help.as_str(), sugg, Applicability::Unspecified);
+
+ add_optional_msgs(diag);
+ });
} else if suggestions.len() == 2 {
let (_, end_span, end_sugg) = suggestions.pop().unwrap();
let (_, start_span, start_sugg) = suggestions.pop().unwrap();
span_lint_and_then(
cx,
- SHARED_CODE_IN_IF_BLOCKS,
+ BRANCHES_SHARING_CODE,
start_span,
- "All if blocks contain the same code at the start and the end. Here at the start:",
+ "all if blocks contain the same code at the start and the end. Here at the start",
move |diag| {
- diag.span_note(end_span, "And here at the end:");
+ diag.span_note(end_span, "and here at the end");
diag.span_suggestion(
start_span,
- "Consider moving the start statements out like this:",
+ "consider moving the start statements out like this",
start_sugg,
Applicability::Unspecified,
);
diag.span_suggestion(
end_span,
- "And consider moving the end statements out like this:",
+ "and consider moving the end statements out like this",
end_sugg,
Applicability::Unspecified,
);
+
+ add_optional_msgs(diag);
},
);
}
}
-pub struct SymbolFinderVisitor<'a, 'tcx> {
+/// This visitor collects `HirId`s and Symbols of defined symbols and `HirId`s of used values.
+struct UsedValueFinderVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
+
+ /// The `HirId`s of defined values in the scanned statements
defs: FxHashSet<HirId>,
+
+ /// The Symbols of the defined symbols in the scanned statements
+ def_symbols: FxHashSet<Symbol>,
+
+ /// The `HirId`s of the used values
uses: FxHashSet<HirId>,
}
-impl<'a, 'tcx> SymbolFinderVisitor<'a, 'tcx> {
+impl<'a, 'tcx> UsedValueFinderVisitor<'a, 'tcx> {
fn new(cx: &'a LateContext<'tcx>) -> Self {
- SymbolFinderVisitor {
+ UsedValueFinderVisitor {
cx,
defs: FxHashSet::default(),
+ def_symbols: FxHashSet::default(),
uses: FxHashSet::default(),
}
}
}
-impl<'a, 'tcx> Visitor<'tcx> for SymbolFinderVisitor<'a, 'tcx> {
+impl<'a, 'tcx> Visitor<'tcx> for UsedValueFinderVisitor<'a, 'tcx> {
type Map = Map<'tcx>;
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
fn visit_local(&mut self, l: &'tcx rustc_hir::Local<'tcx>) {
let local_id = l.pat.hir_id;
self.defs.insert(local_id);
+
+ if let Some(sym) = l.pat.simple_ident() {
+ self.def_symbols.insert(sym.name);
+ }
+
if let Some(expr) = l.init {
intravisit::walk_expr(self, expr);
}
}
fn visit_qpath(&mut self, qpath: &'tcx rustc_hir::QPath<'tcx>, id: HirId, _span: rustc_span::Span) {
- if let rustc_hir::QPath::Resolved(_, ref path) = *qpath {
+ if let rustc_hir::QPath::Resolved(_, path) = *qpath {
if path.segments.len() == 1 {
if let rustc_hir::def::Res::Local(var) = self.cx.qpath_res(qpath, id) {
self.uses.insert(var);
projects / rust.git / blobdiff