Re-export more `rustc_span::symbol` things from `rustc_span`. #134243

nnethercote · 2024-12-13T06:02:55Z

rustc_span::symbol defines some things that are re-exported from rustc_span, such as Symbol and sym. But it doesn't re-export some closely related things such as Ident and kw. So you can do use rustc_span::{Symbol, sym} but you have to do use rustc_span::symbol::{Ident, kw}, which is inconsistent for no good reason.

This commit re-exports Ident, kw, and MacroRulesNormalizedIdent, and changes many rustc_span::symbol:: qualifiers to rustc_span::. This is a 300+ net line of code reduction, mostly because many files with two use rustc_span items can be reduced to one.

r? @jieyouxu

rustbot · 2024-12-13T06:03:06Z

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in need_type_info.rs

cc @lcnr

HIR ty lowering was modified

cc @fmease

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in check-cfg diagnostics

cc @Urgau

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

`rustc_span::symbol` defines some things that are re-exported from `rustc_span`, such as `Symbol` and `sym`. But it doesn't re-export some closely related things such as `Ident` and `kw`. So you can do `use rustc_span::{Symbol, sym}` but you have to do `use rustc_span::symbol::{Ident, kw}`, which is inconsistent for no good reason. This commit re-exports `Ident`, `kw`, and `MacroRulesNormalizedIdent`, and changes many `rustc_span::symbol::` qualifiers to `rustc_span::`. This is a 300+ net line of code reduction, mostly because many files with two `use rustc_span` items can be reduced to one.

nnethercote · 2024-12-13T06:24:22Z

compiler/rustc_span/src/lib.rs

@@ -68,7 +68,7 @@ mod span_encoding;
 pub use span_encoding::{DUMMY_SP, Span};

 pub mod symbol;
-pub use symbol::{Symbol, sym};
+pub use symbol::{Ident, MacroRulesNormalizedIdent, Symbol, kw, sym};


Sorry for the tedious review. This is the line of note. The rest is just shortening rustc_span::symbol:: qualifiers.

oli-obk · 2024-12-13T06:47:06Z

I don't think I find this valuable. Reducing/merging imports is not very useful imo unless it allows removing of crate deps via reexports

nnethercote · 2024-12-13T06:59:42Z

I do think it's valuable. I think it's silly that you can import Symbol and sym via rustc_span, but the extremely similar things Ident and kw must be imported via rustc_span::symbol. It's one of those annoying rough edges and it shows up in a zillion files.

jieyouxu · 2024-12-13T09:03:36Z

I don't feel too strongly either way since the nesting doesn't seem very steep. Anyway, I'll look at this tmrw since there's several files to go through.

ytmimi · 2024-12-13T14:59:14Z

@nnethercote I'd like to opt out of these changes for rustfmt.

bors · 2024-12-13T23:58:41Z

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

jieyouxu

Thanks for the PR. I looked at the changes individually:

We should drop rustfmt changes as @ytmimi requested.
We should ask T-rustdoc if they want to opt-in to the rustdoc import changes (or at least in this PR).
We should ask T-clippy if they want to opt-in to the clippy import changes (or at least in this PR).
Compiler changes seem reasonable to me. If we want to make the re-exports/imports of {Ident, kw, sym} by downstream compiler crates and tool users more consistent, I guess better get it done earlier than later.

jieyouxu · 2024-12-14T10:19:05Z

compiler/rustc_lint/src/lints.rs

-        if name != rustc_span::symbol::kw::StaticLifetime {
+        if name != rustc_span::kw::StaticLifetime {


Question: can this just also import kw or is kw used as a local var here somewhere?

jieyouxu · 2024-12-14T10:22:11Z

compiler/rustc_metadata/src/rmeta/decoder.rs

-                            rustc_span::symbol::kw::Crate
+                            rustc_span::kw::Crate


Question: can this just import kw?

jieyouxu · 2024-12-14T10:35:28Z

src/librustdoc/clean/auto_trait.rs

Question: @rust-lang/rustdoc would you like to opt-in to the import change (in this PR), or do you want to keep this untouched?

jieyouxu · 2024-12-14T10:36:37Z

src/tools/clippy/book/src/development/common_tools_writing_lints.md

Question: @rust-lang/clippy do you want to opt-in to this import change (in this PR)? Or do you want to keep this untouched?

jieyouxu · 2024-12-14T10:40:12Z

src/tools/rustfmt/src/attr.rs

Problem: @nnethercote as @ytmimi requested, can you opt-out of the import changes for rustfmt?

rustbot assigned jieyouxu Dec 13, 2024

nnethercote force-pushed the re-export-more-rustc_span branch from 588869a to 5857bd7 Compare December 13, 2024 06:22

nnethercote commented Dec 13, 2024

View reviewed changes

jieyouxu requested changes Dec 14, 2024

View reviewed changes

rustbot 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 Dec 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-export more `rustc_span::symbol` things from `rustc_span`. #134243

Re-export more `rustc_span::symbol` things from `rustc_span`. #134243

nnethercote commented Dec 13, 2024

rustbot commented Dec 13, 2024

nnethercote Dec 13, 2024

oli-obk commented Dec 13, 2024

nnethercote commented Dec 13, 2024 •

edited

Loading

jieyouxu commented Dec 13, 2024 •

edited

Loading

ytmimi commented Dec 13, 2024

bors commented Dec 13, 2024

jieyouxu left a comment

jieyouxu Dec 14, 2024

jieyouxu Dec 14, 2024

jieyouxu Dec 14, 2024 •

edited

Loading

jieyouxu Dec 14, 2024 •

edited

Loading

jieyouxu Dec 14, 2024

		if name != rustc_span::symbol::kw::StaticLifetime {
		if name != rustc_span::kw::StaticLifetime {

Re-export more rustc_span::symbol things from rustc_span. #134243

Are you sure you want to change the base?

Re-export more rustc_span::symbol things from rustc_span. #134243

Conversation

nnethercote commented Dec 13, 2024

rustbot commented Dec 13, 2024

nnethercote Dec 13, 2024

Choose a reason for hiding this comment

oli-obk commented Dec 13, 2024

nnethercote commented Dec 13, 2024 • edited Loading

jieyouxu commented Dec 13, 2024 • edited Loading

ytmimi commented Dec 13, 2024

bors commented Dec 13, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Dec 14, 2024

Choose a reason for hiding this comment

jieyouxu Dec 14, 2024

Choose a reason for hiding this comment

jieyouxu Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Dec 14, 2024

Choose a reason for hiding this comment

Re-export more `rustc_span::symbol` things from `rustc_span`. #134243

Re-export more `rustc_span::symbol` things from `rustc_span`. #134243

nnethercote commented Dec 13, 2024 •

edited

Loading

jieyouxu commented Dec 13, 2024 •

edited

Loading

jieyouxu Dec 14, 2024 •

edited

Loading

jieyouxu Dec 14, 2024 •

edited

Loading