Skip to content

Commit

Permalink
Auto merge of #7075 - xFrednet:7054-fp-branches-sharing-code, r=camst…
Browse files Browse the repository at this point in the history
…effen,flip1995

Fixing FPs for the `branches_sharing_code` lint

Fixes #7053
Fixes #7054
And an additional CSS adjustment to support dark mode for every inline code. It currently only works in paragraphs, which was an oversight on my part 😅. [Current Example](https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name)

This also includes ~50 lines of doc comments and is therefor not as big as the changes would indicate. 🐧

---

changelog: none

All of these bugs were introduced in this dev version and are therefor not worth a change log entry.

r? `@phansch`
cc: `@camsteffen` since you have a pretty good overview of the `SpanlessEq` implementation 🙃
  • Loading branch information
bors committed Apr 16, 2021
2 parents 28dbcd8 + 0b4af72 commit eaf0f3d
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 28 deletions.
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 @@ -1300,21 +1300,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

0 comments on commit eaf0f3d

Please sign in to comment.