Stream: t-compiler

Topic: crates-dependency-tracking


nikomatsakis (Jan 16 2019 at 14:08, on Zulip):

So @davidtwco / @mw -- I'd like to discuss https://github.com/rust-lang/rust/pull/55613. I can't decide if I think the dependency tracking change will be a problem or not. =)

davidtwco (Jan 16 2019 at 14:09, on Zulip):

(existing discussion on Zulip is here)

davidtwco (Jan 16 2019 at 14:09, on Zulip):

I'm happy to make any changes, not invested in that solution - it's just all I could find that worked.

mw (Jan 16 2019 at 14:11, on Zulip):

from a first glance, it looks like this could be trouble since there are several pieces of data that go untracked with these changes.

mw (Jan 16 2019 at 14:13, on Zulip):

why exactly is the list of crates not a query anymore?

davidtwco (Jan 16 2019 at 14:14, on Zulip):

When a new crate was a loaded after the query was first run (for diagnostic purposes) and the results cached then subsequent invocations of the query don't contain those crates.

mw (Jan 16 2019 at 14:15, on Zulip):

I see... I would have expected that all crates are loaded before the query engine starts anyway.

mw (Jan 16 2019 at 14:16, on Zulip):

I imagine other code relies on that too.

mw (Jan 16 2019 at 14:16, on Zulip):

Do you have a link to an example? I.e. something that shows which crates are loaded so late?

davidtwco (Jan 16 2019 at 14:16, on Zulip):

All the crates that get used are loaded - but the PR aims to load all those that have an --extern flag (including those that aren't used) so that we can suggest things from them.

davidtwco (Jan 16 2019 at 14:17, on Zulip):

ie. when a trait is implemented for a u32 in an extern crate and you use a function defined by that trait on a u32 without having used anything else from that crate - then it wouldn't say try importing external_crate::TraitWithFunctionYouUsed.

mw (Jan 16 2019 at 14:18, on Zulip):

Could you add an additional query that contains the used crate-nums + any other crate nums that have to be loaded?

mw (Jan 16 2019 at 14:20, on Zulip):

that query would then only be called when an error message is produced

nikomatsakis (Jan 16 2019 at 14:21, on Zulip):

This is what I was thinking too—

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

that we definitely want a single answer to the query, but we perhaps need a "maximal" query

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

i.e., what are all the crate nums that could be loaded

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

and we use that in diagnostic cases

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

(vs a "minimal" query: what are all the crate nums that were needed to resolve names)

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

(which is what we have now)

nikomatsakis (Jan 16 2019 at 14:22, on Zulip):

well, sort of

mw (Jan 16 2019 at 14:24, on Zulip):

yes, that

davidtwco (Jan 16 2019 at 14:26, on Zulip):

There are a handful of places throughout the compiler that this alternate query would need to be used.

Initially, I did something similar - I speculatively loaded the crates and then created a set with those new numbers and the existing ones - but there are a bunch of places in the compiler that eventually get called to verify that traits exist with the correct functions that call crates themselves and they'd need adjusted too - adjusting just the place where candidates for suggestions are found isn't enough.

I guess I could create a query that does that - which calls crates as it is now and then does the speculative loading - returning and caching the maximal set of crate nums. I'd need to then find the handful of places where crates is called elsewhere that would be required for this suggestion and then change those too?

davidtwco (Jan 16 2019 at 14:29, on Zulip):

(I think I understand what you are suggesting, I started writing that message before niko's comments which clarified that slightly)

nikomatsakis (Jan 16 2019 at 14:31, on Zulip):

ah, right, I remember that now

nikomatsakis (Jan 16 2019 at 14:32, on Zulip):

that there are parts of the code that kind of 'don't know' the purpose for which they are invoked

nikomatsakis (Jan 16 2019 at 14:32, on Zulip):

though I'm a bit surprised that invoke this crates query in the end

nikomatsakis (Jan 16 2019 at 14:32, on Zulip):

do you happen to remember any examples? I guess I could ripgrep around

davidtwco (Jan 16 2019 at 14:33, on Zulip):

I can find one of them where I realized that was the issue.

nikomatsakis (Jan 16 2019 at 14:33, on Zulip):

that'd be help[ful

davidtwco (Jan 16 2019 at 14:33, on Zulip):

right here

davidtwco (Jan 16 2019 at 14:34, on Zulip):

(and I tried changing that location to try get the speculative crate numbers in case it was just those two locations, but that caused ICEs)

davidtwco (Jan 16 2019 at 14:36, on Zulip):

this was the initial location where I do the speculative loading, changing just this to have those extra crate numbers doesn't ICE but misses an error.

davidtwco (Jan 16 2019 at 14:37, on Zulip):

(that is, it either suggests traits that are implemented but not in scope OR traits that can be implemented and aren't in scope - I forget which, it was a while ago)

davidtwco (Jan 16 2019 at 14:39, on Zulip):

(it does both now, with this change)

mw (Jan 16 2019 at 14:44, on Zulip):

How much of a performance hit would it be to load all known crates eagerly? I imagine that usually most crates are used, right?

nikomatsakis (Jan 16 2019 at 14:46, on Zulip):

not necessarily

davidtwco (Jan 16 2019 at 14:46, on Zulip):

I'm not sure. It'd probably affect the playground worst - since it has 100 crates with --extern flags that most often aren't used. There is only a hit in the error case with the current approach.

nikomatsakis (Jan 16 2019 at 14:46, on Zulip):

certainly things like example and test crates don't fit that trend

nikomatsakis (Jan 16 2019 at 14:46, on Zulip):

(I haven't looked @davidtwco at the links you sent yet, sorry)

nikomatsakis (Jan 16 2019 at 14:48, on Zulip):

ok, looking now. interesting.

nikomatsakis (Jan 16 2019 at 14:48, on Zulip):

I mean I guess another option is just to "not do this"

nikomatsakis (Jan 16 2019 at 14:48, on Zulip):

I wanted it but I'm not sure how much pain it's worth :)

nikomatsakis (Jan 16 2019 at 14:48, on Zulip):

and/or maybe we can provide less precise diagnostics

nikomatsakis (Jan 16 2019 at 14:49, on Zulip):

we're using this test to decide whether the trait is implemented for the receiver type, right?

nikomatsakis (Jan 16 2019 at 14:49, on Zulip):

conceivably we could just tell the user "this method exists in this trait which is not imported"

nikomatsakis (Jan 16 2019 at 14:49, on Zulip):

and not specify basically whether it is implemented or not

nikomatsakis (Jan 16 2019 at 14:50, on Zulip):

(I personally don't get much value from knowing whether it is implemented -- usually I have a trait in mind, and if it is not implemented, I often want to fix that)

davidtwco (Jan 16 2019 at 14:51, on Zulip):

we're using this test to decide whether the trait is implemented for the receiver type, right?

Yeah. But I'm fairly sure it's called in other places too because changing just that location and the original one caused ICEs.

nikomatsakis (Jan 16 2019 at 14:51, on Zulip):

well

nikomatsakis (Jan 16 2019 at 14:52, on Zulip):

not sure quite what you mean

nikomatsakis (Jan 16 2019 at 14:52, on Zulip):

that is, not sure quite which changes you made :)

davidtwco (Jan 16 2019 at 14:52, on Zulip):

If I only made sure that the new crate numbers were present in both those locations then other parts of the compiler would start to ICE.

nikomatsakis (Jan 16 2019 at 14:55, on Zulip):

I see. What I meant was more that:

nikomatsakis (Jan 16 2019 at 14:55, on Zulip):

if we don't try to verify that the trait is implemented, maybe it works out

davidtwco (Jan 16 2019 at 14:56, on Zulip):

After I read your message against I was confused as to why I pointed that out.

davidtwco (Jan 16 2019 at 14:56, on Zulip):

I understand what you mean now.

nikomatsakis (Jan 16 2019 at 14:56, on Zulip):

like I said, it may not be worth it at this point...

nikomatsakis (Jan 16 2019 at 14:56, on Zulip):

...though I still feel like it's helpful

nikomatsakis (Jan 16 2019 at 14:56, on Zulip):

and this is a particular point where people get confused

nikomatsakis (Jan 16 2019 at 14:56, on Zulip):

(the need to import a trait)

davidtwco (Jan 16 2019 at 14:57, on Zulip):

There was at least one person that voiced objection to the other change that was similar to this.

davidtwco (Jan 16 2019 at 14:57, on Zulip):

https://github.com/integer32llc/rust-playground/issues/445

nikomatsakis (Jan 16 2019 at 18:13, on Zulip):

yeah, true

nikomatsakis (Jan 16 2019 at 18:13, on Zulip):

playground is sort of an extreme case

Last update: Nov 22 2019 at 05:45UTC