suggestions from other crates only work when an explicit extern crate is present

[nikomatsakis]

UPDATE: There has been some progress on this issue. crate:: prefixes now work, but you still don't get suggestions from other crates except when you write an explicit extern crate.

When you get name resolution suggestions from resolve after an error, they should use crate:: prefixes where appropriate.

Example (play):

#![feature(crate_visibility_modifier)]
#![feature(crate_in_paths)]

mod bar {
    crate struct Foo;
}

fn main() {
    Foo;
}

gives

error[E0425]: cannot find value `Foo` in this scope
 --> src/main.rs:9:5
  |
9 |     Foo;
  |     ^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
  |
4 | use bar::Foo;
  |

but I want to see use crate::bar::Foo suggested.

[nikomatsakis]

It looks like the suggestions are issued here:

rust/src/librustc_resolve/lib.rs

Lines 4436 to 4463 in c131bdc

	let better = if better { "better " } else { "" };
	let msg_diff = match path_strings.len() {
	1 => " is found in another module, you can import it",
	_ => "s are found in other modules, you can import them",
	};
	let msg = format!("possible {}candidate{} into scope", better, msg_diff);
	if let Some(span) = span {
	for candidate in &mut path_strings {
	// produce an additional newline to separate the new use statement
	// from the directly following item.
	let additional_newline = if found_use {
	""
	} else {
	"\n"
	};
	*candidate = format!("use {};\n{}", candidate, additional_newline);
	}
	err.span_suggestions(span, &msg, path_strings);
	} else {
	let mut msg = msg;
	msg.push(':');
	for candidate in path_strings {
	msg.push('\n');
	msg.push_str(&candidate);
	}
	}

The behavior I think we want is this:

If the crate_in_paths feature is enabled, then we should suggest use crate::bar::Foo when bar is from a module and not an external crate.

The function that prepares those suggestions appears to be this one:

rust/src/librustc_resolve/lib.rs

Lines 4398 to 4402 in c131bdc

	fn path_names_to_string(path: &Path) -> String {
	names_to_string(&path.segments.iter()
	.map(|seg| seg.ident)
	.collect::<Vec<_>>())
	}

Here is an example of checking for whether the feature is enabled:

rust/src/librustc_resolve/lib.rs

Line 3493 in c131bdc

if !self.session.features_untracked().crate_in_paths {

[nikomatsakis]

@qmx expressed some interest in tackling this.

[nikomatsakis]

Hmm, now i'm worried that this will apply even when it shouldn't. Consider this testcase:

fn main() {
    let x: &Debug = &22;
}

This currently prints:

error[E0412]: cannot find type `Debug` in this scope
 --> src/main.rs:2:13
  |
2 |     let x: &Debug = &22;
  |             ^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
  |
1 | use std::fmt::Debug;
  |

and I think we do not want it to print use crate::std::fmt::Debug.

I think this suggests an alternative fix to what I recommended before (and what @qmx did in #51456) -- but I'd like to get @petrochenkov's opinion. Before I suggested changing how the ImportSuggestion values are printed out, but now I think we should change them at the source:

rust/src/librustc_resolve/lib.rs

Lines 4003 to 4015 in a32e979

	/// When name resolution fails, this method can be used to look up candidate
	/// entities with the expected name. It allows filtering them using the
	/// supplied predicate (which should be used to only accept the types of
	/// definitions expected e.g. traits). The lookup spans across all crates.
	///
	/// NOTE: The method does not look into imports, but this is not a problem,
	/// since we report the definitions (thus, the de-aliased imports).
	fn lookup_import_candidates<FilterFn>(&mut self,
	lookup_name: Name,
	namespace: Namespace,
	filter_fn: FilterFn)
	-> Vec<ImportSuggestion>
	where FilterFn: Fn(Def) -> bool

Right now, this works by walking through the crate and finding candidates. I suspect (but we should test) that this means it will not work when the extern_prelude option is enabled and no explicit extern crate exists, because there is nothing to walk over. Therefore, the change I propose is as follows:

1. Modify the function to (if crate_in_paths is enabled) to push a crate at the front of the import suggestions it is currently creating. This is because all of them are found by trawling the crate source.
2. Then, if extern_prelude is enabled, add a loop that executes first. This would go over all of the crates that are available from the command line, load them up, and search for the item within (basically the same as the existing search, but beginning from the crate directly). In this case, we would not prepend crate:: to the resulting path, but instead the name of the crate we are searching.

• One flaw I can see here is that it may be that the crate is locally shadowed. I suspect we can overlook that possibility for now.

[nikomatsakis]

@petrochenkov do the above notes seem ok?

[petrochenkov]

Looks reasonable.

[nikomatsakis]

To get a bit more detailed in how to make this modification, ultimately I envision we'll have a helper like this:

fn lookup_import_candidates_from_module(
    &mut self, 
    lookup_name: Name, 
    namespace: Namespace, 
    start_module: Module<'_>,
    filter_fn: impl Fn(Def) -> bool,
) -> Vec<ImportSuggestion> {
    ...
}

This will be roughly the same as the existing lookup_import_candidates, except that instead of starting from self.graph_root, it will seed the worklist with start_module:

rust/src/librustc_resolve/lib.rs

Line 4020 in a32e979

worklist.push((self.graph_root, Vec::new(), false));

Then, later, when it finds a suggestion:

rust/src/librustc_resolve/lib.rs

Line 4053 in a32e979

candidates.push(ImportSuggestion { path: path });

It will check if start_module == self.graph_root and the crate_in_paths feature is enabled. In that case, it adds crate:: to the start of the suggestion.

This is step 1. Step 2 in a second. =)

[nikomatsakis]

OK, so step 2...

The basic idea was to iterate over each of the "available" crates. For each crate, we would load the crate (if not already loaded), and then use the lookup_import_candidates_with_module fn to walk and make a suggestion.

So the first question is where we find that list of available crates and how to load it. Here are some pointers. For example, here is where we find the set of things in the prelude:

rust/src/librustc_resolve/lib.rs

Lines 1627 to 1628 in fefe816

	let mut extern_prelude: FxHashSet<Name> =
	session.opts.externs.iter().map(|kv| Symbol::intern(kv.0)).collect();

which ends up in the Resolver struct. And this is where we resolve paths that rely on external prelude:

rust/src/librustc_resolve/lib.rs

Lines 1928 to 1929 in fefe816

	// `record_used` means that we don't try to load crates during speculative resolution
	if record_used && ns == TypeNS && self.extern_prelude.contains(&ident.name) {

It seems sort of like this is the snippet of code to "load a crate if needed":

rust/src/librustc_resolve/lib.rs

Lines 1937 to 1939 in fefe816

	let crate_id = self.crate_loader.process_path_extern(ident.name, ident.span);
	let crate_root = self.get_module(DefId { krate: crate_id, index: CRATE_DEF_INDEX });
	self.populate_module_if_necessary(crate_root);

I think the idea would be to refactor that last snippet of code into a helper fn, and then invoke it from lookup_import_candidates. So lookup_import_candidates would look something like:

    fn lookup_import_candidates<FilterFn>(&mut self,
                                          lookup_name: Name,
                                          namespace: Namespace,
                                          filter_fn: FilterFn)
                                          -> Vec<ImportSuggestion>
        where FilterFn: Fn(Def) -> bool
    {
        let mut suggestions = vec![];

        suggestions.extend(
            self.lookup_import_candidates_from_module(
                lookup_name, namespace, self.graph_root, keywords::Crate.name(), filter_fn
            )
        );

        if extern_prelude_feature_is_enabled {
            let extern_prelude_names = self.extern_prelude.clone();
            for &krate_name in extern_prelude_names.iter() {
                let extern_prelude_module = self.load_extern_prelude_crate_if_needed(krate_name);
                // ^^ that is the new helper method we added

                suggestions.extend(
                    self.lookup_import_candidates_from_module(
                        lookup_name, namespace, extern_prelude_module, krate_name, filter_fn
                    )
                );
            }
        }
    }

OK, there is a slight flaw -- I think that lookup_import_candidates_from_module should take not a bool ("in root") but rather a Name with the crate name to prepend (which would be crate in the case of the root module). I've modified the code above accordingly.