-use crate::utils::{
- both, count_eq, eq_expr_value, first_line_of_span, get_enclosing_block, get_parent_expr, if_sequence, in_macro,
- indent_of, parent_node_is_if_expr, reindent_multiline, run_lints, search_same, snippet, snippet_opt,
- span_lint_and_note, span_lint_and_then, ContainsName, SpanlessEq, SpanlessHash,
+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;
///
/// **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
- 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);
- // 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;
+ };
+
+ // BRANCHES_SHARING_CODE prerequisites
+ if has_conditional_else || (start_eq == 0 && end_eq == 0 && (has_expr && !expr_eq)) {
return;
}
intravisit::walk_stmt(&mut start_walker, stmt);
}
- emit_shared_code_in_if_blocks_lint(
+ emit_branches_sharing_code_lint(
cx,
start_eq,
0,
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);
end_walker.defs.drain().for_each(|x| {
block_defs.insert(x);
});
}
- emit_shared_code_in_if_blocks_lint(
+ emit_branches_sharing_code_lint(
cx,
start_eq,
end_eq,
}
}
-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;
let r_stmts = win[1].stmts;
// `SpanlessEq` now keeps track of the locals and is therefore context sensitive clippy#6752.
- // The comparison therefor needs to be done in a way that builds the correct context.
+ // 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();
if block_expr_eq;
if l_stmts.len() == r_stmts.len();
if l_stmts.len() == current_start_eq;
- if run_lints(cx, &[IF_SAME_THEN_ELSE], win[0].hir_id);
- if run_lints(cx, &[IF_SAME_THEN_ELSE], win[1].hir_id);
+ 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,
"same as this",
);
- return (0, 0, false);
+ return None;
}
}
expr_eq &= block_expr_eq;
}
- let has_expr = blocks[0].expr.is_some();
- if has_expr && !expr_eq {
+ if !expr_eq {
end_eq = 0;
}
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(
})
}
-fn emit_shared_code_in_if_blocks_lint(
+fn emit_branches_sharing_code_lint(
cx: &LateContext<'tcx>,
start_stmts: usize,
end_stmts: usize,
}
suggestions.push(("end", span, suggestion.to_string()));
- add_expr_note = !cx.typeck_results().expr_ty(if_expr).is_unit()
+ 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.");
+ 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.");
+ diag.warn("Some moved values might need to be renamed to avoid wrong references");
}
};
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_then(cx, SHARED_CODE_IN_IF_BLOCKS, span, msg.as_str(), |diag| {
+ 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);
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",
move |diag| {
}
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