-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Attribute cleanups #127308
Attribute cleanups #127308
Conversation
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment was marked as resolved.
This comment was marked as resolved.
These should have been removed in rust-lang#127233 when the positions were changed from `usize` to `u32`.
406244b
to
11db400
Compare
compiler/rustc_ast/src/ast.rs
Outdated
@@ -2848,11 +2848,11 @@ impl NormalAttr { | |||
} | |||
|
|||
#[derive(Clone, Encodable, Decodable, Debug, HashStable_Generic)] | |||
pub struct AttrItem { | |||
pub struct Meta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this renaming, "attribute items" and "meta items" are different things and shouldn't be mixed.
Meta items (struct MetaItem
) are a restricted subset of attribute items, used for working with built-in attributes in the compiler (most of which fit into that subset) as an implementation detail.
The meta
matcher in macros matches attr items, not meta items, but we cannot just rename it because it's user-facing and stable.
NtMeta
could be renamed to NtAttrItem
to better match its essence rather than surface syntax, but why bother, it's removed in #124141 anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially do some larger naming reform to change AttrItem
to Meta
, and MetaItem
to something like RestrictedMeta
, but not as a part of a cleanup PR.
I'm actually not sure where the name "meta" came from (it is ancient) and whether it would make sense if we started naming things now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The basic point is that the current naming of types with the compiler is bad. Off the top of your head, do you know the distinction between NormalAttr::tokens
and AttrItem::tokens
? I sure don't, I have to look at the comments every time.
Looking at the reference:
InnerAttribute :
# ! [ Attr ]OuterAttribute :
# [ Attr ]Attr :
SimplePath AttrInput?AttrInput :
DelimTokenTree
| = Expression
So:
- "Attribute" is the whole thing, including the leading
#
and (if present)!
. That's fine. - "Attr" is the part within the brackets. That's a bad name, much too similar to "Attribute", and when "Attribute" is inevitably abbreviated to "Attr" they'll be indistinguishable. (I called this "Meta" in this PR which matches the
meta
fragment specifier, but is awkwardly close to the similar-but-slightly different "meta item".) - "Attribute item" doesn't appear at all, either in this syntax or anywhere else on the page.
- "Meta item" is the restricted syntax used within the brackets for most builtin attributes. It's not a great name, but it's sufficiently different that it'll do.
The main problem is the lack of a good name for what the reference calls "Attr", i.e. the part within the brackets, not necessarily restricted to meta item syntax. That's the problem I'm trying to solve here. Something like "AttrContents" might work, and NormalAttr::item
could become NormalAttr::contents
. I want to avoid "item" as much as possible, it's too easily confused with normal items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like "AttrContents" might work
The meta section of TLBORM says "The meta
fragment matches the contents of an attribute".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two serious contenders here.
- "Attribute"/"Meta"/"Meta item": what this PR does. Pros: "Meta" matches the
meta
fragment specifier. Cons: "Meta" and "MetaItem" are subtly different, which is potentially confusing. - "Attribute"/"AttrContents"/"Meta item". Pros: clearer terminology. Cons: doesn't match
meta
fragment specifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the current naming is bad, the current scheme is as "strong contender" as those in the comment above, and "item" is a generic word for an element if a sequence that is clear from the context (like we don't mix "meta" in attributes and "meta" in metadata, for example).
I had #[cfg_attr(pred, item1, item2, ..., item3)]
and the #[item1, item2, ..., item3]
proposal when coming up with the name. "Contents" doesn't fit here, but "item" does, and "meta" does too just because it's meaningless by itself.
I'd rather avoid churn here than start bikeshedding.
Off the top of your head, do you know the distinction between
NormalAttr::tokens
andAttrItem::tokens
?
This seems unrelated to the structure naming, but the answer is that I don't know and I'm not supposed to!
They return tokens in some form, lazy or not, the details I can lookup once in a year when I need them.
I do remember that the one from the trait returns lazy tokens exactly as they are kept in AST structures, so a mutable ref can be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think the naming is bad. It confuses me all the time. I didn't propose the name change just for fun, but because I found it made the code easier to understand. "Attribute item" is a name that doesn't come up in the reference, isn't sufficiently different to "Attribute", and overloads "item".
I had #[cfg_attr(pred, item1, item2, ..., item3)] and the rust-lang/rfcs#2600 proposal when coming up with the name. "Contents" doesn't fit here, but "item" does, and "meta" does too just because it's meaningless by itself.
The AttrItem
type currently describes everything within the brackets. That doesn't line up with item1
, item2
, etc. A meta
specifier matches everything in the brackets, but you rejected my renaming of AttrItem
as Meta
.
In #[a, b, c]
, what word/phrase would you use to describe the a, b, c
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you rejected my renaming of
AttrItem
asMeta
I think it's fine as long as MetaItem
is renamed to something else (#127308 (comment)), it's very generic but at least it will match the user-visible naming.
In
#[a, b, c]
, what word/phrase would you use to describe thea, b, c
part?
Attribute items, Vec<AttrItem>
, as in the current naming.
name that doesn't come up in the reference
That's probably the least convincing argument to me, the compiler code, RFCs and lang team are the source for terminology, and the reference follows, with a delay. If I weren't too lazy I could update the reference when doing the implementation, and it would say "attribute items" now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see I won't convince you. I will note that "meta item" occurs in lots of places in the compiler, including several error messages. "Attribute item" doesn't appear anywhere other than the AttrItem
type.
To distinguish it from the `HasTokens` method.
The only place it is meaningfully used is in a panic message in `TokenStream::from_ast`. But `node.span()` doesn't need to be printed because `node` is also printed and it must contain the span.
Currently the second element is a `Vec<(FlatToken, Spacing)>`. But the vector always has zero or one elements, and the `FlatToken` is always `FlatToken::AttrTarget` (which contains an `AttributesData`), and the spacing is always `Alone`. So we can simplify it to `Option<AttributesData>`. An assertion in `to_attr_token_stream` can can also be removed, because `new_tokens.len()` was always 0 or 1, which means than `range.len()` is always greater than or equal to it, because `range.is_empty()` is always false (as per the earlier assertion).
- `AttributesData` -> `AttrsTarget` - `AttrTokenTree::Attributes` -> `AttrTokenTree::AttrsTarget` - `FlatToken::AttrTarget` -> `FlatToken::AttrsTarget`
`Option<LazyAttrTokenStream>` is the type that's actually used in all the aST nodes.
All the branches produce either zero or one elements.
11db400
to
9e0aab7
Compare
I have updated:
|
@bors r+ |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127179 (Print `TypeId` as hex for debugging) - rust-lang#127189 (LinkedList's Cursor: method to get a ref to the cursor's list) - rust-lang#127236 (doc: update config file path in platform-support/wasm32-wasip1-threads.md) - rust-lang#127297 (Improve std::Path's Hash quality by avoiding prefix collisions) - rust-lang#127308 (Attribute cleanups) - rust-lang#127354 (Describe Sized requirements for mem::offset_of) - rust-lang#127409 (Emit a wrap expr span_bug only if context is not tainted) - rust-lang#127447 (once_lock: make test not take as long in Miri) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#127308 - nnethercote:Attribute-cleanups, r=petrochenkov Attribute cleanups More refactoring done while trying to fix the final remaining test failure for rust-lang#124141. r? `@petrochenkov`
… r=petrochenkov More attribute cleanups A follow-up to rust-lang#127308. r? `@petrochenkov`
…s, r=petrochenkov More attribute cleanups A follow-up to rust-lang#127308. r? `@petrochenkov`
…s, r=petrochenkov More attribute cleanups A follow-up to rust-lang#127308. r? ``@petrochenkov``
…s, r=petrochenkov More attribute cleanups A follow-up to rust-lang#127308. r? ```@petrochenkov```
Rollup merge of rust-lang#127558 - nnethercote:more-Attribute-cleanups, r=petrochenkov More attribute cleanups A follow-up to rust-lang#127308. r? ```@petrochenkov```
More refactoring done while trying to fix the final remaining test failure for #124141.
r? @petrochenkov