Skip to content

Commit

Permalink
Rollup merge of rust-lang#119554 - matthewjasper:remove-guard-distinc…
Browse files Browse the repository at this point in the history
…tion, r=compiler-errors

Fix scoping for let chains in match guards

If let guards were previously represented as a different type of guard in HIR and THIR. This meant that let chains in match guards were not handled correctly because they were treated exactly like normal guards.

- Remove `hir::Guard` and `thir::Guard`.
- Make the scoping different between normal guards and if let guards also check for let chains.

closes rust-lang#118593
  • Loading branch information
matthiaskrgr committed Jan 5, 2024
2 parents b418117 + f73e37d commit 84c4b52
Show file tree
Hide file tree
Showing 11 changed files with 56 additions and 112 deletions.
4 changes: 2 additions & 2 deletions clippy_lints/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use core::fmt::{self, Write};
use rustc_errors::Applicability;
use rustc_hir::hir_id::HirIdSet;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{Block, Expr, ExprKind, Guard, HirId, Let, Pat, Stmt, StmtKind, UnOp};
use rustc_hir::{Block, Expr, ExprKind, HirId, Pat, Stmt, StmtKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use rustc_span::{Span, SyntaxContext, DUMMY_SP};
Expand Down Expand Up @@ -465,7 +465,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> {
let mut is_map_used = self.is_map_used;
for arm in arms {
self.visit_pat(arm.pat);
if let Some(Guard::If(guard) | Guard::IfLet(&Let { init: guard, .. })) = arm.guard {
if let Some(guard) = arm.guard {
self.visit_non_tail_expr(guard);
}
is_map_used |= self.visit_cond_arm(arm.body);
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/manual_clamp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use clippy_utils::{
use itertools::Itertools;
use rustc_errors::{Applicability, Diagnostic};
use rustc_hir::def::Res;
use rustc_hir::{Arm, BinOpKind, Block, Expr, ExprKind, Guard, HirId, PatKind, PathSegment, PrimTy, QPath, StmtKind};
use rustc_hir::{Arm, BinOpKind, Block, Expr, ExprKind, HirId, PatKind, PathSegment, PrimTy, QPath, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::Ty;
use rustc_session::impl_lint_pass;
Expand Down Expand Up @@ -394,7 +394,7 @@ fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Opt
// Find possible min/max branches
let minmax_values = |a: &'tcx Arm<'tcx>| {
if let PatKind::Binding(_, var_hir_id, _, None) = &a.pat.kind
&& let Some(Guard::If(e)) = a.guard
&& let Some(e) = a.guard
{
Some((e, var_hir_id, a.body))
} else {
Expand Down
8 changes: 4 additions & 4 deletions clippy_lints/src/matches/collapsible_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use clippy_utils::{
};
use rustc_errors::MultiSpan;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{Arm, Expr, Guard, HirId, Let, Pat, PatKind};
use rustc_hir::{Arm, Expr, HirId, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_span::Span;

Expand All @@ -16,7 +16,7 @@ use super::COLLAPSIBLE_MATCH;
pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) {
for arm in arms {
check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body));
check_arm(cx, true, arm.pat, arm.body, arm.guard, Some(els_arm.body));
}
}
}
Expand All @@ -35,7 +35,7 @@ fn check_arm<'tcx>(
outer_is_match: bool,
outer_pat: &'tcx Pat<'tcx>,
outer_then_body: &'tcx Expr<'tcx>,
outer_guard: Option<&'tcx Guard<'tcx>>,
outer_guard: Option<&'tcx Expr<'tcx>>,
outer_else_body: Option<&'tcx Expr<'tcx>>,
) {
let inner_expr = peel_blocks_with_stmt(outer_then_body);
Expand Down Expand Up @@ -71,7 +71,7 @@ fn check_arm<'tcx>(
// the binding must not be used in the if guard
&& outer_guard.map_or(
true,
|(Guard::If(e) | Guard::IfLet(Let { init: e, .. }))| !is_local_used(cx, *e, binding_id)
|e| !is_local_used(cx, e, binding_id)
)
// ...or anywhere in the inner expression
&& match inner {
Expand Down
23 changes: 5 additions & 18 deletions clippy_lints/src/matches/match_like_matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{is_lint_allowed, is_wild, span_contains_comment};
use rustc_ast::{Attribute, LitKind};
use rustc_errors::Applicability;
use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat, PatKind, QPath};
use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Pat, PatKind, QPath};
use rustc_lint::{LateContext, LintContext};
use rustc_middle::ty;
use rustc_span::source_map::Spanned;
Expand Down Expand Up @@ -41,14 +41,8 @@ pub(super) fn check_match<'tcx>(
find_matches_sugg(
cx,
scrutinee,
arms.iter().map(|arm| {
(
cx.tcx.hir().attrs(arm.hir_id),
Some(arm.pat),
arm.body,
arm.guard.as_ref(),
)
}),
arms.iter()
.map(|arm| (cx.tcx.hir().attrs(arm.hir_id), Some(arm.pat), arm.body, arm.guard)),
e,
false,
)
Expand All @@ -67,14 +61,7 @@ where
I: Clone
+ DoubleEndedIterator
+ ExactSizeIterator
+ Iterator<
Item = (
&'a [Attribute],
Option<&'a Pat<'b>>,
&'a Expr<'b>,
Option<&'a Guard<'b>>,
),
>,
+ Iterator<Item = (&'a [Attribute], Option<&'a Pat<'b>>, &'a Expr<'b>, Option<&'a Expr<'b>>)>,
{
if !span_contains_comment(cx.sess().source_map(), expr.span)
&& iter.len() >= 2
Expand Down Expand Up @@ -115,7 +102,7 @@ where
})
.join(" | ")
};
let pat_and_guard = if let Some(Guard::If(g)) = first_guard {
let pat_and_guard = if let Some(g) = first_guard {
format!(
"{pat} if {}",
snippet_with_applicability(cx, g.span, "..", &mut applicability)
Expand Down
17 changes: 4 additions & 13 deletions clippy_lints/src/matches/needless_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clippy_utils::{
};
use rustc_errors::Applicability;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{Arm, BindingAnnotation, ByRef, Expr, ExprKind, Guard, ItemKind, Node, Pat, PatKind, Path, QPath};
use rustc_hir::{Arm, BindingAnnotation, ByRef, Expr, ExprKind, ItemKind, Node, Pat, PatKind, Path, QPath};
use rustc_lint::LateContext;
use rustc_span::sym;

Expand Down Expand Up @@ -66,18 +66,9 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>])
let arm_expr = peel_blocks_with_stmt(arm.body);

if let Some(guard_expr) = &arm.guard {
match guard_expr {
// gives up if `pat if expr` can have side effects
Guard::If(if_cond) => {
if if_cond.can_have_side_effects() {
return false;
}
},
// gives up `pat if let ...` arm
Guard::IfLet(_) => {
return false;
},
};
if guard_expr.can_have_side_effects() {
return false;
}
}

if let PatKind::Wild = arm.pat.kind {
Expand Down
50 changes: 21 additions & 29 deletions clippy_lints/src/matches/redundant_guards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use clippy_utils::visitors::{for_each_expr, is_local_used};
use rustc_ast::{BorrowKind, LitKind};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, MatchSource, Node, Pat, PatKind};
use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, Pat, PatKind};
use rustc_lint::LateContext;
use rustc_span::symbol::Ident;
use rustc_span::{Span, Symbol};
Expand All @@ -21,20 +21,19 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
};

// `Some(x) if matches!(x, y)`
if let Guard::If(if_expr) = guard
&& let ExprKind::Match(
scrutinee,
[
arm,
Arm {
pat: Pat {
kind: PatKind::Wild, ..
},
..
if let ExprKind::Match(
scrutinee,
[
arm,
Arm {
pat: Pat {
kind: PatKind::Wild, ..
},
],
MatchSource::Normal,
) = if_expr.kind
..
},
],
MatchSource::Normal,
) = guard.kind
&& let Some(binding) = get_pat_binding(cx, scrutinee, outer_arm)
{
let pat_span = match (arm.pat.kind, binding.byref_ident) {
Expand All @@ -45,14 +44,14 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
emit_redundant_guards(
cx,
outer_arm,
if_expr.span,
guard.span,
snippet(cx, pat_span, "<binding>"),
&binding,
arm.guard,
);
}
// `Some(x) if let Some(2) = x`
else if let Guard::IfLet(let_expr) = guard
else if let ExprKind::Let(let_expr) = guard.kind
&& let Some(binding) = get_pat_binding(cx, let_expr.init, outer_arm)
{
let pat_span = match (let_expr.pat.kind, binding.byref_ident) {
Expand All @@ -71,8 +70,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
}
// `Some(x) if x == Some(2)`
// `Some(x) if Some(2) == x`
else if let Guard::If(if_expr) = guard
&& let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
else if let ExprKind::Binary(bin_op, local, pat) = guard.kind
&& matches!(bin_op.node, BinOpKind::Eq)
// Ensure they have the same type. If they don't, we'd need deref coercion which isn't
// possible (currently) in a pattern. In some cases, you can use something like
Expand All @@ -96,16 +94,15 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
emit_redundant_guards(
cx,
outer_arm,
if_expr.span,
guard.span,
snippet(cx, pat_span, "<binding>"),
&binding,
None,
);
} else if let Guard::If(if_expr) = guard
&& let ExprKind::MethodCall(path, recv, args, ..) = if_expr.kind
} else if let ExprKind::MethodCall(path, recv, args, ..) = guard.kind
&& let Some(binding) = get_pat_binding(cx, recv, outer_arm)
{
check_method_calls(cx, outer_arm, path.ident.name, recv, args, if_expr, &binding);
check_method_calls(cx, outer_arm, path.ident.name, recv, args, guard, &binding);
}
}
}
Expand Down Expand Up @@ -216,7 +213,7 @@ fn emit_redundant_guards<'tcx>(
guard_span: Span,
binding_replacement: Cow<'static, str>,
pat_binding: &PatBindingInfo,
inner_guard: Option<Guard<'_>>,
inner_guard: Option<&Expr<'_>>,
) {
span_lint_and_then(
cx,
Expand All @@ -242,12 +239,7 @@ fn emit_redundant_guards<'tcx>(
(
guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),
inner_guard.map_or_else(String::new, |guard| {
let (prefix, span) = match guard {
Guard::If(e) => ("if", e.span),
Guard::IfLet(l) => ("if let", l.span),
};

format!(" {prefix} {}", snippet(cx, span, "<guard>"))
format!(" if {}", snippet(cx, guard.span, "<guard>"))
}),
),
],
Expand Down
20 changes: 9 additions & 11 deletions clippy_lints/src/matches/redundant_pattern_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::LangItem::{self, OptionNone, OptionSome, PollPending, PollReady, ResultErr, ResultOk};
use rustc_hir::{Arm, Expr, ExprKind, Guard, Node, Pat, PatKind, QPath, UnOp};
use rustc_hir::{Arm, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, GenericArgKind, Ty};
use rustc_span::{sym, Span, Symbol};
Expand Down Expand Up @@ -277,8 +277,6 @@ pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op
let mut sugg = format!("{}.{good_method}", snippet(cx, result_expr.span, "_"));

if let Some(guard) = maybe_guard {
let Guard::If(guard) = *guard else { return }; // `...is_none() && let ...` is a syntax error

// wow, the HIR for match guards in `PAT if let PAT = expr && expr => ...` is annoying!
// `guard` here is `Guard::If` with the let expression somewhere deep in the tree of exprs,
// counter to the intuition that it should be `Guard::IfLet`, so we need another check
Expand Down Expand Up @@ -319,7 +317,7 @@ fn found_good_method<'tcx>(
cx: &LateContext<'_>,
arms: &'tcx [Arm<'tcx>],
node: (&PatKind<'_>, &PatKind<'_>),
) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> {
) -> Option<(&'static str, Option<&'tcx Expr<'tcx>>)> {
match node {
(
PatKind::TupleStruct(ref path_left, patterns_left, _),
Expand Down Expand Up @@ -409,7 +407,7 @@ fn get_good_method<'tcx>(
cx: &LateContext<'_>,
arms: &'tcx [Arm<'tcx>],
path_left: &QPath<'_>,
) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> {
) -> Option<(&'static str, Option<&'tcx Expr<'tcx>>)> {
if let Some(name) = get_ident(path_left) {
let (expected_item_left, should_be_left, should_be_right) = match name.as_str() {
"Ok" => (Item::Lang(ResultOk), "is_ok()", "is_err()"),
Expand Down Expand Up @@ -478,7 +476,7 @@ fn find_good_method_for_match<'a, 'tcx>(
expected_item_right: Item,
should_be_left: &'a str,
should_be_right: &'a str,
) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> {
) -> Option<(&'a str, Option<&'tcx Expr<'tcx>>)> {
let first_pat = arms[0].pat;
let second_pat = arms[1].pat;

Expand All @@ -496,8 +494,8 @@ fn find_good_method_for_match<'a, 'tcx>(

match body_node_pair {
(ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) {
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())),
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard)),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard)),
_ => None,
},
_ => None,
Expand All @@ -511,7 +509,7 @@ fn find_good_method_for_matches_macro<'a, 'tcx>(
expected_item_left: Item,
should_be_left: &'a str,
should_be_right: &'a str,
) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> {
) -> Option<(&'a str, Option<&'tcx Expr<'tcx>>)> {
let first_pat = arms[0].pat;

let body_node_pair = if is_pat_variant(cx, first_pat, path_left, expected_item_left) {
Expand All @@ -522,8 +520,8 @@ fn find_good_method_for_matches_macro<'a, 'tcx>(

match body_node_pair {
(ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) {
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())),
(LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard)),
(LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard)),
_ => None,
},
_ => None,
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/mixed_read_write_in_expression.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_note};
use clippy_utils::{get_parent_expr, path_to_local, path_to_local_id};
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind};
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Local, Node, Stmt, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_session::declare_lint_pass;
Expand Down Expand Up @@ -119,7 +119,7 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> {
ExprKind::Match(e, arms, _) => {
self.visit_expr(e);
for arm in arms {
if let Some(Guard::If(if_expr)) = arm.guard {
if let Some(if_expr) = arm.guard {
self.visit_expr(if_expr);
}
// make sure top level arm expressions aren't linted
Expand Down
10 changes: 2 additions & 8 deletions clippy_lints/src/utils/author.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,17 +318,11 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
self.pat(field!(arm.pat));
match arm.value.guard {
None => chain!(self, "{arm}.guard.is_none()"),
Some(hir::Guard::If(expr)) => {
Some(expr) => {
bind!(self, expr);
chain!(self, "let Some(Guard::If({expr})) = {arm}.guard");
chain!(self, "let Some({expr}) = {arm}.guard");
self.expr(expr);
},
Some(hir::Guard::IfLet(let_expr)) => {
bind!(self, let_expr);
chain!(self, "let Some(Guard::IfLet({let_expr}) = {arm}.guard");
self.pat(field!(let_expr.pat));
self.expr(field!(let_expr.init));
},
}
self.expr(field!(arm.body));
}
Expand Down

0 comments on commit 84c4b52

Please sign in to comment.