Properly mark loop as diverging if it has no breaks

[compiler-errors]

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

This is because the await operator desugars to approximately:

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 !:

rust/compiler/rustc_hir_typeck/src/expr.rs

Lines 240 to 243 in 0b5eb7b

	// Any expression that produces a value of type `!` must have diverged
	if ty.is_never() {
	self.diverges.set(self.diverges.get() | Diverges::always(expr.span));
	}

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 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!:

rust/compiler/rustc_hir_typeck/src/expr.rs

Lines 713 to 716 in 0b5eb7b

	// If we encountered a `break`, then (no surprise) it may be possible to break from the
	// loop... unless the value being returned from the loop diverges itself, e.g.
	// `break return 5` or `break loop {}`.
	ctxt.may_break |= !self.diverges.get().is_always();

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 (Silence unreachable code lint from await desugaring #64930). Un-suppressing this code will result in spurious unreachable expression errors pointing to internal await machinery.
2. 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 breaks 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

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

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.

[fmease]

Very well-written explanation, thanks!

[fmease]

@bors r+

[bors]

📌 Commit 74754b8 has been approved by fmease

It is now in the queue for this repository.

[compiler-errors]

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 f().await now marks subsequent statements as unused due to more accurately tracking divergence.

@rustbot label +relnotes

For relnotes: The compiler now triggers the unreachable code warning properly for async functions that don't return/are -> !.

[Kobzol]

@rust-timer build ffc0e2e

[rust-timer]

Finished benchmarking commit (ffc0e2e): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.7%]	20
Regressions ❌ (secondary)	1.0%	[0.2%, 2.1%]	36
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.7%]	20

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.3%	[-3.3%, -3.3%]	1

Cycles

Results (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.

	mean	range	count
Regressions ❌ (primary)	2.7%	[1.4%, 8.1%]	7
Regressions ❌ (secondary)	3.3%	[3.2%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[1.4%, 8.1%]	7

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 757.595s -> 759.601s (0.26%)
Artifact size: 336.69 MiB -> 336.74 MiB (0.02%)

[compiler-errors]

?????????????????

[compiler-errors]

Oh wait, I probably know what caused it. I changed the warn_if_unreachable function.

[Kobzol]

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.