Don't emit allow attributes in try/contract desugaring by mejrs · Pull Request #164122 · rust-lang/rust

mejrs · 2026-10-10T21:00:06Z

rustbot · 2026-10-10T21:00:11Z

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 · 2026-10-10T21:00:21Z

@bors try @rust-timer queue

Don't emit allow attributes in try/contract desugaring

rust-bors · 2026-10-10T22:04:16Z

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

rust-timer · 2026-10-10T23:57:10Z

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 · 2026-10-11T08:21:33Z

-        // `ControlFlow::Break(residual) =>
-        //     #[allow(unreachable_code)]
-        //     return Try::from_residual(residual),`
+        // `ControlFlow::Break(residual) => return Try::from_residual(residual),`


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?

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

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 · 2026-10-11T08:25:08Z

    ///         // If there is an enclosing `try {...}`:
    ///         break 'catch_target Residual::into_try_type(residual),


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 · 2026-10-11T09:06:21Z

+            // Don't lint *within* the `?` operator
+            Some(DesugaringKind::QuestionMark) => return,
+
+            // Don't lint *within* contract attrs
+            Some(DesugaringKind::Contract) => return,


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 · 2026-10-11T10:11:11Z

            Some(DesugaringKind::Await) => return,

+            // Don't lint *within* the `?` operator
+            Some(DesugaringKind::QuestionMark) => return,


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 · 2026-10-11T14:41:27Z

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 · 2026-10-11T15:38:26Z

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 · 2026-10-11T16:22:02Z

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

Edit 2: but at least removing this line does make a test fail (tests/ui/try-block/try-block-unreachable-code-lint.rs) in ways that supports us not having a "don't warn about unreachable code in desugaring spans" catch-all.

mejrs · 2026-10-11T17:07:25Z

pushed some comments and yeeted the QuestionMark desugaring check. dont have the spoons to look at the contract stuff rn

hanna-kruppe · 2026-10-12T16:10:03Z

Thanks for the added/improved comments. I'm not very worried about the contracts part: the changes look sensible to me, it works (and breaks) as I'd expect when I experimented with it, and if I did miss anything it's not as bad as for stable features.

But I don't feel comfortable approving the try desugaring changes right now because there's something weird going on that I don't fully understand. I'd have to take more time to dig into that, and I probably won't get around to that soon (next weekend at earliest). Feel free to find/roll another reviewer who's more confident about this.

mejrs · 2026-10-13T16:15:36Z

Feel free to find/roll another reviewer who's more confident about this.

I think that's maybe @scottmcm? Would you be willing to review the changes to the desugaring?

rustbot · 2026-10-19T14:02:19Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rust-bors · 2026-10-25T12:43:56Z

💔 Test for 6863a39 failed: CI. Failed job:

auto - test-aarch64-apple-macos-26-2 (web logs, enhanced plaintext logs)

rust-log-analyzer · 2026-10-25T12:44:00Z

The job test-aarch64-apple-macos-26-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Important

For more information how to resolve CI failures of this job, visit this link.

JonathanBrouwer · 2026-10-25T13:03:25Z

Is this PR just getting incredibly unlucky?
@bors try jobs=test-aarch64-apple-macos-26-2,test-aarch64-msvc-1

Don't emit allow attributes in try/contract desugaring try-job: test-aarch64-apple-macos-26-2 try-job: test-aarch64-msvc-1

JonathanBrouwer · 2026-10-25T13:06:19Z

@bors retry
Pretty sure it is...

JonathanBrouwer · 2026-10-25T15:03:45Z

@bors rollup=iffy
Creating a rollup of perf-sensitive PRs

…=hanna-kruppe Don't emit allow attributes in try/contract desugaring

…uwer Rollup of 3 perf-sensitive pull requests Successful merges: - #163845 (Add various `#[inline]` annotations for simple functions on hot paths that don't get inlined) - #164122 (Don't emit allow attributes in try/contract desugaring) - #164260 (create fast path for attributes created in builtin macro expansions) Failed merges: - #164790 (Split `LintLevelsBuilder::add` into `add_ast` and `add_hir`)

rust-bors · 2026-10-25T16:51:04Z

💔 Test for acf7c30 failed: CI. Failed job:

try - test-aarch64-apple-macos-26-2 (web logs, enhanced plaintext logs)

rust-log-analyzer · 2026-10-25T17:00:08Z

The job test-aarch64-apple-macos-26-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Important

For more information how to resolve CI failures of this job, visit this link.

JonathanBrouwer · 2026-10-25T17:07:47Z

(spurious)

…=hanna-kruppe Don't emit allow attributes in try/contract desugaring

…uwer Rollup of 4 perf-sensitive pull requests Successful merges: - #163845 (Add various `#[inline]` annotations for simple functions on hot paths that don't get inlined) - #164122 (Don't emit allow attributes in try/contract desugaring) - #164260 (create fast path for attributes created in builtin macro expansions) - #164300 (Deduplicate DefId encodings in the query cache) Failed merges: - #164790 (Split `LintLevelsBuilder::add` into `add_ast` and `add_hir`)

…uwer Rollup of 5 pull requests Successful merges: - #163845 (Add various `#[inline]` annotations for simple functions on hot paths that don't get inlined) - #164122 (Don't emit allow attributes in try/contract desugaring) - #164260 (create fast path for attributes created in builtin macro expansions) - #164300 (Deduplicate DefId encodings in the query cache) - #164929 (Revert "rustc_codegen_gcc subtree update") Failed merges: - #164790 (Split `LintLevelsBuilder::add` into `add_ast` and `add_hir`)

Rollup merge of #164122 - mejrs:no-new_ast_lowering-attrs, r=hanna-kruppe Don't emit allow attributes in try/contract desugaring

rust-timer · 2026-10-26T03:48:57Z

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (eeff8b4): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.7%	[-1.9%, -0.1%]	103
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.1%]	23
All ❌✅ (primary)	-0.7%	[-1.9%, -0.1%]	103

Max RSS (memory usage)

Results (primary 0.1%, secondary 0.4%)

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

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 1.7%]	13
Regressions ❌ (secondary)	0.7%	[0.5%, 1.2%]	7
Improvements ✅ (primary)	-1.1%	[-3.3%, -0.5%]	10
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	0.1%	[-3.3%, 1.7%]	23

Cycles

Results (primary -0.9%, secondary 1.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.4%, 1.1%]	4
Regressions ❌ (secondary)	2.2%	[0.6%, 5.1%]	5
Improvements ✅ (primary)	-1.2%	[-3.2%, -0.4%]	20
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.4%]	2
All ❌✅ (primary)	-0.9%	[-3.2%, 1.1%]	24

Binary size

This perf run didn't have relevant results for this metric.

Artifact size: 405.91 MiB -> 406.11 MiB (0.05%)

rustbot assigned hanna-kruppe Oct 10, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 10, 2026

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2026

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Oct 10, 2026

Auto merge of #164122 - mejrs:no-new_ast_lowering-attrs, r=<try>

8b7218a

Don't emit allow attributes in try/contract desugaring

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2026

mejrs force-pushed the no-new_ast_lowering-attrs branch 2 times, most recently from ac3d7ab to 1b0cff3 Compare October 11, 2026 07:42

hanna-kruppe reviewed Oct 11, 2026

View reviewed changes

hanna-kruppe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2026

mejrs force-pushed the no-new_ast_lowering-attrs branch from 1b0cff3 to 8ede13b Compare October 11, 2026 16:27

mejrs added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2026

This comment has been minimized.

Sign in to view

mejrs force-pushed the no-new_ast_lowering-attrs branch from 8ede13b to b385c26 Compare October 19, 2026 14:02

This comment has been minimized.

Sign in to view

rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2026

rust-bors Bot pushed a commit that referenced this pull request Oct 25, 2026

Auto merge of #164122 - mejrs:no-new_ast_lowering-attrs, r=<try>

acf7c30

Don't emit allow attributes in try/contract desugaring try-job: test-aarch64-apple-macos-26-2 try-job: test-aarch64-msvc-1

This comment has been minimized.

Sign in to view

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2026

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Oct 25, 2026

Rollup merge of rust-lang#164122 - mejrs:no-new_ast_lowering-attrs, r…

9f2d25e

…=hanna-kruppe Don't emit allow attributes in try/contract desugaring

JonathanBrouwer mentioned this pull request Oct 25, 2026

Rollup of 3 perf-sensitive pull requests #164911

Closed

rust-bors Bot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 25, 2026

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Oct 25, 2026

Rollup merge of rust-lang#164122 - mejrs:no-new_ast_lowering-attrs, r…

f84f441

…=hanna-kruppe Don't emit allow attributes in try/contract desugaring

JonathanBrouwer mentioned this pull request Oct 25, 2026

Rollup of 4 perf-sensitive pull requests #164921

Closed

JonathanBrouwer mentioned this pull request Oct 25, 2026

Rollup of 5 perf-sensitive pull requests #164931

Merged

rust-bors Bot merged commit 0a4720d into rust-lang:main Oct 25, 2026
14 of 16 checks passed

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 25, 2026

rust-bors Bot pushed a commit that referenced this pull request Oct 25, 2026

Unrolled build for #164122 in rollup 164931

eeff8b4

Rollup merge of #164122 - mejrs:no-new_ast_lowering-attrs, r=hanna-kruppe Don't emit allow attributes in try/contract desugaring

rustbot added this to the 1.101.0 milestone Oct 25, 2026

		/// // If there is an enclosing `try {...}`:
		/// break 'catch_target Residual::into_try_type(residual),

Conversation

mejrs commented Oct 10, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 10, 2026

Uh oh!

mejrs commented Oct 10, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Oct 10, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Oct 10, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

hanna-kruppe left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mejrs commented Oct 11, 2026

Uh oh!

hanna-kruppe commented Oct 11, 2026

Uh oh!

hanna-kruppe commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mejrs commented Oct 11, 2026

Uh oh!

hanna-kruppe commented Oct 12, 2026

Uh oh!

mejrs commented Oct 13, 2026

Uh oh!

This comment has been minimized.

rustbot commented Oct 19, 2026

Uh oh!

This comment has been minimized.

rust-bors Bot commented Oct 25, 2026

Uh oh!

rust-log-analyzer commented Oct 25, 2026

Uh oh!

JonathanBrouwer commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

JonathanBrouwer commented Oct 25, 2026

Uh oh!

JonathanBrouwer commented Oct 25, 2026

Uh oh!

rust-bors Bot commented Oct 25, 2026

Uh oh!

rust-log-analyzer commented Oct 25, 2026

Uh oh!

JonathanBrouwer commented Oct 25, 2026

Uh oh!

Uh oh!

rust-timer commented Oct 26, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mejrs commented Oct 10, 2026 •

edited by rustbot

Loading

hanna-kruppe left a comment •

edited by rustbot

Loading

hanna-kruppe commented Oct 11, 2026 •

edited

Loading

JonathanBrouwer commented Oct 25, 2026 •

edited

Loading