Skip to content

Commit

Permalink
Rollup merge of #94248 - compiler-errors:fix-while-loop-bad-delay, r=…
Browse files Browse the repository at this point in the history 
…petrochenkov

Fix ICE when passing block to while-loop condition

We were incorrectly delaying a bug when we passed _any_ block (that evaluated to `()`) to a while loop. This PR makes the check a bit more sophisticated.

We should only suppress the error here in cases that are equivalent to those we find in #93574 (i.e. only while loop conditions that have destructuring assignment expressions in them).

Fixes #93997
cc `@estebank` who added this code

I would not be opposed to removing the delay-bug altogether, and just emitting this error always. I much prefer duplicate errors over no errors.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Feb 28, 2022
2 parents a040e2f + 025b7c4 commit 9340791
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 71 deletions.
35 changes: 24 additions & 11 deletions compiler/rustc_typeck/src/check/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
err.span_label(lhs.span, "cannot assign to this expression");

let mut parent = self.tcx.hir().get_parent_node(lhs.hir_id);
self.comes_from_while_condition(lhs.hir_id, |expr| {
err.span_suggestion_verbose(
expr.span.shrink_to_lo(),
"you might have meant to use pattern destructuring",
"let ".to_string(),
Applicability::MachineApplicable,
);
});

err.emit();
}

// Check if an expression `original_expr_id` comes from the condition of a while loop,
// as opposed from the body of a while loop, which we can naively check by iterating
// parents until we find a loop...
pub(super) fn comes_from_while_condition(
&self,
original_expr_id: HirId,
then: impl FnOnce(&hir::Expr<'_>),
) {
let mut parent = self.tcx.hir().get_parent_node(original_expr_id);
while let Some(node) = self.tcx.hir().find(parent) {
match node {
hir::Node::Expr(hir::Expr {
Expand All @@ -861,21 +881,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
),
..
}) => {
// Check if our lhs is a child of the condition of a while loop
let expr_is_ancestor = std::iter::successors(Some(lhs.hir_id), |id| {
// Check if our original expression is a child of the condition of a while loop
let expr_is_ancestor = std::iter::successors(Some(original_expr_id), |id| {
self.tcx.hir().find_parent_node(*id)
})
.take_while(|id| *id != parent)
.any(|id| id == expr.hir_id);
// if it is, then we have a situation like `while Some(0) = value.get(0) {`,
// where `while let` was more likely intended.
if expr_is_ancestor {
err.span_suggestion_verbose(
expr.span.shrink_to_lo(),
"you might have meant to use pattern destructuring",
"let ".to_string(),
Applicability::MachineApplicable,
);
then(expr);
}
break;
}
Expand All @@ -888,8 +903,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}
}

err.emit();
}

// A generic function for checking the 'then' and 'else' clauses in an 'if'
Expand Down
134 changes: 74 additions & 60 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -768,55 +768,57 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let prev_diverges = self.diverges.get();
let ctxt = BreakableCtxt { coerce: Some(coerce), may_break: false };

let (ctxt, ()) = self.with_breakable_ctxt(blk.hir_id, ctxt, || {
for (pos, s) in blk.stmts.iter().enumerate() {
self.check_stmt(s, blk.stmts.len() - 1 == pos);
}
let (ctxt, ()) =
self.with_breakable_ctxt(blk.hir_id, ctxt, || {
for (pos, s) in blk.stmts.iter().enumerate() {
self.check_stmt(s, blk.stmts.len() - 1 == pos);
}

// check the tail expression **without** holding the
// `enclosing_breakables` lock below.
let tail_expr_ty = tail_expr.map(|t| self.check_expr_with_expectation(t, expected));

let mut enclosing_breakables = self.enclosing_breakables.borrow_mut();
let ctxt = enclosing_breakables.find_breakable(blk.hir_id);
let coerce = ctxt.coerce.as_mut().unwrap();
if let Some(tail_expr_ty) = tail_expr_ty {
let tail_expr = tail_expr.unwrap();
let span = self.get_expr_coercion_span(tail_expr);
let cause = self.cause(span, ObligationCauseCode::BlockTailExpression(blk.hir_id));
coerce.coerce(self, &cause, tail_expr, tail_expr_ty);
} else {
// Subtle: if there is no explicit tail expression,
// that is typically equivalent to a tail expression
// of `()` -- except if the block diverges. In that
// case, there is no value supplied from the tail
// expression (assuming there are no other breaks,
// this implies that the type of the block will be
// `!`).
//
// #41425 -- label the implicit `()` as being the
// "found type" here, rather than the "expected type".
if !self.diverges.get().is_always() {
// #50009 -- Do not point at the entire fn block span, point at the return type
// span, as it is the cause of the requirement, and
// `consider_hint_about_removing_semicolon` will point at the last expression
// if it were a relevant part of the error. This improves usability in editors
// that highlight errors inline.
let mut sp = blk.span;
let mut fn_span = None;
if let Some((decl, ident)) = self.get_parent_fn_decl(blk.hir_id) {
let ret_sp = decl.output.span();
if let Some(block_sp) = self.parent_item_span(blk.hir_id) {
// HACK: on some cases (`ui/liveness/liveness-issue-2163.rs`) the
// output would otherwise be incorrect and even misleading. Make sure
// the span we're aiming at correspond to a `fn` body.
if block_sp == blk.span {
sp = ret_sp;
fn_span = Some(ident.span);
// check the tail expression **without** holding the
// `enclosing_breakables` lock below.
let tail_expr_ty = tail_expr.map(|t| self.check_expr_with_expectation(t, expected));

let mut enclosing_breakables = self.enclosing_breakables.borrow_mut();
let ctxt = enclosing_breakables.find_breakable(blk.hir_id);
let coerce = ctxt.coerce.as_mut().unwrap();
if let Some(tail_expr_ty) = tail_expr_ty {
let tail_expr = tail_expr.unwrap();
let span = self.get_expr_coercion_span(tail_expr);
let cause =
self.cause(span, ObligationCauseCode::BlockTailExpression(blk.hir_id));
coerce.coerce(self, &cause, tail_expr, tail_expr_ty);
} else {
// Subtle: if there is no explicit tail expression,
// that is typically equivalent to a tail expression
// of `()` -- except if the block diverges. In that
// case, there is no value supplied from the tail
// expression (assuming there are no other breaks,
// this implies that the type of the block will be
// `!`).
//
// #41425 -- label the implicit `()` as being the
// "found type" here, rather than the "expected type".
if !self.diverges.get().is_always() {
// #50009 -- Do not point at the entire fn block span, point at the return type
// span, as it is the cause of the requirement, and
// `consider_hint_about_removing_semicolon` will point at the last expression
// if it were a relevant part of the error. This improves usability in editors
// that highlight errors inline.
let mut sp = blk.span;
let mut fn_span = None;
if let Some((decl, ident)) = self.get_parent_fn_decl(blk.hir_id) {
let ret_sp = decl.output.span();
if let Some(block_sp) = self.parent_item_span(blk.hir_id) {
// HACK: on some cases (`ui/liveness/liveness-issue-2163.rs`) the
// output would otherwise be incorrect and even misleading. Make sure
// the span we're aiming at correspond to a `fn` body.
if block_sp == blk.span {
sp = ret_sp;
fn_span = Some(ident.span);
}
}
}
}
coerce.coerce_forced_unit(
coerce.coerce_forced_unit(
self,
&self.misc(sp),
&mut |err| {
Expand All @@ -825,19 +827,31 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if expected_ty == self.tcx.types.bool {
// If this is caused by a missing `let` in a `while let`,
// silence this redundant error, as we already emit E0070.
let parent = self.tcx.hir().get_parent_node(blk.hir_id);
let parent = self.tcx.hir().get_parent_node(parent);
let parent = self.tcx.hir().get_parent_node(parent);
let parent = self.tcx.hir().get_parent_node(parent);
let parent = self.tcx.hir().get_parent_node(parent);
match self.tcx.hir().find(parent) {
Some(hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::Loop(_, _, hir::LoopSource::While, _),
..
})) => {

// Our block must be a `assign desugar local; assignment`
if let Some(hir::Node::Block(hir::Block {
stmts:
[hir::Stmt {
kind:
hir::StmtKind::Local(hir::Local {
source: hir::LocalSource::AssignDesugar(_),
..
}),
..
}, hir::Stmt {
kind:
hir::StmtKind::Expr(hir::Expr {
kind: hir::ExprKind::Assign(..),
..
}),
..
}],
..
})) = self.tcx.hir().find(blk.hir_id)
{
self.comes_from_while_condition(blk.hir_id, |_| {
err.downgrade_to_delayed_bug();
}
_ => {}
})
}
}
}
Expand All @@ -851,9 +865,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
},
false,
);
}
}
}
});
});

if ctxt.may_break {
// If we can break from the block, then the block's exit is always reachable
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/typeck/while-loop-block-cond.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fn main() {
while {} {}
//~^ ERROR mismatched types [E0308]
}
9 changes: 9 additions & 0 deletions src/test/ui/typeck/while-loop-block-cond.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
error[E0308]: mismatched types
--> $DIR/while-loop-block-cond.rs:2:11
|
LL | while {} {}
| ^^ expected `bool`, found `()`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.

0 comments on commit 9340791

Please sign in to comment.