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.
@Albin Stjerna awesome :tada:
I looked at it back in november and once again earlier quickly, but it did look good to me
@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)
I did add a few but I’m not sure I actually test them
It’s probably a good idea to add some :)
This is for the compiletests right?
let me link you to the folder it'll be easier
so that you see the compile flags dance
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 :)
@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 ?
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
(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:)
merged, will work on removing the lalrpop warnings now
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)
it did work this time,
polonius-engine 0.12 published
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
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 ?)
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!
@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)
@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 :)
(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)
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
Because some now fixed errors are expected
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 :)
@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 :)
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
But I’ll check because I don’t remember clearly
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 :)
(and that subset errors needed to be specifically emitted under -Zpolonius, if the situation is identical)
to clear things up I built the PR locally:
the errors are indeed move/init errors
this seems to match what I was saying, that rustc wouldn't emit the move errors that polonius computes ?
@Albin Stjerna does that match your understanding as well ?