Skip to content

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

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.

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

Merged
merged 1 commit into from
May 23, 2025
Merged

Conversation

nnethercote
Copy link
Contributor

Because the empty string is not a keyword.

r? @petrochenkov

Because the empty string is not a keyword.
@rustbot rustbot 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
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

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

@nnethercote
Copy link
Contributor 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.

@fmease
Copy link
Member

fmease commented May 22, 2025

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.

@blyxyas
Copy link
Member

blyxyas commented May 22, 2025

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.

@nnethercote
Copy link
Contributor 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.

@petrochenkov
Copy link
Contributor

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.

@petrochenkov
Copy link
Contributor

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").

@petrochenkov 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 May 22, 2025
@nnethercote
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented May 22, 2025

📌 Commit 849cabf has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 May 22, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2025
…rochenkov

Rename `kw::Empty` as `sym::empty`.

Because the empty string is not a keyword.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136400 (Improve handling of rustdoc lints when used with raw doc fragments.)
 - rust-lang#140967 (Async drop poll shim for error dropee generates noop body)
 - rust-lang#141019 (Update std doctests for android)
 - rust-lang#141062 (Update IDEs to use rustfmt 2024, fix Zed settings)
 - rust-lang#141109 (discuss deadlocks in the std::io::pipe() example)
 - rust-lang#141126 (rustdoc JSON: Don't apply `#[repr]` privacy heuristics)
 - rust-lang#141376 (Rename `kw::Empty` as `sym::empty`.)
 - rust-lang#141383 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request May 23, 2025
Rollup of 7 pull requests

Successful merges:

 - #136400 (Improve handling of rustdoc lints when used with raw doc fragments.)
 - #140967 (Async drop poll shim for error dropee generates noop body)
 - #141019 (Update std doctests for android)
 - #141109 (discuss deadlocks in the std::io::pipe() example)
 - #141126 (rustdoc JSON: Don't apply `#[repr]` privacy heuristics)
 - #141376 (Rename `kw::Empty` as `sym::empty`.)
 - #141383 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 225ed8b into rust-lang:master May 23, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 23, 2025
@nnethercote nnethercote 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