refactor rustc_on_unimplemented's filtering by mejrs · Pull Request #155940 · rust-lang/rust

mejrs · 2026-04-28T18:22:59Z

Previously when you had a

pub struct Directive {
    pub is_rustc_attr: bool,
    pub condition: Option<OnUnimplementedCondition>,
    pub subcommands: ThinVec<Directive>,
    pub message: Option<(Span, FormatString)>,
    ...
}

that condition would control the emission of the message, label, notes etc. I've changed that to

pub struct Directive {
    pub is_rustc_attr: bool,
    pub filters: ThinVec<(Filter, Directive)>,
    pub message: Option<(Span, FormatString)>,
    ...

so that the message etc is always emitted, and there's a vec of tuples with (filter, directive) where the filter controls whether that directive is even emitted, which i think is much clearer. That also makes it easier to not have to do the reverse iteration thing and this makes it so that notes are emitted in declaration order (with nonfiltered options always last).

The rename is because I plan on making it available to other diagnostic attributes at some point (very wip) so OnUnimplementedCondition and the like would have to be renamed anyway.

rustbot · 2026-04-28T18:23:03Z

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

rustbot · 2026-04-28T18:23:06Z

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
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 73 candidates
Random selection from 20 candidates

jdonszelmann · 2026-04-28T22:02:06Z

r? jdonszelmann (I don't mind taking this one for tomorrow :)

jdonszelmann · 2026-04-29T14:01:24Z

-    pub subcommands: ThinVec<Directive>,
+    /// This is never nested more than once, i.e. the directives in this
+    /// thinvec have no filters of their own.
+    pub filters: ThinVec<(Filter, Directive)>,


are toplevel and nested directives any different? like are some fields like mesage always None at the top level? Just wondering if it would maybe make sense to have a TopLevelDirective and a Directive for example

View changes since the review

You can write a rustc_on_unimplemented attribute that populates all those fields, at both top level and secondary level. Just the filters in that secondary level are always empty.

Regular diagnostic attributes are just a much more restricted form of that, where only message, label and note can be populated.

if it would maybe make sense to have a TopLevelDirective and a Directive for example

It's possible; this structure would essentially be:

pub struct DiagnosticAttrDirective { pub message: Option<(Span, FormatString)>, pub label: Option<(Span, FormatString)>, pub notes: ThinVec<FormatString>, } pub struct TopRustcOnUnImplDirective { pub filters: ThinVec<(Filter, RustcOnUnImplDirective)>, pub message: Option<(Span, FormatString)>, pub label: Option<(Span, FormatString)>, pub notes: ThinVec<FormatString>, pub parent_label: Option<FormatString>, } pub struct RustcOnUnImplDirective { pub message: Option<(Span, FormatString)>, pub label: Option<(Span, FormatString)>, pub notes: ThinVec<FormatString>, pub parent_label: Option<FormatString>, }

presumably we can do clever things with generics so we can reuse things and you end up with fields such as filters: ThinVec<!> etc to enforce this at the type level.

The downside of all of this is that everything becomes more complex, boilerplate-y and harder to read. The upside is that it's harder to misuse. But making things harder to misuse is only really important for the unwashed masses. This is important when you write a http framework because arbitrary people are are programming against your code. It's not as important in this case because diagnostic attr parsing infra is only used by people programming in the same crate. These are the same people that might as well just mess up the types themselves - so what is the point?

jdonszelmann · 2026-04-29T14:06:02Z

@rustbot author

rustbot · 2026-04-29T14:06:07Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

mejrs · 2026-04-29T21:54:54Z

@rustbot ready

jdonszelmann · 2026-04-30T21:26:04Z

Alright, I agree with that reasoning. @bors r+ rollup

rust-bors · 2026-04-30T21:26:07Z

📋 This PR cannot be approved because it has merge conflicts. Please resolve the conflicts and try again.

jdonszelmann · 2026-05-01T14:58:39Z

@bors r+ rollup

rust-bors · 2026-05-01T14:58:43Z

📌 Commit f9e052d has been approved by jdonszelmann

It is now in the queue for this repository.

refactor rustc_on_unimplemented's filtering Previously when you had a ```rust pub struct Directive { pub is_rustc_attr: bool, pub condition: Option<OnUnimplementedCondition>, pub subcommands: ThinVec<Directive>, pub message: Option<(Span, FormatString)>, ... } ``` that condition would control the emission of the message, label, notes etc. I've changed that to ```rust pub struct Directive { pub is_rustc_attr: bool, pub filters: ThinVec<(Filter, Directive)>, pub message: Option<(Span, FormatString)>, ... ``` so that the message etc is always emitted, and there's a vec of tuples with (filter, directive) where the filter controls whether that directive is even emitted, which i think is much clearer. That also makes it easier to not have to do the reverse iteration thing and this makes it so that notes are emitted in declaration order (with nonfiltered options always last). The rename is because I plan on making it available to other diagnostic attributes at some point (very wip) so `OnUnimplementedCondition` and the like would have to be renamed anyway.

Rollup of 7 pull requests Successful merges: - #152487 (core: drop unmapped ZSTs in array `map`) - #155940 (refactor rustc_on_unimplemented's filtering) - #156020 (Improve source code for `librustdoc/visit_ast.rs`) - #156021 (Clean up some traits) - #156028 (Add a `Local::arg(i)` helper constructor) - #156037 (Add AcceptContext::expect_no_args) - #156040 (Add missing alias to mailmap)

refactor rustc_on_unimplemented's filtering Previously when you had a ```rust pub struct Directive { pub is_rustc_attr: bool, pub condition: Option<OnUnimplementedCondition>, pub subcommands: ThinVec<Directive>, pub message: Option<(Span, FormatString)>, ... } ``` that condition would control the emission of the message, label, notes etc. I've changed that to ```rust pub struct Directive { pub is_rustc_attr: bool, pub filters: ThinVec<(Filter, Directive)>, pub message: Option<(Span, FormatString)>, ... ``` so that the message etc is always emitted, and there's a vec of tuples with (filter, directive) where the filter controls whether that directive is even emitted, which i think is much clearer. That also makes it easier to not have to do the reverse iteration thing and this makes it so that notes are emitted in declaration order (with nonfiltered options always last). The rename is because I plan on making it available to other diagnostic attributes at some point (very wip) so `OnUnimplementedCondition` and the like would have to be renamed anyway.

Rollup of 7 pull requests Successful merges: - #155940 (refactor rustc_on_unimplemented's filtering) - #156020 (Improve source code for `librustdoc/visit_ast.rs`) - #156021 (Clean up some traits) - #156028 (Add a `Local::arg(i)` helper constructor) - #156037 (Add AcceptContext::expect_no_args) - #156040 (Add missing alias to mailmap) - #156048 (Make `diverging_type_vars` a vec of `TyVid`)

…uwer Rollup of 9 pull requests Successful merges: - #156030 (Make stable hashing names consistent (part 1)) - #156020 (Improve source code for `librustdoc/visit_ast.rs`) - #156021 (Clean up some traits) - #156028 (Add a `Local::arg(i)` helper constructor) - #156037 (Add AcceptContext::expect_no_args) - #156040 (Add missing alias to mailmap) - #156048 (Make `diverging_type_vars` a vec of `TyVid`) - #156053 (Reuse CTFE MIR for constructors.) - #156059 (compiler: Print valid `-Zmir-enable-passes` names if invalid name is used) Failed merges: - #155940 (refactor rustc_on_unimplemented's filtering) - #156065 (Remove unused spans from AttributeKind)

rustbot · 2026-05-02T16:00:20Z

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.

mejrs · 2026-05-02T20:16:27Z

@bors r=jdonszelmann

rust-bors · 2026-05-02T20:16:30Z

📌 Commit 0610172 has been approved by jdonszelmann

It is now in the queue for this repository.

refactor rustc_on_unimplemented's filtering Previously when you had a ```rust pub struct Directive { pub is_rustc_attr: bool, pub condition: Option<OnUnimplementedCondition>, pub subcommands: ThinVec<Directive>, pub message: Option<(Span, FormatString)>, ... } ``` that condition would control the emission of the message, label, notes etc. I've changed that to ```rust pub struct Directive { pub is_rustc_attr: bool, pub filters: ThinVec<(Filter, Directive)>, pub message: Option<(Span, FormatString)>, ... ``` so that the message etc is always emitted, and there's a vec of tuples with (filter, directive) where the filter controls whether that directive is even emitted, which i think is much clearer. That also makes it easier to not have to do the reverse iteration thing and this makes it so that notes are emitted in declaration order (with nonfiltered options always last). The rename is because I plan on making it available to other diagnostic attributes at some point (very wip) so `OnUnimplementedCondition` and the like would have to be renamed anyway.

…uwer Rollup of 5 pull requests Successful merges: - #152277 (Validate source snippet when format input is raw string) - #155940 (refactor rustc_on_unimplemented's filtering) - #156065 (Remove unused spans from AttributeKind) - #156079 (Move and rename the `clone-never.rs` test) - #156091 (change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools`)

Rollup of 5 pull requests Successful merges: - #155666 (Interning cleanups) - #155940 (refactor rustc_on_unimplemented's filtering) - #156065 (Remove unused spans from AttributeKind) - #156079 (Move and rename the `clone-never.rs` test) - #156091 (change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools`)

Rollup merge of #155940 - mejrs:filter, r=jdonszelmann refactor rustc_on_unimplemented's filtering Previously when you had a ```rust pub struct Directive { pub is_rustc_attr: bool, pub condition: Option<OnUnimplementedCondition>, pub subcommands: ThinVec<Directive>, pub message: Option<(Span, FormatString)>, ... } ``` that condition would control the emission of the message, label, notes etc. I've changed that to ```rust pub struct Directive { pub is_rustc_attr: bool, pub filters: ThinVec<(Filter, Directive)>, pub message: Option<(Span, FormatString)>, ... ``` so that the message etc is always emitted, and there's a vec of tuples with (filter, directive) where the filter controls whether that directive is even emitted, which i think is much clearer. That also makes it easier to not have to do the reverse iteration thing and this makes it so that notes are emitted in declaration order (with nonfiltered options always last). The rename is because I plan on making it available to other diagnostic attributes at some point (very wip) so `OnUnimplementedCondition` and the like would have to be renamed anyway.

…uwer Rollup of 9 pull requests Successful merges: - rust-lang/rust#156030 (Make stable hashing names consistent (part 1)) - rust-lang/rust#156020 (Improve source code for `librustdoc/visit_ast.rs`) - rust-lang/rust#156021 (Clean up some traits) - rust-lang/rust#156028 (Add a `Local::arg(i)` helper constructor) - rust-lang/rust#156037 (Add AcceptContext::expect_no_args) - rust-lang/rust#156040 (Add missing alias to mailmap) - rust-lang/rust#156048 (Make `diverging_type_vars` a vec of `TyVid`) - rust-lang/rust#156053 (Reuse CTFE MIR for constructors.) - rust-lang/rust#156059 (compiler: Print valid `-Zmir-enable-passes` names if invalid name is used) Failed merges: - rust-lang/rust#155940 (refactor rustc_on_unimplemented's filtering) - rust-lang/rust#156065 (Remove unused spans from AttributeKind)

rustbot assigned dingxiangfei2009 Apr 28, 2026

This comment has been minimized.

Sign in to view

mejrs force-pushed the filter branch from 36c72d7 to 47604e5 Compare April 28, 2026 20:48

rustbot assigned jdonszelmann and unassigned dingxiangfei2009 Apr 28, 2026

jdonszelmann reviewed Apr 29, 2026

View reviewed changes

rustbot 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 Apr 29, 2026

rustbot 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 Apr 29, 2026

This comment has been minimized.

Sign in to view

mejrs force-pushed the filter branch from 47604e5 to f9e052d Compare April 30, 2026 22:39

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 May 1, 2026

GuillaumeGomez mentioned this pull request May 1, 2026

Rollup of 7 pull requests #156056

Closed

GuillaumeGomez mentioned this pull request May 1, 2026

Rollup of 7 pull requests #156066

Closed

This was referenced May 2, 2026

Rollup of 7 pull requests #156076

Closed

Rollup of 9 pull requests #156078

Merged

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 May 2, 2026

This comment has been minimized.

Sign in to view

mejrs added 2 commits May 2, 2026 17:59

rustc_on_unimplemented: emit notes in declared order

d6969da

rename "condition" to "filter"

0610172

mejrs force-pushed the filter branch from f9e052d to 0610172 Compare May 2, 2026 16:00

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2026

JonathanBrouwer mentioned this pull request May 2, 2026

Rollup of 5 pull requests #156095

Closed

jhpratt mentioned this pull request May 3, 2026

Rollup of 5 pull requests #156097

Merged

rust-bors Bot merged commit b7c6551 into rust-lang:main May 3, 2026
11 checks passed

rustbot added this to the 1.97.0 milestone May 3, 2026

Uh oh!

Conversation

mejrs commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 28, 2026

Uh oh!

rustbot commented Apr 28, 2026

Uh oh!

This comment has been minimized.

jdonszelmann commented Apr 28, 2026

Uh oh!

jdonszelmann Apr 29, 2026 • 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!

mejrs Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

jdonszelmann commented Apr 29, 2026

Uh oh!

rustbot commented Apr 29, 2026

Uh oh!

mejrs commented Apr 29, 2026

Uh oh!

This comment has been minimized.

jdonszelmann commented Apr 30, 2026

Uh oh!

rust-bors Bot commented Apr 30, 2026

Uh oh!

This comment has been minimized.

jdonszelmann commented May 1, 2026

Uh oh!

rust-bors Bot commented May 1, 2026

Uh oh!

This comment has been minimized.

rustbot commented May 2, 2026

Uh oh!

mejrs commented May 2, 2026

Uh oh!

rust-bors Bot commented May 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mejrs commented Apr 28, 2026 •

edited

Loading

jdonszelmann Apr 29, 2026 •

edited by rustbot

Loading