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

Merged
merged 1 commit into from
Dec 18, 2024

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
src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved
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 2 times, most recently from a349719 to 4d6ed1d Compare December 16, 2024 03:43
@bors
Copy link
Contributor

bors commented Dec 17, 2024

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

@jieyouxu
Copy link
Member

@nnethercote if you don't want to wait for T-clippy/T-rustdoc, we could also split this PR into compiler-only changes vs tools changes, where the tool changes can be sent to the relevant teams. I can r+ the compiler changes specifically.

@nnethercote nnethercote force-pushed the re-export-more-rustc_span branch from 4d6ed1d to dd17220 Compare December 18, 2024 02:34
`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 in `compiler/` to
`rustc_span::`. This is a 200+ 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 dd17220 to 2620eb4 Compare December 18, 2024 02:48
@nnethercote
Copy link
Contributor Author

I've trimmed this down to just the compiler changes and rebased. It's conflict-prone, so:

@bors r=jieyouxu p=1

@bors
Copy link
Contributor

bors commented Dec 18, 2024

📌 Commit 2620eb4 has been approved by jieyouxu

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 Dec 18, 2024
@nnethercote nnethercote requested a review from jieyouxu December 18, 2024 02:51
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Dec 18, 2024

⌛ Testing commit 2620eb4 with merge a89ca2c...

@bors
Copy link
Contributor

bors commented Dec 18, 2024

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing a89ca2c to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a89ca2c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.9% [0.9%, 0.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary -4.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.9% [-4.9%, -4.9%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 768.784s -> 769.265s (0.06%)
Artifact size: 330.32 MiB -> 330.34 MiB (0.01%)

@nnethercote nnethercote deleted the re-export-more-rustc_span branch December 18, 2024 09:36
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 merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants