Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Too much kw::Empty usage #137978

Open
nnethercote opened this issue Mar 4, 2025 · 4 comments
Open

Too much kw::Empty usage #137978

nnethercote opened this issue Mar 4, 2025 · 4 comments
Assignees
@nnethercote
Labels
A-technical-debt Area: Internal cleanup work needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nnethercote
Copy link
Contributor

kw::Empty is the name of the empty Symbol. It's used a lot. Most places it's used, it's hard to tell if it means "empty symbol" or "no symbol". I.e. it more or less reinvents the null pointer, something that Rust was supposed to get rid of.

I think a lot of the uses could be removed by using Option<Symbol> instead of Symbol, making the "no symbol" cases much clearer.
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 4, 2025
@nnethercote nnethercote self-assigned this Mar 4, 2025
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 4, 2025 •
edited
Loading

#137977 is the first step here. The backstory: I grepped for kw::Empty, looked at the very first occurrence in the output, worked out how to eliminate it, removing 17 kw::Empty occurrences, and I unintentionally fixed a crashtest in the process. A promising start.

@nnethercote nnethercote added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-technical-debt Area: Internal cleanup work labels Mar 4, 2025
This was referenced Mar 11, 2025
Merged
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 12, 2025
@bors
Auto merge of rust-lang#138384 - nnethercote:hir-ItemKind-idents, r=<…
e0759ff 
…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`
@nnethercote nnethercote mentioned this issue Mar 14, 2025
Merged
@nnethercote
Copy link
Contributor Author

#138482 fixed some HIR printing bugs. That PR didn't reduce kw::Empty usage, but I found the bugs while working on related kw::Empty reduction. More evidence that kw::Empty makes it easy to get things wrong.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 17, 2025
@matthiaskrgr
Rollup merge of rust-lang#138384 - nnethercote:hir-ItemKind-idents, r…
e1acc68 
…=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`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 18, 2025
@rust-timer
Unrolled build for rust-lang#138384
5736993 
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`
@nnethercote
Copy link
Contributor Author

#138685 removes some more kw::Empty uses.

@nnethercote
Copy link
Contributor Author

#138384 removed some dubious use of kw::Empty to give an empty identifier to anonymous items (e.g. Impl, ForeignMod).

This was referenced Mar 25, 2025
Merged
Open
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 25, 2025
@matthiaskrgr
Rollup merge of rust-lang#138924 - nnethercote:less-kw-Empty-3, r=com…
ffc5717 
…piler-errors

Reduce `kw::Empty` usage, part 3

Remove some more `kw::Empty` uses, in support of rust-lang#137978.

r? `@davidtwco`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 25, 2025
@rust-timer
Unrolled build for rust-lang#138924
d733fd9 
Rollup merge of rust-lang#138924 - nnethercote:less-kw-Empty-3, r=compiler-errors

Reduce `kw::Empty` usage, part 3

Remove some more `kw::Empty` uses, in support of rust-lang#137978.

r? `@davidtwco`
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 26, 2025
@jhpratt
Rollup merge of rust-lang#138917 - nnethercote:rustdoc-remove-useless…
04cbe28 
…, r=GuillaumeGomez

rustdoc: remove useless `Symbol::is_empty` checks.

There are a number of `is_empty` checks that can never fail. This commit removes them, in support of rust-lang#137978.

r? `@GuillaumeGomez`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nnethercote nnethercote

Labels
A-technical-debt Area: Internal cleanup work needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nnethercote @rustbot