bootstrap: Move all non-module items out of the crate root - #161277
bootstrap: Move all non-module items out of the crate root#161277Zalathar wants to merge 5 commits into
Conversation
|
This PR modifies If appropriate, please update This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
rustbot
added
A-bootstrap-stamp
|
r? @clubby789 rustbot has assigned @clubby789. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Does this have to be combined with turning everything into |
|
The deliberate misuse of It’s a huge pain to have to constantly struggle against a minefield full of lying visibility specifications, especially when those lies have real consequences in the language. Tracking down false visibility-conflict errors really sucks. |
This comment has been minimized.
This comment has been minimized.
Items with implicit `pub(self)` visibility in the crate root are effectively `pub(crate)`, which causes friction when trying to move them elsewhere.
This intermediate commit helps to preserve line history.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
I guess I agree in general, I just haven't personally experienced such issues in bootstrap 🤷 And since this pretty huge refactoring across several PRs didn't even enable us remove any non-trivial code, I wonder what improvements does it bring to you (since you drive it), because I don't see them (I'm fine with doing the refactoring, I just want to understand the motivation better).
If you feel strongly about this and want to ensure that we use pub(crate) in bootstrap, is there a lint we could enable to enforce it? Otherwise I'm pretty sure we will just continue using pub.
| @@ -0,0 +1,1878 @@ | |||
| use std::cell::Cell; | |||
There was a problem hiding this comment.
We don't really have any notion of a "session" in bootstrap. This file looks to me like we just took a bunch of commonly used stuff and moved it from lib.rs to a different file. What's the benefit of that?
There was a problem hiding this comment.
After previous PRs, this file mostly consists of Build, its methods, and a few pub(crate) types that are used by those methods and don't have a more obvious home elsewhere.
Moving Build out of the crate root requires me to come up with a module name, but I specifically didn't want to go with build, for two reasons:
- It invites confusion with
build.rs. - GitHub disables some search/navigation features for directories named
build/, with no opt-out. This module currently doesn't have submodules, but I didn't want arbitrary barriers to adding submodules in the future.
Since I had to choose something, I tentatively arrived at session, with the idea that later we could maybe rename Build to Session to help improve the very murky distinction between Build and Builder.
There was a problem hiding this comment.
The Build and Builder naming is indeed confusing, so I'm 👍 for trying to figure out something better there.
There was a problem hiding this comment.
As for why we should move this code at all, one of the big reasons is that the crate root has very unusual and awkward interactions with item visibility:
- Every item in the crate root effectively has at least
pub(crate)visibility, even if is declared with no visibility or withpub(self). Private items cannot exist in the crate root, because every other module is a descendant of it. And if you try, you'll silently get something else instead. - Because of those visibility problems, every top-level
usein the crate root is effectively a crate-wide re-export. This is undesirable, and causes a lot of inconsistency and confusion, especially when IDEs end up auto-importing from the wrong place by mistake. - Any
pubitem in the crate root is a publicly-exported item. The compiler has no way to know some of those items aren't supposed to be exported, because we're literally telling it that they should be exported.
Furthermore, none of these items can really make a compelling argument that they should be in the crate root. Everything else in the crate is split into submodules (mainly utils and core), and having an arbitrary subset of items sitting around in the crate root creates a weird inconsistency with no useful significance that I can see. It's literally just stuff that was thrown into lib.rs and that nobody ever got around to moving before.
There was a problem hiding this comment.
Actually one of my motivations for doing the move first is that I didn’t want to do the renaming under all the additional friction of trying to modify things in the crate root.
|
You can r=me, unless you want to make more changes. |
With everything moved out of the crate root, this can be enforced by (There are several hundred of those, but some experiments suggest that the migration should be a relatively simple regex-replace.) |
|
Thanks for your patience on this one. I wasn't expecting to have to explain so much because it seemed so self-evident to me, but it's good to have the motivation stated more clearly. @bors r=Kobzol |
rust-bors
Bot
added
S-waiting-on-bors
bootstrap: Move all non-module items out of the crate root - Previously: rust-lang#161219 --- After several rounds of preparation, this PR moves all non-module items out of bootstrap's crate root. Having non-trivial code in the crate root is generally a bad idea. Anything defined or imported there is unconditionally visible throughout the entire crate, leading to messy imports and unclear abstraction boundaries, and causing friction when code needs to be moved elsewhere. The commits have been structured to preserve as much line history as possible, and to minimize the number of other changes in the commit that performs the actual move, while remaining functional at every intermediate step.
…uwer Rollup of 16 pull requests Successful merges: - #159071 ([PAC] Encoder and hash (1/8)) - #161277 (bootstrap: Move all non-module items out of the crate root) - #161332 (Some `GlobalCtxt`/`Session` cleanups) - #161344 (Update the `rustc-perf` submodule) - #150931 (rustdoc: Always document `#[repr(transparent)]` if `#[rustc_pub_transparent]` is applied) - #160582 (Add `remove hidden_glob_reexports item breaks downstream` test) - #160876 (remove unwrap from write_mir_fn_graphviz) - #160927 (Enhance EII UI tests) - #161070 (fix arm homogeneous aggregate ABI) - #161236 (Download auto jobs in citool in parallel) - #161254 (Reserve capacity for 3% anon nodes) - #161283 (Tighten the language used for documenting `TargetOptions::llvm_abiname`) - #161291 (Rename `ProjectionPredicate` and `TraitPredicate`) - #161299 (Remove a bunch of unnecessary explicit lifetimes) - #161307 (make ARM maintainers pingable) - #161308 (Add regression test for rustc diagnostic to recognize variables in match guards)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #161356 (comment) |
rust-bors
Bot
added
S-waiting-on-author
|
This pull request was unapproved. This PR was contained in a rollup (#161356), which was unapproved. |
5 participants
After several rounds of preparation, this PR moves all non-module items out of bootstrap's crate root.
Having non-trivial code in the crate root is generally a bad idea. Anything defined or imported there is unconditionally visible throughout the entire crate, leading to messy imports and unclear abstraction boundaries, and causing friction when code needs to be moved elsewhere.
The commits have been structured to preserve as much line history as possible, and to minimize the number of other changes in the commit that performs the actual move, while remaining functional at every intermediate step.