NLL: need 3rd round of review comparing .stderr and .nll.stderr output

[pnkfelix]

NLL keeps getting better and better. But so does our test suite (or at least the percentage of our test suite that is exercising --compare-mode=nll)!

So once again, we need to do another round of review of the differences between AST borrowck diagnostic output and NLL borrowck diagnostic output.

• see NLL: must review all .nll.stderr files #49862 and NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663 for the previous rounds of review

Review started with the .nll.stderr files in the repository as of commit f99911a Tue Oct 23 17:44:19 2018 +0000

though I am actually going to review the state of these files that results from merging in PR #55221

Each file is mapped to a "card" on the project: https://github.com/rust-lang/rust/projects/10

You can use the search function to find a card (though note that most of the cards use a path like ui/vec/vec-mut-iter-borrow.nll.stderr and the search bar look for text within a token. So you need to type the path starting from ui/... into the search bar. I may go through and add spaces in the paths to counteract this.)

[pnkfelix]

triage of unassigned milestone issues at NLL meeting, assigning to self

[pnkfelix]

Note: This is assigned to me, but I don't actually intend to start working on it there are fewer diagnostic changes in flight. In particular, I at least plan to address #54556 first

[pnkfelix]

(This gets "all the labels" because it is actually a work item that can uncover arbitrary kinds of bugs as one goes through the exercise...)

[davidtwco]

Can we avoid using the same format for the results of this review as we did the last round of review? - the large table and comments was hard to keep up-to-date and not great for discoverability. I'd suggest a bunch of issues and a "NLL Diagnostic Review" project to view them all.

[pnkfelix]

@davidtwco sure I'm happy to take suggestions. The method used in the previous round of reviews ( #52663) was meant to be an improvement on what was done before that (#49862), but I'm not wedded to any particular methodology, as long as I can operate with reasonable efficiency.

In particular, if I happen to follow the methodology from #52663 for expediency's sake (e.g. because I am not familiar with this "project" feature of github), I won't object to someone translating the results into some other format.

[davidtwco]

@pnkfelix In order to use projects, I think we would just need issues. The main issue I found with having one large issue with comments is that it is hard to keep it up to date and know what still needs fixed - if we have issues then they get visited when looking over all the NLL issues and can be closed by PRs.

[nikomatsakis]

I'm bumping this to the release milestone -- it's not going to be an RC2 blocker, I don't think.

[pnkfelix]

okay I'm going to start on this task now. Closing #52663 so that everything will be tracked here (or on the associated project).

[pnkfelix]

The list of tests to look at in the description is "complete", but I then started making individual cards for each test file on the project, thinking that would be a lighter weight method for tracking the tests.

Then I stopped after only a short while, because there are 460 such tests: The problem is that the cards are, AFAICT, only manipulatable via the Web UI (i.e. the mouse, drag-and-drop. But a text box in the description is something that I can cut-and-paste into emacs and edit out of band, far more efficiently. Not sure yet what I'm going to do.

So I clearly haven't made up my mind yet as to whether to encode the to-do list via the checkboxes on this issue's description, or via cards on the project.

---

(And still yet another thought I've had is that the list of differences might drop significantly if I spent a day looking at the issues around #53004, which would probably be a good thing to do before the beta is cut, if possible...)

[pnkfelix]

Oh I should also at least be doing some double-checking atop PR #55221

[pnkfelix]

okay I finished doing the initial survey; see https://github.com/rust-lang/rust/projects/10. (The work-list was in the first columns. All of the .nll.stderr files have been put into categories, which are organized into columns. The diagnostic columns (which in the current presentation are the first three columns) are all cases where none of them are important enough to worry about making a backport to resolve them; in other words they're all relatively low priority IMO.)

But there are other columns.

[pnkfelix]

in particular there is an "investigate further" column with 28 entries. So I'll plan to go back over those next.

[pnkfelix]

T-compiler triage: there are still 27 entries in the "investigate further" column. I'll try to get to them; if I dont resolve them by Monday, I'll unassign myself.

[pnkfelix]

I made a lot of progress on this tonight; there are only three cards left in the "Investigate Further" column.

[pnkfelix]

There are still potential work items on the cards that remain. E.g. I have noted on a number of them that e.g. a given test should be revised to use // revisions rather than compare-mode=nll. But those work items do not block the Release milestone.

[pnkfelix]

Okay I finished the review.

As noted above, there are still work items on the cards on the project (https://github.com/rust-lang/rust/projects/10).

But the main thing I wanted to address for the Release milestone, namely the actual review task, is now dealt with.

[pnkfelix]

marking as NLL-deferred to reflect the lower priority nature of going through the set of work items on the project cards.

[pnkfelix]

the remaining work items do not have NLL-sound issues, so I'm removing that label from this issue.

[pnkfelix]

Re-triaging for #56754. P-medium.

[pnkfelix]

I'd say that since we have now actually removed the AST-borrowck, this is effectively a non-issue anymore.