Stream: t-compiler/wg-polonius

Topic: meeting 2020.02.18


Albin Stjerna (Feb 17 2020 at 19:39, on Zulip):

Hi! I'm busy tomorrow but I'll report early: I have fixed the errors that CI and @lqd helped alerting me to, and I think the Polonius PR is ready to merge. I have documented all caveats I can think of in polonius#135. My plan is to document everything in the Book in a separate PR, because this one is blocking the outstanding rust#68993.

lqd (Feb 17 2020 at 19:45, on Zulip):

@Albin Stjerna awesome :tada:

lqd (Feb 17 2020 at 19:47, on Zulip):

I looked at it back in november and once again earlier quickly, but it did look good to me

lqd (Feb 17 2020 at 19:50, on Zulip):

@Albin Stjerna I know niko looked it over already but did you happen to add some smoke-tests in the rustc PR btw ? just a few that will be always run (so they have to be fast and not OOM ;) to make sure move errors don't randomly break when some other unrelated code is changed (there are some of those smoke tests for other -Z polonius features in a nll/polonius test subfolder)

Albin Stjerna (Feb 17 2020 at 19:51, on Zulip):

I did add a few but I’m not sure I actually test them

Albin Stjerna (Feb 17 2020 at 19:51, on Zulip):

It’s probably a good idea to add some :)

Albin Stjerna (Feb 17 2020 at 19:52, on Zulip):

This is for the compiletests right?

lqd (Feb 17 2020 at 19:52, on Zulip):

yeah

lqd (Feb 17 2020 at 19:52, on Zulip):

let me link you to the folder it'll be easier

lqd (Feb 17 2020 at 19:52, on Zulip):

so that you see the compile flags dance

lqd (Feb 17 2020 at 19:53, on Zulip):

https://github.com/rust-lang/rust/tree/master/src/test/ui/nll/polonius

lqd (Feb 17 2020 at 19:55, on Zulip):

any one of those failing tests (like move errors) asks to be run under polonius (with or without using the polonius compare mode) to ensure it will succeed/fail even if the compare-mode itself isn't used (which it can't really be, because of the OOMs but that's a different issue :)

lqd (Feb 17 2020 at 22:17, on Zulip):

@Albin Stjerna polonius#135 looks good to me — in any case, whatever nits I may have (and I don't have any) are less important than the fact that the rustc polonius compare-mode tests (minus the ones OOM-ing) pass, and there are some new tests exercizing these awesome new polonius move errors :) since you've designed the rules with niko last time, and this matches closely, I say let's merge now, wdyt ?

lqd (Feb 17 2020 at 22:20, on Zulip):

then I can try to publish (it didn't work last time, but maybe it will this time :) so that the rustc PR is unblocked

lqd (Feb 17 2020 at 22:22, on Zulip):

(when you do update that PR to the future new polonius-engine version and a couple of move errors smoke tests, don't forget to rustfmt that one failing file — and you can then at your leisure @bors r=nikomatsakis,lqd :rocket:)

lqd (Feb 17 2020 at 22:56, on Zulip):

merged, will work on removing the lalrpop warnings now

lqd (Feb 17 2020 at 23:02, on Zulip):

it's only in test code (since the warnings are in the lalrpop generated code from polonius-parser) but still, https://github.com/rust-lang/polonius/pull/142 – I'll merge that once CI passes, to try and publish the new release (hopefully it'll work this time)

lqd (Feb 18 2020 at 00:01, on Zulip):

it did work this time, polonius-engine 0.12 published

lqd (Feb 18 2020 at 15:10, on Zulip):

Albin Stjerna said:

I did add a few but I’m not sure I actually test them

where are those btw ? I don't think I saw them in #68993

lqd (Feb 18 2020 at 15:12, on Zulip):

I didn't see either the places where rustc would read the polonius move errors and emit them ? (maybe this commit is solely dedicated to fact renaming ?)

Albin Stjerna (Feb 19 2020 at 16:14, on Zulip):

I didn't see either the places where rustc would read the polonius move errors and emit them ? (maybe this PR is solely dedicated to fact renaming ? if that's the case, disregard what I've said about adding compiletests to check the emitted move errors of course; I probably misunderstood that this PR was the rustc side of your polonius move error work)

No that’s correct and you’re right; I should add some smoke tests to the Rust PR!

lqd (Feb 19 2020 at 17:36, on Zulip):

@Albin Stjerna what would they test if move errors are not emitted yet ? the existing smoke tests under x.py test will test that "rustc master + polonius 0.11" has the same behaviour as "rustc + your PR + polonius 0.12", and you'd check that the tests under the polonius compare-mode (minus OOMs) behave similarly (as you've already been doing)

lqd (Feb 19 2020 at 17:49, on Zulip):

@Albin Stjerna however, if your PR does make rustc emit the move errors that polonius 0.12 computes -- which is what I'm not sure about, hence the confusion :) the PR description only mentions the fact format, and fact generation to compute move errors -- then yeah for sure we need smoke tests :)

lqd (Feb 19 2020 at 17:50, on Zulip):

(I mean, the move errors could be emitted using the same codepath as other polonius errors; which is something that wasn't possible with subset errors for example)

Albin Stjerna (Feb 19 2020 at 17:52, on Zulip):

Err, I think the new errors are a proper subset of the old errors and that they are emitted using the same code path, which means that the tests as they currently are wouldn’t pass

Albin Stjerna (Feb 19 2020 at 17:53, on Zulip):

Because some now fixed errors are expected

lqd (Feb 19 2020 at 17:54, on Zulip):

yeah I don't know, I can't easily tell for sure as I'm less familiar about them than you are, if they are perfectly matching what is expected or somehow ignored and the existing old codepath emits them equally well :)

lqd (Feb 19 2020 at 18:08, on Zulip):

@Albin Stjerna a possible way to test could be to temporarily disable computing move errors in polonius -- if a given move error test then fails under -Zpolonius, we'll know it works :)

Albin Stjerna (Feb 19 2020 at 18:23, on Zulip):

No wait, now I remember. Move errors should not be reported in Rust, the situation should be identical to subset errors; as far as I remember the responses from Polonius aren’t “connected” to anything

Albin Stjerna (Feb 19 2020 at 18:24, on Zulip):

But I’ll check because I don’t remember clearly

lqd (Feb 19 2020 at 18:24, on Zulip):

polonius sends them to rustc here https://github.com/rust-lang/polonius/blob/master/polonius-engine/src/output/mod.rs#L172-L174, but I don't see rustc reading move_errors hence my confusion :)

lqd (Feb 19 2020 at 18:25, on Zulip):

(and that subset errors needed to be specifically emitted under -Zpolonius, if the situation is identical)

lqd (Feb 20 2020 at 10:58, on Zulip):

to clear things up I built the PR locally:

lqd (Feb 20 2020 at 11:00, on Zulip):
lqd (Feb 20 2020 at 11:01, on Zulip):
lqd (Feb 20 2020 at 11:02, on Zulip):

the errors are indeed move/init errors

lqd (Feb 20 2020 at 11:04, on Zulip):

this seems to match what I was saying, that rustc wouldn't emit the move errors that polonius computes ?

lqd (Feb 20 2020 at 11:11, on Zulip):

@Albin Stjerna does that match your understanding as well ?

Albin Stjerna (Feb 21 2020 at 16:00, on Zulip):

Yes!

Last update: Apr 03 2020 at 17:30UTC