Stream: t-compiler/wg-nll

Topic: #55118 migrate compare mode


davidtwco (Oct 16 2018 at 21:58, on Zulip):

@pnkfelix do we want to replace the nll compare mode? Or do we want to add a new compare mode that is now the default - that way there's still a "pure nll" option available?

pnkfelix (Oct 16 2018 at 21:59, on Zulip):

I think we should just replace the current compare mode

pnkfelix (Oct 16 2018 at 21:59, on Zulip):

I don't want to take the time for something more invasive

davidtwco (Oct 16 2018 at 21:59, on Zulip):

The only downside I can see to keeping the nll option around is that it might mean those .nll.stderr files get out of date.

davidtwco (Oct 16 2018 at 21:59, on Zulip):

I have it working locally keeping the nll mode and adding a new default migrate mode.

davidtwco (Oct 16 2018 at 21:59, on Zulip):

Just wondering if that's how we want to do it.

davidtwco (Oct 16 2018 at 21:59, on Zulip):

Easy enough to change.

pnkfelix (Oct 16 2018 at 22:00, on Zulip):

No I wanted to keep the name the same

davidtwco (Oct 16 2018 at 22:00, on Zulip):

Alright, I'll change it to that.

pnkfelix (Oct 16 2018 at 22:00, on Zulip):

like, literally most of the .nll.stderr files will just get re-used as is, I think

pnkfelix (Oct 16 2018 at 22:00, on Zulip):

the main exceptions are: 1. cases where the errors are downgraded to warnings

pnkfelix (Oct 16 2018 at 22:01, on Zulip):

and 2. cases where the old region code enabled via borrowck=migrate causes new errors to be emitted

pnkfelix (Oct 16 2018 at 22:01, on Zulip):

If you encounter a case where the change from error to warning seems really bad

pnkfelix (Oct 16 2018 at 22:02, on Zulip):

then I would change that test (or those tests, if there's >1) to use the revision system

davidtwco (Oct 16 2018 at 22:02, on Zulip):

So far, the only problematic cases are where there is a forbid attribute making a warning an error - which is downgraded again because there isn't a signal_error call where a lint is emitted. So trying to work out if there is a way to check the lint level for UNUSED_MUT at the emit site so I can conditionally signal the error.

pnkfelix (Oct 16 2018 at 22:03, on Zulip):

then I would change that test (or those tests, if there's >1) to use the revision system

this test is an example of what I mean by "the revision system": https://github.com/rust-lang/rust/blob/8a7048b72b7e9499dfae3f946baa92fc9e62a6b8/src/test/ui/borrowck/issue-45983.rs#L19

pnkfelix (Oct 16 2018 at 22:04, on Zulip):

Can you point me to a specific instance where the forbid thing you describe is arising?

pnkfelix (Oct 16 2018 at 22:04, on Zulip):

that may be an example of something very wrong in the way I implemented the migration strategy...

davidtwco (Oct 16 2018 at 22:05, on Zulip):

This is an example.

davidtwco (Oct 16 2018 at 22:05, on Zulip):

There are only three cases.

pnkfelix (Oct 16 2018 at 22:05, on Zulip):

give me a second, I want to see how this behaves

pnkfelix (Oct 16 2018 at 22:06, on Zulip):

This is bad

pnkfelix (Oct 16 2018 at 22:06, on Zulip):

This is like, an example of something we should fix

pnkfelix (Oct 16 2018 at 22:06, on Zulip):

since it represents a change of behavior between edition 2015 and edition 2018

pnkfelix (Oct 16 2018 at 22:06, on Zulip):

playground

davidtwco (Oct 16 2018 at 22:07, on Zulip):

Yeah, I'm trying to spot a nice way to fix it.

pnkfelix (Oct 16 2018 at 22:07, on Zulip):

I think you should file a separate bug for this

pnkfelix (Oct 16 2018 at 22:07, on Zulip):

we don't have to resolve it as part of the PR that changes compare-mode=nll

davidtwco (Oct 16 2018 at 22:08, on Zulip):

I won't be able to land this without fixing it as Travis wouldn't pass, and the changes involved in this are relatively minor, I don't think it would be too big a deal to fix it in this.

pnkfelix (Oct 16 2018 at 22:08, on Zulip):

we can make this test use the revision system instead (and have it do // ignore-compare-mode-nll

pnkfelix (Oct 16 2018 at 22:08, on Zulip):

and I still think there's mucho value in separating the two things here

davidtwco (Oct 16 2018 at 22:08, on Zulip):

Sure thing, I'll file an issue.

pnkfelix (Oct 16 2018 at 22:09, on Zulip):

if the change really is trivial, then okay. But make sure its a totally separate commit from the rest of the commits in the PR

pnkfelix (Oct 16 2018 at 22:09, on Zulip):

(and as you can tell, I would be fine with the fix to this being a separate PR entirely)

pnkfelix (Oct 16 2018 at 22:10, on Zulip):

Anyway, thanks for looking at this. Issues like this are exactly why I'm happy we are addressing this now, and not a week from now. :smile:

davidtwco (Oct 16 2018 at 22:16, on Zulip):

Tons of diagnostics get way worser with this mode :frown:

pnkfelix (Oct 16 2018 at 22:19, on Zulip):

That's useful to know

pnkfelix (Oct 16 2018 at 22:19, on Zulip):

does it seem like its due to us running the old region code?

pnkfelix (Oct 16 2018 at 22:20, on Zulip):

i.e. did you peek at what the .stderr files look like for the AST-borrowck ?

pnkfelix (Oct 16 2018 at 22:20, on Zulip):

or are the regressions in diagnostic quality not immediately attributable to AST-borrowck stufF?

pnkfelix (Oct 16 2018 at 22:20, on Zulip):

(also, make sure that you kept -Z two-phase-borrows turned on in whatever you did

davidtwco (Oct 16 2018 at 22:21, on Zulip):

It's the old region code.

davidtwco (Oct 16 2018 at 22:21, on Zulip):

It still has -Z two-phase-borrows.

pnkfelix (Oct 16 2018 at 22:21, on Zulip):

okay. Well the region code is ... at least not a regression from AST-borrowck, right?

pnkfelix (Oct 16 2018 at 22:22, on Zulip):

If there's some case where the combination of the old region code with the new MIR-borrowck diagnostics ends up looking worse than either of AST-borrowck or MIR-borrowck alone, then we definitely want to know that ...

davidtwco (Oct 16 2018 at 22:22, on Zulip):

No, it's the same as it was with the AST borrow checker. It's just a shame to see the nice NLL errors being removed in the diffs.

pnkfelix (Oct 16 2018 at 22:22, on Zulip):

(unfortunately I cannot think of a good way to automatically flag such cases)

davidtwco (Oct 16 2018 at 22:23, on Zulip):

This also complicates the diagnostic review since you won't be seeing our actual NLL output.

pnkfelix (Oct 16 2018 at 22:24, on Zulip):

well that may be an argument for adopting separate compare-modes eventually. Though I personally suspect the better way to go is to either 1. use #![feature(nll)] on individual tests, or 2. use the revision system to opt into it.

pnkfelix (Oct 16 2018 at 22:24, on Zulip):

(By "better", I here mean "less likely to cause non-NLL developers to gnash their teeth at how long tests are taking")

pnkfelix (Oct 16 2018 at 22:25, on Zulip):

Do you have an estimate of how many tests seriously regress due to this?

davidtwco (Oct 16 2018 at 22:25, on Zulip):

Yeah, there's not a nice solution without making more tests run, which isn't nice.

pnkfelix (Oct 16 2018 at 22:25, on Zulip):

Because another option we could consider would be to stop running the old region code

davidtwco (Oct 16 2018 at 22:25, on Zulip):

I get 207 failures - three of which are the lint issue.

pnkfelix (Oct 16 2018 at 22:25, on Zulip):

I wouldn't want to do that for RC2, its too close. but we could maybe do it for Release.

davidtwco (Oct 16 2018 at 22:26, on Zulip):

In fact, there are some cases where there are new errors.

pnkfelix (Oct 16 2018 at 22:26, on Zulip):

oh really, only 207 [ui (nll)] tests didn't match after your change?

davidtwco (Oct 16 2018 at 22:26, on Zulip):

Yeah.

pnkfelix (Oct 16 2018 at 22:26, on Zulip):

That's promising. I had worried it would be more.

davidtwco (Oct 16 2018 at 22:26, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/lint-unused-mut-self.rs - this test went from three errors to four.

pnkfelix (Oct 16 2018 at 22:27, on Zulip):

I think I have an issue about this somewhere

pnkfelix (Oct 16 2018 at 22:27, on Zulip):

"NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL" #53004

pnkfelix (Oct 16 2018 at 22:27, on Zulip):

may or may not be related

davidtwco (Oct 16 2018 at 22:28, on Zulip):

AST output: https://gist.github.com/davidtwco/30d5f557c940dc0bfb3546262ca02682
NLL output: https://gist.github.com/davidtwco/441bf1fba1372fe022cceb22fa2959d0

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

@pnkfelix Submitted #55134. I fixed the lint issue that we ran into and looked briefly through the test changes and didn't spot any that seemed wrong.

pnkfelix (Oct 16 2018 at 23:05, on Zulip):

great!

pnkfelix (Oct 16 2018 at 23:06, on Zulip):

hmm, so if we don't buffer lints... do they end up getting emitted before errors in the output now?

davidtwco (Oct 16 2018 at 23:07, on Zulip):

We never buffered any of the other lints from NLL. Only that particular one.

pnkfelix (Oct 16 2018 at 23:07, on Zulip):

fair enough

davidtwco (Oct 16 2018 at 23:07, on Zulip):

(there are only three total, so it's not a huge amount anyway)

davidtwco (Oct 16 2018 at 23:08, on Zulip):

Changing that didn't make any test output change though so presumably they always came up before.

pnkfelix (Oct 16 2018 at 23:09, on Zulip):

well, one might be able to construct a test that exposes the difference. But its not worth worrying about...

davidtwco (Oct 16 2018 at 23:10, on Zulip):

Yeah, probably.

davidtwco (Oct 16 2018 at 23:11, on Zulip):

If you'd prefer I do a more involved fix so that AST borrow check is aware when lints it emits become errors then I'd be happy to do so as part of this PR or as a follow-up.

pnkfelix (Oct 16 2018 at 23:11, on Zulip):

No its okay

davidtwco (Oct 16 2018 at 23:11, on Zulip):

Then lints in MIR could be buffered too.

pnkfelix (Oct 16 2018 at 23:11, on Zulip):

I'm currently more wondering how to prioritize this

pnkfelix (Oct 16 2018 at 23:12, on Zulip):

since its going to collide with anything that touches most of these files

davidtwco (Oct 16 2018 at 23:12, on Zulip):

(I'd like it if you did the diagnostic review before landing this so that we get a more accurate idea of where NLL's diagostics are at, I don't mind rebasing for days on end)

pnkfelix (Oct 16 2018 at 23:12, on Zulip):

but no

pnkfelix (Oct 16 2018 at 23:12, on Zulip):

that's part of my point

pnkfelix (Oct 16 2018 at 23:13, on Zulip):

the diagnostic review should come after this change

davidtwco (Oct 16 2018 at 23:13, on Zulip):

(that said, after having rebased this for days on end, I'd probably disagree with current me)

pnkfelix (Oct 16 2018 at 23:13, on Zulip):

I mean, why review the diagnostics for the thing we aren't deploying (deploying first, that is)?

pnkfelix (Oct 16 2018 at 23:13, on Zulip):

(right?)

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

I don't think we're going to invest time in improving any of the errors specific to migrate mode though, are we? It's either good NLL errors that can be improved or AST errors that mask the NLL ones that are in some cases better or needing improvement. I mean, it might be worth making some changes so less NLL errors are masked but other than that..?

pnkfelix (Oct 16 2018 at 23:15, on Zulip):

My short term goal for the diagnostic review is to identify things that should block RC2

davidtwco (Oct 16 2018 at 23:15, on Zulip):

Just my 2c though, I might be way off.

pnkfelix (Oct 16 2018 at 23:16, on Zulip):

which, to me, means checking the migrate mode

pnkfelix (Oct 16 2018 at 23:16, on Zulip):

but I can understand what you are saying

pnkfelix (Oct 16 2018 at 23:17, on Zulip):

... maybe we should try to enlist other voices in this conversation. If nothing else, I'm too tired to really come to a solid conclusion on that matter.

pnkfelix (Oct 16 2018 at 23:17, on Zulip):

@WG-compiler-nll ^

davidtwco (Oct 16 2018 at 23:17, on Zulip):

That makes sense, but that'll either be a review to a NLL error or an AST error? Since migrate mode is one of those two (or a combination of both?). If you identify an improvement to a NLL error, we can improve that. If you identify a improvement to a AST error, we aren't going to improve that - it might mask a NLL error that time was already spent improving?

davidtwco (Oct 16 2018 at 23:18, on Zulip):

I'm not sure.

pnkfelix (Oct 16 2018 at 23:18, on Zulip):

can you do me a favor and make a separate PR for commit d8db5299fae13a5ef321fd8f7be7364d735fecbb ?

pnkfelix (Oct 16 2018 at 23:18, on Zulip):

I think we should make sure that lands

pnkfelix (Oct 16 2018 at 23:18, on Zulip):

and I'm worried that the rest of this is going to be in rebase hell

davidtwco (Oct 16 2018 at 23:18, on Zulip):

Sure thing.

davidtwco (Oct 16 2018 at 23:20, on Zulip):

@pnkfelix #55135

pnkfelix (Oct 16 2018 at 23:21, on Zulip):

That makes sense, but that'll either be a review to a NLL error or an AST error? Since migrate mode is one of those two (or a combination of both?). I

I guess I'm worried about situations like issue #53004 ("NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL"). I.e., cases where migrate mode is behaving in non-intuitive ways

pnkfelix (Oct 16 2018 at 23:22, on Zulip):

i don't want to do another round of review for migrate mode alone after doing a round on NLL alone

davidtwco (Oct 16 2018 at 23:22, on Zulip):

I guess in my mind I hadn't really considered that part of a diagnostic review as it wouldn't really result in any improvements to diagnostics, just improvements to migrate mode?

pnkfelix (Oct 16 2018 at 23:22, on Zulip):

Let me sleep on this. And, more importantly, re-read what you wrote above again in the morning

davidtwco (Oct 16 2018 at 23:23, on Zulip):

(which are improvements to diagnostics of course, but the errors themselves aren't really changing)

pnkfelix (Oct 16 2018 at 23:23, on Zulip):

(I'm still trying to get my head around where doing the review of migrate mode goes wrong ... is it just in the effort involved in trying to tease out whether a given diagnostic is arising from MIR-borrowck vs AST-borrowck? Ah ... good old -Z borrowck=compare ...)

davidtwco (Oct 16 2018 at 23:26, on Zulip):

IMO the nicest solution (although, as you've correctly mentioned, infeasible because it'd mean there'd be more tests getting run and output needing kept up to date) would be to have a pure NLL mode and a migrate mode. A migrate review would only need to look at *.migrate.stderr files and would only consider whether migrate is doing something strange. And then a *.nll.stderr review would just be able to look at diagnostic quality.

davidtwco (Oct 16 2018 at 23:27, on Zulip):

Though, I'm also going to make the caveat that I might read that tomorrow morning and think it's an awful idea because it's late.

nikomatsakis (Oct 17 2018 at 13:20, on Zulip):

I don't think I quite follow what is being debated here. I guess option 1 is:

I see pros/cons to both, but it seems to me that, schedule wise, it is most urgent to ensure migration "seems good", and we can revisit NLL "at our leisure".

To that end, there is a third-way add the NLL compare mode but do not run it on the builders. You can then run it manually to generate the comparison files for the purposes of comparison.

pnkfelix (Oct 17 2018 at 13:21, on Zulip):

Okay then I'm going to see about r+'ing the PR. It probably needs to be rebased already.

davidtwco (Oct 17 2018 at 13:22, on Zulip):

I don't think it does.

davidtwco (Oct 17 2018 at 13:22, on Zulip):

At least, I've not got the usual ping that tells me it does.

davidtwco (Oct 17 2018 at 13:22, on Zulip):

Unless that only happens when r+'d.

davidtwco (Oct 17 2018 at 13:26, on Zulip):

I could pretty quickly add a third compare mode just now if we want - I had that implemented yesterday so I know I'd go about it.

davidtwco (Oct 17 2018 at 13:27, on Zulip):

Only downside of it is that the tests wouldn't assert that all the third-mode.stderr files are up to date.

davidtwco (Oct 17 2018 at 13:27, on Zulip):

So if you ran them on master after waiting a while then it might fail.

nikomatsakis (Oct 17 2018 at 13:30, on Zulip):

my assumption was that you would not check-in the 3rd mode files

nikomatsakis (Oct 17 2018 at 13:30, on Zulip):

but rather run with --bless

nikomatsakis (Oct 17 2018 at 13:30, on Zulip):

and then just discard the results

nikomatsakis (Oct 17 2018 at 13:30, on Zulip):

after doing the review

nikomatsakis (Oct 17 2018 at 13:30, on Zulip):

I don't think we should check in stuff we don't test with the automated builders

davidtwco (Oct 17 2018 at 13:32, on Zulip):

I see, so it would be temporarily enabled just for the review?

nikomatsakis (Oct 17 2018 at 13:32, on Zulip):

right, you just run it locally; of course maybe that's super annoying

nikomatsakis (Oct 17 2018 at 13:32, on Zulip):

but that was my thought

nikomatsakis (Oct 17 2018 at 13:32, on Zulip):

in any case once we ship compare-mode

nikomatsakis (Oct 17 2018 at 13:32, on Zulip):

er

nikomatsakis (Oct 17 2018 at 13:32, on Zulip):

once we ship RC2 I mean

nikomatsakis (Oct 17 2018 at 13:33, on Zulip):

I hope to make migrate-mode the default for all editions fairly soon

nikomatsakis (Oct 17 2018 at 13:33, on Zulip):

at which point "compare-mode" against migrate doesn't even make sense, and we would presumably start doing compare-mode of migrate vs nll

davidtwco (Oct 17 2018 at 17:06, on Zulip):

This PR failed on Travis - it looked spurious but I didn't get a chance to really look at it.

nikomatsakis (Oct 17 2018 at 21:30, on Zulip):

I moved https://github.com/rust-lang/rust/issues/54528 (next round of review) to the Rust 2018 Release milestone btw

Last update: Nov 21 2019 at 15:05UTC