Attribute cleanups #104861

nnethercote · 2022-11-25T04:33:08Z

Best reviewed one commit at a time.

compiler/rustc_builtin_macros/src/derive.rs

`check_builtin_attribute` calls `parse_meta` to convert an `Attribute` to a `MetaItem`, which it then checks. However, many callers of `check_builtin_attribute` start with a `MetaItem`, and then convert it to an `Attribute` by calling `cx.attribute(meta_item)`. This `MetaItem` to `Attribute` to `MetaItem` conversion is silly. This commit adds a new function `check_builtin_meta_item`, which can be called instead from these call sites. `check_builtin_attribute` also now calls it. The commit also renames `check_meta` as `check_attr` to better match its arguments.

I have found the distinction confusing.

So that `Attribute` and `MetaItem` are listed first, and then the component types are below them in a logical order.

In `Expander::expand` the code currently uses `mk_attr_outer` to convert a `MetaItem` to an `Attribute`, and then follows that with `meta_item_list` which converts back. This commit avoids the unnecessary conversions. There was one wrinkle: the existing conversions caused the bogus `<>` on `Default<>` to be removed. With the conversion gone, we get a second error message about the `<>`. This is a rare case, so I think it probably doesn't matter much.

nnethercote · 2022-11-29T01:55:07Z

I rebased.

This comment dates back to at least 2013, and is no longer relevant. (There used to be an `allow` attribute, but that's no longer present.)

It has a single call site.

There is code for converting `Attribute` (syntactic) to `MetaItem` (semantic). There is also code for the reverse direction. The reverse direction isn't really necessary; it's currently only used when generating attributes, e.g. in `derive` code. This commit adds some new functions for creating `Attributes`s directly, without involving `MetaItem`s: `mk_attr_word`, `mk_attr_name_value_str`, `mk_attr_nested_word`, and `ExtCtxt::attr_{word,name_value_str,nested_word}`. These new methods replace the old functions for creating `Attribute`s: `mk_attr_inner`, `mk_attr_outer`, and `ExtCtxt::attribute`. Those functions took `MetaItem`s as input, and relied on many other functions that created `MetaItems`, which are also removed: `mk_name_value_item`, `mk_list_item`, `mk_word_item`, `mk_nested_word_item`, `{MetaItem,MetaItemKind,NestedMetaItem}::token_trees`, `MetaItemKind::attr_args`, `MetaItemLit::{from_lit_kind,to_token}`, `ExtCtxt::meta_word`. Overall this cuts more than 100 lines of code and makes thing simpler.

nnethercote · 2022-11-29T07:49:00Z

And now I added three new commits.

petrochenkov · 2022-11-29T19:37:26Z

The last commit is something I also wanted to do, there's no need to go through meta-items when generating AST output from built-in macros.
@bors r+

bors · 2022-11-29T19:37:28Z

📌 Commit ba1751a has been approved by petrochenkov

It is now in the queue for this repository.

nnethercote · 2022-11-29T22:11:42Z

I don't think this will affect perf, but the queue is long so there is time to check.

@bors r-

nnethercote · 2022-11-29T22:12:20Z

@bors try @rust-timer queue

bors · 2022-11-29T22:12:29Z

⌛ Trying commit ba1751a with merge a42da13bfac2206c0dd77e74950e6fb81f473449...

bors · 2022-11-30T00:36:13Z

☀️ Try build successful - checks-actions
Build commit: a42da13bfac2206c0dd77e74950e6fb81f473449 (a42da13bfac2206c0dd77e74950e6fb81f473449)

rust-timer · 2022-11-30T04:23:00Z

Finished benchmarking commit (a42da13bfac2206c0dd77e74950e6fb81f473449): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	2
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.5%]	7
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.1%, 2.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

petrochenkov · 2022-11-30T19:01:59Z

@bors r+ rollup=maybe

bors · 2022-11-30T19:02:00Z

📌 Commit ba1751a has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2022-12-01T07:13:49Z

⌛ Testing commit ba1751a with merge d6c4de0...

bors · 2022-12-01T10:02:50Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing d6c4de0 to master...

rust-timer · 2022-12-01T11:23:11Z

Finished benchmarking commit (d6c4de0): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.1%]	3
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.5%]	6
All ❌✅ (primary)	-0.2%	[-0.2%, -0.1%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-	-	0

rustbot assigned petrochenkov Nov 25, 2022

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 Nov 25, 2022

petrochenkov reviewed Nov 25, 2022

View reviewed changes

compiler/rustc_builtin_macros/src/derive.rs Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2022

nnethercote force-pushed the attr-cleanups branch from 6b478b1 to cf66c6a Compare November 28, 2022 03:11

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2022

This comment was marked as resolved.

Sign in to view

nnethercote added 3 commits November 29, 2022 12:08

Improve comments about attributes and meta items.

5a2fd1a

I have found the distinction confusing.

Reorder some types.

fc2a8a0

So that `Attribute` and `MetaItem` are listed first, and then the component types are below them in a logical order.

nnethercote force-pushed the attr-cleanups branch from cf66c6a to 958c4eb Compare November 29, 2022 01:33

nnethercote force-pushed the attr-cleanups branch from 958c4eb to a709f87 Compare November 29, 2022 01:54

nnethercote added 3 commits November 29, 2022 17:13

Remove an out-of-date comment.

ac7a749

This comment dates back to at least 2013, and is no longer relevant. (There used to be an `allow` attribute, but that's no longer present.)

Inline and remove MetaItemLit::from_lit_kind.

d1b61a3

It has a single call site.

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 Nov 29, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 29, 2022

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 29, 2022

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 30, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2022

bors merged commit d6c4de0 into rust-lang:master Dec 1, 2022

rustbot added this to the 1.67.0 milestone Dec 1, 2022

bors mentioned this pull request Dec 1, 2022

Use ThinVec more in the AST #104754

Merged

nnethercote deleted the attr-cleanups branch December 1, 2022 21:17

Attribute cleanups #104861

Attribute cleanups #104861

Uh oh!

Conversation

nnethercote commented Nov 25, 2022

Uh oh!

Uh oh!

This comment was marked as resolved.

nnethercote commented Nov 29, 2022

Uh oh!

nnethercote commented Nov 29, 2022

Uh oh!

petrochenkov commented Nov 29, 2022

Uh oh!

bors commented Nov 29, 2022

Uh oh!

nnethercote commented Nov 29, 2022

Uh oh!

nnethercote commented Nov 29, 2022

Uh oh!

This comment has been minimized.

bors commented Nov 29, 2022

Uh oh!

bors commented Nov 30, 2022

Uh oh!

This comment has been minimized.

rust-timer commented Nov 30, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

petrochenkov commented Nov 30, 2022

Uh oh!

bors commented Nov 30, 2022

Uh oh!

bors commented Dec 1, 2022

Uh oh!

bors commented Dec 1, 2022

Uh oh!

rust-timer commented Dec 1, 2022

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants