Fix incorrect cfg structured suggestion and make suggestion verbose
#137773
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Keep a span for the attribute *within* the square brackets as part of the `AttrKind`. ``` error: `cfg` is not followed by parentheses --> $DIR/cfg-attr-syntax-validation.rs:4:1 | LL | #[cfg = 10] | ^^^^^^^^^^^ | help: expected syntax is | LL - #[cfg = 10] LL + #[cfg(/* predicate */)] | ``` Noticed in rust-lang#137343 (comment).
compiler-errors
changed the title
cfg structured suggestioncfg structured suggestion and make suggestion verbose
| help: expected syntax is | ||
| | | ||
| LL - #[cfg = 10] | ||
| LL + #[cfg(/* predicate */)] |
There was a problem hiding this comment.
Maybe keep the value? Seems like often ppl may have written #[cfg = "linux"]
There was a problem hiding this comment.
We should do that only if we are certain that the value is a valid flag.
There was a problem hiding this comment.
The user provided input, why would we just overwrite it with a comment?
There was a problem hiding this comment.
The predicates can only be either bare identifiers or ident = lit.
If they write #[cfg = ident] they get
error: attribute value must be a literal
--> $DIR/cfg-attr-syntax-validation.rs:28:9
|
LL | #[cfg = a]
| ^
which is a separate error.
If they write #[cfg = "linux"], then we have to inspect the literal and reverse map it to target_os so that we suggest #[cfg(target_os = "linux)]. If we don't do that, and someone writes #[cfg = "foo"] or #[cfg = true], what should we suggest other than the generic predicate? In those cases we should link to https://doc.rust-lang.org/reference/conditional-compilation.html#set-configuration-options, which is the most comprehensive explanation of what is valid there (even though I don't like linking to the reference if there are more newbie friendly documents, which we don't seem to have for cfg). If we keep that reverse mapping, does that mean we also have to dynamically create the reverse mapping for all features in the current crate as well?
Note that I am not against doing any of that, it's just that this PR was to fix the incorrect suggestion, which needs to happen regardless of additional logic to make the suggestion more comprehensive.
| @@ -3047,6 +3048,7 @@ pub struct AttrItem { | |||
| pub args: AttrArgs, | |||
| // Tokens for the meta item, e.g. just the `foo` within `#[foo]` or `#![foo]`. | |||
| pub tokens: Option<LazyAttrTokenStream>, | |||
| pub span: Span, | |||
There was a problem hiding this comment.
Pls add docs with examples what it refers to
| @@ -645,10 +652,11 @@ pub fn mk_attr_word( | |||
| unsafety: Safety, | |||
| name: Symbol, | |||
| span: Span, | |||
| inner_span: Span, | |||
There was a problem hiding this comment.
I think no caller needs this to be different as there are no args
rustbot
added
S-waiting-on-author
Keep a span for the attribute within the square brackets as part of the
AttrKind.Noticed in #137343 (comment).