suggesting for traits do not search the "prelude crates"

[davidtwco]

Fixes #54618.

r? @nikomatsakis
cc @eddyb

[davidtwco]

As of submitting this, there's currently an issue with one of the suggestions improved by this PR, see this thread on Zulip.

[eddyb]

cc @petrochenkov

[TimNN]

Ping from triage @davidtwco: It looks like your PR failed on travis.

[davidtwco]

Thanks @TimNN - currently awaiting some further instruction on Zulip for this otherwise I'd be all over fixing the Travis failure!

[TimNN]

Ping from triage. Thanks for the update, @davidtwco. I'll mark this PR as blocked for now.

[davidtwco]

Managed to solve the issue that was blocking this PR, it should be ready for review.

However, solving this issue required making the tcx.crates() function no longer rely on a query (that is, the all_crate_nums query has been removed). As the result of this query was being cached, it was not including new speculatively loaded crates. all_crate_nums didn't do much though, so I suspect the performance hit will be minimal - we should do a perf run to be sure.

[nikomatsakis]

argh looks like I failed to finish my review, posting partial comments here

[nikomatsakis]

If this is creating a new vector each time, perhaps change the return type to Vec<..>? Also, I worry a bit about the incremental computation side of this. It feels like we should be very careful using a method named untracked =) it may be that the method name is outdated though or that the setup needs to be adjusted. I would expect us to be tracking the set of prelude names as part of the incremental state

[davidtwco]

I've changed the last commit so it also updates the return type of this as it is a new Vec<..> each time.

The reason that I changed this was because after speculatively loading a new crate for the purpose of diagnostics, the result of the crates query wouldn't change. This was fine for the purposes of adding a note like this:

    = note: the following trait defines an item `extern_baz`, perhaps you need to implement it:
            candidate #1: `baz::BazTrait`

But when adding a note like that below, it didn't work:

 help: the following trait is implemented but not in scope, perhaps add a `use` for it:
    |
 LL | use baz::BazTrait;

This is because the larger check that worked out whether this was appropriate to suggest called the crates query in a few places and the missing speculatively-loaded crates would cause the logic to always consider a trait from a speculatively-loaded trait unsuitable.

As I understand it, it isn't possible to invalidate the cache of the crates query after speculatively-loading crates (see this message from @eddyb).

The crates query just iterates over self.metas in the cstore and so will only ever contain the crates that exist when it was first invoked and then cached. I wrote some more detail here about my investigation into why only the first case got a suggestion, @oli-obk then suggested stopping the crates function from being a query.

[nikomatsakis]

Something about the crates function is making me nervous. I have no problem with it not being a query, I just want to make sure that the data it reads from is somehow part of the incremental hash. I am basically concerned that we might wind up with two executions that differ because the crate which was being speculatively loaded is a different one the second time (or contains different data).

@michaelwoerister, maybe you have an opinion about whether this will work out? The problem is that the crates query was returning the set of CrateNum that came from the crate metadata (and this result was presumably being hashed). But for error handling purposes we sometimes go and load additional crates after that point. Then we want the full set of CrateNum that includes this data so we can iterate over it.

I feel like there's a sort of "leak" happening here of information. There is shared mutable state (the set of loaded crates) and somehow it's not quite being tracked. (In practice, it's probably harmless, but still...).

Not sure the best way to model this.

[nikomatsakis]

We discussed this some on Zulip -- the consensus was that indeed there is a problem with incremental here. The question was whether we can find a solution that doesn't require changing the crates query, perhaps by giving less precise suggestions?

[davidtwco]

I've experimented with adding a maximal query that loads all crates and returns all crate nums and then tried using that in the handful of places I previously identified would be needed to get the suggestion to show up - but I wasn't able to get this working and it was causing some ICEs. Further, some of the code that would need to use this query would be invoked even when there isn't a error - effectively loading all crates all the time.

If we undo the most recent commit then we lose the suggestion in the below snippet:

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/test/ui/rust-2018/trait-import-suggestions.stderr#L23-L33

However, we do keep the suggestion in this case:

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/test/ui/rust-2018/extern-trait-impl-suggestions.stderr#L1-L12

If that's acceptable then it might be worthwhile landing that and follow-up if we can think of a way to do this that doesn't modify the crates query.

[nikomatsakis]

@davidtwco I don't quite understand what the difference is between the case where you get a suggestion and the one where you don't.

However, one thing I wanted to ask: right now, we do some amount of trait selection in order to figure out if the trait is implemented and hence just needs to be imported or whether the trait needs to be implemented. Personally, I usually find this distinction kind of annoying and useless, but it's perhaps because I know Rust well. I essentially just want a list of the traits that feature the method: often, if it is not implemented already, I want to implement it.

If we were not trying to figure out whether the trait was implemented, but we were merely looking to see if it featured the given method, do you think that would still trigger the ICE-prone code paths?

[davidtwco]

@davidtwco I don't quite understand what the difference is between the case where you get a suggestion and the one where you don't.

@nikomatsakis In the case where we do error, all that happpens is that we look at all suggestible traits (which is produced with the speculatively loaded crate numbers manually included) and then check for an appropriate associated item - whatever is involved in doing this doesn't involve loading the crates (and would therefore also need the speculatively loaded crate numbers):

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc_typeck/check/method/suggest.rs#L594-L611

In the case where we won't error, we end up with the trait (always originating from the all_suggestible_traits query that this PR modifies) at the code below where we do a more in-depth check that involves checking for methods that have the right receiver type:

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc_typeck/check/method/probe.rs#L983-L985

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc_typeck/check/method/probe.rs#L1130-L1135

...which ends up at (sorry, I got lazy writing descriptions of the call stack)...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc_typeck/check/method/probe.rs#L1198-L1209

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/traits/query/evaluate_obligation.rs#L23

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/traits/query/evaluate_obligation.rs#L47-L62

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc_traits/evaluate_obligation.rs#L16-L35

...which ends up at (phew, I'm glad I took notes) ...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/traits/select.rs#L1205-L1206

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/traits/select.rs#L2014-L2029

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/ty/trait_def.rs#L88-L93

...which ends up at...

https://github.com/rust-lang/rust/blob/c546dc4b765c8cde5c73c54086e09e956287b664/src/librustc/ty/trait_def.rs#L174-L180

...and that call to tcx.crates() doesn't include the speculatively loaded crate number and so it returns that there is no valid traits that aren't in scope. This is why stopping crates() from returning a cached result fixed the issue. Note that changing just this location to also have the speculative crates causes an ICE - presumably there's somewhere else I didn't find that needed changed too. At least, that's what I was able to work out when I dug into this before the holidays.

However, one thing I wanted to ask: right now, we do some amount of trait selection in order to figure out if the trait is implemented and hence just needs to be imported or whether the trait needs to be implemented. Personally, I usually find this distinction kind of annoying and useless, but it's perhaps because I know Rust well. I essentially just want a list of the traits that feature the method: often, if it is not implemented already, I want to implement it.

If we were not trying to figure out whether the trait was implemented, but we were merely looking to see if it featured the given method, do you think that would still trigger the ICE-prone code paths?

I don't believe so.

[bors]

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

[davidtwco]

I’m going to close this - the minor diagnostic improvements that this PR would bring (should the unresolved questions about the implementation be resolved) aren’t worth the increased complexity required.