Stream: t-compiler/wg-nll

Topic: #54528 3rd round of {,.nll}.stderr review


pnkfelix (Oct 08 2018 at 09:45, on Zulip):

Hey @davidtwco, out of curiosity, did we not create new issues when warranted on #52663 ?

pnkfelix (Oct 08 2018 at 09:45, on Zulip):

/me goes to do a quick skim

davidtwco (Oct 08 2018 at 09:46, on Zulip):

We did for a handful of them.

pnkfelix (Oct 08 2018 at 09:46, on Zulip):

I think my main problem is that I wanted some way to keep track of partial progress through the list of deviations

davidtwco (Oct 08 2018 at 09:46, on Zulip):

I started working through the list again last week and there were a handful that were already fixed or the issues had been closed but the table not updated.

pnkfelix (Oct 08 2018 at 09:47, on Zulip):

which was sort of handled by the "Need Further Review" bucket on #52663

pnkfelix (Oct 08 2018 at 09:47, on Zulip):

yeah I see; keeping the table up to date is indeed a problem

davidtwco (Oct 08 2018 at 09:47, on Zulip):

It might make sense to do that in a comment on the “do another review” issue.

pnkfelix (Oct 08 2018 at 09:48, on Zulip):

okay that sounds good to me: to try to keep the gross summary of progress and work items on #54528 itself

pnkfelix (Oct 08 2018 at 09:48, on Zulip):

but to try to avoid keeping too much detail about the specific deviations that need addressing

pnkfelix (Oct 08 2018 at 09:48, on Zulip):

so what's the deal with this project thing...

davidtwco (Oct 08 2018 at 09:48, on Zulip):

Having issues for the buckets means that they can be closed by PRs that resolve them.

pnkfelix (Oct 08 2018 at 09:49, on Zulip):

where do the categories come from .. . oh I see, each project gets to make its own set of "Cards" ?

davidtwco (Oct 08 2018 at 09:50, on Zulip):

Here’s an example of one: https://github.com/rust-lang/rust/projects/8

davidtwco (Oct 08 2018 at 09:50, on Zulip):

I just think it would be a nice way to view it.

pnkfelix (Oct 08 2018 at 09:50, on Zulip):

yeah I'm skimming over https://github.com/rust-lang/rust/projects/9

davidtwco (Oct 08 2018 at 09:50, on Zulip):

Though it’s not that important, GitHub issues are more than fine.

pnkfelix (Oct 08 2018 at 09:50, on Zulip):

(since I'm more immediately familiar with a few of the issues there)

davidtwco (Oct 08 2018 at 09:50, on Zulip):

I’m not sure @nikomatsakis is a fan of projects.

pnkfelix (Oct 08 2018 at 09:51, on Zulip):

the other question is

pnkfelix (Oct 08 2018 at 09:51, on Zulip):

What PR's are currently up that I'd like to land before I start this round of review

pnkfelix (Oct 08 2018 at 09:51, on Zulip):

I know you have some

pnkfelix (Oct 08 2018 at 09:52, on Zulip):

which maybe you can just answer the Q here instead of me putting it up a third time: Is there a reason you've been restricting your filters to only work on Projections whose .base is a Local ?

davidtwco (Oct 08 2018 at 09:53, on Zulip):

If you look for PRs that I’m the author of then there’s a handful.

davidtwco (Oct 08 2018 at 09:53, on Zulip):

Not particularly, that’s just how I’ve been doing it. Normally I’ll need a local to find a type or something.

pnkfelix (Oct 08 2018 at 09:53, on Zulip):

(for example, are the resulting diagnostics misleading or just worse if you try to generalize to a .base that is an arbitrary Place ?)

pnkfelix (Oct 08 2018 at 09:53, on Zulip):

You can ask a Place for its type

pnkfelix (Oct 08 2018 at 09:54, on Zulip):

its not the most obvious thing, but the .place_ty method takes a tcx and gives back a PlaceTy enum that can then be turned into a Ty

davidtwco (Oct 08 2018 at 09:54, on Zulip):

If you’ve mentioned that before and I’ve not been making sure I don’t do that in other PRs then my bad.

pnkfelix (Oct 08 2018 at 09:54, on Zulip):

(the reason we have the PlaceTy enum to be able to keep track of more specific info when the Place is a projection into an Enum variant.)

davidtwco (Oct 08 2018 at 10:14, on Zulip):

Anyway, #54831, #54825, #54802 and #54848 are the current diagnostic PRs I've got open and I'm still working on one of the buckets in the previous review.

davidtwco (Oct 08 2018 at 10:27, on Zulip):

@pnkfelix fixed both of those PRs.

pnkfelix (Oct 08 2018 at 11:45, on Zulip):

@davidtwco that's great, thanks for following up

pnkfelix (Oct 24 2018 at 12:39, on Zulip):

I'm barely scratching the surface of this, and yet I'm already considering whether it would save time overall for me to first pause and look into resolving some of the instances of "NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL" #53004 that I am encountering...

pnkfelix (Oct 24 2018 at 12:59, on Zulip):

(and then I realized/remembered that PR #55221 should address a number of instances of #53004)

pnkfelix (Oct 29 2018 at 11:48, on Zulip):

Hey is there anyone looking for something to do?

pnkfelix (Oct 29 2018 at 11:48, on Zulip):

I ask because I am building up a queue of "investigate further" issues during my sweep of the "compare .stderr to .nll.stderr output files"

pnkfelix (Oct 29 2018 at 11:49, on Zulip):

link to "investigate further" column

davidtwco (Oct 29 2018 at 11:49, on Zulip):

I'll have some time this evening.

pnkfelix (Oct 29 2018 at 11:49, on Zulip):

(there's also one potentially quite serious issue that's in a different column but I think I might look at that one myself later today...)

davidtwco (Oct 29 2018 at 11:50, on Zulip):

It looks like there aren't many issues coming out of the review though, which is good - a handful of potential improvements and some redundancies.

davidtwco (Oct 29 2018 at 11:52, on Zulip):

(I guess it's still early days, didn't see the "yet to be looked at" column)

pnkfelix (Oct 29 2018 at 11:52, on Zulip):

yes. I'm not surprised by this outcome, but it nonetheless makes me happy

pnkfelix (Oct 29 2018 at 11:52, on Zulip):

I am very glad we switched to testing the migrate mode

pnkfelix (Oct 29 2018 at 11:53, on Zulip):

I probably should try to keep track of how fast I am churning through the todo list

pnkfelix (Oct 29 2018 at 11:53, on Zulip):

to figure out if its worth trying to delegate the load

pnkfelix (Oct 29 2018 at 12:30, on Zulip):

(i'm also very close to suggesting that we hack the compare-mode=nll to do some sort of "normalization"... ignore trivial span differences, ignore distinctions like "use of ..." vs "borrow of ..." ...)

davidtwco (Nov 03 2018 at 18:05, on Zulip):

@pnkfelix I'm thinking that it might be worth archiving the cards on the diagnostic review that are addressed - so straight away archiving the "equivalent" or "improvement" columns and then any of the "migrate downgrade" cards that your PR for that fixed. Would be helpful for only looking over things that need work.

pnkfelix (Nov 03 2018 at 19:32, on Zulip):

Where does archived stuff go?

davidtwco (Nov 03 2018 at 20:27, on Zulip):

I’m not sure. Presumably there’s some option to show them again.

pnkfelix (Nov 05 2018 at 10:05, on Zulip):

lets figure out the answer to that question first. Then we can consider adopting the protocol you suggest.

davidtwco (Nov 05 2018 at 10:06, on Zulip):

So, I looked into this a little. There's a option to show archived cards and restore them. Apparently a card was archived six days ago. pasted image

pnkfelix (Nov 05 2018 at 11:08, on Zulip):

hmm okay. I have now figured out how to access that list of Archived Cards

pnkfelix (Nov 05 2018 at 11:09, on Zulip):

So yes, I think it sounds good to adopt the protocol you suggest. But first I want to make sure that the single currently archived card actually belongs there

davidtwco (Nov 05 2018 at 11:10, on Zulip):

I'm not sure how you found the process for using a project while doing the review. But being able to pick a card, click "convert to issue", and then have a easy issue number to refer to it and close via a PR and then just archiving that card is a nicer workflow from the "working through the review items" point of view compared to the issue w/ comments we had.

pnkfelix (Nov 05 2018 at 15:31, on Zulip):

the project system is okay i suppose. I prefer text-based interactions to mouse-based ones when it comes to really repetitive stuff, so that was the main drawback I was able to identify for my own personal workflow compared to updating comments on a issue. (However, I did note that updating comments is not really workable once a comment gets too long, and that thus invalidates the claim that a text-based workflow is actually usable in practice...)

pnkfelix (Nov 05 2018 at 15:33, on Zulip):

I will say that its been good to actually go through the cards and archive them (and link to the relevant PR or Issue where they are now addressed), since there was at least one test that had a card that I failed to transcribe into #55533 when I made the test list there.

davidtwco (Nov 05 2018 at 15:34, on Zulip):

(deleted)

pnkfelix (Nov 05 2018 at 15:39, on Zulip):

Oops, sorry: @davidtwco I added another commit to PR #55700

pnkfelix (Nov 05 2018 at 15:39, on Zulip):

I'll stop doing that now, I didn't realize you were going to get to the review so fast.

davidtwco (Nov 05 2018 at 15:40, on Zulip):

Feel free to keep going, I can take a look again later.

davidtwco (Nov 05 2018 at 15:40, on Zulip):

Was going to wait for a Travis pass before pinging bors anyway.

pnkfelix (Nov 05 2018 at 15:44, on Zulip):

I suppose my biggest question about project workflow is that I currently take the approach of treating cards like lightweight entities

pnkfelix (Nov 05 2018 at 15:44, on Zulip):

and so I often end up with several cards that are all related; i.e. they want to be handled by a single Github issue

pnkfelix (Nov 05 2018 at 15:45, on Zulip):

I don't know whether this is a case where I am simply wrong to treat cards as lightweight, or if I should hope/expect Github to make further enhancements that would allow me to more easily group sets of cards and then assign all of them to a single issue

davidtwco (Nov 05 2018 at 15:45, on Zulip):

In those cases, I'd edit them to reference the same issue; in other cases where a card corresponds to a single unit of work (like the unions one I looked at over the weekend), then I'd can convert to issue.

davidtwco (Nov 05 2018 at 15:46, on Zulip):

It would be nice to have more grouping/bulk selection. Or even a way of adding some sort of checklist items to a single card.

pnkfelix (Nov 05 2018 at 15:46, on Zulip):

Yes some sort of checkmark based selection of sets of cards could be useful for mass modifications

pnkfelix (Nov 05 2018 at 15:47, on Zulip):

so far I've been getting by with using search for filters

davidtwco (Nov 05 2018 at 15:47, on Zulip):

I'm going to move the empty columns to the end.

davidtwco (Nov 05 2018 at 15:48, on Zulip):

I think in most cases, GitHub probably expected that projects would have issues as cards rather than lots of notes.

pnkfelix (Nov 05 2018 at 15:48, on Zulip):

I'll go a step further and try to put the higher priority columns at the beginning

davidtwco (Nov 05 2018 at 15:49, on Zulip):

That's not really viable in our case with all the tests, but I can see how it would probably work a little nicer.

davidtwco (Nov 05 2018 at 15:49, on Zulip):

Should the "expected enhancement" cards be archived?

davidtwco (Nov 05 2018 at 15:49, on Zulip):

There's no work to be done for those?

pnkfelix (Nov 05 2018 at 15:50, on Zulip):

I think it would be good to change those to use revisions (if they don't already) and add // ignore-compare-mode-nll

pnkfelix (Nov 05 2018 at 15:51, on Zulip):

basically, I'd like the expected changes injected by NLL to be flagged explicitly in the test code, via // revisions: ast nll and then associated //[nll]~ ERROR ... annotations.

pnkfelix (Nov 05 2018 at 15:51, on Zulip):

you can see some instances of this pattern in PR #55700

davidtwco (Nov 05 2018 at 15:51, on Zulip):

We could probably hide the "motivation" card on the leftmost column too since the project description has that information (in the "Projects" listing; or by pressing "menu" when in the project).

pnkfelix (Nov 05 2018 at 15:52, on Zulip):

ah I was just musing about where that text could go

davidtwco (Nov 05 2018 at 15:53, on Zulip):

I think I prefer having compare modes over revisions, it'd be nice to be able to do the error annotations for the compare mode rather than need to switch everything to revisions.

davidtwco (Nov 05 2018 at 15:53, on Zulip):

I guess there's not that much of a difference though.

davidtwco (Nov 05 2018 at 15:55, on Zulip):

(out of scope, but it'd be nice if compare modes were just "global revisions" that applied implicitly to every test, then you'd get that for free)

pnkfelix (Nov 05 2018 at 15:58, on Zulip):

hmm. And an error that is meant to apply to the base mode alone would be annotated with, what, //[]~ ERROR ... ?

davidtwco (Nov 05 2018 at 15:58, on Zulip):

Or just //~ ERROR.

pnkfelix (Nov 05 2018 at 15:58, on Zulip):

(vs //~ ERROR ... which I would like to apply to all of the modes under comparison)

davidtwco (Nov 05 2018 at 15:59, on Zulip):

Ah, yeah, that's a good idea.

davidtwco (Nov 05 2018 at 15:59, on Zulip):

//[]~ ERROR or //[default]~ ERROR would work fine.

pnkfelix (Nov 05 2018 at 15:59, on Zulip):

I mean, if you don't do that, then you hit a huge annotation burden...

davidtwco (Nov 05 2018 at 15:59, on Zulip):

Yeah, I hadn't thought about that. Makes sense.

davidtwco (Nov 05 2018 at 16:01, on Zulip):

I prefer the x.py output for compare modes where it runs one compare mode and then the next over that of revisions where each revision is run one after another before the next test. In my ideal situation it'd run each revision one at a time, with only the N tests that have that revision (where N = all tests if it's a "global revision").

pnkfelix (Nov 05 2018 at 16:02, on Zulip):

hmm, why do you prefer that?

davidtwco (Nov 05 2018 at 16:02, on Zulip):

I think I just like knowing when I watch it run that the ast compare mode should all pass, but some of nll might not, and then seeing that confirmed as it goes.

davidtwco (Nov 05 2018 at 16:03, on Zulip):

There's no functional difference.

pnkfelix (Nov 05 2018 at 16:03, on Zulip):

I don't know if I have a preference. When I'm working on fine-tuning the tests, after the first test run, future runs tend to have a huge set of ignored tests, so I guess I haven't noticed too much of a difference.

pnkfelix (Nov 05 2018 at 16:03, on Zulip):

Well you do have to get the whole AST suite passing before you get any feedback about the NLL compare mode ones

davidtwco (Nov 05 2018 at 16:03, on Zulip):

Yeah, I'm not really sure why I prefer it.

davidtwco (Nov 05 2018 at 16:04, on Zulip):

I guess I just like seeing the separation.

pnkfelix (Nov 05 2018 at 16:04, on Zulip):

(which I then take to be an advantage of revisions over compare mode; but thats minor, and I suppose easy to bypass by just feeding in an explicit subset of the tests)

pnkfelix (Nov 05 2018 at 16:04, on Zulip):

anyway I'm basically falling back on "I think I don't have a preference..." :smile:

nikomatsakis (Nov 08 2018 at 16:05, on Zulip):

(vs //~ ERROR ... which I would like to apply to all of the modes under comparison)

this would be great, @pnkfelix, I've wanted this before — also it's surprising today because it's basically just ignored

pnkfelix (Nov 08 2018 at 16:06, on Zulip):

yeah I think I filed an issue for that too

pnkfelix (Nov 08 2018 at 16:07, on Zulip):

this one: "compiletest: tests using revisions treat files with just //~ ERROR has having no errors" #55695

Last update: Nov 21 2019 at 13:10UTC