Stream: t-compiler/wg-nll

Topic: #60680 full NLL crater runs


lqd (May 16 2019 at 20:03, on Zulip):

@pnkfelix let me know if I can help with #60680, for the runs themselves and/or triaging

lqd (May 16 2019 at 20:24, on Zulip):

(I can take the issue if you'd rather focus on something else)

centril (May 16 2019 at 20:58, on Zulip):

@lqd that would be great; crater is a bit borked atm but if you can get the PR up I can take care of scheduling crater when it works

lqd (May 16 2019 at 20:58, on Zulip):

yeah I know it's waiting on the cargo update

lqd (May 16 2019 at 20:59, on Zulip):

it's unlikely I'll be able to do the PR before the other one merges though, I can try tomorrow

lqd (May 16 2019 at 21:02, on Zulip):

IIUC it's #60874, which is being tested now

centril (May 16 2019 at 21:02, on Zulip):

Tomorrow would be awesome

lqd (May 16 2019 at 21:05, on Zulip):

@centril do you know if the two ways mentioned in the issue (denying the warning, changing the borrowck default) needed to be done in different PRs/crater runs to gather data for each ? or both in one run ?

centril (May 16 2019 at 21:05, on Zulip):

@lqd I think you want them in different PRs

centril (May 16 2019 at 21:06, on Zulip):

or they would overlap I think

lqd (May 16 2019 at 21:06, on Zulip):

probable

lqd (May 16 2019 at 21:07, on Zulip):

alright, will do

lqd (May 17 2019 at 09:26, on Zulip):

@pnkfelix re your comment yeah, I'll open the 2 PRs at lunch break, IIRC one of the ways is just changing the default borrowck, and the other always returning false for each borrowckmode when asked "do we also want to use AST" (right ?)

pnkfelix (May 17 2019 at 09:27, on Zulip):

hmm

pnkfelix (May 17 2019 at 09:27, on Zulip):

I agree that changing the borrowck default is how I'd go about doing the "change default borrowck mode to NLL"

pnkfelix (May 17 2019 at 09:28, on Zulip):

I hadn't considered the approach you describe for the other goal (of making the future-compat warnings under migrate mode denied instead of warned)

pnkfelix (May 17 2019 at 09:28, on Zulip):

if it works, great. :smiley:

lqd (May 17 2019 at 09:29, on Zulip):

that may be too optimistic/stupid of me

lqd (May 17 2019 at 09:30, on Zulip):

because in a sense it could be seen as strictly 2 different ways of doing the same thing (eg: not calling AST ends up never downgrading an error to a warning, and therefore equivalent to having the mode be full NLL even though we are in migrate mode still), thus maybe wrong

lqd (May 17 2019 at 09:30, on Zulip):

I'll look more through the code where we downgrade the error to a warning

pnkfelix (May 17 2019 at 09:35, on Zulip):

oh, yeah: Maybe just comment out the downgrade path!

pnkfelix (May 17 2019 at 09:35, on Zulip):

implementation-via-code-deletion!

lqd (May 17 2019 at 11:13, on Zulip):

@pnkfelix I opened the simpler 1st PR, and wanted to validate my changes for the 2nd one: do you have an example where I could see the difference made by having errors from denying the lint in migrate mode, with having errors from turning on NLLs ?

pnkfelix (May 17 2019 at 11:14, on Zulip):

by "turning on NLLs", you mean going from migrate mode to full NLL?

lqd (May 17 2019 at 11:14, on Zulip):

yeah

pnkfelix (May 17 2019 at 11:15, on Zulip):

well there are cases where the full NLL's improved region analysis causes differences

pnkfelix (May 17 2019 at 11:15, on Zulip):

didn't we change compare-mode=nll to do this?

pnkfelix (May 17 2019 at 11:16, on Zulip):

see for example #59299

lqd (May 17 2019 at 11:16, on Zulip):

oh alright yeah that must be it

pnkfelix (May 17 2019 at 11:17, on Zulip):

see also PR #60171

pnkfelix (May 17 2019 at 11:18, on Zulip):

which I think I'm now going to r+

lqd (May 17 2019 at 11:19, on Zulip):

would the regionck errors be AST borrowck errors ?

lqd (May 17 2019 at 11:20, on Zulip):

and thus, still present in migrate mode

Matthew Jasper (May 17 2019 at 11:20, on Zulip):

Regionck errors are supposed in a different way.

pnkfelix (May 17 2019 at 11:20, on Zulip):

("suppressed"? )

lqd (May 17 2019 at 11:22, on Zulip):

so just removing this, called 20 lines earlier, would have no impact on regionck errors, while borrowck mode would have ?

Matthew Jasper (May 17 2019 at 11:23, on Zulip):

@pnkfelix yes

pnkfelix (May 17 2019 at 11:23, on Zulip):

@lqd I think that's right. And I think this is one expected cause of deviation between the two PR's we're looking for.

Matthew Jasper (May 17 2019 at 11:24, on Zulip):

Correct, regionck errors use the SuppressRegionErrors type (I'm on mobile so you'll have to search for it yourself).

lqd (May 17 2019 at 11:24, on Zulip):

alright thank you both :)

lqd (May 17 2019 at 11:25, on Zulip):

I'l look for one of the regionck issues test, just to check it does what we want and then open the PR

Matthew Jasper (May 17 2019 at 11:27, on Zulip):

You can look through the nll compare mode pr to see some examples.

lqd (May 17 2019 at 11:28, on Zulip):

(for those following at home this is where regionck errors are suppressed according to borrowck mode)

lqd (May 17 2019 at 12:28, on Zulip):

alright I opened #60911 and #60914 to be double checked

lqd (May 17 2019 at 12:30, on Zulip):

also I do not have try nor crater permissions to actually launch the runs

lqd (May 17 2019 at 12:31, on Zulip):

finally: I remember last summer we ran crater with cap-lints=warn and wondered whether we needed this still (I think so but I'm not sure)

lqd (May 17 2019 at 19:06, on Zulip):

@Matthew Jasper thanks for taking care of the try/crater commands (I've noticed craterbot can now detect the try build commit so we don't have to input them anymore)

Matthew Jasper (May 19 2019 at 09:10, on Zulip):

ouch: https://github.com/rust-lang/rust/pull/60914#issuecomment-493739422

centril (May 19 2019 at 10:04, on Zulip):

The good news is that many of these are not on crates.io

Matthew Jasper (May 19 2019 at 10:09, on Zulip):

Yes, and a lot are due to a few key crates that may need new patch versions or backported patch versions.

lqd (May 19 2019 at 10:30, on Zulip):

we're seeing a lot of the same ones as last year

lqd (May 19 2019 at 10:34, on Zulip):

I'll triage them similarly to last time ā€” but yeah it looks very similar, a few key crates count heavily, eg servo's url has a lot of dependent crates, creating hundreds of downstream regressions, for all the versions before the issue was fixed. nalgrebra also does appear a lot.

Matthew Jasper (May 19 2019 at 10:37, on Zulip):

Does it really count as a regression if running cargo update fixes the problem? :stuck_out_tongue_wink:

lqd (May 19 2019 at 10:38, on Zulip):

:)

Jake Goulding (May 19 2019 at 11:47, on Zulip):

I get the point, but it does affect people negatively if cargo update needs to cross a semver boundary and then your code fails for other reasons

Jake Goulding (May 19 2019 at 11:47, on Zulip):

Which is why the "backported patch" can be so important

lqd (May 19 2019 at 12:51, on Zulip):

I think there's around 130 root regressions

lqd (May 19 2019 at 12:52, on Zulip):

with the vast majority being the same crates and errors as before of course

lqd (May 19 2019 at 13:31, on Zulip):

that is, these 87 crates cause 1859 crates to fail downstream, while these 54 crates fail to build themselves (there is some overlap between these 2 categories)

Matthew Jasper (May 19 2019 at 13:39, on Zulip):

All of those crates that would probably be fixed by updating URL...

lqd (May 19 2019 at 13:41, on Zulip):

yeah url being so widely used, accounts for 900-1000 errors by itself

lqd (May 19 2019 at 13:44, on Zulip):

(and they're in the same major version so cargo updating should work indeed)

lqd (May 22 2019 at 10:01, on Zulip):

ah the other run #60911 is also done. From a quick glance it looks like a hundred more root crates in errors or so than #60914. I'll triage soon and also point out the crates differing between the 2 runs

lqd (May 22 2019 at 10:11, on Zulip):

Iā€™m not sure what kind of data the lang team is looking for, though (hopefully the count/crate/deps list matches at least partially)

pnkfelix (May 22 2019 at 10:17, on Zulip):

Was one of the goals to attempt to address fallout by submitting patches to relevant crates?

pnkfelix (May 22 2019 at 10:17, on Zulip):

I feel like I should know the "standard procedure" here when it comes to fallout from crater runs, but I do not.

pnkfelix (May 22 2019 at 10:18, on Zulip):

(part of the problem is that crater in meant, in part, to give an estimate of fallout across the ecosystem as a whole beyond craters. But it is also meant to just let us know what crates we might fix.)

lqd (May 22 2019 at 10:19, on Zulip):

Was one of the goals to attempt to address fallout by submitting patches to relevant crates?

possibly, yeah. #60680 mostly mentions "just to get a rough idea of how many affected crates there will be"

lqd (May 22 2019 at 10:20, on Zulip):

a lot of the most depended on crates have been since fixed it seems, but the ecosystem sometimes depends on older versions

pnkfelix (May 22 2019 at 10:21, on Zulip):

[...] but the ecosystem sometimes depends on older versions

yeah. I've seen references to people saying that one approach to that is to submit patches to old versions

pnkfelix (May 22 2019 at 10:21, on Zulip):

e.g. when there is a 2.0 and a 3.0 release, and 2.0 breaks, send in a 2.1 patch

pnkfelix (May 22 2019 at 10:22, on Zulip):

of course doing that still requires that the ecosystem dependencies be written in a way that allows for minor version upgrades.

centril (May 22 2019 at 10:31, on Zulip):

(here is where I make a plug for the min lint levels... =P)

centril (May 22 2019 at 10:31, on Zulip):

but yeah, I think we wanted a rough idea

centril (May 22 2019 at 10:32, on Zulip):

I'd suggest creating issues or something... PRs seems too much work for so many crates?

Matthew Jasper (May 22 2019 at 11:44, on Zulip):

The common crates on crates.io should be fixed though.

lqd (May 22 2019 at 12:48, on Zulip):

quick triage: 99 crates causing 1892 failures downstream, 140 crates themselves failing to build. 220 crates in total (all the 131 in #60914 + these 89 new ones)

lqd (May 22 2019 at 16:06, on Zulip):

I guess we can close the 2 PRs ?

centril (May 23 2019 at 06:01, on Zulip):

Yeah I think we can close them; the results aren't going anywhere ^^

centril (May 23 2019 at 06:02, on Zulip):

@lqd if you didn't already, can you add the results of triage to GH?

lqd (May 23 2019 at 06:40, on Zulip):

@centril I added them to each PR already, to show they were "not as bad as the many :eyes: emoji made them look" :) I'll copy them over to the initial issue and close the PRs ā€” done

centril (May 23 2019 at 06:50, on Zulip):

:heart:

Last update: Nov 21 2019 at 13:35UTC