Skip to content

Refactor rustc_on_unimplemented's filter parser #140307

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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Apr 25, 2025

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:
image

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2025

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 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
@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2025

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 mejrs force-pushed the condition_parser branch 2 times, most recently from 574e113 to b2db974 Compare April 27, 2025 13:32
@mejrs
Copy link
Contributor Author

mejrs commented Apr 30, 2025

r? compiler

@rustbot rustbot assigned nnethercote and unassigned estebank Apr 30, 2025
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

Generally looks ok, a bunch of nitpicks below.

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

Choose a reason for hiding this comment

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

s/UnSupported/Unsupported/

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@mejrs mejrs force-pushed the condition_parser branch from b2db974 to 8a88e71 Compare May 1, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants