Skip to content

Commit

Permalink
Unrolled build for #128443
Browse files Browse the repository at this point in the history
Rollup merge of #128443 - compiler-errors:async-unreachable, r=fmease

Properly mark loop as diverging if it has no breaks

Due to specifics about the desugaring of the `.await` operator, HIR typeck doesn't recognize that `.await`ing an `impl Future<Output = !>` will diverge in the same way as calling a `fn() -> !`.

This is because the await operator desugars to approximately:

```rust
loop {
    match future.poll(...) {
        Poll::Ready(x) => break x,
        Poll::Pending => {}
    }
}
```

We know that the value of `x` is `!`, however since `break` is a coercion site, we coerce `!` to some `?0` (the type of the loop expression). Then since the type of the `loop {...}` expression is `?0`, we will not detect the loop as diverging like we do with other expressions that evaluate to `!`:

https://github.com/rust-lang/rust/blob/0b5eb7ba7bd796fb39c8bb6acd9ef6c140f28b65/compiler/rustc_hir_typeck/src/expr.rs#L240-L243

We can technically fix this in two ways:
1. Make coercion of loop exprs more eagerly result in a type of `!` when the only break expressions have type `!`.
2. Make loops understand that all of that if they have only diverging break values, then the loop diverges as well.

(1.) likely has negative effects on inference, and seems like a weird special case to drill into coercion. However, it turns out that (2.) is very easy to implement, we already record whether a loop has any break expressions, and when we do so, we actually skip over any break expressions with diverging values!:

https://github.com/rust-lang/rust/blob/0b5eb7ba7bd796fb39c8bb6acd9ef6c140f28b65/compiler/rustc_hir_typeck/src/expr.rs#L713-L716

Thus, we can consider the loop as diverging if we see that it has no breaks, which is the change implemented in this PR.

This is not usually a problem in regular code for two reasons:
1. In regular code, we already mark `break diverging()` as unreachable if `diverging()` is unreachable. We don't do this for `.await`, since we suppress unreachable errors within `.await` (#64930). Un-suppressing this code will result in spurious unreachable expression errors pointing to internal await machinery.
3. In loops that truly have no breaks (e.g. `loop {}`), we already evaluate the type of the loop to `!`, so this special case is kinda moot. This only affects loops that have `break`s with values of type `!`.

Thus, this seems like a change that may affect more code than just `.await`, but it likely does not in meaningful ways; if it does, it's certainly correct to apply.

Fixes #128434
  • Loading branch information
rust-timer authored Aug 1, 2024
2 parents 28a58f2 + 74754b8 commit ffc0e2e
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 21 deletions.
2 changes: 2 additions & 0 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,6 +1306,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// No way to know whether it's diverging because
// of a `break` or an outer `break` or `return`.
self.diverges.set(Diverges::Maybe);
} else {
self.diverges.set(self.diverges.get() | Diverges::always(expr.span));
}

// If we permit break with a value, then result type is
Expand Down
54 changes: 33 additions & 21 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,30 +48,42 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Produces warning on the given node, if the current point in the
/// function is unreachable, and there hasn't been another warning.
pub(crate) fn warn_if_unreachable(&self, id: HirId, span: Span, kind: &str) {
// FIXME: Combine these two 'if' expressions into one once
// let chains are implemented
if let Diverges::Always { span: orig_span, custom_note } = self.diverges.get() {
// If span arose from a desugaring of `if` or `while`, then it is the condition itself,
// which diverges, that we are about to lint on. This gives suboptimal diagnostics.
// Instead, stop here so that the `if`- or `while`-expression's block is linted instead.
if !span.is_desugaring(DesugaringKind::CondTemporary)
&& !span.is_desugaring(DesugaringKind::Async)
&& !orig_span.is_desugaring(DesugaringKind::Await)
{
self.diverges.set(Diverges::WarnedAlways);
// If span arose from a desugaring of `if` or `while`, then it is the condition itself,
// which diverges, that we are about to lint on. This gives suboptimal diagnostics.
// Instead, stop here so that the `if`- or `while`-expression's block is linted instead.
if span.is_desugaring(DesugaringKind::CondTemporary) {
return;
}

debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind);
// Don't lint if the result of an async block or async function is `!`.
// This does not affect the unreachable lints *within* the body.
if span.is_desugaring(DesugaringKind::Async) {
return;
}

let msg = format!("unreachable {kind}");
self.tcx().node_span_lint(lint::builtin::UNREACHABLE_CODE, id, span, |lint| {
lint.primary_message(msg.clone());
lint.span_label(span, msg).span_label(
orig_span,
custom_note.unwrap_or("any code following this expression is unreachable"),
);
})
}
// Don't lint *within* the `.await` operator, since that's all just desugaring junk.
// We only want to lint if there is a subsequent expression after the `.await`.
if span.is_desugaring(DesugaringKind::Await) {
return;
}

let Diverges::Always { span: orig_span, custom_note } = self.diverges.get() else {
return;
};

// Don't warn twice.
self.diverges.set(Diverges::WarnedAlways);

debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind);

let msg = format!("unreachable {kind}");
self.tcx().node_span_lint(lint::builtin::UNREACHABLE_CODE, id, span, |lint| {
lint.primary_message(msg.clone());
lint.span_label(span, msg).span_label(
orig_span,
custom_note.unwrap_or("any code following this expression is unreachable"),
);
})
}

/// Resolves type and const variables in `ty` if possible. Unlike the infcx
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/async-await/unreachable-lint-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@ edition:2018

#![deny(unreachable_code)]

async fn foo() {
endless().await;
println!("this is unreachable!");
//~^ ERROR unreachable statement
}

async fn endless() -> ! {
loop {}
}

fn main() { }
17 changes: 17 additions & 0 deletions tests/ui/async-await/unreachable-lint-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error: unreachable statement
--> $DIR/unreachable-lint-2.rs:7:5
|
LL | endless().await;
| ----- any code following this expression is unreachable
LL | println!("this is unreachable!");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
|
note: the lint level is defined here
--> $DIR/unreachable-lint-2.rs:3:9
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
= note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 1 previous error

0 comments on commit ffc0e2e

Please sign in to comment.