Skip to content

Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro #151596

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.

Sign up for GitHub

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

Jump to bottom
Open
sgasho wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro #151596

sgasho wants to merge 1 commit into from
+39 −10

Conversation

@sgasho
Copy link
Contributor

@sgasho sgasho commented Jan 24, 2026

related: #150910

I fixed declare_passes macro to detect unregistered enum names

UI Results

+nightly --> before: no warnings
+stage1 --> after: detect SimplifyCfg as an unknown pass

スクリーンショット 2026-01-24 23 53 41

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 24, 2026
@sgasho sgasho marked this pull request as ready for review January 24, 2026 14:58
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2026

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 24, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2026

r? @nnethercote

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

@sgasho sgasho mentioned this pull request Jan 24, 2026
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@sgasho sgasho changed the title Fix -Zmir-enable-passes to detect unregistered enum names Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro Jan 24, 2026
@nnethercote
Copy link
Contributor

nnethercote commented Jan 26, 2026 •
edited
Loading

Update: I changed my mind on this, see below.

This is a reasonable attempt at fixing the issue. But I wonder if it's just a band-aid over the real problem?

Every MIR pass module has a single pass within, except for simplify which has two: SimplifyCfg and SimplifyLocals. Why is that? Could we just split that module into simplify_cfg::SimplifyCfg and simplify_locals::SimplifyLocals? That would remove one layers of $( )? in the macro and I think solve #150910 for free?

The only downside is that this would break external tools relying on the current names. But this kind of API is not guaranteed to be stable, and the breakage is trivial to fix.

@saethlin, what do you think?

nnethercote
nnethercote reviewed Jan 26, 2026
View reviewed changes
compiler/rustc_mir_transform/src/lib.rs Outdated
}

macro_rules! pass_names {
($mod_name:ident : $pass_name:ident { $($ident:ident),* $(,)? }) => {
Copy link
Contributor

@nnethercote nnethercote Jan 26, 2026

Choose a reason for hiding this comment

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

It would be useful to have a brief comment on each rule explaining the case being handled. I.e. the first rule deals with pass groups, the second with lone pass names.

Copy link
Contributor

@nnethercote nnethercote Jan 26, 2026

Choose a reason for hiding this comment

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

And in this first rule, $pass_name would be better called $pass_group, and $ident would be better called $pass_name.

Copy link
Contributor Author

@sgasho sgasho Jan 28, 2026

Choose a reason for hiding this comment

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

I fixed them in the commit below

fac135a

nnethercote
nnethercote approved these changes Jan 26, 2026
View reviewed changes
Copy link
Contributor

@nnethercote nnethercote left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

After looking some more, I think this change is ok. I don't like how the existing macro is structured, where sometimes the thing after the colon is a pass name and sometimes it's a pass group, but fixing that seems beyond the scope of this PR.

View changes since this review

@nnethercote
Copy link
Contributor

nnethercote commented Jan 26, 2026 •
edited
Loading

r=me with the aforementioned renamings but I'll give @saethlin a chance to respond in case I'm overlooking anything.

@nnethercote
Copy link
Contributor

nnethercote commented Jan 26, 2026

cc @clubby789, who wrote this code.

clubby789
clubby789 approved these changes Jan 27, 2026
View reviewed changes
Copy link
Contributor

@clubby789 clubby789 left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This LGTM as a fix, I might look at re-doing the macro in future to be more logical

View changes since this review

@nnethercote
Copy link
Contributor

nnethercote commented Jan 28, 2026

@sgasho: looks good. Can you squash the commits (there's no value in these particular commits being separate) and then we will be good to go.

@sgasho sgasho force-pushed the 150910_SimplifyCfg_passes_warn branch from fac135a to 99591e6 Compare January 29, 2026 00:02
saethlin
saethlin reviewed Jan 29, 2026
View reviewed changes
tests/ui/lint/invalid_value-polymorphic.rs
@@ -1,4 +1,4 @@
//@ compile-flags: --crate-type=lib -Zmir-enable-passes=+InstSimplify
//@ compile-flags: --crate-type=lib -Zmir-enable-passes=+InstSimplify-before-inline
Copy link
Member

@saethlin saethlin Jan 29, 2026

Choose a reason for hiding this comment

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

The fact that you had to change this is making a strong case for the value of this change.

@saethlin
Copy link
Member

saethlin commented Jan 29, 2026

Every MIR pass module has a single pass within, except for simplify which has two: SimplifyCfg and SimplifyLocals. Why is that? Could we just split that module into simplify_cfg::SimplifyCfg and simplify_locals::SimplifyLocals?

I agree that breaking up the simplify module seems like a good idea. I've tripped over the fact that it doesn't have the same symmetry between pass name and module name many times.

I don't have a strong opinion on the implementation, macros always look like token soup to me, but this one is of reasonable size. I mostly just care that this fixes the footgun.

@saethlin
Copy link
Member

saethlin commented Jan 29, 2026

@bors r=nnethercote,saethlin

@rust-bors
Copy link
Contributor

rust-bors bot commented Jan 29, 2026

📌 Commit 99591e6 has been approved by nnethercote,saethlin

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 Jan 29, 2026
Zalathar added a commit to Zalathar/rust that referenced this pull request Jan 29, 2026
@Zalathar
Rollup merge of rust-lang#151596 - sgasho:150910_SimplifyCfg_passes_w…
f76a349 
…arn, r=nnethercote,saethlin

Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro

related: rust-lang#150910

I fixed declare_passes macro to detect unregistered enum names

### UI Results
+nightly --> before: no warnings
+stage1 --> after: detect SimplifyCfg as an unknown pass

<img width="591" height="199" alt="スクリーンショット 2026-01-24 23 53 41" src="https://github.com/user-attachments/assets/ddabaa58-b4c6-4e80-a3c9-f40d866db273" />
@Zalathar Zalathar mentioned this pull request Jan 29, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saethlin saethlin saethlin left review comments

@nnethercote nnethercote nnethercote approved these changes

@clubby789 clubby789 clubby789 approved these changes

Assignees

@nnethercote nnethercote

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sgasho @rustbot @rust-log-analyzer @nnethercote @saethlin @clubby789