Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scoping for let chains in match guards #119554

Merged
merged 4 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 1 addition & 14 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,20 +546,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

fn lower_arm(&mut self, arm: &Arm) -> hir::Arm<'hir> {
let pat = self.lower_pat(&arm.pat);
let guard = arm.guard.as_ref().map(|cond| {
if let ExprKind::Let(pat, scrutinee, span, is_recovered) = &cond.kind {
hir::Guard::IfLet(self.arena.alloc(hir::Let {
hir_id: self.next_id(),
span: self.lower_span(*span),
pat: self.lower_pat(pat),
ty: None,
init: self.lower_expr(scrutinee),
is_recovered: *is_recovered,
}))
} else {
hir::Guard::If(self.lower_expr(cond))
}
});
let guard = arm.guard.as_ref().map(|cond| self.lower_expr(cond));
let hir_id = self.next_id();
let span = self.lower_span(arm.span);
self.lower_attrs(hir_id, &arm.attrs);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3590,7 +3590,7 @@ impl<'b, 'v> Visitor<'v> for ConditionVisitor<'b> {
));
} else if let Some(guard) = &arm.guard {
self.errors.push((
arm.pat.span.to(guard.body().span),
arm.pat.span.to(guard.span),
format!(
"if this pattern and condition are matched, {} is not \
initialized",
Expand Down
22 changes: 1 addition & 21 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,7 @@ pub struct Arm<'hir> {
/// If this pattern and the optional guard matches, then `body` is evaluated.
pub pat: &'hir Pat<'hir>,
/// Optional guard clause.
pub guard: Option<Guard<'hir>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I've always been so confused why this was represented differently in HIR. This was also what caused the bug fixed by #119402, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if_let_guards was implemented before let chains were ready. But it's definitely outstayed its usefulness.

pub guard: Option<&'hir Expr<'hir>>,
/// The expression the arm evaluates to if this arm matches.
pub body: &'hir Expr<'hir>,
}
Expand All @@ -1280,26 +1280,6 @@ pub struct Let<'hir> {
pub is_recovered: Option<ErrorGuaranteed>,
}

#[derive(Debug, Clone, Copy, HashStable_Generic)]
pub enum Guard<'hir> {
If(&'hir Expr<'hir>),
IfLet(&'hir Let<'hir>),
}

impl<'hir> Guard<'hir> {
/// Returns the body of the guard
///
/// In other words, returns the e in either of the following:
///
/// - `if e`
/// - `if let x = e`
pub fn body(&self) -> &'hir Expr<'hir> {
match self {
Guard::If(e) | Guard::IfLet(Let { init: e, .. }) => e,
}
}
}

#[derive(Debug, Clone, Copy, HashStable_Generic)]
pub struct ExprField<'hir> {
#[stable_hasher(ignore)]
Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_hir/src/intravisit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -619,13 +619,8 @@ pub fn walk_stmt<'v, V: Visitor<'v>>(visitor: &mut V, statement: &'v Stmt<'v>) {
pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm<'v>) {
visitor.visit_id(arm.hir_id);
visitor.visit_pat(arm.pat);
if let Some(ref g) = arm.guard {
match g {
Guard::If(ref e) => visitor.visit_expr(e),
Guard::IfLet(ref l) => {
visitor.visit_let_expr(l);
}
}
if let Some(ref e) = arm.guard {
visitor.visit_expr(e);
}
visitor.visit_expr(arm.body);
}
Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_hir_analysis/src/check/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,14 +177,24 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
}

fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) {
fn has_let_expr(expr: &Expr<'_>) -> bool {
match &expr.kind {
hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
hir::ExprKind::Let(..) => true,
_ => false,
}
}

let prev_cx = visitor.cx;

visitor.terminating_scopes.insert(arm.hir_id.local_id);

visitor.enter_node_scope_with_dtor(arm.hir_id.local_id);
visitor.cx.var_parent = visitor.cx.parent;

if let Some(hir::Guard::If(expr)) = arm.guard {
if let Some(expr) = arm.guard
&& !has_let_expr(expr)
{
visitor.terminating_scopes.insert(expr.hir_id.local_id);
}

Expand Down
14 changes: 3 additions & 11 deletions compiler/rustc_hir_pretty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1874,17 +1874,9 @@ impl<'a> State<'a> {
self.print_pat(arm.pat);
self.space();
if let Some(ref g) = arm.guard {
match *g {
hir::Guard::If(e) => {
self.word_space("if");
self.print_expr(e);
self.space();
}
hir::Guard::IfLet(&hir::Let { pat, ty, init, .. }) => {
self.word_nbsp("if");
self.print_let(pat, ty, init);
}
}
self.word_space("if");
self.print_expr(g);
self.space();
}
self.word_space("=>");

Expand Down
11 changes: 2 additions & 9 deletions compiler/rustc_hir_typeck/src/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let mut other_arms = vec![]; // Used only for diagnostics.
let mut prior_arm = None;
for arm in arms {
if let Some(g) = &arm.guard {
if let Some(e) = &arm.guard {
self.diverges.set(Diverges::Maybe);
match g {
hir::Guard::If(e) => {
self.check_expr_has_type_or_error(e, tcx.types.bool, |_| {});
}
hir::Guard::IfLet(l) => {
self.check_expr_let(l);
}
};
self.check_expr_has_type_or_error(e, tcx.types.bool, |_| {});
}

self.diverges.set(Diverges::Maybe);
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_hir_typeck/src/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,12 +669,8 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
);
self.walk_pat(discr_place, arm.pat, arm.guard.is_some());

match arm.guard {
Some(hir::Guard::If(e)) => self.consume_expr(e),
Some(hir::Guard::IfLet(l)) => {
self.walk_local(l.init, l.pat, None, |t| t.borrow_expr(l.init, ty::ImmBorrow))
}
None => {}
if let Some(ref e) = arm.guard {
self.consume_expr(e)
}

self.consume_expr(arm.body);
Expand Down
9 changes: 1 addition & 8 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,20 +519,13 @@ pub struct FruInfo<'tcx> {
#[derive(Clone, Debug, HashStable)]
pub struct Arm<'tcx> {
pub pattern: Box<Pat<'tcx>>,
pub guard: Option<Guard<'tcx>>,
pub guard: Option<ExprId>,
pub body: ExprId,
pub lint_level: LintLevel,
pub scope: region::Scope,
pub span: Span,
}

/// A `match` guard.
#[derive(Clone, Debug, HashStable)]
pub enum Guard<'tcx> {
If(ExprId),
IfLet(Box<Pat<'tcx>>, ExprId),
}

#[derive(Copy, Clone, Debug, HashStable)]
pub enum LogicalOp {
/// The `&&` operator.
Expand Down
11 changes: 3 additions & 8 deletions compiler/rustc_middle/src/thir/visit.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{
AdtExpr, Arm, Block, ClosureExpr, Expr, ExprKind, Guard, InlineAsmExpr, InlineAsmOperand, Pat,
AdtExpr, Arm, Block, ClosureExpr, Expr, ExprKind, InlineAsmExpr, InlineAsmOperand, Pat,
PatKind, Stmt, StmtKind, Thir,
};

Expand Down Expand Up @@ -213,13 +213,8 @@ pub fn walk_arm<'thir, 'tcx: 'thir, V: Visitor<'thir, 'tcx>>(
visitor: &mut V,
arm: &'thir Arm<'tcx>,
) {
match arm.guard {
Some(Guard::If(expr)) => visitor.visit_expr(&visitor.thir()[expr]),
Some(Guard::IfLet(ref pat, expr)) => {
visitor.visit_pat(pat);
visitor.visit_expr(&visitor.thir()[expr]);
}
None => {}
if let Some(expr) = arm.guard {
visitor.visit_expr(&visitor.thir()[expr])
}
visitor.visit_pat(&arm.pattern);
visitor.visit_expr(&visitor.thir()[arm.body]);
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
cond,
Some(condition_scope),
condition_scope,
source_info
source_info,
true,
));

this.expr_into_dest(destination, then_blk, then)
Expand Down Expand Up @@ -173,6 +174,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Some(condition_scope),
condition_scope,
source_info,
true,
)
});
let (short_circuit, continuation, constant) = match op {
Expand Down