Listing of the test failures of the polonius compare mode, now that #60171 has merged. (This PR switched the NLL compare-mode from -Zborrowck=migrate
to -Zborrowck=mir
, the polonius compare-mode already being -Zborrowck=mir -Zpolonius
. Otherwise there'd be 200+ compare-mode failures because of the NLL migrate mode default)
Each test failure will have:
when applicable: that is, when the difference is actually meaningful and not to be ignored or --bless
ed.
My current ideas were:
Summary:
Locations:All
hack") and for which I have a local fixNotable PRs:
Tests the special handling for closures escaping the current frame while holding on to things owned by the function.
NLL returns E0373, and Polonius a regular "borrowed value does not live long enough" E0597.
Category: diagnostics.
Resolution: bless the output, and file an issue about the specific diagnostic.
-> blessed in this commit
At first glance, looks like a real NLL/Polonius difference, i.e. a bug (related to loops ?)
The 2 E0502s are the same. Polonius has 2 additional E0499s.
There are other "interesting cases" involving loops: in #8 and #20.
After talking with Matthew: there are problems in fact generation which could be related here (rather than being problems in Polonius per se).
Category: bug in fact generation.
Resolution: As with some of the rand
cases reductions, this seems like missing killed
facts. The Call
terminator doesn't kill the loans of the destination place. StorageDead
doesn't kill the loans either. I have a fix locally.
-> fixed in this commit
Note (unrelated to Polonius): this comment mentions wanting to use loop {}
but the test does not do that.
This is just a difference from the test construction: it's ignored in compare-mode-nll and manually tests the migrate and NLL modes. The polonius compare-mode is therefore compared against the "migrate" revision but ran with full NLLs.
There is no actual difference in the errors output by NLLs and Polonius.
Category: not an issue.
-> ignored in this commit.
NLLs and Polonius have similar errors, but NLL seems to have a special cased diagnostic when returning temporary values ("returns a value referencing data owned by the current function" for the returned expression) which Polonius sees as a regular use ("borrow later used here" for the returned expression) ?
Of course, the former being more specific avoids the latter's more generic and inapplicable "note: consider using a let
binding to create a longer lived value".
In effect, the errors point at different places: NLL's E0515 point to the return of the temporary while Polonius' E0716 points at the creation of the temporary (but also notes with the use/drop). It would be interesting to look at the polonius-engine
errors directly.
Resolution:polonius-engine
returns the errors at the StorageDead
of the temporaries at the end of the scope, which seems correct. So I assume this is just another issue to file about diagnostics under Polonius. It seems ok to bless the output in its current state.
-> blessed in this commit
migrate2015
and nll2015
— outputs from NLL, Polonius This is just a difference from the test construction, it's ignore-compare-mode-nll and manually checks migrate/nll over edition2015/2018.
This failure is because the migrate2015
revision is ran with -Zpolonius
. There is no actual difference in the errors output by NLLs and Polonius.
Category: not an issue.
Resolution: ignore this test with Polonius for now.
-> ignored in this commit.
migrate2018
and nll2018
— outputs from NLL, Polonius This is the same as #5, with a different revision: the migrate2018
revision is ran with -Zpolonius
, creating this failure. There is no actual difference in the errors output by NLLs and Polonius.
Category: not an issue.
Resolution: same as with failure #5 (♫ A little bit of NLLs in my life. A little bit of Polonius by my side. Failure Number Five ♪ — © Lou Bega + lqd 2019)
-> ignored in this commit.
Both have the same 15 errors, but have differences in diagnostics/notes:
NLL:
error[E0502]: cannot borrow `*reg` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-surprise-no-conflict.rs:141:5
|
LL | fn register_plugins<'a>(mk_reg: impl Fn() -> &'a mut Registry<'a>) {
| -- lifetime `'a` defined here
...
LL | reg.register_univ(Box::new(CapturePass::new(®.sess_mut)));
| ^^^^^^^^^^^^^^^^^^-----------------------------------------^
| | | |
| | | immutable borrow occurs here
| | cast requires that `reg.sess_mut` is borrowed for `'a`
| mutable borrow occurs here
Polonius:
error[E0502]: cannot borrow `*reg` as mutable because it is also borrowed as immutable
--> $DIR/two-phase-surprise-no-conflict.rs:141:5
|
LL | reg.register_univ(Box::new(CapturePass::new(®.sess_mut)));
| ^^^^-------------^^^^^^^^^^^^^^^^^^^^^^^^^^^-------------^^^
| | | |
| | | immutable borrow occurs here
| | immutable borrow later used by call
| mutable borrow occurs here
Is this diagnostic special-cased in NLLs for casts as the note mentions ? (is there one cast desugared here ?)
Category: diagnostics.
Resolution: seems ok to bless the polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit
Same errors, but with a difference in diagnostics about the user-provided type annotations for E0597.
NLL:
LL | let x: &'static u32 = {
| ------------ type annotation requires that `y` is borrowed for `'static`
Polonius:
LL | let x: &'static u32 = {
| - borrow later stored here
Category: diagnostics.
Resolution: seems ok to bless the polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit.
Both have the same errors, and are very similar to the "cast requiring 'static borrows" ones from #7, but for E0597 here.
There's a difference in a diagnostic's notes (the "casts" note on O::new()
are sometimes missing in Polonius).
Polonius sometimes also has notes like borrow might be used here, when "o1" is dropped and runs the destructor for type "std::boxed::Box<dyn Obj<'_>>"
, is this a special-cased diagnostic about Box and its destructor in NLLs (so that it doesn't appear in the errors) ?
Category: diagnostics.
Resolution: seems ok to bless the polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit
A test about turning the NLL feature gate on, ignored by the NLL compare-mode.
Category: not an issue.
Resolution: ignore this test with Polonius for now.
-> ignored in this commit.
The NLL error seems special-cased for borrows escaping generators (E0521 instead of Polonius' usual E0597), but the source of error seems the same.
Category: is this just a diagnostics special case, or a difference in behaviour with generators ?
Resolution: seems ok to bless the Polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit.
Once again, the difference is in the test construction, it is ignored in compare-mode NLL and tested manually with revisions, and exists because the migrate
revision is ran with -Zpolonius
. There is no actual difference in the errors output by NLLs and Polonius.
Category: not an issue.
Resolution: as for the other cases, ignore this in compare-mode polonius (and later test polonius with a manual revision).
-> ignored in this commit.
Polonius has 1 error, NLLs have 3, but those 2 missing look to be "fixed by Polonius".
The 2 fixed NLL errors, tagged "Ideally, this would not error.":
error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable
--> $DIR/get_default.rs:21:17
|
LL | fn ok(map: &mut Map) -> &String {
| - let's call the lifetime of this reference `'1`
LL | loop {
LL | match map.get() {
| --- immutable borrow occurs here
LL | Some(v) => {
LL | return v;
| - returning this value requires that `*map` is borrowed for `'1`
...
LL | map.set(String::new()); // Ideally, this would not error.
| ^^^ mutable borrow occurs here
error[E0502]: cannot borrow `*map` as mutable because it is also borrowed as immutable
--> $DIR/get_default.rs:37:17
|
LL | fn err(map: &mut Map) -> &String {
| - let's call the lifetime of this reference `'1`
LL | loop {
LL | match map.get() {
| --- immutable borrow occurs here
...
LL | return v;
| - returning this value requires that `*map` is borrowed for `'1`
...
LL | map.set(String::new()); // Ideally, just AST would error here
| ^^^ mutable borrow occurs here
error: aborting due to 3 previous errors
There is one diagnostic difference with Polonius, which misses the "let's call the lifetime of this reference" notes:
LL | fn err(map: &mut Map) -> &String {
| - let's call the lifetime of this reference `'1`
Category: diagnostics.
Resolution: seems ok to bless the polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit.
References issue #46589 "MIR borrowck doesn't accept the example of iterating and updating a mutable reference"
The history of this issue is a bit hard to pinpoint at first glance: this initial issue expected to work "in the future", and indeed works today in both NLL and Polonius.
An example was added later as a test, and the error which was removed from the NLL output, and caused this issue to be fixed, is present in the Polonius output.
From a cursory look, it feels related to assignment to projections ? (which we don't handle in fact generation yet, so Polonius might be missing the expected killed
facts here).
Particularly, this simple deref bb8[0]: (*_2) = &mut (*_4);
doesn't kill the loans on _2
.
-> Fixed in this commit.
The NLL error in the match statement seems expected to be accepted by Polonius (it's borrowing and killing the same variable) ? Or is that another Polonius bug here ?
Those are the same 2 errors, but in the reverse order.
Category: not an issue.
Resolution: if this order is deterministic, the polonius output can be blessed.
-> However, as of writing this, this test passes now.
Polonius finds an error for each of the invalid data.0 =
assignments, while NLLs maybe only see the 1st one ? This playground seems to validate this hypothesis: commenting the 1st assignment emits only an error for the 2nd one, not the 3rd.
Update: the fact that Polonius emits 3 errors seems like a bug, even if it's interesting. Fixing fact generation of the killed
relation, especially around assignments to projections, makes Polonius output a single error, like NLL. With this fix the Polonius and NLL output are the same.
Category: not an issue.
-> blessed in this commit.
This one looks like #4: NLLs and Polonius have similar errors, but NLLs seem to have a special cased diagnostic when returning temporary values.
Resolution: same as #4, the output seems to only differ in how diagnostics are done over the Polonius output.
-> blessed in this commit
Looks like an "interesting difference". Polonius has one more error, about the assignment to z, which is borrowed in earlier iterations of the loop. Could be related to #2.
Polonius:
error[E0506]: cannot assign to `z` because it is borrowed
--> $DIR/regions-escape-loop-via-vec.rs:6:21
|
LL | let mut z = x;
| ^ assignment to borrowed `z` occurs here
LL | _y.push(&mut z);
| -- ------ borrow of `z` occurs here
| |
| borrow later used here
After talking with Matthew: there are problems in fact generation which could be related here (rather than being problems in Polonius per se).
Category: bug in fact generation.
Resolution: As with some of the rand
cases reductions, this seems like missing killed
facts. The Call
terminator doesn't kill the loans of the destination place. StorageDead
doesn't kill the loans either.
-> fixed in this commit
Looks like a mix between #8, with the diagnostics difference about user-provided type annotations, and #9, with the possible Box special-casing.
There is also another diagnostic difference in the last error (E0506), with
NLL:
LL | factorial = Some(Box::new(f));
| ^^^^^^^^^ assignment to borrowed `factorial` occurs here
Polonius:
LL | factorial = Some(Box::new(f));
| ^^^^^^^^^
| |
| assignment to borrowed `factorial` occurs here
| borrow later used here
in that Polonius displays a "… here" note twice.
Resolution: the errors are at the same location, and only the diagnostics differ. Seems ok to bless the polonius output, and file an issue about this specific diagnostic.
-> blessed in this commit.
rand
crate difference (not a rustc test but something that needs to be done anyway) There is a difference between NLLs and Polonius, found in rand 0.4.6
(extraction, reduction) which Polonius rejects and NLLs accept.
This once again involves loops, like #2 (which also involves the function's parameters like here) and #18.
-> looks very much like issue-47680 but surprisingly Polonius doesn't like it.
After talking with Matthew: there are problems in fact generation which could be related here (rather than being problems in Polonius per se).
Category: bug in fact generation.
Resolution: The Call
terminator doesn't kill the loans of the destination place. StorageDead
doesn't kill the loans either.
-> fixed in this commit
(Added on 2019/06/11)
A test of the NLL migrate mode, ignored in NLL compare mode.
Category: not an issue.
-> ignored in this commit.
(Added on 2019/06/11)
This just lacks the expected json output for -Z polonius
.
Category: not an issue
Resolution: bless the output.
-> blessed in this commit.
(Added on 2019/06/11)
Same as #22, this just lacks the expected json output for -Z polonius
.
Category: not an issue
Resolution: bless the output.
-> blessed in this commit.
(Added on 2019/06/26)
References the issue #38591 "rustc overflows its stack when type checking a simple program polymorphmic recursion"
A case of polymorphic recursion, which passes with NLL, but fails with Polonius with an overflow.
This error also occurs without Polonius when asking to dump the NLL facts, so it seems to be a bug triggered by the fact generation itself.
After talking with Matthew: this can happen easily, and requires a bit more investigation to know if it'd be easy or hard to fix.
Investigation notes:
compute_drop_data
runs the DropckOutlives queryWhy does it only overflow with nll-facts / polonius ?
Fixed in #64749.
(Added on 2019/07/15)
This is just a difference from the test construction: it’s ignored in compare-mode-nll and manually tests the migrate and NLL modes. The polonius compare-mode is therefore compared against the “migrate” revision but ran with full NLLs.
There is no actual difference in the errors output by NLLs and Polonius.
Category: not an issue.
-> ignored in this commit.
At this point the run-pass
test suite was merged into the ui
test suite.
(Added on 2019/09/16)
A new test from the recent async/await test push. Does not have polonius expectations.
This looks like an instance of case #1 but in an async closure. NLL returns E0373, and Polonius a regular “borrowed value does not live long enough” E0597.
Category: diagnostics.
Resolution: bless the output, and deal with this once we're looking at diagnostics.
-> Blessed in this commit.
(Added on 2019/09/16)
This looks like an instance of case #4, a special cased diagnostic when returning temporary values.
Category: diagnostics.
Resolution: bless the output, and deal with this once we're looking at diagnostics.
-> blessed in this commit
(Added on 2019/09/16)
This test was handled before as case #9. Here's the diff over the existing .polonius.stderr expectation.
Looks like some diagnostics differences, and especially some different ordering or naming in the notes, which was changed in this commit and looks wrong.
Category: diagnostics.
Resolution: bless the output, and deal with this once we're looking at diagnostics.
-> blessed in this commit
(Added on 2019/09/16)
This just lacks the expected json output for -Z polonius
.
Category: not an issue
Resolution: bless the output.
-> blessed in this commit
(Added on 2019/09/16)
This just lacks the expected json output for -Z polonius
.
Category: not an issue
Resolution: bless the output.
-> blessed in this commit
(Added on 2019/09/16)
Recent test from #63692
OOM.
memory allocation of 33550368768 bytes failed
Looks similar to the one in case 32.
450 MB NLL MIR.
130752 outlives constraints with no specific location x 170646 points in the CFG.
36337 "live" locals; 4610 regions outliving free regions.
I think in both cases, it's the many variables created by assert_eq! and panic! whose outlives constraints are being materialized. Replacing them with assert!
compiles without OOMs (but slow-ish, 8s on my machine while NLL is around 1s). The NLL MIR dump is 100MB smaller.
(Added on 2019/09/16)
This used to be in the run-pass
and is now in the ui
test suite (and so, was missing from this write-up as it focused on the latter).
The OOM is about materializing a lot of outlives constraints. It's possible this big number is caused by the consts/statics in this test.
10948 outlives constraints x 136664 points in the CFG are materialized here.
(After changing assert_eq!
to assert!
here again, it compiles fast.)
This function contains 161875 regions, of which 391 directly outlive a free region, and 2737 regions do transitively.
(Added on 2019/09/20)
This diagnostics test compiles with NLLs and Polonius, so it's marked ignore-compare-mode-nll
and should be ignored with Polonius mode.
Category: not a Polonius issue (but an NLL one).
Fixed by #64749 for both NLL and Polonius
(Added on 2020/02/28)
This was blessed in #67016 where I've mentioned this was because I believe the difference to be related to general closures handling that would require higher-rankedness help from chalk.
Category: waiting for chalk ?!
Since #69482, there's an additional diagnostics difference: a trivial diagnostics formatting change: diff for NLL output.
Category: not an issue per se.
Resolution: bless the new output
-> blessed in this commit
(Added on 2020/02/28)
A couple of trivial changes which should be blessed:
a grammar and a wording change: diff for NLL output
an interesting formatting change in universal regions relation suggestions:
fn foo4<'a, 'b, 'c>(x: &'a usize) -> (&'b usize, &'c usize) {
// FIXME: ideally, we suggest 'a: 'b + 'c, but as of today (may 04, 2019), the null error
// reporting stops after the first error in a MIR def so as not to produce too many errors, so
// currently we only report 'a: 'b. The user would then re-run and get another error.
(x, x) //~ERROR lifetime may not live long enough
}
As mentioned in the FIXME, under NLL a single suggestion is offered:
= help: consider adding the following bound: `'a: 'b`
while under Polonius 2 errors with both 'a: 'b
and 'a: 'c
are proposed.
This test failed because of a new suggestion in the second error (I think), which combines both suggestions:
help: add bound `'a: 'b + 'c`
that the FIXME mentioned. (The redundant suggestions are still present, as shown below. But overall I believe this is just due to the outlives suggestion mechanism not handling this case correctly yet)
error: lifetime may not live long enough
--> /home/lqd/work/rust/rust/src/test/ui/nll/outlives-suggestion-simple.rs:22:5
|
LL | fn foo4<'a, 'b, 'c>(x: &'a usize) -> (&'b usize, &'c usize) {
| -- -- lifetime `'b` defined here
| |
| lifetime `'a` defined here
...
LL | (x, x) //~ERROR lifetime may not live long enough
| ^^^^^^ returning this value requires that `'a` must outlive `'b`
|
= help: consider adding the following bound: `'a: 'b`
error: lifetime may not live long enough
--> /home/lqd/work/rust/rust/src/test/ui/nll/outlives-suggestion-simple.rs:22:5
|
LL | fn foo4<'a, 'b, 'c>(x: &'a usize) -> (&'b usize, &'c usize) {
| -- -- lifetime `'c` defined here
| |
| lifetime `'a` defined here
...
LL | (x, x) //~ERROR lifetime may not live long enough
| ^^^^^^ returning this value requires that `'a` must outlive `'c`
|
= help: consider adding the following bound: `'a: 'c`
help: add bound `'a: 'b + 'c`
Category: not an issue.
Resolution: bless the new output.
-> blessed in this commit
(Added on 2020/02/28)
Those 2 changes stem from completing move errors in polonius 0.12, and using it in rustc from #69482. They are test failures as Polonius sees no errors in the code, and thus fail with error: ui test compiled successfully!
.
Both tests contain a FIXME about the error, 1 and 2, and Polonius is both fixing those here, but introducing a new one in case #38.
Category: not an issue iff #38 is fixed, otherwise move errors and borrowcking are stepping on each other's toes and it's a bug.
Resolution: polonius bug, move error false positive
-> fixed in polonius-engine
0.12.1 and this commit in fact generation
(Added on 20/02/28)
Same origin as #36 and #37, however the expected borrowck error is not emitted.
Category: polonius bug
Resolution: polonius bug, move error false positive
-> fixed in polonius-engine
0.12.1 and this commit in fact generation
(Added on 20/02/28)
Same grammar change found in #35, diff for NLL output.
Category: not an issue.
Resolution: bless the updated grammar output.
-> blessed in this commit
(Added on 20/02/28)
Some impl Trait
fixes from #67681: better locating the cause of a universal region error with a user annotation – diff for NLL output.
Equally applicable to Polonius.
Category: not an issue.
Resolution: bless the updated output.
-> blessed in this commit
(Added on 20/02/28)
Like #34, this was blessed in #67016 where I mentioned that this test has some diagnostics differences due to:
'static
is handled in Polonius, and how it differs in how NLL produces related errors (possibly creating duplicate errors here) – and intertwined in this test with closures.Category: probably warrants a second opinion and/or more detailed investigation.
Since #69482, there's an additional diagnostics difference: this is a trivial wording change from #35 and #39. The NLL output didn't change, because the new wording is only present in the polonius errors:
50 --> $DIR/closure-substs.rs:29:9
51 |
52 LL | |x: &i32, b: fn(&'static i32)| {
- | - - `b` is declared here, outside of the closure body
+ | - - `b` declared here, outside of the closure body
54 | |
55 | `x` is a reference that is only valid in the closure body
56 LL | b(x);
Category: not an issue per se.
Resolution: bless the updated output.
-> blessed in this commit
(Added on 20/08/06)
A recent test addition, not blessed yet.
Polonius sees one more error than NLL, which is somewhat expected:
Borrowck doesn't get a chance to run, but if it did it should error here.
Category: not an issue.
Resolution: bless the output
(Added on 20/08/06)
Already blessed test. The NLL test output changed with PR #72362 removing ReScope
, and the new Polonius output matches it.
Category: not an issue.
Resolution: bless the output
(Added on 20/08/06)
A recent NLL type-checker bug fix from PR #72493 changed the test output for NLLs, in both migrate mode and full NLLs.
Full NLLs see 3 errors where the migrate mode sees 2, and Polonius only sees the same 2.
The error it misses is:
error: lifetime may not live long enough
--> $DIR/hrtb-just-for-static.rs:30:5
|
LL | fn give_some<'a>() {
| -- lifetime `'a` defined here
LL | want_hrtb::<&'a u32>()
| ^^^^^^^^^^^^^^^^^^^^ requires that `'a` must outlive `'static`
|
= help: consider replacing `'a` with `'static`
for this function.
This looks like a case requiring Chalk integration to be able to correctly detect the error.
Category: Probably wait for chalk ?
Resolution: Bless the output until then ?
(Added on 20/08/06)
Like previously mentioned, and also in #43, Polonius doesn't see the HRTB error. The test fails only because rustc's test output now contains the number of emitted warnings since PR #69926:
-error: aborting due to previous error
+error: aborting due to previous error; 4 warnings emitted
Category: Not an issue
Resolution: Bless the output
(Added on 20/08/06)
This test fails because it involves an HRTB like the previous 2, and Polonius doesn't see the error without Chalk, accepting the code.
I do not understand why this test started to fail, however: it is older than the previous PRs updating Polonius test expectations and should have failed then.
Category: Likely requires chalk
(Added on 20/08/06)
A recent test with a very big MIR if expression, causing an infinite loop or OOM somewhere.
It doesn't occur when just triggering fact generation, which does complete on this test, so it looks to be somewhere in polonius.
Fact generation is however very slow.
Category: Very likely a Polonius bug.
Resolution: investigate precisely which parts of the process cause this issue.
(Added on 20/08/06)
Recent test from the leak-check PR. Ignored in NLL compare-mode because of #73154. The full NLL type-checker currently accepts this case of higher-ranked functions, while the old non-NLL type-checker did not. Polonius of course accepts it as well as it doesn't deal with higher-rankedness.
Category: An issue either in NLL solely, or needing both NLL changes and chalk integration
Resolution: ignore the test in the polonius compare mode
(Added on 20/08/06)
Polonius sees 7 errors where NLL sees 5.
The first one is:
error: lifetime may not live long enough
--> $DIR/region-multiple-lifetime-bounds-on-fns-where-clause.rs:10:5
|
LL | fn b<'a, 'b, 'c>(x: &mut &'a isize, y: &mut &'b isize, z: &mut &'c isize) {
| -- -- lifetime `'c` defined here
| |
| lifetime `'b` defined here
...
LL | *z = *y;
| ^^^^^^^ assignment requires that `'b` must outlive `'c`
|
= help: consider adding the following bound: `'b: 'c`
help: add bound `'b: 'a + 'c`
On this function
fn b<'a, 'b, 'c>(x: &mut &'a isize, y: &mut &'b isize, z: &mut &'c isize) {
// Illegal now because there is no `'b:'a` declaration.
*x = *y; //~ ERROR E0623
*z = *y; //~ ERROR E0623
}
the migrate mode sees the same 2 errors as Polonius, while full NLLs only see one. Since the test expectations require an error on line 10 I'm confused as to how this test passes. Probably the annotations are not checked in compare-mode and only the blessed output is compared.
This looks like a case of full NLLs stopping at the firt error, rather than missing errors because of a bug. So the polonius behaviour here looks correct.
For the 2nd additional error, things are similar: on this function
fn c<'a, 'b, 'c>(x: &mut &'a isize, y: &mut &'b isize, z: &mut &'c isize) {
// Here we try to call `foo` but do not know that `'a` and `'b` are
// related as required.
a(x, y, z); //~ ERROR lifetime mismatch [E0623]
}
with fn a<'a, 'b, 'c>(x: &mut &'a isize, y: &mut &'b isize, z: &mut &'c isize) where 'b: 'a + 'c
.
Polonius sees the 2 expected subset errors to NLL's seemingly stopping at the 1st.
Category: Not an issue
Resolution: Bless the output