Refactor rustc_on_unimplemented's filter parser by mejrs · Pull Request #140307 · rust-lang/rust

mejrs · 2025-04-25T18:08:31Z

Followup to #139091; I plan on moving most of this code into rustc_attr_parsing at some point, but want to land this separately first.

I have taken care to preserve the original behavior as much as I could:

All but one of the new error variants are replacements for the ones originally emitted by the cfg parsing machinery; so these errors are not "new".
the InvalidFlag variant is new, this PR turns this (from being ignored and silently doing nothing) into an error:
```
#[rustc_on_unimplemented(on(something, message = "y"))]
//~^ ERROR invalid boolean flag
//~^^ NOTE expected one of `crate_local`, `direct` or `from_desugaring`, not `something`
trait InvalidFlag {}
```
This does not occur anywhere except in this test. I couldn't find a way that I liked to keep allowing this or to do nothing, erroring was the cleanest solution.
There are a bunch of FIXME throughout this and the previous PR, I plan on addressing those in follow up prs..

Finally, this gets rid of the "longest" dependency in rustc:

rustbot · 2025-04-25T18:08:37Z

r? @estebank

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

rustbot · 2025-04-25T18:08:39Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

mejrs · 2025-04-30T23:01:12Z

r? compiler

nnethercote

Generally looks ok, a bunch of nitpicks below.

compiler/rustc_trait_selection/messages.ftl

compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs

nnethercote · 2025-05-01T03:24:55Z

compiler/rustc_trait_selection/src/errors.rs

+        span: Span,
+    },
+    #[diag(trait_selection_rustc_on_unimplemented_on_clause_literal, code = E0232)]
+    UnSupportedLiteral {


s/UnSupported/Unsupported/

Also, the variant name doesn't really match the message name: "unsupported" vs "unimplemented". It would be better if they were close to each other. Likewise for a couple of the other variants.

I'm not sure what you mean. The unimplemented in the flt name references the name of the attribute, not what went wrong. But I have edited it to ...unsupported_literal_in_on

Maybe consider having trait_selection_rou_* rather than trait_selection_rustc_on_unimplemented_*? I errored on the side of not abbreviating.

Generally the name of the error type and the name of the error message are similar, as is the text of the error. Here are some case unlike that. For example:

trait_selection_rustc_on_unimplemented_invalid_boolean "invalid boolean flag" InvalidFlag

One uses "boolean", one uses "flag", one uses "boolean flag". More consistency would help.

I hadn't really thought about that, thank you. I've made some changes to the naming.

compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs

nnethercote · 2025-05-05T04:09:38Z

@bors r+

bors · 2025-05-05T04:09:41Z

📌 Commit 4671081 has been approved by nnethercote

It is now in the queue for this repository.

Rollup of 4 pull requests Successful merges: - rust-lang#135734 (Correct `extract_if` sample equivalent.) - rust-lang#140307 (Refactor rustc_on_unimplemented's filter parser) - rust-lang#140644 (Revert "Avoid unused clones in Cloned<I> and Copied<I>") - rust-lang#140648 (Update `compiler-builtins` to 0.1.157) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#140307 - mejrs:condition_parser, r=nnethercote Refactor rustc_on_unimplemented's filter parser Followup to rust-lang#139091; I plan on moving most of this code into `rustc_attr_parsing` at some point, but want to land this separately first. I have taken care to preserve the original behavior as much as I could: - All but one of the new error variants are replacements for the ones originally emitted by the cfg parsing machinery; so these errors are not "new". - the `InvalidFlag` variant is new, this PR turns this (from being ignored and silently doing nothing) into an error: ```rust #[rustc_on_unimplemented(on(something, message = "y"))] //~^ ERROR invalid boolean flag //~^^ NOTE expected one of `crate_local`, `direct` or `from_desugaring`, not `something` trait InvalidFlag {} ``` This does not occur anywhere except in this test. I couldn't find a way that I liked to keep allowing this or to do nothing, erroring was the cleanest solution. - There are a bunch of FIXME throughout this and the previous PR, I plan on addressing those in follow up prs.. Finally, this gets rid of the "longest" dependency in rustc: ![image](https://github.com/user-attachments/assets/3c3eb3a0-b7b3-40d9-aada-a752e28c8678)

Rollup of 4 pull requests Successful merges: - rust-lang#135734 (Correct `extract_if` sample equivalent.) - rust-lang#140307 (Refactor rustc_on_unimplemented's filter parser) - rust-lang#140644 (Revert "Avoid unused clones in Cloned<I> and Copied<I>") - rust-lang#140648 (Update `compiler-builtins` to 0.1.157) r? `@ghost` `@rustbot` modify labels: rollup

rustc_on_unimplemented cleanups Addresses some of the fixmes from rust-lang#139091 and rust-lang#140307. - switch from `_Self` to `Self` in library - properly validate that arguments in the `on` filter and the format strings are actually valid See rust-lang/rustc-dev-guide#2357 for the relevant documentation.

Rollup merge of rust-lang#141130 - mejrs:use_self, r=compiler-errors rustc_on_unimplemented cleanups Addresses some of the fixmes from rust-lang#139091 and rust-lang#140307. - switch from `_Self` to `Self` in library - properly validate that arguments in the `on` filter and the format strings are actually valid See rust-lang/rustc-dev-guide#2357 for the relevant documentation.

rustc_on_unimplemented cleanups Addresses some of the fixmes from rust-lang#139091 and rust-lang#140307. - switch from `_Self` to `Self` in library - properly validate that arguments in the `on` filter and the format strings are actually valid See rust-lang/rustc-dev-guide#2357 for the relevant documentation.

rustc_on_unimplemented cleanups Addresses some of the fixmes from rust-lang/rust#139091 and rust-lang/rust#140307. - switch from `_Self` to `Self` in library - properly validate that arguments in the `on` filter and the format strings are actually valid See rust-lang/rustc-dev-guide#2357 for the relevant documentation.

Improve rustc_on_unimplemented ui test

d2d844c

rustbot assigned estebank Apr 25, 2025

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 Apr 25, 2025

mejrs force-pushed the condition_parser branch 2 times, most recently from 574e113 to b2db974 Compare April 27, 2025 13:32

rustbot assigned nnethercote and unassigned estebank Apr 30, 2025

nnethercote reviewed May 1, 2025

View reviewed changes

mejrs force-pushed the condition_parser branch from b2db974 to 8a88e71 Compare May 1, 2025 11:27

Refactor rustc_on_unimplemented's filter parser

4671081

mejrs force-pushed the condition_parser branch from 8a88e71 to 4671081 Compare May 2, 2025 20:08

bors 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 5, 2025

tgross35 mentioned this pull request May 5, 2025

Rollup of 4 pull requests #140650

Merged

bors merged commit e9a50b8 into rust-lang:master May 5, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone May 5, 2025

mejrs mentioned this pull request May 17, 2025

rustc_on_unimplemented cleanups #141130

Merged

Uh oh!

Conversation

mejrs commented Apr 25, 2025

Uh oh!

rustbot commented Apr 25, 2025

Uh oh!

rustbot commented Apr 25, 2025

Uh oh!

mejrs commented Apr 30, 2025

Uh oh!

nnethercote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnethercote May 1, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote May 1, 2025

Choose a reason for hiding this comment

Uh oh!

mejrs May 1, 2025

Choose a reason for hiding this comment

Uh oh!

mejrs May 1, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote May 2, 2025

Choose a reason for hiding this comment

Uh oh!

mejrs May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nnethercote commented May 5, 2025

Uh oh!

bors commented May 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants