Skip to content

Simplify the G in Diag<'a, G> - #162630

Open
nnethercote wants to merge 2 commits into
rust-lang:mainfrom
nnethercote:simplify-Diag-G
Open

Simplify the G in Diag<'a, G>#162630
nnethercote wants to merge 2 commits into
rust-lang:mainfrom
nnethercote:simplify-Diag-G

Conversation

@nnethercote

@nnethercote nnethercote commented Sep 11, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

The EmissionGuarantee trait isn't necessary, and just complicates things. Details in individual commits.

r? @oli-obk

@rustbot

rustbot commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

HIR ty lowering was modified

cc @fmease

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

The rustc-dev-guide subtree was changed. If your future PRs only touch the subtree, consider submitting them directly to rust-lang/rustc-dev-guide, which is where the document is primarily maintained (and has faster CI).

cc @BoxyUwU, @tshepang

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustc_macros::diagnostics was changed

cc @TaKO8Ki, @JonathanBrouwer

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

Some changes occurred to constck

cc @fee1-dead

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustc-dev-guide Area: rustc-dev-guide A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Sep 11, 2026
@nnethercote

nnethercote commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

The first two commits here are definite wins. I'm more ambivalent about the third commit: it makes some things better and some things worse and I'm not sure if it's a good idea overall.

ada4a
ada4a reviewed Sep 11, 2026
View reviewed changes
Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated
RalfJung
RalfJung reviewed Sep 11, 2026
View reviewed changes
Comment thread compiler/rustc_errors/src/diagnostic.rs Outdated
RalfJung
RalfJung reviewed Sep 11, 2026
View reviewed changes
Comment thread compiler/rustc_errors/src/diagnostic.rs Outdated
oli-obk
oli-obk approved these changes Sep 11, 2026
View reviewed changes

@oli-obk oli-obk left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially a bit sceptical about the third commit, but overall I think it's an improvement. The idea of carrying the emission guarantee as a generic param is nice, but ended up not that useful in practice

@bors r+

View changes since this review

@rust-bors

rust-bors Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 4377560 has been approved by oli-obk

It is now in the queue for this repository.

@rust-bors 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 Sep 11, 2026
@oli-obk

oli-obk commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@bors r- I raced my review with other reviews by not checking that something changed oops

@rust-bors rust-bors Bot 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 11, 2026
@rust-bors

rust-bors Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

This pull request was unapproved.

View changes since this unapproval

@Kobzol

Kobzol commented Sep 11, 2026

Copy link
Copy Markdown
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 11, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 11, 2026
@bors
Auto merge of #162630 - nnethercote:simplify-Diag-G, r=<try>
ecf783e 
Simplify the `G` in `Diag<'a, G>`
@rust-bors

rust-bors Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: ecf783e (ecf783ea825f14de5262e03ad65fd0a2d0bfab8b)
Base parent: ca0a647 (ca0a6473ffde01deb7fce24cc04864cf723e14a0)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (ecf783e): comparison URL.

Overall result: ❌ regressions - please read:

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.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@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.2% [0.1%, 0.2%] 5
Regressions ❌
(secondary)		 0.2% [0.1%, 0.3%] 6
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.2% [0.1%, 0.2%] 5

Max RSS (memory usage)

Results (primary -0.7%, secondary 2.1%)

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)		 2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)		 -0.7% [-0.9%, -0.6%] 4
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.7% [-0.9%, -0.6%] 4

Cycles

Results (primary 0.6%, secondary -4.7%)

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

mean range count
Regressions ❌
(primary)		 0.6% [0.4%, 1.0%] 16
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.7% [-7.6%, -2.6%] 9
All ❌✅ (primary) 0.6% [0.4%, 1.0%] 16

Binary size

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

Bootstrap: 494.036s -> 485.329s (-1.76%)
Artifact size: 407.63 MiB -> 408.47 MiB (0.21%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 11, 2026
@Kobzol

Kobzol commented Sep 11, 2026

Copy link
Copy Markdown
Member

Nice bootstrap wins!

@nnethercote
Avoid some struct_foo(..).emit() chains
603380e 
These can be replaced with `foo(..)`.

Also make the return type of `report_unterminated_block_comment` more
precise.
@nnethercote
Remove EmissionGuarantee
09bf6de 
It's a trait that is used to give `Diag::emit` different return types
depending on the `G` in `Diag<'_, G>`. It works, but it's overkill.
There are only four different `G` types in practice (`BugAbort`,
`FatalAbort`, `ErrorGuaranteed`, and `()`) and having inherent `emit`
methods for each of those four concrete `Diag<'_, G>` types is good
enough.

This commit makes that change. This removes `EmissionGuarantee`
trait bounds from many places, which is nice.
@nnethercote

nnethercote commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I have repushed without the third commit. I have an idea for also getting rid of the lifetime in Diag, so I will leave this PR for the uncontroversial changes.

Let's recheck perf, just for completeness:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 12, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Sep 12, 2026
@bors
Auto merge of #162630 - nnethercote:simplify-Diag-G, r=<try>
da9b623 
Simplify the `G` in `Diag<'a, G>`
@rust-bors

rust-bors Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: da9b623 (da9b623f1ddeb46973bd1e54da5ef2a325645322)
Base parent: 0fc1413 (0fc141305da7a8a222f65aef1f1acc739c46282b)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (da9b623): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary -0.7%, secondary 0.3%)

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

mean range count
Regressions ❌
(primary)		 0.5% [0.5%, 0.6%] 3
Regressions ❌
(secondary)		 3.0% [2.5%, 3.6%] 2
Improvements ✅
(primary)		 -1.0% [-2.1%, -0.5%] 10
Improvements ✅
(secondary)		 -2.4% [-2.9%, -1.9%] 2
All ❌✅ (primary) -0.7% [-2.1%, 0.6%] 13

Cycles

Results (primary 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.8% [0.4%, 2.1%] 9
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.8% [-1.4%, -0.4%] 8
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [-1.4%, 2.1%] 17

Binary size

Results (primary 0.0%, secondary 0.0%)

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

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

Bootstrap: 492.706s -> 492.567s (-0.03%)
Artifact size: 406.91 MiB -> 406.89 MiB (-0.00%)

@rustbot rustbot removed perf-regression Performance regression. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 12, 2026
@nnethercote

nnethercote commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

@oli-obk: are you ok with the first two commits? I'll do another PR later with the third commit in it and probably some other stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@ada4a ada4a ada4a left review comments
@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustc-dev-guide Area: rustc-dev-guide A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nnethercote @rustbot @oli-obk @Kobzol @rust-timer @RalfJung @ada4a