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

Exclude dependencies of std for diagnostics #135278

Merged
merged 5 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// all visible traits. If there's one clear winner, just suggest that.

let visible_traits: Vec<_> = tcx
.all_traits()
.visible_traits()
.filter(|trait_def_id| {
let viz = tcx.visibility(*trait_def_id);
let def_id = self.item_def_id();
Expand Down
105 changes: 71 additions & 34 deletions compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use rustc_session::lint::{self, BuiltinLintDiag};
use rustc_session::output::validate_crate_name;
use rustc_session::search_paths::PathKind;
use rustc_span::edition::Edition;
use rustc_span::{DUMMY_SP, Ident, Span, Symbol, sym};
use rustc_span::{DUMMY_SP, Ident, STDLIB_STABLE_CRATES, Span, Symbol, sym};
use rustc_target::spec::{PanicStrategy, Target, TargetTuple};
use tracing::{debug, info, trace};

Expand Down Expand Up @@ -390,19 +390,51 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
None
}

// The `dependency` type is determined by the command line arguments(`--extern`) and
// `private_dep`. However, sometimes the directly dependent crate is not specified by
// `--extern`, in this case, `private-dep` is none during loading. This is equivalent to the
// scenario where the command parameter is set to `public-dependency`
fn is_private_dep(&self, name: &str, private_dep: Option<bool>) -> bool {
self.sess.opts.externs.get(name).map_or(private_dep.unwrap_or(false), |e| e.is_private_dep)
&& private_dep.unwrap_or(true)
/// Determine whether a dependency should be considered private.
///
/// Dependencies are private if they get extern option specified, e.g. `--extern priv:mycrate`.
/// This is stored in metadata, so `private_dep` can be correctly set during load. A `Some`
/// value for `private_dep` indicates that the crate is known to be private or public (note
/// that any `None` or `Some(false)` use of the same crate will make it public).
///
/// Sometimes the directly dependent crate is not specified by `--extern`, in this case,
/// `private-dep` is none during loading. This is equivalent to the scenario where the
/// command parameter is set to `public-dependency`
fn is_private_dep(
&self,
name: Symbol,
private_dep: Option<bool>,
dep_root: Option<&CratePaths>,
) -> bool {
// Standard library crates are never private.
if STDLIB_STABLE_CRATES.contains(&name) {
tracing::info!("returning false for {name} is private");
return false;
}

let extern_private = self.sess.opts.externs.get(name.as_str()).map(|e| e.is_private_dep);

// Any descendants of `std` should be private. These crates are usually not marked
// private in metadata, so we ignore that field.
if extern_private.is_none()
&& dep_root.map_or(false, |d| STDLIB_STABLE_CRATES.contains(&d.name))
{
return true;
}

match (extern_private, private_dep) {
// Explicit non-private via `--extern`, explicit non-private from metadata, or
// unspecified with default to public.
(Some(false), _) | (_, Some(false)) | (None, None) => false,
// Marked private via `--extern priv:mycrate` or in metadata.
(Some(true) | None, Some(true) | None) => true,
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess something like this looks better

if extern_private.is_none() && dep_root.is_some_and(|d| STDLIB_STABLE_CRATES.contains(&d.name))
{
    return true;
}

match (extern_private, private_dep) {
    // Explicit non-private or default to public
    (Some(false), _) | (_, Some(false)) | (None, None) => false,
    // Explicit private
    _ => true,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, applied (filled out _ since there were only two cases left)

}

fn register_crate(
&mut self,
host_lib: Option<Library>,
root: Option<&CratePaths>,
dep_root: Option<&CratePaths>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the point of replacing root with def_root here, it might be simpler to keep the previous one

Copy link
Contributor Author

@tgross35 tgross35 Jan 14, 2025

Choose a reason for hiding this comment

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

The changes to this file are somewhat unrelated now, they were more relevant for the initial implementation. But I still think this makes sense, even if off topic - before this change, root could sometimes refer to a CrateRoot and sometimes to the dependency root's CratePaths (like here), it was pretty confusing to track through the call stack because the meaning switched back and forth.

So this patch just makes the same name always refer to the same thing, root = any CrateRoot and dep_root = always the crate at the root of the dependency tree.

Copy link
Member

Choose a reason for hiding this comment

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

That's OK

lib: Library,
dep_kind: CrateDepKind,
name: Symbol,
Expand All @@ -414,7 +446,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
let Library { source, metadata } = lib;
let crate_root = metadata.get_root();
let host_hash = host_lib.as_ref().map(|lib| lib.metadata.get_root().hash());
let private_dep = self.is_private_dep(name.as_str(), private_dep);
let private_dep = self.is_private_dep(name, private_dep, dep_root);

// Claim this crate number and cache it
let feed = self.cstore.intern_stable_crate_id(&crate_root, self.tcx)?;
Expand All @@ -430,14 +462,14 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
// Maintain a reference to the top most crate.
// Stash paths for top-most crate locally if necessary.
let crate_paths;
let root = if let Some(root) = root {
root
let dep_root = if let Some(dep_root) = dep_root {
dep_root
} else {
crate_paths = CratePaths::new(crate_root.name(), source.clone());
&crate_paths
};

let cnum_map = self.resolve_crate_deps(root, &crate_root, &metadata, cnum, dep_kind)?;
let cnum_map = self.resolve_crate_deps(dep_root, &crate_root, &metadata, cnum, dep_kind)?;

let raw_proc_macros = if crate_root.is_proc_macro_crate() {
let temp_root;
Expand Down Expand Up @@ -559,23 +591,21 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
&'b mut self,
name: Symbol,
mut dep_kind: CrateDepKind,
dep: Option<(&'b CratePaths, &'b CrateDep)>,
dep_of: Option<(&'b CratePaths, &'b CrateDep)>,
) -> Result<CrateNum, CrateError> {
info!("resolving crate `{}`", name);
if !name.as_str().is_ascii() {
return Err(CrateError::NonAsciiName(name));
}
let (root, hash, host_hash, extra_filename, path_kind, private_dep) = match dep {
Some((root, dep)) => (
Some(root),
Some(dep.hash),
dep.host_hash,
Some(&dep.extra_filename[..]),
PathKind::Dependency,
Some(dep.is_private),
),
None => (None, None, None, None, PathKind::Crate, None),
};

let dep_root = dep_of.map(|d| d.0);
let dep = dep_of.map(|d| d.1);
let hash = dep.map(|d| d.hash);
let host_hash = dep.map(|d| d.host_hash).flatten();
let extra_filename = dep.map(|d| &d.extra_filename[..]);
let path_kind = if dep.is_some() { PathKind::Dependency } else { PathKind::Crate };
let private_dep = dep.map(|d| d.is_private);

let result = if let Some(cnum) = self.existing_match(name, hash, path_kind) {
(LoadResult::Previous(cnum), None)
} else {
Expand All @@ -599,7 +629,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
dep_kind = CrateDepKind::MacrosOnly;
match self.load_proc_macro(&mut locator, path_kind, host_hash)? {
Some(res) => res,
None => return Err(locator.into_error(root.cloned())),
None => return Err(locator.into_error(dep_root.cloned())),
}
}
}
Expand All @@ -612,7 +642,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
// not specified by `--extern` on command line parameters, it may be
// `private-dependency` when `register_crate` is called for the first time. Then it must be updated to
// `public-dependency` here.
let private_dep = self.is_private_dep(name.as_str(), private_dep);
let private_dep = self.is_private_dep(name, private_dep, dep_root);
let data = self.cstore.get_crate_data_mut(cnum);
if data.is_proc_macro_crate() {
dep_kind = CrateDepKind::MacrosOnly;
Expand All @@ -623,7 +653,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
}
(LoadResult::Loaded(library), host_library) => {
info!("register newly loaded library for `{}`", name);
self.register_crate(host_library, root, library, dep_kind, name, private_dep)
self.register_crate(host_library, dep_root, library, dep_kind, name, private_dep)
}
_ => panic!(),
}
Expand Down Expand Up @@ -663,16 +693,20 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
}))
}

// Go through the crate metadata and load any crates that it references
/// Go through the crate metadata and load any crates that it references.
fn resolve_crate_deps(
&mut self,
root: &CratePaths,
dep_root: &CratePaths,
crate_root: &CrateRoot,
metadata: &MetadataBlob,
krate: CrateNum,
dep_kind: CrateDepKind,
) -> Result<CrateNumMap, CrateError> {
debug!("resolving deps of external crate");
debug!(
"resolving deps of external crate `{}` with dep root `{}`",
crate_root.name(),
dep_root.name
);
if crate_root.is_proc_macro_crate() {
return Ok(CrateNumMap::new());
}
Expand All @@ -685,14 +719,17 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
crate_num_map.push(krate);
for dep in deps {
info!(
"resolving dep crate {} hash: `{}` extra filename: `{}`",
dep.name, dep.hash, dep.extra_filename
"resolving dep `{}`->`{}` hash: `{}` extra filename: `{}`",
crate_root.name(),
dep.name,
dep.hash,
dep.extra_filename
);
let dep_kind = match dep_kind {
CrateDepKind::MacrosOnly => CrateDepKind::MacrosOnly,
_ => dep.kind,
};
let cnum = self.maybe_resolve_crate(dep.name, dep_kind, Some((root, &dep)))?;
let cnum = self.maybe_resolve_crate(dep.name, dep_kind, Some((dep_root, &dep)))?;
crate_num_map.push(cnum);
}

Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_metadata/src/locator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ pub(crate) struct CrateLocator<'a> {

#[derive(Clone)]
pub(crate) struct CratePaths {
name: Symbol,
pub(crate) name: Symbol,
source: CrateSource,
}

Expand Down Expand Up @@ -765,10 +765,10 @@ impl<'a> CrateLocator<'a> {
self.extract_lib(rlibs, rmetas, dylibs).map(|opt| opt.map(|(_, lib)| lib))
}

pub(crate) fn into_error(self, root: Option<CratePaths>) -> CrateError {
pub(crate) fn into_error(self, dep_root: Option<CratePaths>) -> CrateError {
CrateError::LocatorCombined(Box::new(CombinedLocatorError {
crate_name: self.crate_name,
root,
dep_root,
triple: self.tuple,
dll_prefix: self.target.dll_prefix.to_string(),
dll_suffix: self.target.dll_suffix.to_string(),
Expand Down Expand Up @@ -914,7 +914,7 @@ struct CrateRejections {
/// otherwise they are ignored.
pub(crate) struct CombinedLocatorError {
crate_name: Symbol,
root: Option<CratePaths>,
dep_root: Option<CratePaths>,
triple: TargetTuple,
dll_prefix: String,
dll_suffix: String,
Expand Down Expand Up @@ -987,7 +987,7 @@ impl CrateError {
}
CrateError::LocatorCombined(locator) => {
let crate_name = locator.crate_name;
let add_info = match &locator.root {
let add_info = match &locator.dep_root {
None => String::new(),
Some(r) => format!(" which `{}` depends on", r.name),
};
Expand All @@ -1012,7 +1012,7 @@ impl CrateError {
path.display()
));
}
if let Some(r) = locator.root {
if let Some(r) = locator.dep_root {
for path in r.source.paths() {
found_crates.push_str(&format!(
"\ncrate `{}`: {}",
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2128,6 +2128,8 @@ rustc_queries! {
eval_always
desc { "calculating the stability index for the local crate" }
}
/// All available crates in the graph, including those that should not be user-facing
/// (such as private crates).
query crates(_: ()) -> &'tcx [CrateNum] {
eval_always
desc { "fetching all foreign CrateNum instances" }
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2078,12 +2078,23 @@ impl<'tcx> TyCtxt<'tcx> {
self.limits(()).move_size_limit
}

/// All traits in the crate graph, including those not visible to the user.
pub fn all_traits(self) -> impl Iterator<Item = DefId> + 'tcx {
iter::once(LOCAL_CRATE)
.chain(self.crates(()).iter().copied())
.flat_map(move |cnum| self.traits(cnum).iter().copied())
}

/// All traits that are visible within the crate graph (i.e. excluding private dependencies).
pub fn visible_traits(self) -> impl Iterator<Item = DefId> + 'tcx {
let visible_crates =
self.crates(()).iter().copied().filter(move |cnum| self.is_user_visible_dep(*cnum));

iter::once(LOCAL_CRATE)
.chain(visible_crates)
.flat_map(move |cnum| self.traits(cnum).iter().copied())
}

#[inline]
pub fn local_visibility(self, def_id: LocalDefId) -> Visibility {
self.visibility(def_id).expect_local()
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,11 @@ impl<'tcx> TyCtxt<'tcx> {
/// [public]: TyCtxt::is_private_dep
/// [direct]: rustc_session::cstore::ExternCrate::is_direct
pub fn is_user_visible_dep(self, key: CrateNum) -> bool {
// `#![rustc_private]` overrides defaults to make private dependencies usable.
if self.features().enabled(sym::rustc_private) {
Copy link
Member

Choose a reason for hiding this comment

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

However, this is a very internal feature, so it doesn't seem worth doing any additional filtering here.

Not really. There are a lot of external custom drivers that need to enable this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting that it is worth filtering here to only expose crates that are dependencies of std? Or just that the message is inaccurate.

Copy link
Member

Choose a reason for hiding this comment

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

I think the override for rustc_private should be removed. Even with rustc_private enabled you generally shouldn't do something like extern crate libc; from the sysroot.

return true;
}

// | Private | Direct | Visible | |
// |---------|--------|---------|--------------------|
// | Yes | Yes | Yes | !true || true |
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_passes/src/diagnostic_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,14 @@ fn all_diagnostic_items(tcx: TyCtxt<'_>, (): ()) -> DiagnosticItems {
// Initialize the collector.
let mut items = DiagnosticItems::default();

// Collect diagnostic items in other crates.
for &cnum in tcx.crates(()).iter().chain(std::iter::once(&LOCAL_CRATE)) {
// Collect diagnostic items in visible crates.
for cnum in tcx
.crates(())
.iter()
.copied()
.filter(|cnum| tcx.is_user_visible_dep(*cnum))
.chain(std::iter::once(LOCAL_CRATE))
{
for (&name, &def_id) in &tcx.diagnostic_items(cnum).name_to_id {
collect_item(tcx, &mut items, name, def_id);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2189,7 +2189,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
let required_trait_path = self.tcx.def_path_str(trait_ref.def_id());
let traits_with_same_path: UnordSet<_> = self
.tcx
.all_traits()
.visible_traits()
.filter(|trait_def_id| *trait_def_id != trait_ref.def_id())
.map(|trait_def_id| (self.tcx.def_path_str(trait_def_id), trait_def_id))
.filter(|(p, _)| *p == required_trait_path)
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/privacy/sysroot-private.default.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
error[E0405]: cannot find trait `Equivalent` in this scope
--> $DIR/sysroot-private.rs:26:18
|
LL | trait Trait2<K>: Equivalent<K> {}
| ^^^^^^^^^^ not found in this scope

error[E0412]: cannot find type `K` in this scope
--> $DIR/sysroot-private.rs:31:35
|
LL | fn trait_member<T>(val: &T, key: &K) -> bool {
| - ^
| |
| similarly named type parameter `T` defined here
|
help: a type parameter with a similar name exists
|
LL | fn trait_member<T>(val: &T, key: &T) -> bool {
| ~
help: you might be missing a type parameter
|
LL | fn trait_member<T, K>(val: &T, key: &K) -> bool {
| +++

error[E0220]: associated type `ExpressionStack` not found for `Trait`
--> $DIR/sysroot-private.rs:21:31
|
LL | type AssociatedTy = dyn Trait<ExpressionStack = i32, Bar = i32>;
| ^^^^^^^^^^^^^^^ help: `Trait` has the following associated type: `Bar`

error[E0425]: cannot find function `memchr2` in this scope
--> $DIR/sysroot-private.rs:39:5
|
LL | memchr2(b'a', b'b', buf)
| ^^^^^^^ not found in this scope

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0220, E0405, E0412, E0425.
For more information about an error, try `rustc --explain E0220`.
Loading
Loading