Refactor rustc_on_unimplemented's filter parser #140307
Conversation
rustbot
added
S-waiting-on-review
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
mejrs
force-pushed
the
condition_parser
branch
2 times, most recently
from
574e113 to
b2db974
Compare
|
r? compiler |
compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/traits/on_unimplemented_condition.rs
Outdated
Show resolved
Hide resolved
| span: Span, | ||
| }, | ||
| #[diag(trait_selection_rustc_on_unimplemented_on_clause_literal, code = E0232)] | ||
| UnSupportedLiteral { |
There was a problem hiding this comment.
s/UnSupported/Unsupported/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe consider having trait_selection_rou_* rather than trait_selection_rustc_on_unimplemented_*? I errored on the side of not abbreviating.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Show resolved
Hide resolved
|
@bors r+ |
bors
added
S-waiting-on-bors
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: 
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
Followup to #139091; I plan on moving most of this code into
rustc_attr_parsingat some point, but want to land this separately first.I have taken care to preserve the original behavior as much as I could:
InvalidFlagvariant is new, this PR turns this (from being ignored and silently doing nothing) into an error:Finally, this gets rid of the "longest" dependency in rustc:
