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

Awaiting a function returning a never type does not warn for unreachable code after the await #128434

Closed
ungrichtepfl opened this issue Jul 31, 2024 · 1 comment · Fixed by #128443
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-async Working group: Async & await

Comments

@ungrichtepfl
Copy link

ungrichtepfl commented Jul 31, 2024

Code

pub async fn endless_async() -> ! {
    loop {
        tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
    }
}

pub async fn should_warn() {
    endless_async().await;
    println!("This code is unreachable");  // no warning about unreachable code
}

pub fn endless() -> ! {
    loop {
        std::thread::sleep(std::time::Duration::from_secs(1));
    }
}

pub fn warns() {
    endless();
    println!("This code is unreachable");  // warning: unreachable statement
}

Current output

warning: unreachable statement
  --> src/lib.rs:20:5
   |
19 |     endless();
   |     --------- any code following this expression is unreachable
20 |     println!("This code is unreachable");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
   |
   = note: `#[warn(unreachable_code)]` on by default
   = note: this warning originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

Desired output

warning: unreachable statement
  --> src/lib.rs:9:5
   |
 8 |     endless_async().await;
   |     --------------------- any code following this expression is unreachable
 9 |     println!("This code is unreachable");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
   |
   = note: `#[warn(unreachable_code)]` on by default
   = note: this warning originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: unreachable statement
  --> src/lib.rs:20:5
   |
19 |     endless();
   |     --------- any code following this expression is unreachable
20 |     println!("This code is unreachable");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
   |
   = note: `#[warn(unreachable_code)]` on by default
   = note: this warning originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

Rationale and extra context

No response

Other cases

No response

Rust Version

rustc 1.80.0 (051478957 2024-07-21)
binary: rustc
commit-hash: 051478957371ee0084a7c0913941d2a8c4757bb9
commit-date: 2024-07-21
host: x86_64-unknown-linux-gnu
release: 1.80.0
LLVM version: 18.1.7

Anything else?

Same behaviour with the current nightly version:

rustc 1.82.0-nightly (f8060d282 2024-07-30)
binary: rustc
commit-hash: f8060d282d42770fadd73905e3eefb85660d3278
commit-date: 2024-07-30
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 18.1.7
@ungrichtepfl ungrichtepfl added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 31, 2024
@compiler-errors compiler-errors added the WG-async Working group: Async & await label Jul 31, 2024
@compiler-errors
Copy link
Member

I'll take a look at this 👀

This almost certainly has something to do with the await desugaring into a poll loop

@compiler-errors compiler-errors self-assigned this Jul 31, 2024
tgross35 added a commit to tgross35/rust that referenced this issue Jul 31, 2024
… 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` (rust-lang#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 rust-lang#128434
@bors bors closed this as completed in ac67d10 Aug 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 1, 2024
Rollup merge of rust-lang#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` (rust-lang#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 rust-lang#128434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-async Working group: Async & await
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants