make rustc_attr_parsing less dominant in the rustc crate graph
#140874
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
rustbot
added
A-attributes
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE machinery Some changes occurred to constck cc @fee1-dead Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in compiler/rustc_passes/src/check_attr.rs The Miri subtree was changed cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs Some changes occurred in compiler/rustc_codegen_ssa These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
rustbot
added
A-rustdoc-json
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -5,7 +5,7 @@ use rustc_middle::ty::Ty; | |||
| use rustc_middle::ty::layout::IntegerExt; | |||
| use rustc_middle::{bug, ty}; | |||
| use rustc_span::Span; | |||
| use {rustc_ast as ast, rustc_attr_parsing as attr, rustc_hir as hir}; | |||
| use {rustc_ast as ast, rustc_attr_data_structures as attrs, rustc_hir as hir}; | |||
There was a problem hiding this comment.
I think changing this back to attr would be nicer (and would make the diff smaller)
There was a problem hiding this comment.
There already is some rustc_attr_parsing as attr in the compiler, so using rustc_attr_data_structures as attr elsewhere feels wrong to me. Maybe we should rebind rustc_attr_parsing to something else?
Besides, rustc_attr_data_structures is where the attributes live, so keeping the plural sounds right to me. But I'm fine with changing it as long as things stay consistent.
There was a problem hiding this comment.
Note also that there are some modules named attr and imports like use rustc_ast::attr; in various crates, so a style choice to have rustc_attr_data_structures as attr will lead to conflicts as we migrate more attributes.
| @@ -40,7 +40,7 @@ use rustc_span::hygiene::{ExpnIndex, MacroKind, SyntaxContextKey}; | |||
| use rustc_span::{self, ExpnData, ExpnHash, ExpnId, Ident, Span, Symbol}; | |||
| use rustc_target::spec::{PanicStrategy, TargetTuple}; | |||
| use table::TableBuilder; | |||
| use {rustc_ast as ast, rustc_attr_parsing as attr, rustc_hir as hir}; | |||
| use {rustc_ast as ast, rustc_attr_data_structures as attrs, rustc_hir as hir}; | |||
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| use rustc_abi::ExternAbi; | |||
| use rustc_ast::ast; | |||
| use rustc_attr_parsing::DeprecatedSince; | |||
| use rustc_attr_data_structures::{self as attrs, DeprecatedSince}; | |||
There was a problem hiding this comment.
Any reason for the rename here?
There was a problem hiding this comment.
If I change
rust/src/librustdoc/json/conversions.rs
Line 156 in 9a7e19f
to
pub(crate) fn from_deprecation(deprecation: rustc_attr_data_structures::Deprecation) -> Deprecation {Then rustfmt will turn that into
pub(crate) fn from_deprecation(
deprecation: rustc_attr_data_structures::Deprecation,
) -> Deprecation {
}And I can't import rustc_attr_data_structures::Deprecation because that conflicts with rustdoc's Deprecation.
IMO as attrs makes for the nicest diff.
It has/had a glob re-export of
rustc_attr_data_structures, which is a crate much lower in the graph, and a lot of crates were using it just (or mostly) for that re-export, while they can rely onrustc_attr_data_structuresdirectly.Previous graph:
Graph with this PR:
The first commit keeps the re-export, and just changes the dependency if possible. The second commit is the "breaking change" which removes the re-export, and "explicitly" adds the
rustc_attr_data_structuresdependency where needed. It also switches over some src/tools/*.The second commit is actually a lot more involved than I expected. Please let me know if it's a better idea to back it out and just keep the first commit.