Move `ast::Item::ident` into `ast::ItemKind` #138740

nnethercote · 2025-03-20T09:10:21Z

The follow-up to #138384, which did the same thing for hir::ItemKind.

r? @fmease

nnethercote · 2025-03-20T11:04:16Z

@bors try @rust-timer queue

bors · 2025-03-20T11:05:28Z

⌛ Trying commit 87677af with merge 960f748...

…try> Move `ast::Item::ident` into `ast::ItemKind` r? `@ghost`

bors · 2025-03-20T13:11:47Z

☀️ Try build successful - checks-actions
Build commit: 960f748 (960f7488ddd0a883d61daf2c4e954ec575bd58aa)

rust-timer · 2025-03-20T15:39:49Z

Finished benchmarking commit (960f748): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary -0.7%, secondary -1.1%)

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)	1.1%	[0.6%, 1.5%]	2
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-1.6%	[-2.0%, -1.4%]	4
Improvements ✅ (secondary)	-2.4%	[-2.8%, -1.8%]	3
All ❌✅ (primary)	-0.7%	[-2.0%, 1.5%]	6

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary -0.2%)

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.1%	[0.0%, 0.1%]	33
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.0%]	19
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.0%, 0.1%]	52

Bootstrap: 773.468s -> 775.556s (0.27%)
Artifact size: 365.48 MiB -> 365.41 MiB (-0.02%)

nnethercote · 2025-03-20T19:00:19Z

Performance is basically neutral. If anything, the improvements slightly outweigh the regressions, but all changes are very small.

@rustbot label: +perf-regression-triaged

rustbot · 2025-03-21T05:24:34Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

nnethercote · 2025-03-21T05:26:06Z

@fmease: thanks for reviewing, and apologies for the size of the final commit. I can move the four preliminary commits into a separate PR if that would be helpful.

bors · 2025-03-25T17:50:07Z

☔ The latest upstream changes (presumably #138923) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2025-03-25T21:24:50Z

I rebased.

bors · 2025-03-26T00:06:33Z

☔ The latest upstream changes (presumably #138933) made this pull request unmergeable. Please resolve the merge conflicts.

fmease

r=me after rebase and important nitpicks addressed. I like it very much, thanks!

I also posted a bunch of stylistic suggestions some of which probably aren't that important in retrospect. Feel free to ignore them (or address them) at your discretion. I don't want to require unnecessary work

src/tools/rustfmt/src/visitor.rs

tests/ui/custom_test_frameworks/full.rs

compiler/rustc_builtin_macros/src/test.rs

compiler/rustc_ast/src/visit.rs

src/tools/clippy/clippy_lints/src/empty_line_after.rs

src/tools/clippy/clippy_utils/src/ast_utils/mod.rs

src/tools/rustfmt/src/items.rs

compiler/rustc_resolve/src/late/diagnostics.rs

fmease · 2025-04-01T02:23:11Z

This PR is super bitrotty.
@bors p=1

`global_allocator_spans` and `alloc_error_handler_span` are identical except for `name`.

Instead of putting the item inside it, just pass the ident and visibility (the only things needed) alongside it where necessary. This helps with the next commit, which will move the ident's location. Specifically, it gets rid of the `match visitor_kind` in `rewrite_type_alias`.

`FmtVisitor::visit_mac` has an `Option<Ident>` arg which is always either `None` or `Some(kw::Empty)`, because `ItemKind::MacCall` always has an empty ident. This value is passed through various functions until it reaches `rewrite_macro_name`, which treats `None` and `Some(kw::Empty)` the same. In other words, the argument is useless. This commit removes it. There is no change in behaviour. The commit also changes a few `symbol::Ident` occurrences to `Ident` in `macros.rs`; `Symbol` is imported in that file so `Ident` might as well be, too. (This is a good example of why it's a bad idea for `Itemt` to have an `ident` field when various item kinds don't have an identifier. It's easy to get confused when "empty identifier" is used to mean "no identifier". This will be fixed in a subsequent commit.)

`expand_test_case` looks for any item with a `#[test_case]` attribute and adds a `test_path_symbol` attribute to it while also fiddling with the item's ident's span. This is pretty weird, because `#[test_case]` is only valid on `fn`/`const`/`static` items, as far as I can tell. But you don't currently get an error or warning if you use it on other kinds of items. This commit changes things so that a `#[test_case]` item is modified only if it is `fn`/`const`/`static`. This is relevant for moving idents from `Item` to `ItemKind`, because some item kinds don't have an ident, e.g. `impl` blocks. The commit also does the following. - Renames a local variable `test_id` as `test_ident`. - Changes a `const` to `static` in `tests/ui/custom_test_frameworks/full.rs` to give the `static` case some test coverage. - Adds a `struct` and `impl` to the same test to give some test coverage to the non-affected item kinds. These have a `FIXME` comment identifying the weirdness here. Hopefully this will be useful breadcrumbs for somebody else in the future.

`ast::Item` has an `ident` field. - It's always non-empty for these item kinds: `ExternCrate`, `Static`, `Const`, `Fn`, `Mod`, `TyAlias`, `Enum`, `Struct`, `Union`, `Trait`, `TraitAlias`, `MacroDef`, `Delegation`. - It's always empty for these item kinds: `Use`, `ForeignMod`, `GlobalAsm`, `Impl`, `MacCall`, `DelegationMac`. There is a similar story for `AssocItemKind` and `ForeignItemKind`. Some sites that handle items check for an empty ident, some don't. This is a very C-like way of doing things, but this is Rust, we have sum types, we can do this properly and never forget to check for the exceptional case and never YOLO possibly empty identifiers (or possibly dummy spans) around and hope that things will work out. The commit is large but it's mostly obvious plumbing work. Some notable things. - `ast::Item` got 8 bytes bigger. This could be avoided by boxing the fields within some of the `ast::ItemKind` variants (specifically: `Struct`, `Union`, `Enum`). I might do that in a follow-up; this commit is big enough already. - For the visitors: `FnKind` no longer needs an `ident` field because the `Fn` within how has one. - In the parser, the `ItemInfo` typedef is no longer needed. It was used in various places to return an `Ident` alongside an `ItemKind`, but now the `Ident` (if present) is within the `ItemKind`. - In a few places I renamed identifier variables called `name` (or `foo_name`) as `ident` (or `foo_ident`), to better match the type, and because `name` is normally used for `Symbol`s. It's confusing to see something like `foo_name.name`.

rustbot · 2025-04-01T05:23:31Z

Some changes occurred in compiler/rustc_builtin_macros/src/autodiff.rs

cc @ZuseZ4

nnethercote · 2025-04-01T05:24:10Z

Thanks for the detailed review. I made all the changes you suggested except where I mentioned otherwise. Should be good to go.

@bors r=fmease

bors · 2025-04-01T05:24:13Z

📌 Commit ec10833 has been approved by fmease

It is now in the queue for this repository.

bors · 2025-04-01T07:21:32Z

⌛ Testing commit ec10833 with merge ed20157...

bors · 2025-04-01T10:31:21Z

☀️ Test successful - checks-actions
Approved by: fmease
Pushing ed20157 to master...

github-actions · 2025-04-01T10:33:29Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 97ea17b (parent) -> ed20157 (this PR)

Test differences

Show 66 test diffs

Additionally, 66 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Job duration changes

dist-apple-various: 7149.7s -> 9328.9s (30.5%)
x86_64-apple-1: 6940.9s -> 7392.3s (6.5%)
armhf-gnu: 4407.3s -> 4683.3s (6.3%)
x86_64-mingw-2: 6692.9s -> 7029.0s (5.0%)
dist-aarch64-linux: 7405.9s -> 7738.6s (4.5%)
dist-riscv64-linux: 5022.8s -> 5233.1s (4.2%)
dist-armv7-linux: 5233.4s -> 5451.8s (4.2%)
dist-aarch64-apple: 5300.9s -> 5490.2s (3.6%)
dist-x86_64-netbsd: 5001.1s -> 5179.7s (3.6%)
aarch64-gnu: 6339.2s -> 6564.9s (3.6%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-04-01T12:41:25Z

Finished benchmarking commit (ed20157): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.1%]	3
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.3%]	2
All ❌✅ (primary)	-0.2%	[-0.3%, -0.1%]	3

Max RSS (memory usage)

Results (primary -0.3%, secondary 2.2%)

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.8%	[0.4%, 1.5%]	4
Regressions ❌ (secondary)	2.8%	[1.2%, 3.9%]	7
Improvements ✅ (primary)	-1.8%	[-2.8%, -1.2%]	3
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-0.3%	[-2.8%, 1.5%]	7

Cycles

Results (secondary -3.2%)

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
Improvements ✅ (secondary)	-3.2%	[-3.4%, -2.9%]	3
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.2%)

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.1%	[0.0%, 0.1%]	34
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-1.0%, -0.0%]	19
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.0%, 0.1%]	53

Bootstrap: 773.194s -> 773.019s (-0.02%)
Artifact size: 366.00 MiB -> 365.92 MiB (-0.02%)

- fix errors caused by the move of `ast::Item::ident` (see rust-lang#138740) - move the logic of getting `sig`, `vis`, and `ident` from two seperate `match` statements into one (less repetition especially with the nested `match`)

…mease Move `ast::Item::ident` into `ast::ItemKind` The follow-up to rust-lang#138384, which did the same thing for `hir::ItemKind`. r? `@fmease`

I accidentally added this in rust-lang#138740.

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 20, 2025

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 Mar 20, 2025

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2025

Auto merge of rust-lang#138740 - nnethercote:ast-ItemKind-idents, r=<…

960f748

…try> Move `ast::Item::ident` into `ast::ItemKind` r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 20, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 20, 2025

nnethercote force-pushed the ast-ItemKind-idents branch from 87677af to c977a9d Compare March 21, 2025 05:24

nnethercote marked this pull request as ready for review March 21, 2025 05:24

rustbot assigned fmease Mar 21, 2025

nnethercote force-pushed the ast-ItemKind-idents branch from c977a9d to 8ba9610 Compare March 25, 2025 21:24

fmease approved these changes Apr 1, 2025

View reviewed changes

fmease 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 Apr 1, 2025

nnethercote added 2 commits April 1, 2025 13:34

Factor out some shared code.

59307fd

`global_allocator_spans` and `alloc_error_handler_span` are identical except for `name`.

nnethercote added 4 commits April 1, 2025 13:42

Address review comments.

ec10833

nnethercote force-pushed the ast-ItemKind-idents branch from 8ba9610 to ec10833 Compare April 1, 2025 05:23

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 1, 2025

ZuseZ4 mentioned this pull request Apr 1, 2025

fix usage of autodiff macro with inner functions #138314

Open

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 1, 2025

bors merged commit ed20157 into rust-lang:master Apr 1, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 1, 2025

rustbot removed the perf-regression Performance regression. label Apr 1, 2025

fmease removed the perf-regression-triaged The performance regression has been triaged. label Apr 1, 2025

fmease mentioned this pull request Apr 1, 2025

Lint empty_line_after_doc_comments emits a slightly butchered diagnostic note if the following item is of a kind that doesn't have a name rust-lang/rust-clippy#14515

Open

nnethercote deleted the ast-ItemKind-idents branch April 1, 2025 19:14

nnethercote added a commit to nnethercote/rust that referenced this pull request Apr 3, 2025

Remove an unwanted comment.

aa1b7bf

I accidentally added this in rust-lang#138740.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `ast::Item::ident` into `ast::ItemKind` #138740

Move `ast::Item::ident` into `ast::ItemKind` #138740

nnethercote commented Mar 20, 2025 •

edited

Loading

nnethercote commented Mar 20, 2025

This comment has been minimized.

bors commented Mar 20, 2025

bors commented Mar 20, 2025

This comment has been minimized.

rust-timer commented Mar 20, 2025

nnethercote commented Mar 20, 2025

rustbot commented Mar 21, 2025

nnethercote commented Mar 21, 2025

bors commented Mar 25, 2025

nnethercote commented Mar 25, 2025

bors commented Mar 26, 2025

fmease left a comment •

edited

Loading

fmease commented Apr 1, 2025

rustbot commented Apr 1, 2025

nnethercote commented Apr 1, 2025

bors commented Apr 1, 2025

bors commented Apr 1, 2025

bors commented Apr 1, 2025

github-actions bot commented Apr 1, 2025

rust-timer commented Apr 1, 2025

Move ast::Item::ident into ast::ItemKind #138740

Move ast::Item::ident into ast::ItemKind #138740

Conversation

nnethercote commented Mar 20, 2025 • edited Loading

nnethercote commented Mar 20, 2025

This comment has been minimized.

bors commented Mar 20, 2025

bors commented Mar 20, 2025

This comment has been minimized.

rust-timer commented Mar 20, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

nnethercote commented Mar 20, 2025

rustbot commented Mar 21, 2025

nnethercote commented Mar 21, 2025

bors commented Mar 25, 2025

nnethercote commented Mar 25, 2025

bors commented Mar 26, 2025

fmease left a comment • edited Loading

Choose a reason for hiding this comment

fmease commented Apr 1, 2025

rustbot commented Apr 1, 2025

nnethercote commented Apr 1, 2025

bors commented Apr 1, 2025

bors commented Apr 1, 2025

bors commented Apr 1, 2025

github-actions bot commented Apr 1, 2025

Test differences

Job duration changes

rust-timer commented Apr 1, 2025

Overall result: ✅ improvements - no action needed

Move `ast::Item::ident` into `ast::ItemKind` #138740

Move `ast::Item::ident` into `ast::ItemKind` #138740

nnethercote commented Mar 20, 2025 •

edited

Loading

fmease left a comment •

edited

Loading