Don't emit allow attributes in try/contract desugaring

[mejrs]

No description provided.

[rustbot]

r? @hanna-kruppe

rustbot has assigned @hanna-kruppe.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 19 candidates

[mejrs]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8b7218a (8b7218ac4d11ab00b2196dc99aeb473c1b18af19)
Base parent: 6e4cba3 (6e4cba3380a6f9e1e4d7672e11e40372bf04e873)

[rust-timer]

Finished benchmarking commit (8b7218a): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	3
Improvements ✅ (primary)	-0.6%	[-2.0%, -0.1%]	111
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.1%]	66
All ❌✅ (primary)	-0.6%	[-2.0%, -0.1%]	111

Max RSS (memory usage)

Results (primary -0.8%, secondary 5.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.2%	[2.4%, 10.5%]	6
Improvements ✅ (primary)	-0.8%	[-1.0%, -0.6%]	8
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-1.0%, -0.6%]	8

Cycles

Results (primary 1.3%, secondary 4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.0%, 2.6%]	4
Regressions ❌ (secondary)	8.1%	[2.6%, 11.4%]	4
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-3.1%	[-3.9%, -2.3%]	2
All ❌✅ (primary)	1.3%	[-2.3%, 2.6%]	5

Binary size

Results (primary 0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	123
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	105
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	123

Bootstrap: 488.311s -> 490.216s (0.39%)
Artifact size: 407.22 MiB -> 406.67 MiB (-0.13%)

[hanna-kruppe]

This makes sense to me in the abstract, but I'm not confident that I fully understand the ramifications. I tried experimenting a bit, but so far I haven't managed to find any test that even showcases why we need to allow unreachable code in parts of the try desugaring. That's a bit concerning, mostly for my ability to review this, but also for test coverage.

View changes since this review

[hanna-kruppe]

Nit: this comment doesn't reflect the variations:

• break instead of return within try blocks
• Try::into_residual vs Try::from_residual depending on the kind of try block

That's a pre-existing problem, but maybe you want to fix it while we're here?

[mejrs]

I wonder if we should just delete this documentation, it feels like it's just going to become wrong again in the future.

[hanna-kruppe]

I do think it's helpful to have it spelled out in one place (maybe not in both places). Even as outdated as it was, it helped me pattern match what the code is doing and notice the gaps. Reverse engineering the whole desugaring from the code alone seems harder to me.

[hanna-kruppe]

Nit: this comment is outdated since #149489, which added the option break 'catch_target Residual::from_residual(residual), for heterogeneous try blocks -- also pre-existing but may be worth a drive-by fix.

[hanna-kruppe]

I think this applies to more HIR nodes than the previous #[allow(..)] did because all nodes produced by the lowering have the same span with the desugaring bit set. For try blocks that's the whole match including patterns, not just the expressions following the arms.

Is this right? If yes, is it observable? If not, why not?

[hanna-kruppe]

If I drop this line, all of tests/ui/ still passes. (The contracts counterpart is exercised by the test suite.) I think we need at least one test for this, ideally several because there's at minimum two expressions (possibly also unreachable patterns, see earlier comment).

[mejrs]

I tried experimenting a bit, but so far I haven't managed to find any test that even showcases why we need to allow unreachable code in parts of the try desugaring. That's a bit concerning, mostly for my ability to review this, but also for test coverage.

The test is https://github.com/rust-lang/rust/blob/main/tests/ui/reachable/unreachable-try-pattern.rs

It was added in #39127

In any case, I think the allow is now unnecessary because we desugar differently. Back then the second arm would desugar into a bunch of function calls, now it doesn't.

For example,

fn main(){
    let x: Option<!> = None;
    match x {
        None => {},
        Some(never) => { From::from(never) },
    }
}

emits

warning: unreachable call
 --> src/main.rs:5:26
  |
5 |         Some(never) => { From::from(never) },
  |                          ^^^^^^^^^^ ----- any code following this expression is unreachable
  |                          |
  |                          unreachable call
  |
  = note: `#[warn(unreachable_code)]` (part of `#[warn(unused)]`) on by default

but this doesn't

fn main(){
    let x: Option<!> = None;
    match x {
        None => {},
        Some(never) => { never },
    }
}

I'm not sure why the attribute is on the first arm. Maybe the unreachable code lint is implemented differently now.

[hanna-kruppe]

I saw that test, but since it doesn’t fail when removing the #[allow]s on main or that early return in this PR, it doesn’t really help me understand what’s going on or judge if the test is sufficient.

I assume you’re right that something has changed to make the allow/early-return unnecessary, but I don’t understand what/how. The try desugaring still produces a call in the ControlFlow::Break arm. The other option that comes to mind is that some other part of the code suppresses unreachable code warnings from desugared spans somehow, but if so, I haven’t found it so far and a bunch of other “don’t warn if this is a desugaring” code would also be unnecessary now.

[hanna-kruppe]

Hm, this seems very relevant:

rust/compiler/rustc_hir_typeck/src/expr.rs

Lines 291 to 294 in 0617e57

	// If `expr` is a result of desugaring the try block and is an ok-wrapped
	// diverging expression (e.g. it arose from desugaring of `try { return }`),
	// we skip issuing a warning because it is autogenerated code.
	ExprKind::Call(..) if expr.span.is_desugaring(DesugaringKind::TryBlock) => {}

Edit: oops, no, DesugaringKind::QuestionMark != DesugaringKind::TryBlock