Restrict sysroot crate imports to those defined in this repo. #143548

Diggsey · 2025-07-06T22:06:11Z

It's common to import dependencies from the sysroot via extern crate rather than use an explicit cargo dependency, when it's necessary to use the same dependency version as used by rustc itself. However, this is dangerous for crates.io crates, since rustc may not pull in the dependency on some targets, or may pull in multiple versions. In both cases, the extern crate fails to resolve.

To address this, re-export all such dependencies from the appropriate rustc_* crates, and use this alias from crates which would otherwise need to use extern crate.

See #143492 for an example of the kind of issue that can occur.

rustbot · 2025-07-06T22:06:16Z

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-06T22:06:21Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in compiler/rustc_sanitizers

cc @rcvalle

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

ytmimi · 2025-07-06T22:23:54Z

src/tools/rustfmt/Cargo.toml

-tracing = { version = "0.1.37", default-features = false, features = ["std"] }
+tracing = { workspace = true }


I don't think this is a valid change for rustfmt. Although rustfmt is part of the r-l/rust workspace, it's independently developed in https://github.com/rust-lang/rustfmt, and I'm pretty sure this would prevent us from build from source when working on the git subtree, which isn't structured as a cargo workspace. I'd like to opt out of all rustfmt changes from this PR, but feel free to open a new PR directly to https://github.com/rust-lang/rustfmt

Ah, will update 👍

@ytmimi is it sufficient to revert the Cargo.toml changes, or do the other changes need to go via the other repo as well?

Also, there are some other workspace members that are also subtrees, like clippy, and miri. I didn't look at any tools besides rustfmt, but it's possible maintainers of those tools would prefer PRs be opened up directly to their respective repos as well.

@ytmimi happy to split if needed but the wording you linked says:

The exception is that when rustc changes are required to implement a new tool feature or test, that should happen in one collective rustc PR.

@ytmimi is it sufficient to revert the Cargo.toml changes, or do the other changes need to go via the other repo as well?

I'd rather that all changes were reverted, thanks!

Using workspace dependencies for the Cargo.toml files in compiler (except in codegen_gcc and codegen_clf) is entirely fine, and a good idea for the reasons you mention. Only the subtree crates like Miri, clippy, rustfmt cannot use workspace dependencies.

The root problem here is not the use of workspace dependencies, the problem is that rustfmt has tracing in its Cargo.toml, which it shouldn't. It should use rustc's tracing instead.

Ah, turns out workspace dependencies do not work since the workspace Cargo.toml is not distributed in the rustc-dev package... see Zulip.

It's common to import dependencies from the sysroot via `extern crate` rather than use an explicit cargo dependency, when it's necessary to use the same dependency version as used by rustc itself. However, this is dangerous for crates.io crates, since rustc may not pull in the dependency on some targets, or may pull in multiple versions. In both cases, the `extern crate` fails to resolve. To address this, re-export all such dependencies from the appropriate `rustc_*` crates, and use this alias from crates which would otherwise need to use `extern crate`.

rustbot · 2025-07-06T22:54:55Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rust-log-analyzer · 2025-07-07T00:00:00Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

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

 use rustc_span::{Span, sym};
-use rustc_data_structures::thin_vec::ThinVec;
 
 use crate::Input;
 use crate::parse::session::ParseSess;
{ "type": "test", "name": "test::self_tests", "event": "failed", "stdout": "Ran 8 self tests.\n\nthread 'test::self_tests' panicked at src/tools/rustfmt/src/test/mod.rs:420:5:\nassertion `left == right` failed: 1 self tests failed\n  left: 1\n right: 0\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
test test::system_tests ... ok
test test::idempotence_tests ... ok

test result: FAILED. 181 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.19s

antoyo · 2025-07-07T00:06:43Z

compiler/rustc_codegen_gcc/src/lib.rs

-// These crates are pulled from the sysroot because they are part of
-// rustc's public API, so we need to ensure version compatibility.


Is this version compatibility still a concern?
I'm not sure what it's referring to, but I just want to make sure that removing this is not going to break something else.

I added this comment in a previous PR as essentially a TODO for this PR. Now that crates are re-exported there's no need to pull them by name from the sysroot in order to ensure the versions match.

RalfJung · 2025-07-07T05:35:54Z

src/librustdoc/html/markdown.rs

 use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
 use rustc_errors::{Diag, DiagMessage};
 use rustc_hir::def_id::LocalDefId;
 use rustc_middle::ty::TyCtxt;
 pub(crate) use rustc_resolve::rustdoc::main_body_opts;
 use rustc_resolve::rustdoc::may_be_doc_link;
+use rustc_resolve::rustdoc::pulldown_cmark::{
+    self, BrokenLink, CodeBlockKind, CowStr, Event, LinkType, Options, Parser, Tag, TagEnd, html,
+};
 use rustc_span::edition::Edition;
 use rustc_span::{Span, Symbol};
 use tracing::{debug, trace};


There are still tracing imports all over rustdoc... and indeed, tracing is oddly a cargo dependency here. That seems wrong? Tracing has a global singleton so rustc drivers should use the version from the sysroot, not their own, to ensure there's one shared logging system across the rustc core + driver.

Probably best to do that in a separate PR, but maybe add a FIXME in the Cargo.toml to at least document this?

Rustdoc intentionally doesn't use rustc's copy of tracing to be able to enable trace logs for rustdoc without having to recompile rustc:

rust/src/librustdoc/lib.rs

Lines 165 to 170 in c720f49

// When using CI artifacts with `download-rustc`, tracing is unconditionally built

// with `--features=static_max_level_info`, which disables almost all rustdoc logging. To avoid

// this, compile our own version of `tracing` that logs all levels.

// NOTE: this compiles both versions of tracing unconditionally, because

// - The compile time hit is not that bad, especially compared to rustdoc's incremental times, and

// - Otherwise, there's no warning that logging is being ignored when `download-rustc` is enabled

Ah, interesting. In Miri we use higher log levels to avoid that, since having two entirely separate loggers became quite annoying, but maybe that's less of a problem for rustdoc.

RalfJung

The Miri changes LGTM, except the ones in the vendored file where only the header at the top should be edited.

I'm fine with having these Miri changes here, seems easier to do this centrally than coordinate half a dozen PRs.

RalfJung · 2025-07-07T05:37:28Z

src/tools/miri/src/bin/log/tracing_chrome.rs

This file was deliberately not formatted since it is a copy from another repo. Please undo all the formatting here.

Ah I wondered why these formatting changes appeared!

RalfJung · 2025-07-07T05:42:19Z

src/tools/miri/src/bin/log/tracing_chrome.rs

@@ -613,10 +571,9 @@ struct JsonVisitor<'a> {
    object: &'a mut Object,
 }

-impl<'a> tracing_subscriber::field::Visit for JsonVisitor<'a> {
+impl<'a> rustc_log::tracing_subscriber::field::Visit for JsonVisitor<'a> {


To avoid diff in the core of the file, please just import these modules at the top.

oli-obk · 2025-07-07T08:57:15Z

Do you think it is feasible to add an internal lint that rejects extern crate items where the crate name doesn't start with rustc_ or sth like that?

Diggsey · 2025-07-07T09:17:19Z

@oli-obk I'll look into this. How does testing work for sub-trees? Would each sub-tree need its own copy of the lint, or do the subtree repos have some way to run rust-wide lints?

RalfJung · 2025-07-07T09:23:45Z

"internal" lints live in rustc like normal lints, but are gated behind a nightly-only flag. Miri enables them in their CI, I don't know if ./x check miri also enables them.

oli-obk · 2025-07-07T09:25:17Z

We have a list of internal lints that rustc has and enables everywhere. We'd have to limit it to crates with #![allow(internal_features)] or sth like it. Most other lints are obviously limited to that as they look for e.g. TyKind .

RalfJung · 2025-07-07T09:27:51Z

Not sure what you mean about restricting it, the lints need -Zunstable-options -Wrustc::internal to be enabled, right?

oli-obk · 2025-07-07T09:33:18Z

Ah right, my bad

fbstj · 2025-07-07T11:11:01Z

compiler/rustc_data_structures/src/lib.rs

@@ -43,6 +43,7 @@ use std::fmt;
 pub use atomic_ref::AtomicRef;
 pub use ena::{snapshot_vec, undo_log, unify};
 pub use rustc_index::static_assert_size;
+pub use {either, indexmap, smallvec, thin_vec};


Suggested change

pub use {either, indexmap, smallvec, thin_vec};

// Re-export some data-structure crates

pub use {either, indexmap, smallvec, thin_vec};

or something like the comment on tracing?

rustbot assigned oli-obk Jul 6, 2025

ytmimi reviewed Jul 6, 2025

View reviewed changes

Diggsey force-pushed the db-limit-extern-crate-usage branch from 8e8dd82 to c613bce Compare July 6, 2025 22:54

antoyo reviewed Jul 7, 2025

View reviewed changes

RalfJung reviewed Jul 7, 2025

View reviewed changes

fbstj reviewed Jul 7, 2025

View reviewed changes

		tracing = { version = "0.1.37", default-features = false, features = ["std"] }
		tracing = { workspace = true }

		// These crates are pulled from the sysroot because they are part of
		// rustc's public API, so we need to ensure version compatibility.

	// When using CI artifacts with `download-rustc`, tracing is unconditionally built
	// with `--features=static_max_level_info`, which disables almost all rustdoc logging. To avoid
	// this, compile our own version of `tracing` that logs all levels.
	// NOTE: this compiles both versions of tracing unconditionally, because
	// - The compile time hit is not that bad, especially compared to rustdoc's incremental times, and
	// - Otherwise, there's no warning that logging is being ignored when `download-rustc` is enabled

	pub use {either, indexmap, smallvec, thin_vec};
	// Re-export some data-structure crates
	pub use {either, indexmap, smallvec, thin_vec};

Restrict sysroot crate imports to those defined in this repo. #143548

Are you sure you want to change the base?

Restrict sysroot crate imports to those defined in this repo. #143548

Conversation

Diggsey commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 6, 2025

Uh oh!

rustbot commented Jul 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diggsey Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jul 6, 2025

Uh oh!

rust-log-analyzer commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jul 7, 2025

Uh oh!

Diggsey commented Jul 7, 2025

Uh oh!

RalfJung commented Jul 7, 2025

Uh oh!

oli-obk commented Jul 7, 2025

Uh oh!

RalfJung commented Jul 7, 2025

Uh oh!

oli-obk commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Diggsey commented Jul 6, 2025 •

edited

Loading

Diggsey Jul 6, 2025 •

edited

Loading

RalfJung Jul 7, 2025 •

edited

Loading

RalfJung Jul 7, 2025 •

edited

Loading

RalfJung left a comment •

edited

Loading