Move `hir::Item::ident` into `hir::ItemKind`. #138384

nnethercote · 2025-03-12T04:58:03Z

hir::Item has an ident field.

It's always non-empty for these item kinds: ExternCrate, Static, Const, Fn, Macro, Mod, TyAlias, Enum, Struct, Union, Trait, TraitAalis.
It's always empty for these item kinds: ForeignMod, GlobalAsm, Impl.
For Use, it is non-empty for UseKind::Single and empty for UseKind::{Glob,ListStem}.

All of this is quite non-obvious; the only documentation is a single comment saying "The name might be a dummy name in case of anonymous items". 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.

This is step towards kw::Empty elimination (#137978).

r? @fmease

nnethercote · 2025-03-12T07:39:33Z

@bors try @rust-timer queue

…try> Move `hir::Item::ident` into `hir::ItemKind`. `hir::Item` has an `ident` field. - It's always non-empty for these item kinds: `ExternCrate`, `Static`, `Const`, `Fn`, `Macro`, `Mod`, `TyAlias`, `Enum`, `Struct`, `Union`, Trait`, TraitAalis`. - It's always empty for these item kinds: `ForeignMod`, `GlobalAsm`, `Impl`. - For `Use`, it is non-empty for `UseKind::Single` and empty for `UseKind::{Glob,ListStem}`. All of this is quite non-obvious; the only documentation is a single comment saying "The name might be a dummy name in case of anonymous items". 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. This is step towards `kw::Empty` elimination (rust-lang#137978). r? `@ghost`

bors · 2025-03-12T07:41:45Z

⌛ Trying commit 2d77b39 with merge e0759ff...

bors · 2025-03-12T09:44:30Z

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

rust-timer · 2025-03-12T10:59:41Z

Finished benchmarking commit (e0759ff): 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-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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -1.0%)

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)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.0%, -1.0%]	1

Cycles

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

Binary size

Results (primary -0.0%, secondary -0.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)	0.1%	[0.0%, 0.1%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	65
Improvements ✅ (secondary)	-0.1%	[-0.7%, -0.0%]	26
All ❌✅ (primary)	-0.0%	[-0.2%, 0.1%]	71

Bootstrap: 779.599s -> 780.108s (0.07%)
Artifact size: 365.29 MiB -> 365.27 MiB (-0.01%)

rustbot · 2025-03-12T11:32:25Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in match checking

cc @Nadrieril

HIR ty lowering was modified

cc @fmease

nnethercote · 2025-03-12T11:33:22Z

Best reviewed one commit at a time.

nnethercote · 2025-03-12T11:34:33Z

@bors rollup=maybe

fmease

34 / 83 viewed. Continuing later.

src/tools/clippy/clippy_utils/src/lib.rs

src/tools/clippy/clippy_lints/src/redundant_pub_crate.rs

src/tools/clippy/clippy_lints/src/len_zero.rs

src/tools/clippy/clippy_lints/src/attrs/utils.rs

src/tools/clippy/clippy_lints/src/attrs/mod.rs

nnethercote · 2025-03-13T02:08:30Z

Thanks for the fast feedback. I've addressed the comments so far in a subsequent commit.

bors · 2025-03-13T05:48:59Z

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

fmease

71 / 83. Finishing later

compiler/rustc_ast_lowering/src/item.rs

compiler/rustc_passes/src/check_attr.rs

fmease

Thanks, I really like this refactoring! It's a great idea!
I have some concerns and small suggestions (don't forget about my 2nd review). Then r=me

(Causing the ItemKind variant sizes to diverge even further from another does displease me slightly but as shown perf doesn't seem to suffer from it, so okay).

src/librustdoc/visit_ast.rs

src/tools/clippy/clippy_lints/src/arbitrary_source_item_ordering.rs

src/librustdoc/clean/mod.rs

src/tools/clippy/clippy_lints/src/arbitrary_source_item_ordering.rs

src/tools/clippy/clippy_lints/src/redundant_pub_crate.rs

It determines if a function should have any `inline` attributes checked. For `ItemKind::Fn` it returns true or false depending on the details of the function; for anything other item kind it returns *true*. This latter case should instead be *false*. (In the nearby and similar functions `is_relevant_impl` and `is_relevant_trait` the non-function cases return false.) The effect of this is that non-functions are no longer checked. But rustc already disallows `inline` on any non-function items. So if anything its a tiny performance win, because that was useless anyway.

nnethercote · 2025-03-16T23:10:21Z

Thanks, I really like this refactoring! It's a great idea! I have some concerns and small suggestions (don't forget about my 2nd review). Then r=me

There are enough comments that I'll give you a chance to check over the changes I made, which are all in the second commit. (I'll squash that before merging.)

(Causing the ItemKind variant sizes to diverge even further from another does displease me slightly but as shown perf doesn't seem to suffer from it, so okay).

The size of Item hasn't changed, so changes to ItemKind don't seem important.

fmease · 2025-03-17T12:52:16Z

src/tools/clippy/clippy_lints/src/redundant_pub_crate.rs

-            let span = item.span.with_hi(item.kind.ident().unwrap().span.hi());
+            // FIXME: `DUMMY_SP` isn't right here, because it causes the
+            // resulting span to begin at the start of the file.
+	    let span = item.span.with_hi(


Suggested change

let span = item.span.with_hi(

let span = item.span.with_hi(

\t → 8×\x20

fmease · 2025-03-17T12:56:35Z

r=me with tab replaced and last commit squashed as discussed

`hir::Item` has an `ident` field. - It's always non-empty for these item kinds: `ExternCrate`, `Static`, `Const`, `Fn`, `Macro`, `Mod`, `TyAlias`, `Enum`, `Struct`, `Union`, Trait`, TraitAalis`. - It's always empty for these item kinds: `ForeignMod`, `GlobalAsm`, `Impl`. - For `Use`, it is non-empty for `UseKind::Single` and empty for `UseKind::{Glob,ListStem}`. All of this is quite non-obvious; the only documentation is a single comment saying "The name might be a dummy name in case of anonymous items". 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. - A similar transformation makes sense for `ast::Item`, but this is already a big change. That can be done later. - Lots of assertions are added to item lowering to ensure that identifiers are empty/non-empty as expected. These will be removable when `ast::Item` is done later. - `ItemKind::Use` doesn't get an `Ident`, but `UseKind::Single` does. - `lower_use_tree` is significantly simpler. No more confusing `&mut Ident` to deal with. - `ItemKind::ident` is a new method, it returns an `Option<Ident>`. It's used with `unwrap` in a few places; sometimes it's hard to tell exactly which item kinds might occur. None of these unwraps fail on the test suite. It's conceivable that some might fail on alternative input. We can deal with those if/when they happen. - In `trait_path` the `find_map`/`if let` is replaced with a loop, and things end up much clearer that way. - `named_span` no longer checks for an empty name; instead the call site now checks for a missing identifier if necessary. - `maybe_inline_local` doesn't need the `glob` argument, it can be computed in-function from the `renamed` argument. - `arbitrary_source_item_ordering::check_mod` had a big `if` statement that was just getting the ident from the item kinds that had one. It could be mostly replaced by a single call to the new `ItemKind::ident` method. - `ItemKind` grows from 56 to 64 bytes, but `Item` stays the same size, and that's what matters, because `ItemKind` only occurs within `Item`.

nnethercote · 2025-03-17T19:30:48Z

@bors r=fmease

bors · 2025-03-17T19:30:51Z

📌 Commit f2ddbcd has been approved by fmease

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138384 (Move `hir::Item::ident` into `hir::ItemKind`.) - rust-lang#138508 (Clarify "owned data" in E0515.md) - rust-lang#138531 (Store test diffs in job summaries and improve analysis formatting) - rust-lang#138533 (Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly) - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename) - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch) - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#138384 - nnethercote:hir-ItemKind-idents, r=fmease Move `hir::Item::ident` into `hir::ItemKind`. `hir::Item` has an `ident` field. - It's always non-empty for these item kinds: `ExternCrate`, `Static`, `Const`, `Fn`, `Macro`, `Mod`, `TyAlias`, `Enum`, `Struct`, `Union`, Trait`, TraitAalis`. - It's always empty for these item kinds: `ForeignMod`, `GlobalAsm`, `Impl`. - For `Use`, it is non-empty for `UseKind::Single` and empty for `UseKind::{Glob,ListStem}`. All of this is quite non-obvious; the only documentation is a single comment saying "The name might be a dummy name in case of anonymous items". 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. This is step towards `kw::Empty` elimination (rust-lang#137978). r? `@fmease`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138384 (Move `hir::Item::ident` into `hir::ItemKind`.) - rust-lang#138508 (Clarify "owned data" in E0515.md) - rust-lang#138531 (Store test diffs in job summaries and improve analysis formatting) - rust-lang#138533 (Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly) - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename) - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch) - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`) r? `@ghost` `@rustbot` modify labels: rollup

rustbot added A-attributes S-waiting-on-review T-compiler T-rustdoc T-rustdoc-frontend labels Mar 12, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf label Mar 12, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf label Mar 12, 2025

nnethercote marked this pull request as ready for review March 12, 2025 11:32

rustbot assigned fmease Mar 12, 2025

fmease reviewed Mar 12, 2025

View reviewed changes

nnethercote force-pushed the hir-ItemKind-idents branch from 2d77b39 to 05ff9cd Compare March 13, 2025 02:07

fmease reviewed Mar 13, 2025

View reviewed changes

fmease approved these changes Mar 14, 2025

View reviewed changes

fmease added S-waiting-on-author and removed S-waiting-on-review labels Mar 14, 2025

nnethercote force-pushed the hir-ItemKind-idents branch from 05ff9cd to a672c2e Compare March 16, 2025 23:08

fmease approved these changes Mar 17, 2025

View reviewed changes

nnethercote force-pushed the hir-ItemKind-idents branch from a672c2e to f2ddbcd Compare March 17, 2025 19:30

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 17, 2025

matthiaskrgr mentioned this pull request Mar 17, 2025

Rollup of 7 pull requests #138630

Merged

bors merged commit e1acc68 into rust-lang:master Mar 18, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 18, 2025

nnethercote deleted the hir-ItemKind-idents branch March 18, 2025 23:43

matthiaskrgr mentioned this pull request Mar 19, 2025

ICE: no name for HirId #138723

Closed

nnethercote mentioned this pull request Mar 19, 2025

Too much kw::Empty usage #137978

Open

nnethercote mentioned this pull request Mar 21, 2025

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `hir::Item::ident` into `hir::ItemKind`. #138384

Move `hir::Item::ident` into `hir::ItemKind`. #138384

nnethercote commented Mar 12, 2025 •

edited

Loading

nnethercote commented Mar 12, 2025

This comment has been minimized.

bors commented Mar 12, 2025

bors commented Mar 12, 2025

This comment has been minimized.

rust-timer commented Mar 12, 2025

rustbot commented Mar 12, 2025

nnethercote commented Mar 12, 2025

nnethercote commented Mar 12, 2025

fmease left a comment

nnethercote commented Mar 13, 2025

bors commented Mar 13, 2025

fmease left a comment

fmease left a comment •

edited

Loading

nnethercote commented Mar 16, 2025

fmease Mar 17, 2025 •

edited

Loading

fmease commented Mar 17, 2025

nnethercote commented Mar 17, 2025

bors commented Mar 17, 2025

Move hir::Item::ident into hir::ItemKind. #138384

Move hir::Item::ident into hir::ItemKind. #138384

Conversation

nnethercote commented Mar 12, 2025 • edited Loading

nnethercote commented Mar 12, 2025

This comment has been minimized.

bors commented Mar 12, 2025

bors commented Mar 12, 2025

This comment has been minimized.

rust-timer commented Mar 12, 2025

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

rustbot commented Mar 12, 2025

nnethercote commented Mar 12, 2025

nnethercote commented Mar 12, 2025

fmease left a comment

Choose a reason for hiding this comment

nnethercote commented Mar 13, 2025

bors commented Mar 13, 2025

fmease left a comment

Choose a reason for hiding this comment

fmease left a comment • edited Loading

Choose a reason for hiding this comment

nnethercote commented Mar 16, 2025

fmease Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

fmease commented Mar 17, 2025

nnethercote commented Mar 17, 2025

bors commented Mar 17, 2025

Move `hir::Item::ident` into `hir::ItemKind`. #138384

Move `hir::Item::ident` into `hir::ItemKind`. #138384

nnethercote commented Mar 12, 2025 •

edited

Loading

fmease left a comment •

edited

Loading

fmease Mar 17, 2025 •

edited

Loading