-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Properly mark loop as diverging if it has no breaks #128443
Conversation
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote this method to split up the 3 special cases and properly document them. No functional changes other than:
orig_span.is_desugaring(DesugaringKind::Await)
to
span.is_desugaring(DesugaringKind::Await)
Since the former suppresses any unreachable warnings originating from an await, but we actually want to suppress any unreachable warnings that occur within an await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well-written explanation, thanks!
@bors r+ |
I consider this to be strictly a bugfix, and therefore deserves no team approval, but I do think that it's worth mentioning in the relnotes just in case a crate maintainer notices that @rustbot label +relnotes For relnotes: The compiler now triggers the unreachable code warning properly for async functions that don't return/are |
… 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
Rollup of 8 pull requests Successful merges: - rust-lang#127567 (std: implement the `once_wait` feature) - rust-lang#127624 (Migrate and rename `issue-47551`, `issue-35164` and `issue-69368` `run-make` tests to rmake) - rust-lang#128162 (Cleanup sys module to match house style) - rust-lang#128437 (improve bootstrap to allow selecting llvm tools individually) - rust-lang#128443 (Properly mark loop as diverging if it has no breaks) - rust-lang#128449 (Temporarily switch `ambiguous_negative_literals` lint to allow) - rust-lang#128450 (Create COFF archives for non-LLVM backends) - rust-lang#128452 (derive(SmartPointer): require pointee to be maybe sized) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#127567 (std: implement the `once_wait` feature) - rust-lang#128162 (Cleanup sys module to match house style) - rust-lang#128296 (Update target-spec metadata for loongarch64 targets) - rust-lang#128443 (Properly mark loop as diverging if it has no breaks) - rust-lang#128449 (Temporarily switch `ambiguous_negative_literals` lint to allow) - rust-lang#128452 (derive(SmartPointer): require pointee to be maybe sized) r? `@ghost` `@rustbot` modify labels: rollup
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
@rust-timer build ffc0e2e |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (ffc0e2e): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -3.3%, secondary 2.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 2.7%, secondary 3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 757.595s -> 759.601s (0.26%) |
????????????????? |
Oh wait, I probably know what caused it. I changed the |
Seems like a bit more time is spent in typeck. Not sure if this was expected, but this seems like a correctness fix, and the regression in primary benchmarks is only in check builds, so nothing that terrible. If you have any ideas how to fix, would be nice to send a PR, otherwise we can probably let it slide. |
…able, r=<try> Check divergence value first before doing span operations in `warn_if_unreachable` It's more expensive to extract the span's desugaring first rather than check the value of the divergence enum. For some reason I inverted these checks, probably for readability, but as a consequence I regressed perf: rust-lang#128443 (comment) r? fmease
…able, r=fmease Check divergence value first before doing span operations in `warn_if_unreachable` It's more expensive to extract the span's desugaring first rather than check the value of the divergence enum. For some reason I inverted these checks, probably for readability, but as a consequence I regressed perf: rust-lang#128443 (comment) r? fmease
…mease Check divergence value first before doing span operations in `warn_if_unreachable` It's more expensive to extract the span's desugaring first rather than check the value of the divergence enum. For some reason I inverted these checks, probably for readability, but as a consequence I regressed perf: rust-lang/rust#128443 (comment) r? fmease
Due to specifics about the desugaring of the
.await
operator, HIR typeck doesn't recognize that.await
ing animpl Future<Output = !>
will diverge in the same way as calling afn() -> !
.This is because the await operator desugars to approximately:
We know that the value of
x
is!
, however sincebreak
is a coercion site, we coerce!
to some?0
(the type of the loop expression). Then since the type of theloop {...}
expression is?0
, we will not detect the loop as diverging like we do with other expressions that evaluate to!
:rust/compiler/rustc_hir_typeck/src/expr.rs
Lines 240 to 243 in 0b5eb7b
We can technically fix this in two ways:
!
when the only break expressions have type!
.(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!:
rust/compiler/rustc_hir_typeck/src/expr.rs
Lines 713 to 716 in 0b5eb7b
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:
break diverging()
as unreachable ifdiverging()
is unreachable. We don't do this for.await
, since we suppress unreachable errors within.await
(Silence unreachable code lint from await desugaring #64930). Un-suppressing this code will result in spurious unreachable expression errors pointing to internal await machinery.loop {}
), we already evaluate the type of the loop to!
, so this special case is kinda moot. This only affects loops that havebreak
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