Bug #21221: Show candidates for names not in scope

[VladUreche]

This commit adds functionality that allows the name resolution pass
to search for entities (traits/types/enums/structs) by name, in
order to show recommendations along with the errors.

For now, only E0405 and E0412 have suggestions attached, as per the
request in bug #21221, but it's likely other errors can also benefit
from the ability to generate suggestions.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[mitaa]

This should probably stay debug!(...);

[VladUreche]

Oops, sorry, will fix in a moment.

[VladUreche]

The previous commit failed to build after the rebase on the latest master, as a couple of names changed in the name resolution data structures. Should be fixed now.

[oli-obk]

change to else if !paths.is_empty() to remove the empty if-block above

[VladUreche]

@oli-obk Thank you for the suggestions! I will push an update asap.

[bors]

☔ The latest upstream changes (presumably #31685) made this pull request unmergeable. Please resolve the merge conflicts.

[VladUreche]

Thanks Bors, nice helpful robot :)
I addressed the merge in the last commit.

[nikomatsakis]

cc @jseyfried@petrochenkov, resident resolve experts :)

[nikomatsakis]

Nit: put these on two separate lines (lining up the .) -- seems a bit long for one line to me

[nikomatsakis]

OK, did a read through and left some notes -- this looks pretty good to me, actually. I like the suggestion infrastructure, I wonder if there are other places it could profitably be employed.

I left some nits and some larger comments.

[VladUreche]

@jseyfried, can you please have a look at the conditions now?
And in particular, I'm not sure if all modules from outside crates have is_external_crate set to true or just the crate itself -- to be sure, I propagated is_extern in the worklist: VladUreche@ddeb147#diff-aeb0880081a991f34aef2ab889e1fb7aR3614

I will address merging the CandidateAccessibility and CandidateSuggestions structs now.

[VladUreche]

@nikomatsakis, the commit messages should be much shorter now: VladUreche@ddeb147#diff-aeb0880081a991f34aef2ab889e1fb7aL3735

[jseyfried]

This should be just !in_module_is_extern || name_binding.is_public()

[VladUreche]

Oh, since we prune the search by not adding to the worklist. Good point!

[VladUreche]

After making the changes, I realized why I did not go with pruning:

 session.fileline_help(
span,
&format!("there are {} other candidates that are not \
accessible.", not_accessible_count),
);

When pruning the search, we do not count all inaccessible definitions with the given name.

[jseyfried]

Right now we are collecting all crate local items in lookup_results_accessible, so lookup_results_everything has all the paths that lookup_results_accessible has except for inaccessible external items. I don't think we should report inaccessible external items under any circumstances, so there's no need for the two lists.

[jseyfried]

If you want two lists, I think lookup_results_accessible should have all accessible items and lookup_results_everything should also have inaccessible items from the current crate. In that case, you'd need that ancestor checking method to distinguish between inaccessible and accessible items in the current crate. I don't think it's worth it to implement this distinction.

[VladUreche]

@jseyfried, the last commit merges the two data structures for storing candidates.
The pruning technique you suggested works only if we're okay with:

• not knowing how many inaccessible definitions exist (and/or whether they exist at all)
• not knowing what they are (in case there is no accessible definition)
  If that's okay, I can get the pruning working again.

Otherwise, once you had a look at the code, I can squash the commits again.

[jseyfried]

I think we should always report all items in the current crate and all accessible items in external crates and we should never report inaccessible items from external crates.

[jseyfried]

Also, feel free to squash whenever

[VladUreche]

Thanks! I removed the inaccessible definitions list and re-enabled the pruning. I'll have the new commit ready in a few minutes.

[jseyfried]

A couple things:

• To pub use something, it has to be pub, so this example would be an error unless trait T was pub.
• Since we don't care about accessibility in the current crate, we would report foo::bar::T, right?
• name_binding.is_import() is can only be true for bindings in the current crate since our crate metadata does not distinguish between public imports and public items. If this code were in an external crate, we would report foo::T as desired.

[VladUreche]

That simplifies things a lot. I was under the impression that you can "reveal" private parts of a crate through pub use statements. If imports are only visible on the current crate, we're perfectly fine ignoring them 👍

Thanks a lot for explaining this!

[VladUreche]

To answer the 2nd bullet point, yes, we would report foo::bar::T.

[jseyfried]

No problem, I probably should have mentioned this earlier :)
pub use statements actually can "reveal" private parts of a crate, for example

mod foo {pubtraitT{}}pubuse foo::T;// FYI, we wouldn't be able to do this if `trait T` were not `pub`

Here, T would not be accessible to other traits if we didn't pub use foo::T.
What I mean by not distinguishing between public imports and public items is that when we load this external crate, both bindings for T will be !binding.is_import() -- that is, we "forget" if bindings came from items or from imports. In other words, we wouldn't be able to distinguish the above external crate from

mod foo {pubuseT;}pubtraitT;

[VladUreche]

Which, if I'm interpreting well, means we should report external_crate::T as a valid suggestion. Let me add this as a test case 👍

[jseyfried]

Indeed, good idea

[VladUreche]

@jseyfried, thanks for the latest round of feedback. I removed in_module_is_accessible and made trait T private in src/test/compile-fail/issue-21221-2.rs. The commit is squashed :)

[jseyfried]

Alright, this look good to me -- thanks @VladUreche!

@nikomatsakis We decided to report all items from the current crate (ignoring re-exports) and all accessible items from external crates.

[VladUreche]

The last commit adds a test case which checks the re-exports from outside crates.
@jseyfried, @nikomatsakis thanks a lot for the feedback!

Just for the record, there are five possible extensions I'm aware of:

1. Crate local re-exports (including renames)
2. Filtering paths (e.g. if the programmer wrote ops::Mul we wouldn't suggest foo::Mul unless there's no trait Mul that starts with prefix ops)
3. fuzzy matching (e.g. ops::Muk results in a suggestion of ops::Mul)
4. Ordering by "distance" from the current module
5. Command-line option to avoid the 5-candidate limit

[nikomatsakis]

@bors r+

[bors]

📌 Commit 88af8fa has been approved by nikomatsakis

[nikomatsakis]

@VladUreche thanks for sticking with it :)

[bors]

⌛ Testing commit 88af8fa with merge cfabd17...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt