Bug #21221: Show candidates for names not in scope #31674

VladUreche · 2016-02-15T12:10:56Z

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 · 2016-02-15T12:11:12Z

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 · 2016-02-15T12:56:09Z

src/compiletest/errors.rs

@@ -91,7 +91,7 @@ fn parse_expected(last_nonfollow_error: Option<usize>,
        (which, line)
    };

-    debug!("line={} which={:?} kind={:?} msg={:?}", line_num, which, kind, msg);
+    println!("line={} which={:?} kind={:?} msg={:?}", line_num, which, kind, msg);


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

Oops, sorry, will fix in a moment.

VladUreche · 2016-02-15T16:20:37Z

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 · 2016-02-16T15:15:32Z

src/librustc_resolve/lib.rs

+        suggestion_str.push_str(&path_strings[0]);
+        suggestion_str.push_str("`, which you need to import with the `use` keyword.");
+        suggestion_vec.push(suggestion_str);
+    } else {


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

VladUreche · 2016-02-16T18:14:45Z

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

bors · 2016-02-17T08:14:24Z

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

VladUreche · 2016-02-17T12:06:23Z

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

nikomatsakis · 2016-02-18T01:10:43Z

cc @jseyfried @petrochenkov, resident resolve experts :)

nikomatsakis · 2016-02-18T01:33:22Z

src/librustc/middle/def.rs

            }
        }
    }

+    pub fn def_id(&self) -> DefId {
+        self.def_id_opt().expect(&format!("attempted .def_id() on invalid def: {:?}", self))


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

nikomatsakis · 2016-02-18T01:58:11Z

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 · 2016-02-18T21:34:27Z

@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 · 2016-02-18T21:36:08Z

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

jseyfried · 2016-02-18T21:37:34Z

src/librustc_resolve/lib.rs

+                        // declared as public
+                        lookup_results_everything.push(path.clone());
+                        if !in_module_is_extern ||
+                           name_binding.is_public() && in_module_is_accessible {


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

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

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.

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.

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 · 2016-02-18T22:33:48Z

@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 · 2016-02-18T23:02:12Z

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 · 2016-02-18T23:06:16Z

Also, feel free to squash whenever

VladUreche · 2016-02-18T23:16:37Z

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

jseyfried · 2016-02-18T23:25:44Z

src/librustc_resolve/lib.rs

+    ///     mod bar { trait T {}; };
+    ///     // make private T accessible through an import:
+    ///     pub use bar::T as T;
+    /// }


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.

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!

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

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

mod foo { pub trait T {} } pub use 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 { pub use T; } pub trait T;

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 👍

Indeed, good idea

VladUreche · 2016-02-18T23:47:39Z

@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 · 2016-02-19T00:06:50Z

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.

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 rust-lang#21221, but it's likely other errors can also benefit from the ability to generate suggestions.

VladUreche · 2016-02-19T00:16:59Z

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:

Crate local re-exports (including renames)
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)
fuzzy matching (e.g. ops::Muk results in a suggestion of ops::Mul)
Ordering by "distance" from the current module
Command-line option to avoid the 5-candidate limit

nikomatsakis · 2016-02-19T21:56:20Z

@bors r+

bors · 2016-02-19T21:56:21Z

📌 Commit 88af8fa has been approved by nikomatsakis

nikomatsakis · 2016-02-19T21:56:28Z

@VladUreche thanks for sticking with it :)

bors · 2016-02-20T10:40:43Z

⌛ Testing commit 88af8fa with merge cfabd17...

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.

bors · 2016-02-20T12:38:16Z

☀️ 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

rust-highfive assigned Aatch Feb 15, 2016

mitaa reviewed Feb 15, 2016
View reviewed changes

VladUreche force-pushed the issue/21221 branch 2 times, most recently from 5155347 to 5870906 Compare February 15, 2016 16:18

oli-obk reviewed Feb 16, 2016
View reviewed changes

VladUreche force-pushed the issue/21221 branch from 5870906 to 5f1b4a4 Compare February 16, 2016 18:15

VladUreche mentioned this pull request Feb 16, 2016

rustc should provide a suggestion when using a type/trait from a sibling module that isn't imported #21221

Closed

VladUreche force-pushed the issue/21221 branch from 5f1b4a4 to 686814b Compare February 17, 2016 12:04

nikomatsakis reviewed Feb 18, 2016
View reviewed changes

jseyfried reviewed Feb 18, 2016
View reviewed changes

VladUreche force-pushed the issue/21221 branch 2 times, most recently from e180806 to 2c613a5 Compare February 18, 2016 23:20

jseyfried reviewed Feb 18, 2016
View reviewed changes

VladUreche force-pushed the issue/21221 branch from 2c613a5 to 8489600 Compare February 18, 2016 23:45

VladUreche force-pushed the issue/21221 branch from 8489600 to 88af8fa Compare February 19, 2016 00:10

bors merged commit 88af8fa into rust-lang:master Feb 20, 2016

bors mentioned this pull request Feb 20, 2016

Improve import resolution #31726

Merged

jseyfried mentioned this pull request Jun 29, 2016

Fix non-termination on recursive module re-exports in extern crates #34542

Merged

petrochenkov mentioned this pull request Jul 30, 2017

resolve: Try to fix instability in import suggestions #43552

Merged

petrochenkov mentioned this pull request May 20, 2018

Incorrect hint to import private member #50923

Closed

Bug #21221: Show candidates for names not in scope #31674

Bug #21221: Show candidates for names not in scope #31674

Conversation

VladUreche commented Feb 15, 2016

rust-highfive commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUreche commented Feb 15, 2016

Choose a reason for hiding this comment

VladUreche commented Feb 16, 2016

bors commented Feb 17, 2016

VladUreche commented Feb 17, 2016

nikomatsakis commented Feb 18, 2016

Choose a reason for hiding this comment

nikomatsakis commented Feb 18, 2016

VladUreche commented Feb 18, 2016

VladUreche commented Feb 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUreche commented Feb 18, 2016

jseyfried commented Feb 18, 2016

jseyfried commented Feb 18, 2016

VladUreche commented Feb 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUreche commented Feb 18, 2016

jseyfried commented Feb 19, 2016

VladUreche commented Feb 19, 2016

nikomatsakis commented Feb 19, 2016

bors commented Feb 19, 2016

nikomatsakis commented Feb 19, 2016

bors commented Feb 20, 2016

bors commented Feb 20, 2016