Stream: t-compiler

Topic: #54618 extern crates trait suggestions


davidtwco (Oct 04 2018 at 14:23, on Zulip):

Looking into #54618. The issue is that if a crate isn't loaded but has a --extern then it won't make a suggestion. So I figure when making suggestions I'd need to load any crates from --externflags first so that it can make suggestions from all crates. That means that this is a fairly rare case, since you'd only ever run into it if you add a dependency and don't use it, but expect the suggestion from it in errors. Anyway, I'm not sure how I'd go about loading crates from rustc_typeck. If there was a way to access a CrateLoader I could probably call maybe_process_path_extern on the session.extern_prelude names.

nikomatsakis (Oct 04 2018 at 14:24, on Zulip):

That means that this is a fairly rare case, since you'd only ever run into it if you add a dependency and don't use it, but expect the suggestion from it in errors

I don't think is that rare. e.g., it happens a lot with things like rayon and itertools

nikomatsakis (Oct 04 2018 at 14:24, on Zulip):

which mainly extend types with new methods via traits

davidtwco (Oct 04 2018 at 14:25, on Zulip):

We do similar with the "did you mean to import X" suggestions - we iterate over all --extern crates and then load then and check if we can find a path in it; but that happens at a point where I can call functions on CrateLoader.

nikomatsakis (Oct 04 2018 at 14:25, on Zulip):

yeah I'm not sure about that aspect of it

davidtwco (Oct 04 2018 at 14:25, on Zulip):

@nikomatsakis Well, what I mean is, you'd need to add the crate to Cargo.toml and never import anything or use anything from that crate. That way you'd have an --extern flag to rustc but it'd never actually load the crate.

nikomatsakis (Oct 04 2018 at 14:25, on Zulip):

right, I'm saying this is common :)

nikomatsakis (Oct 04 2018 at 14:25, on Zulip):

in those specific scenarios

davidtwco (Oct 04 2018 at 14:26, on Zulip):

Ah, I see what you mean.

nikomatsakis (Oct 04 2018 at 14:26, on Zulip):

that is, you might reasonably add rayon = "1.0" and then use par_iter()

davidtwco (Oct 04 2018 at 14:26, on Zulip):

Yeah, I see what you mean. I just think it'd be a little bit of a strange workflow to add the crate, then re-run the not-compiling code where you presumably are calling par_iter to see what to import from the suggestion - presumably you'd have found that in documentation by that point.

davidtwco (Oct 04 2018 at 14:27, on Zulip):

It's inconsequential either way.

davidtwco (Oct 04 2018 at 14:31, on Zulip):

One way I think I could do it would be to make rustc_typeck depend on rustc_metadata, then I might be able to construct a CrateLoader with the &'a CrateStoreDyn and &'a Session stored in TyCtxt. Though &'a CrateStoreDyn is not &'a CStore. I don't know that I like that solution that much though.

davidtwco (Oct 04 2018 at 15:33, on Zulip):

@nikomatsakis Do you have any suggestions on how best I'd go about doing this? Effectively I need a function that'll load all crates so I can use them for suggestions while in rustc_typeck.

nikomatsakis (Oct 04 2018 at 15:36, on Zulip):

Yeah, I see what you mean. I just think it'd be a little bit of a strange workflow to add the crate, then re-run the not-compiling code where you presumably are calling par_iter to see what to import from the suggestion - presumably you'd have found that in documentation by that point.

I did it :)

nikomatsakis (Oct 04 2018 at 15:36, on Zulip):

hence how I came to file this bug...

nikomatsakis (Oct 04 2018 at 15:36, on Zulip):

I think though I was sort of expecting the compiler to tell me the trait to import

davidtwco (Oct 04 2018 at 15:36, on Zulip):

I thought it was from the test I added to the other PR.

davidtwco (Oct 04 2018 at 15:37, on Zulip):

I guess in which case, I've also done it.

nikomatsakis (Oct 04 2018 at 15:37, on Zulip):

oh, yes, true :) regardless I have done it

nikomatsakis (Oct 04 2018 at 15:37, on Zulip):

maybe subsequently though

nikomatsakis (Oct 04 2018 at 15:37, on Zulip):

in particular I often lean on the compiler to tell me the right traits...

nikomatsakis (Oct 04 2018 at 15:37, on Zulip):

anyway

nikomatsakis (Oct 04 2018 at 15:37, on Zulip):

I'm not sure let me look into the crate loading business

nikomatsakis (Oct 04 2018 at 15:38, on Zulip):

I'm not sure how much of a big deal it is to load all crates eagerly

nikomatsakis (Oct 04 2018 at 15:38, on Zulip):

cc @eddyb who probably has thoughts — see #54618 for context

davidtwco (Oct 04 2018 at 15:38, on Zulip):

I've just assumed there's a reason we only load them when we encounter a path with them in it.

nikomatsakis (Oct 04 2018 at 15:40, on Zulip):

presumably yes

nikomatsakis (Oct 04 2018 at 15:40, on Zulip):

but...you never know...

davidtwco (Oct 04 2018 at 15:40, on Zulip):

At the very least it's better not to do it if we don't need them.

nikomatsakis (Oct 04 2018 at 15:41, on Zulip):

well, I was wondering in particular if "loading a crate" were quite lazy

nikomatsakis (Oct 04 2018 at 15:41, on Zulip):

then it would seem fine

nikomatsakis (Oct 04 2018 at 15:41, on Zulip):

anyway, you are correct that this CrateLoader gets destroyed

nikomatsakis (Oct 04 2018 at 15:43, on Zulip):

though it doesn't appear to be a very "heavyweight" data structure

nikomatsakis (Oct 04 2018 at 15:43, on Zulip):
pub struct CrateLoader<'a> {
    pub sess: &'a Session,
    cstore: &'a CStore,
    local_crate_name: Symbol,
}
davidtwco (Oct 04 2018 at 15:44, on Zulip):

Yeah, we have everything available at the place where I need it (roughly) to be able to construct a new one - since it seems to just hold operations for adding to the CStore. But, rustc_metadata isn't a dependency of that crate so I can't access it.

davidtwco (Oct 04 2018 at 15:46, on Zulip):

(it's "roughly" because GlobalCtxt, IIRC, has a CrateStoreDyn which implements the same trait as Cstore - so not quite enough to make a CreateLoader but almost)

nikomatsakis (Oct 04 2018 at 15:53, on Zulip):

right and it may be that this separation is intended to be refactored into something stronger

davidtwco (Oct 04 2018 at 15:55, on Zulip):

I'll take a look into some other issues until there's a clearer idea of what approach we want to take here.

Jake Goulding (Oct 05 2018 at 13:58, on Zulip):

since you'd only ever run into it if you add a dependency and don't use it, but expect the suggestion from it in errors

It's not the most compelling case, but this is exactly how the playground works ;-) This error would improve that experience.

davidtwco (Oct 05 2018 at 14:01, on Zulip):

That's a good point, hadn't considered that.

nikomatsakis (Oct 05 2018 at 14:12, on Zulip):

that's kind of neat =)

davidtwco (Oct 11 2018 at 13:56, on Zulip):

Has there been any discussion or thought about the approach to be taken for this issue?

nikomatsakis (Oct 11 2018 at 14:02, on Zulip):

not really :(

eddyb (Nov 01 2018 at 15:04, on Zulip):

@davidtwco where'd you get stuck?

eddyb (Nov 01 2018 at 15:05, on Zulip):

all you need is to plumb this through the query engine https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/lib.rs#L4474-L4475

nagisa (Nov 01 2018 at 15:05, on Zulip):

whoops

davidtwco (Nov 01 2018 at 15:05, on Zulip):

Let's move to the dedicated topic for this.

eddyb (Nov 01 2018 at 15:05, on Zulip):

if the crate loader is separate from the crate store, maybe you can put the former in the latter?

davidtwco (Nov 01 2018 at 15:06, on Zulip):

@eddyb :slight_smile:

davidtwco (Nov 01 2018 at 15:06, on Zulip):

So, I don't think I made any attempts at actually implementing anything, I just looked around for a while and couldn't see a way I'd do it.

eddyb (Nov 01 2018 at 15:07, on Zulip):

well, you put the crate loader in the crate store, if it's not already there

eddyb (Nov 01 2018 at 15:07, on Zulip):

and the crate store can just provide queries, so... that's it

eddyb (Nov 01 2018 at 15:07, on Zulip):

you add a query for maybe_load_extern_crate or something

davidtwco (Nov 01 2018 at 15:07, on Zulip):

One option I considered was making a function that would make a CrateLoader (the contents of which are mostly available) and then just call the correct function on that for each crate, but we only have a type erased crate store, not the actual type that is required.

davidtwco (Nov 01 2018 at 15:08, on Zulip):

I've not looked at the query system much, I'll need to familarize myself with that.

eddyb (Nov 01 2018 at 15:08, on Zulip):

that's why you implement things on teh actual crate store

eddyb (Nov 01 2018 at 15:08, on Zulip):

which gets special treatment from the query engine

eddyb (Nov 01 2018 at 15:09, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/cstore_impl.rs#L101

eddyb (Nov 01 2018 at 15:10, on Zulip):

oh god this is such a hack https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/cstore_impl.rs#L83-L84

eddyb (Nov 01 2018 at 15:11, on Zulip):

okay nvm this wouldn't work like that

eddyb (Nov 01 2018 at 15:12, on Zulip):

@davidtwco okay, uhhh, just add a method to https://github.com/rust-lang/rust/blob/master/src/librustc/middle/cstore.rs#L208

eddyb (Nov 01 2018 at 15:12, on Zulip):

that does the loading

eddyb (Nov 01 2018 at 15:12, on Zulip):

that should work

eddyb (Nov 01 2018 at 15:13, on Zulip):

you can probably figure out how to do an _untracked method with a query elsewhere

davidtwco (Nov 01 2018 at 15:13, on Zulip):

Alright, I'll give that a go and see what I can figure out.

eddyb (Nov 01 2018 at 15:13, on Zulip):

just by grepping for those method names in the trait

davidtwco (Nov 02 2018 at 10:43, on Zulip):

@eddyb I've got it so that I can call the function to load something and I can see it doing that in the logs. However, I've not figured out how to get that to invalidate the all_crate_nums (and following on from that, the all_traits) queries.

davidtwco (Nov 02 2018 at 10:43, on Zulip):

(with the above said, I'm not 100% sure that I've implemented the query correctly, but I can call it, so :shrug:)

eddyb (Nov 02 2018 at 10:47, on Zulip):

ehm, it can't do that :(

eddyb (Nov 02 2018 at 10:47, on Zulip):

you have to make all_traits not use all_crate_nums

eddyb (Nov 02 2018 at 10:48, on Zulip):

or, rather, it should use all_crate_nums in combination with loading everything in the extern prelude, speculatively

davidtwco (Nov 02 2018 at 10:48, on Zulip):

Ah.

eddyb (Nov 02 2018 at 10:48, on Zulip):

and use a set because you might get the same CrateNum more than once

davidtwco (Nov 02 2018 at 10:50, on Zulip):

Thanks, I'll look into doing that.

eddyb (Nov 02 2018 at 10:50, on Zulip):

@nikomatsakis ^^ do you think that's a good approach?

eddyb (Nov 02 2018 at 10:50, on Zulip):

basically I'm saying all_traits should always look in --extern crates even if nothing else loads them

eddyb (Nov 02 2018 at 10:50, on Zulip):

but this might be bad/wrong if all_traits is used for non-diagnostic purposes

davidtwco (Nov 02 2018 at 10:51, on Zulip):

I'm not sure it is just all_traits - there's two computations as far as I can gather - one for out of scope traits (don't think it uses all_traits) and one for traits that it suggests you implement (does use all_traits).

davidtwco (Nov 02 2018 at 10:52, on Zulip):

To handle that, I've been trying to speculatively load all crates as soon as we're about to compute the earliest of those two - in the hopes that with the crates now loaded, they'd both get the benefit.

davidtwco (Nov 02 2018 at 10:52, on Zulip):

But, since it doesn't invalidate the other queries, that doesn't seem to happen.

eddyb (Nov 02 2018 at 10:54, on Zulip):

no, you probably don't want other things to see those crates as loaded

davidtwco (Nov 02 2018 at 11:54, on Zulip):

Alright, so I have this working - mostly.

davidtwco (Nov 02 2018 at 11:54, on Zulip):

It correctly suggests both "you might want to implement this" and "there's an implementation of this, import it".

davidtwco (Nov 02 2018 at 11:55, on Zulip):

In fact, I think I know what the issue I've not explained yet might be.

davidtwco (Nov 02 2018 at 11:56, on Zulip):

Alright, fairly certain I don't.

davidtwco (Nov 02 2018 at 12:00, on Zulip):

So, the issue I'm seeing is that I have this code:

    // Expect a "try using baz::BazTrait" (which involves
    // loading that crate speculatively as it isn't otherwise used).
    x.extern_baz();

    let local = Local;
    // Expect a "consider implementing baz::BazTrait".
    local.extern_baz();

where x is a u32 and has an implementation of BazTrait (which is from the external crate and defines the method extern_baz) and Local is a local type that doesn't implement it.

With the test as above, I get the following error:

   |
LL | struct Local;
   | ------------- method `extern_baz` not found for this
...
LL |     local.extern_baz();
   |           ^^^^^^^^^^
   |
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following trait defines an item `extern_baz`, perhaps you need to implement it:
           candidate #1: `baz::BazTrait`

Which is ideal, that's what I want. However, I don't get an error for the x.extern_baz() line. I've hooked up this change to both of those diagnostics so I should be getting it. I've discovered if I comment out let local = Local; (which causes a "what is local" error, but that's fine), then I get a suggestion like this (as expected) for the x.extern_baz() line:

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

Not sure why I can't get both at once to happen.

davidtwco (Nov 02 2018 at 12:01, on Zulip):

I'm lucky that I made the mistake of not adding let local = Local; initially and noticing the difference once I added it or I'd have taken way longer to realise that statement was having an effect.

davidtwco (Nov 02 2018 at 12:05, on Zulip):

This function is called once for each error location - x.extern_baz() and local.extern_baz(). When local is commented, then out_of_scope_traits for x.extern_baz has one item in it during it's call (as it should). When local isn't commented, then out_of_scope_traits is empty for both of their calls (which is expected for the local case, it goes on to report a "you could implement it though" error correctly).

davidtwco (Nov 02 2018 at 12:06, on Zulip):

It seems that for some reason this branch is used for the x.extern_baz() case when local is uncommented.

davidtwco (Nov 02 2018 at 12:08, on Zulip):

The suggest_traits_to_import call for the x.extern_baz() case happens first irrespective of local being commented.

davidtwco (Nov 02 2018 at 12:20, on Zulip):

I'm struggling to work out what the issue is here, any ideas?

davidtwco (Nov 02 2018 at 12:27, on Zulip):

It seems like it isn't an pre-existing issue with these two suggestions - playground.

davidtwco (Nov 02 2018 at 12:29, on Zulip):

It does seem to work as expected if I have two of the "it's implemented, try using it" suggestion - which makes me think that the issue stems from the overlap of both types of suggestion - "implemented try use" and "not implemented, try implement".

davidtwco (Nov 02 2018 at 12:31, on Zulip):

Similarly, if I have two occurances of the "not implemented, try implement" case then both get a suggestion. That would suggest that the new speculative crate load isn't only working once.

davidtwco (Nov 02 2018 at 14:03, on Zulip):

Huh, now I'm getting it to fail without having the other suggestion at all. I don't know what is different.

davidtwco (Nov 02 2018 at 14:11, on Zulip):

@nikomatsakis do you have any idea why I might be seeing what I've described above?

nikomatsakis (Nov 02 2018 at 14:14, on Zulip):

Hmm

nikomatsakis (Nov 02 2018 at 14:15, on Zulip):

@davidtwco is there an open PR or something so I can "catch up" on what approach you used?

davidtwco (Nov 02 2018 at 14:16, on Zulip):

I've got the log output in a gist here. Will open a PR in a moment.

davidtwco (Nov 02 2018 at 14:18, on Zulip):

Searching for consider_probe has the logs that are as deep as I've tracked it down - these lines.

davidtwco (Nov 02 2018 at 14:20, on Zulip):

@nikomatsakis #55613

davidtwco (Nov 02 2018 at 14:21, on Zulip):

(the stderr file has the desired output in that PR, so it will fail the tests)

davidtwco (Nov 02 2018 at 14:22, on Zulip):

(lines 28-30 for src/test/ui/rust-2018/trait-import-suggestions.stderr don't show up in practice)

nikomatsakis (Nov 02 2018 at 18:28, on Zulip):

sorry will try to look soon :) got distracted

davidtwco (Nov 02 2018 at 22:07, on Zulip):

No worries.

davidtwco (Nov 02 2018 at 22:07, on Zulip):

Thanks.

davidtwco (Nov 12 2018 at 15:13, on Zulip):

@nikomatsakis hope SPLASH went well, I enjoyed your blog post. I've not done any more digging on the issue from this PR - just wanted to bump it - there's no rush, I'm sure you're super busy catching up :slight_smile:

nikomatsakis (Nov 12 2018 at 18:45, on Zulip):

@davidtwco indeed :) thanks

Jake Goulding (Dec 10 2018 at 14:21, on Zulip):

As a data point, there's at least one person who isn't a fan of extern crate suggestions for types in the playground, which is a non-usual case.

davidtwco (Dec 12 2018 at 21:02, on Zulip):

FYI @nikomatsakis, there is this PR that I also ran into a wall with - not had time to revisit it and take a look with fresh eyes yet though.

davidtwco (Dec 18 2018 at 13:57, on Zulip):

Spent some more time digging into what was going on here today. Managed to get a decent 20+ functions deep and end up at this line where despite loading all extern crates using the new query that this PR adds earlier in the execution and having trait_id in that function be from the external crate, the call to tcx.crates() doesn't include the crate - and therefore rustc concludes the trait isn't implemented for u32 when it is.

davidtwco (Dec 18 2018 at 13:58, on Zulip):

Not sure really how to work around that. It is consistent since when we do our speculative crate load here, we still need to append it to the tcx.crates() results afterwards.

davidtwco (Dec 18 2018 at 13:59, on Zulip):

I'll need to look into whether the new query should affect the result of tcx.crates() and if it should, why it isn't, and if it isn't, what alternatives there are.

davidtwco (Dec 18 2018 at 15:39, on Zulip):

Seems like the crates query shouldn't be being run because it's already been cached - as far as I gather from this old message from eddyb - that can't be changed.

Doing what I did in to solve the issue that spurred on that previous message and just combining the output of tcx.crates() and the new query that this adds ends up hitting an ICE here and then here (if you just comment out the first one).

Using the CrateNum that already exists from the DefId of the trait (which isn't ideal, then we'd only find impls from the same crate as the trait, and not impls from any other extern crates that are not imported) then we get the same errors.

Not really sure how to progress again.

davidtwco (Dec 18 2018 at 15:40, on Zulip):

I suspect even if the tcx.crates() query returned the crate num that it would run into the same ICEs anyway.

oli (Dec 18 2018 at 15:54, on Zulip):

Could we make the crates query return a structure that offers an accessor method for the list of crates as it does now, but also has a second accessor for all crates that will delay_span_bug to ensure we only use it in an error reporting path?

oli (Dec 18 2018 at 15:55, on Zulip):

I'm assuming that there's another query that actually loads the crates, so just calling crates will not load all crates, but just reserve CrateNums for them

davidtwco (Dec 18 2018 at 15:59, on Zulip):

I'm assuming that there's another query that actually loads the crates, so just calling crates will not load all crates, but just reserve CrateNums for them

There's a new query added in the PR for this issue that loads a crate given a name (and we use tcx.extern_prelude.keys() to get those) - so the loading should be handled - we'd just need crates to return that new CrateNum too - which I suspect it would do, because the cstore has changed with the previous load and the provider behind crates would pick that up fine - it's just the caching that gets in the way for that.

However, I suspect that even doing that (while probably necessary) wouldn't solve the ICEs that it then runs into - that might be a result of the way that the maybe_load_extern_crate query that this PR added works.

oli (Dec 18 2018 at 16:15, on Zulip):

is crates doing a lot of work? if not, we could maybe stop caching it?

oli (Dec 18 2018 at 16:16, on Zulip):

My original suggestion/question above was whether crates could emit the extern prelude crates from the beginning instead of trying to make it emit more crates later

davidtwco (Dec 18 2018 at 16:44, on Zulip):

crates just iterates over self.metas in the cstore.

Sorry, I misunderstood your suggestion. I don’t think it could (though that’s just my understanding of it).

davidtwco (Dec 18 2018 at 16:45, on Zulip):

I’m not sure though if there are other queries related to crates with cached outputs that would explain the follow-on ICEs though.

davidtwco (Dec 19 2018 at 11:01, on Zulip):

@Oli do you recall how to stop caching a query? I've tried changing this line to eval_always but that doesn't seem to be doing it and I can't see anything else.

oli (Dec 19 2018 at 11:12, on Zulip):

I think you "just" need to not make it be a query by removing the entry and adding a function to TyCtxt

davidtwco (Dec 19 2018 at 11:12, on Zulip):

Alright, I'll try that, thanks.

davidtwco (Dec 19 2018 at 11:25, on Zulip):

@Oli I think that might just have done it! Thanks. I suspect the other ICEs I saw before were just because adding that new CrateNum at just that function wasn't a good idea.

oli (Dec 19 2018 at 11:27, on Zulip):

cool! be sure to do a perf run, but it seems like it's a pretty cheap wrapper around accessing self.metas

Last update: Dec 12 2019 at 01:25UTC