Skip to content

Rename kw::Empty as sym::empty. - #141376

Merged
bors merged 1 commit into
rust-lang:masterfrom
nnethercote:rename-kw-Empty
May 23, 2025
Merged

bors merged 1 commit into
rust-lang:masterfrom
nnethercote:rename-kw-Empty

Conversation

@nnethercote

Copy link
Copy Markdown
Contributor

Because the empty string is not a keyword.

r? @petrochenkov

Because the empty string is not a keyword.
@ghost ghost self-assigned this May 22, 2025
@ghost ghost added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 May 22, 2025

ghost commented May 22, 2025

Copy link
Copy Markdown

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

cc @jdonszelmann

Some changes occurred in compiler/rustc_ast_lowering/src/format.rs

cc @m-ou-se

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

ghost commented May 22, 2025

Copy link
Copy Markdown
Author

@petrochenkov: you previously rejected this change (along with kw::PathRoot and kw::DollarCrate) in #134253 for the following reasons:

Even if they are not lexically identifiers they are commonly stored in Idents in AST/HIR, which basically makes them identical to path segment keywords like self and super.

IIRC, we also relied on kw::Empty being zero (not sure if we are still doing that).

Because of the work I've been doing in #137978, empty identifiers are never stored any more. (There's even an assertion in Ident::new to prevent an empty identifier from being created, because an empty identifier is nonsensical.) And the number of uses of the empty symbol is much lower than it used to be.

Also, we no longer rely on kw::Empty being zero, and it hasn't been zero since the keywords were alphabetized in #138385.

ghost commented May 22, 2025 •

Copy link
Copy Markdown
Member

If I'm allowed to give an unsolicited comment, in my head I always map sym::xxx to the literal string "xxx" (which is true in 99.9% of cases) and understand kw::Xxx to be either a Rust keyword or some special name "of category" Xxx.

So sym::empty would literally map to "empty" (e.g., I could see Clippy using that symbol to warn on fn empty(&self) -> bool recommending is_empty over empty). To me, kw::Empty signals "special symbol of category 'empty'", i.e., "". I know that sym::dummy already violates that rule but to be fair, it's been added very recently.

ghost commented May 22, 2025

Copy link
Copy Markdown
Member

imo: kw::Empty suffers from the same problem as sym::empty suffers. So it's either renamed to something larger like sym::special::blank or just wait until the same brain path that mapped kw::Empty -> special category, maps sym::empty -> special category.

ghost commented May 22, 2025

Copy link
Copy Markdown
Author

There are about twenty symbols where the value doesn't match the name. (ferris: "🦀" is a notable example.)

I just want kw to encode actual keywords. We saw in #134253 there is some disagreement about the exact definition of "keyword" (e.g. _ and 'static), but I think it is clear there is no such thing as an empty keyword because a keyword must be something that can be written in Rust code and there is no way to write an empty keyword in Rust code.

ghost commented May 22, 2025

Copy link
Copy Markdown

I was thinking about #134253 (comment) in the background and did some experiments.

The symbols that end up in AST/HIR identifiers, besides actual lexical identifiers are:

  • lifetime identifiers starting with a quote 'ident
  • pretty-printed impl Traits
  • integer numbers (for tuple field names)
  • kw::Empty
  • kw::DollarCrate
  • kw::PathRoot
  • sym::cfg_trace
  • sym::cfg_attr_trace

So we either need to consistently move some of this stuff to "special identifiers", or just avoid additional entities and put them into sym.
I think I reconsidered my position now, it should be fine to dismantle the "special identifier" group and move Empty/DollarCrate/PathRoot to sym and Underscore to proper keywords.

ghost commented May 22, 2025

Copy link
Copy Markdown

In any case r=me on the changes in this PR as well.

sym::empty is probably fine until we actually need something called empty in the compiler.
The thing after sym:: must be an identifier, so there's always a potential for conflict with some real identifier with the same name.
There's one example of adding an underscore to disambiguate in the symbol list (integer_: "integer").

@ghost ghost 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 May 22, 2025

ghost commented May 22, 2025

Copy link
Copy Markdown
Author

@bors r=petrochenkov

ghost commented May 22, 2025

Copy link
Copy Markdown

📌 Commit 849cabf has been approved by petrochenkov

It is now in the queue for this repository.

@ghost ghost 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 May 22, 2025
@ghost
ghost merged commit 225ed8b into rust-lang:master May 23, 2025
@ghost ghost added this to the 1.89.0 milestone May 23, 2025
@ghost
ghost deleted the rename-kw-Empty branch May 23, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants