Surface multiple "can't find crate" errors at a time

[cfredric]

It would be nice if rustc could surface multiple "can't find crate xxxx" errors at once, instead of one at a time.

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6a10dfbf5f9cc7dc6488fc9c3efaec70

extern crate asdf;
extern crate foobar;

fn main() {}

The current output is:

error[E0463]: can't find crate for `asdf`
 --> src/main.rs:2:1
  |
2 | extern crate asdf;
  | ^^^^^^^^^^^^^^^^^^ can't find crate

For more information about this error, try `rustc --explain E0463`.
error: could not compile `playground` due to previous error

Ideally the output should look like:

error[E0463]: can't find crate for `asdf`
 --> src/main.rs:2:1
  |
2 | extern crate asdf;
  | ^^^^^^^^^^^^^^^^^^ can't find crate

For more information about this error, try `rustc --explain E0463`.
error[E0463]: can't find crate for `foobar`
 --> src/main.rs:3:1
  |
3 | extern crate foobar;
  | ^^^^^^^^^^^^^^^^^^^^ can't find crate

For more information about this error, try `rustc --explain E0463`.
error: could not compile `playground` due to previous error

[jyn514]

I think this should be as simple as removing the abort_if_errors() call in the resolver:

rust/compiler/rustc_metadata/src/locator.rs

Line 1214 in 495322d

sess.abort_if_errors();

[jyn514]

You'll also have to find some reasonable behavior to do for the places currently calling report() and expecting it to never return:

../rust/compiler/rustc_metadata/src/creader.rs
519:            err.report(&self.sess, span, missing_core)

../rust/compiler/rustc_metadata/src/locator.rs
780:        Err(err) => err.report(sess, span, false),

@petrochenkov may have suggestions.

[Manishearth]

I think this should be as simple as removing the abort_if_errors() call in the resolver:

Not so sure, most such calls are introduced because it's tricky to have the compiler keep marching on after it already knows it's operating on bad code. For example, the error for an extern crate serdy typo would get drowned out by a million "cannot find serde::Serialize" errors.

It might be worth getting the resolver to continue looking for crates but nothing else (or something similar? idk).

cc @estebank who probably has thoughts on how to correctly get the compiler to keep marching on after seeing errors that are the upstream cause of a lot of other errors that would be emitted if it kept naïvely marching on. I know we do this well in a bunch of places.

[mcy]

At a minimum it would be ok to:

• Halt macro expansion and discard all macro nodes.
• Discard all type-defining items.
• Recurse only into items inside of function, static, const, and impl bodies.

Our only hope here is probably to search for further extern crates, and use statements that are rooted in crates we've managed to resolve... not sure about use crate:: in this scenario. At a minimum, being able to diagnose

extern crate serdy;
mod ohno {
  extern crate hypur;
}
const _: () = { extern crate aho_korasic; }

would be nice...

(I say this having ~little knowledge of the HIR.)

[mjptree]

@jyn514 I'm looking for a beginner friendly mentored issue to start. Is it okay to claim this one?

[jyn514]

@mjptree feel free to claim this :) but I'm not sure how easy it will be.

[mjptree]

@rustbot claim

[estebank]

In general, when we encounter an error in a type or during parsing we try and introduce a "poison pill" for its corresponding id (DefId, generally) or to the node being computed. This way we can check for these cases and silence errors. Otherwise rustc would complain about trait Foo is not implemented for [type error]. In the case of name resolution I'm pretty sure we already account for failed use, but we are likely not accounting for extern crate, simply because it's less prevalent since 2018 and the efforts to silence this errors (without aborting) happened after that edition landed.

So the file that you likely want to start looking at is https://github.com/rust-lang/rust/blob/9f6ca7482ca885bca14ba6870ed39a783ab939b5/compiler/rustc_resolve/src/lib.rs

On a quick glance, fn record_use shows some special casing that we perform for some prelude entries.

Then I would look at

rust/compiler/rustc_resolve/src/build_reduced_graph.rs

Line 685 in 9f6ca74

ItemKind::ExternCrate(orig_name) => {

particularly

rust/compiler/rustc_resolve/src/build_reduced_graph.rs

Lines 701 to 707 in 9f6ca74

	let crate_id = self.r.crate_loader.process_extern_crate(
	item,
	&self.r.definitions,
	local_def_id,
	);
	self.r.extern_crate_map.insert(local_def_id, crate_id);
	self.r.expect_module(crate_id.as_def_id())

digging into it brought me to

rust/compiler/rustc_metadata/src/creader.rs

Line 1014 in 2834f57

let cnum = self.resolve_crate(name, item.span, dep_kind);

which led me to

rust/compiler/rustc_metadata/src/creader.rs

Lines 510 to 522 in 2834f57

	fn resolve_crate<'b>(
	&'b mut self,
	name: Symbol,
	span: Span,
	dep_kind: CrateDepKind,
	) -> CrateNum {
	self.used_extern_options.insert(name);
	self.maybe_resolve_crate(name, dep_kind, None).unwrap_or_else(|err| {
	let missing_core =
	self.maybe_resolve_crate(sym::core, CrateDepKind::Explicit, None).is_err();
	err.report(&self.sess, span, missing_core)
	})
	}

which is where we are causing the error to be emitted today. We must instead report and continue. To do so, I would try and change the call to resolve_crate with a call to maybe_resolve_crate (provably move the self.used_extern_options.insert(name); to it as well), and create a new ModuleData for unresolved crates (look at what fn get_module is doing).

Ping me if you need more context!

[mjptree]

@estebank Thank you so much for the information! This is really helpful. I ran into some rather unexpected problems yesterday trying to set up my working environment and spent my evening puzzling over some spurious build errors on a freshly cloned repo. I blame it on me having already updated to Windows 11 for now and tackle it one issue at a time, beginning with this one that I committed to :).

I tried to start at

rust/compiler/rustc_metadata/src/locator.rs

Line 934 in d212d90

crate fn report(self, sess: &Session, span: Span, missing_core: bool) -> ! {

like jyn514 already mentioned, and walk backwards from there to all invocations, that could cause a CrateError::report and a subsequent abort.

Like you, I arrived also in

rust/compiler/rustc_metadata/src/creader.rs

Lines 510 to 522 in 2834f57

	fn resolve_crate<'b>(
	&'b mut self,
	name: Symbol,
	span: Span,
	dep_kind: CrateDepKind,
	) -> CrateNum {
	self.used_extern_options.insert(name);
	self.maybe_resolve_crate(name, dep_kind, None).unwrap_or_else(|err| {
	let missing_core =
	self.maybe_resolve_crate(sym::core, CrateDepKind::Explicit, None).is_err();
	err.report(&self.sess, span, missing_core)
	})
	}

which is where we are causing the error to be emitted today.

and also interestingly (or less so - can't judge at this point)

rust/compiler/rustc_metadata/src/locator.rs

Lines 811 to 822 in d212d90

	pub fn find_plugin_registrar(
	sess: &Session,
	metadata_loader: &dyn MetadataLoader,
	span: Span,
	name: Symbol,
	) -> PathBuf {
	match find_plugin_registrar_impl(sess, metadata_loader, name) {
	Ok(res) => res,
	// `core` is always available if we got as far as loading plugins.
	Err(err) => err.report(sess, span, false),
	}
	}

which is probably less relevant, because as far as I can tell

rust/compiler/rustc_plugin_impl/src/load.rs

Lines 27 to 61 in d212d90

	pub fn load_plugins(
	sess: &Session,
	metadata_loader: &dyn MetadataLoader,
	krate: &Crate,
	) -> Vec<PluginRegistrarFn> {
	let mut plugins = Vec::new();
	for attr in &krate.attrs {
	if !attr.has_name(sym::plugin) {
	continue;
	}
	for plugin in attr.meta_item_list().unwrap_or_default() {
	match plugin.ident() {
	Some(ident) if plugin.is_word() => {
	load_plugin(&mut plugins, sess, metadata_loader, ident)
	}
	_ => call_malformed_plugin_attribute(sess, plugin.span()),
	}
	}
	}
	plugins
	}
	fn load_plugin(
	plugins: &mut Vec<PluginRegistrarFn>,
	sess: &Session,
	metadata_loader: &dyn MetadataLoader,
	ident: Ident,
	) {
	let lib = locator::find_plugin_registrar(sess, metadata_loader, ident.span, ident.name);
	let fun = dylink_registrar(sess, ident.span, lib);
	plugins.push(fun);
	}

these functions are not called anywhere at the moment. Does "plugins" in this context refer to proc-macros?

I'll have a closer look at

rust/compiler/rustc_metadata/src/creader.rs

Line 1014 in 2834f57

let cnum = self.resolve_crate(name, item.span, dep_kind);

as you mentioned, as it looks the most promising.

It would be good to see an applied example of the "poison pill" mechanism that you mentioned, in a similar context. Also, I'm not familiar with this crate fn syntax. Is it equivalent to pub(crate) fn?

[estebank]

Plugins were the way you could modify how the compiler behaved before proc-macros were designed. They are still in the codebase, but they were never in a path to stabilization. They worked similarly in rustc, but had full access to the compiler internals, in contrast with proc-macros which only have access to the TokenStream.

By "poison pill" I mean something like Err in the following enum:

enum TyKind {
    Adt(Def, Substs),
    Str,
    Infer(InferTy),
    /// Placeholder used to silence errors
    Err,
}

crate fn is equivalent to pub(crate) fn, but the later works on all editions, while the former works on 2018 onwards. They mean the same.

[mjptree]

I think I'm coming closer to a solution: I was using these two examples as a starting point

1. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=65d3ed90603b0f36454b09b87e2e87e3
2. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=814b9863cc637350cda9de28b4fe7def

Because you already mentioned that in the case of failed use rustc already behaves as desired. I found that the two cases, diverge in

rust/compiler/rustc_interface/src/passes.rs

Lines 260 to 266 in 9dbbbb1

	pub fn configure_and_expand(
	sess: &Session,
	lint_store: &LintStore,
	mut krate: ast::Crate,
	crate_name: &str,
	resolver: &mut Resolver<'_>,
	) -> Result<ast::Crate> {

where extern crate items are resolved here

rust/compiler/rustc_interface/src/passes.rs

Lines 332 to 333 in 9dbbbb1

	// Expand macros now!
	let krate = sess.time("expand_crate", || ecx.monotonic_expander().expand_crate(krate));

whereas use is resolved afterwards here

rust/compiler/rustc_interface/src/passes.rs

Line 432 in 9dbbbb1

resolver.resolve_crate(&krate);

In the latter case, the ImportResolver is able to iterate over determined_imports and indeterminate_imports in ImportResolver::finalize_imports

rust/compiler/rustc_resolve/src/imports.rs

Line 661 in 82af160

pub fn finalize_imports(&mut self) {

where unresolved imports are imported as dummy bindings

rust/compiler/rustc_resolve/src/imports.rs

Lines 687 to 689 in 82af160

	// If the error is a single failed import then create a "fake" import
	// resolution for it so that later resolve stages won't complain.
	self.r.import_dummy_binding(import);

and only collectively thrown, when all imports have been attempted to be resolved. Maybe, if early extern crate resolution fails, I could also add a special dummy import, that gets reported together with failed uses.
One thing that doesn't fit into the picture yet are late extern crate resolutions

rust/compiler/rustc_resolve/src/late.rs

Line 2513 in 68568dc

pub(crate) fn late_resolve_crate(&mut self, krate: &Crate) {

but it doesn't really seem to do anything

rust/compiler/rustc_resolve/src/late.rs

Lines 1077 to 1079 in c8e9497

	ItemKind::ExternCrate(..) | ItemKind::MacroDef(..) => {
	// do nothing, these are just around to be encoded
	}

[petrochenkov]

If you make a PR addressing this please assign it to me.

[mjptree]

A first tentative solution on my fork.

It achieves the desired behaviour, but I'm still a bit reluctant to open a PR just yet:
a) because it feels a bit more like a hack than a solution, and
b) it also increases the verbosity on some other error messsages, specifically:

• [ui] ui\crate-loading\missing-std.rs
• [ui] ui\extern-flag\empty-extern-arg.rs
• [ui] ui\issues\issue-37131.rs
• [ui] ui\issues\issue-49851\compiler-builtins-error.rs

Maybe, if @estebank or @petrochenkov could judge?
If this is in fact sufficient, I would update the test outputs and add a new test then.

[petrochenkov]

I didn't read the thread in detail, but the right strategy is most likely to produce a binding with Res::Err resolution for an extern crate foo; import if foo is an unresolved crate.
We already do the same thing with unresolved use imports.

Then errors for all foo-relative paths like foo::a::b::c will be omitted because we already know that foo is a Res::Err.

[jyn514]

@mjptree if you'd like someone to review the code, please open a PR. You can always make further changes after review :)

[mjptree]

There are still some interplays between types, that I'm not sure I properly understand. My current approach to handling extern crate resolution in build_reduced_graph_for_item is

match self.r.crate_loader.process_extern_crate(
    item,
    &self.r.definitions,
    local_def_id,
) {
    Ok(crate_id) => {
        self.r.extern_crate_map.insert(local_def_id, crate_id);
        self.r.expect_module(crate_id.as_def_id())
    }
    Err(err) => {
        err.report(&self.r.session);
        let fake = self.r.arenas.alloc_import(Import {
            kind: ImportKind::ExternCrate { source: orig_name, target: ident },
            root_id: item.id,
            id: item.id,
            parent_scope: self.parent_scope,
            imported_module: Cell::new(None),
            has_attributes: !item.attrs.is_empty(),
            use_span_with_attributes: item.span_with_attributes(),
            use_span: item.span,
            root_span: item.span,
            span: item.span,
            module_path: Vec::new(),
            vis: Cell::new(vis),
            used: Cell::new(true),
        });
        self.r.import_dummy_binding(fake);
        return;
    }
}

If I try to cargo check the following example program

extern crate foo;
extern crate bar;

use bar::baz;
use boo::buzz;

fn main() {
    foo::fuzz();
    boo::buzz();
    bar::buzz();
    baz::buzz();
}

I receive errors for both failed resolutions of boo

error[E0463]: can't find crate for `foo`
 --> src\main.rs:1:1
  |
1 | extern crate foo;
  | ^^^^^^^^^^^^^^^^^ can't find crate

error[E0463]: can't find crate for `bar`
 --> src\main.rs:2:1
  |
2 | extern crate bar;
  | ^^^^^^^^^^^^^^^^^ can't find crate

error[E0432]: unresolved import `boo`
 --> src\main.rs:5:5
  |
5 | use boo::buzz;
  |     ^^^ use of undeclared crate or module `boo`

error[E0433]: failed to resolve: use of undeclared crate or module `boo`
 --> src\main.rs:9:5
  |
9 |     boo::buzz();
  |     ^^^ use of undeclared crate or module `boo`

error[E0433]: failed to resolve: use of undeclared crate or module `baz`
  --> src\main.rs:11:5
   |
11 |     baz::buzz();
   |     ^^^ use of undeclared crate or module `baz`

Some errors have detailed explanations: E0432, E0433, E0463.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `issue-90702-fix` due to 5 previous errors

I think although the use boo declaration has correctly errored, the boo::buzz() was not identify as a descendant of boo.

The takeaway for me is: It is not sufficient to define a dummy import for the crate names imported via extern crate (foo and bar in the above example) but define modules such that all paths, whose root is either foo or bar are correctly resolved.

I'm not sure which components are responsible for that. So far, my understanding is:

1. There are only two ways for crates to be added: extern crate and --extern. This means that a use declaration itself cannot introduce a new crate unless it is already known to the compiler.
2. use declarations initially create "empty" imports, not referencing any module. The actual refernced module containing all the symbols and symbol paths need to be resolved at later stages in ImportResolver::resolve_imports and ImportResolver::finalize_imports. If in the latter call, the import resolution still identifies no module, then resolution fails definitifely for that import.
3. extern crates are collected in CStore which maps CrateNums to CrateMetadata which in turn is a reference into the actual binary blob of an rmeta file. If I want to allocate a credible dummy module, I need to assign it a DefId. This DefId should ideally come from a CrateNum that I should have previously allocated with the CStore. I can do that, but if the CStore is then ever queried for the crate with this CrateNum, this is a problem.

Tl;dr: I am still not 100% what the characteristics of the ModuleData should be, so that later all name & import resolutions rooted in the failed extern crate fail smoothly without causing a panic! or an extra error message.

[petrochenkov]

The output in #90702 (comment) is exactly what I'd expect from using Res::Err for unresolved extern crate imports
Except for the "failed to resolve: use of undeclared crate or module baz" error, it's strange because baz should be declared as Res::Err.
This is already an improvement that can be submitted as a PR and merged.

Doing something to "deduplicate" the boo-related errors is an entirely independent task, I'm not even sure it's something that needs to be done.

[petrochenkov]

There are only two ways for crates to be added: extern crate and --extern. This means that a use declaration itself cannot introduce a new crate unless it is already known to the compiler.

--extern is nothing, it doesn't load a crate by itself.
It's an information that helps the compiler to find a crate if it's requested from any path like my_crate::zzz::yyy or ::my_crate::zzz::yyy (in import or elsewhere).

use declarations initially create "empty" imports...

This looks correct.

extern crates are collected in CStore which maps CrateNums to CrateMetadata which in turn is a reference into the actual binary blob of an rmeta file. If I want to allocate a credible dummy module, I need to assign it a DefId. This DefId should ideally come from a CrateNum that I should have previously allocated with the CStore. I can do that, but if the CStore is then ever queried for the crate with this CrateNum, this is a problem.

Creating a dummy CrateMetadata/CrateNum is hard and is an unreasonable cost for an issue like this.
Maybe keeping a set of Symbols with previously failed crate searches will help here.
We search a crate name in such set, and if it's found then we return a Res::Err without reporting an error (because this error was reported on a previous loading attempt).
But this is, again, a change that is independent from the Res::Err stuff, so I'd submit it later and separately.

[mjptree]

Here we go. My very first pull request to the Rust compiler...