@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?
I think we should just replace the current compare mode
I don't want to take the time for something more invasive
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.
I have it working locally keeping the
nll mode and adding a new default
Just wondering if that's how we want to do it.
Easy enough to change.
No I wanted to keep the name the same
Alright, I'll change it to that.
like, literally most of the
.nll.stderr files will just get re-used as is, I think
the main exceptions are: 1. cases where the errors are downgraded to warnings
and 2. cases where the old region code enabled via borrowck=migrate causes new errors to be emitted
If you encounter a case where the change from error to warning seems really bad
then I would change that test (or those tests, if there's >1) to use the revision system
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.
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
Can you point me to a specific instance where the
forbid thing you describe is arising?
that may be an example of something very wrong in the way I implemented the migration strategy...
There are only three cases.
give me a second, I want to see how this behaves
This is bad
This is like, an example of something we should fix
since it represents a change of behavior between edition 2015 and edition 2018
Yeah, I'm trying to spot a nice way to fix it.
I think you should file a separate bug for this
we don't have to resolve it as part of the PR that changes compare-mode=nll
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.
we can make this test use the revision system instead (and have it do
and I still think there's mucho value in separating the two things here
Sure thing, I'll file an issue.
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
(and as you can tell, I would be fine with the fix to this being a separate PR entirely)
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:
Tons of diagnostics get way worser with this mode :frown:
That's useful to know
does it seem like its due to us running the old region code?
i.e. did you peek at what the .stderr files look like for the AST-borrowck ?
or are the regressions in diagnostic quality not immediately attributable to AST-borrowck stufF?
(also, make sure that you kept
-Z two-phase-borrows turned on in whatever you did
It's the old region code.
It still has
okay. Well the region code is ... at least not a regression from AST-borrowck, right?
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 ...
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.
(unfortunately I cannot think of a good way to automatically flag such cases)
This also complicates the diagnostic review since you won't be seeing our actual NLL output.
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.
(By "better", I here mean "less likely to cause non-NLL developers to gnash their teeth at how long tests are taking")
Do you have an estimate of how many tests seriously regress due to this?
Yeah, there's not a nice solution without making more tests run, which isn't nice.
Because another option we could consider would be to stop running the old region code
I get 207 failures - three of which are the lint issue.
I wouldn't want to do that for RC2, its too close. but we could maybe do it for Release.
In fact, there are some cases where there are new errors.
oh really, only 207
[ui (nll)] tests didn't match after your change?
That's promising. I had worried it would be more.
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.
I think I have an issue about this somewhere
"NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL" #53004
may or may not be related
@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.
hmm, so if we don't buffer lints... do they end up getting emitted before errors in the output now?
We never buffered any of the other lints from NLL. Only that particular one.
(there are only three total, so it's not a huge amount anyway)
Changing that didn't make any test output change though so presumably they always came up before.
well, one might be able to construct a test that exposes the difference. But its not worth worrying about...
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.
No its okay
Then lints in MIR could be buffered too.
I'm currently more wondering how to prioritize this
since its going to collide with anything that touches most of these files
(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)
that's part of my point
the diagnostic review should come after this change
(that said, after having rebased this for days on end, I'd probably disagree with current me)
I mean, why review the diagnostics for the thing we aren't deploying (deploying first, that is)?
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..?
My short term goal for the diagnostic review is to identify things that should block RC2
Just my 2c though, I might be way off.
which, to me, means checking the migrate mode
but I can understand what you are saying
... 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.
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?
I'm not sure.
can you do me a favor and make a separate PR for commit d8db5299fae13a5ef321fd8f7be7364d735fecbb ?
I think we should make sure that lands
and I'm worried that the rest of this is going to be in rebase hell
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
i don't want to do another round of review for migrate mode alone after doing a round on NLL alone
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?
Let me sleep on this. And, more importantly, re-read what you wrote above again in the morning
(which are improvements to diagnostics of course, but the errors themselves aren't really changing)
(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 ...)
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.
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.
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.
Okay then I'm going to see about r+'ing the PR. It probably needs to be rebased already.
I don't think it does.
At least, I've not got the usual ping that tells me it does.
Unless that only happens when r+'d.
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.
Only downside of it is that the tests wouldn't assert that all the third-mode.stderr files are up to date.
So if you ran them on master after waiting a while then it might fail.
my assumption was that you would not check-in the 3rd mode files
but rather run with
and then just discard the results
after doing the review
I don't think we should check in stuff we don't test with the automated builders
I see, so it would be temporarily enabled just for the review?
right, you just run it locally; of course maybe that's super annoying
but that was my thought
in any case once we ship compare-mode
once we ship RC2 I mean
I hope to make migrate-mode the default for all editions fairly soon
at which point "compare-mode" against migrate doesn't even make sense, and we would presumably start doing compare-mode of migrate vs nll
This PR failed on Travis - it looked spurious but I didn't get a chance to really look at it.
I moved https://github.com/rust-lang/rust/issues/54528 (next round of review) to the Rust 2018 Release milestone btw