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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

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 rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

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

@nnethercote nnethercote force-pushed the re-export-more-rustc_span branch from 588869a to 5857bd7 Compare December 13, 2024 06:22
@@ -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};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2024

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
Copy link
Contributor Author

nnethercote commented Dec 13, 2024

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
Copy link
Member

jieyouxu commented Dec 13, 2024

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
Copy link
Contributor

ytmimi commented Dec 13, 2024

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

@bors
Copy link
Contributor

bors commented Dec 13, 2024

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

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

compiler/rustc_lint/src/lints.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/rmeta/decoder.rs Outdated Show resolved Hide resolved
Copy link
Member

@jieyouxu jieyouxu Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@jieyouxu jieyouxu Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/tools/rustfmt/src/attr.rs Outdated Show resolved Hide resolved
@rustbot 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
@nnethercote
Copy link
Contributor Author

The diff from this file sums up the motivation for this change:

diff --git a/compiler/rustc_ast_lowering/src/format.rs b/compiler/rustc_ast_lowering/src/format.rs
index 653116e1fe0..22aa1e6fc20 100644
--- a/compiler/rustc_ast_lowering/src/format.rs
+++ b/compiler/rustc_ast_lowering/src/format.rs
@@ -6,8 +6,7 @@
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_hir as hir;
 use rustc_session::config::FmtDebug;
-use rustc_span::symbol::{Ident, kw};
-use rustc_span::{Span, Symbol, sym};
+use rustc_span::{Ident, Span, Symbol, kw, sym};

 use super::LoweringContext;

It's silly that Symbol gets a shortcut import but the closely related Ident doesn't. Likewise, sym gets a shortcut but the closely related kw does not.

@nnethercote nnethercote force-pushed the re-export-more-rustc_span branch from 5857bd7 to a349719 Compare December 16, 2024 02:59
`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 nnethercote force-pushed the re-export-more-rustc_span branch from a349719 to 4d6ed1d Compare December 16, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc 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