NLL: Review compile-fail migrated tests #53351

davidtwco · 2018-08-14T15:25:32Z

As part of #53196, compile-fail tests were migrated to ui tests. In doing this, a handful of tests compile successfully in the NLL compare mode, but not in the normal AST compare mode. These tests were marked as "ignore on NLL compare mode" as there isn't an easy way to assert that a test compiles successfully in one compare mode and not in another. These tests are listed below and should be checked to make sure that it is correct that these tests ran successfully in NLL compare mode or if they have a bug:

(exact locations of tests have changed)

borrowck/

regions/

variance/

ui/compile-fail-migration/variance-cell-is-invariant.rs

rust/src/test/ui/variance/variance-cell-is-invariant.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-contravariant-arg-trait-match.rs

rust/src/test/ui/variance/variance-contravariant-arg-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-contravariant-self-trait-match.rs

rust/src/test/ui/variance/variance-contravariant-self-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-covariant-arg-trait-match.rs

rust/src/test/ui/variance/variance-covariant-arg-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-covariant-self-trait-match.rs

rust/src/test/ui/variance/variance-covariant-self-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-invariant-arg-trait-match.rs

rust/src/test/ui/variance/variance-invariant-arg-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll
ui/compile-fail-migration/variance-invariant-self-trait-match.rs

rust/src/test/ui/variance/variance-invariant-self-trait-match.rs

Line 11 in a573305

// ignore-compare-mode-nll

other

The text was updated successfully, but these errors were encountered:

pnkfelix · 2018-08-14T21:23:26Z

aassociated-types-subtyping-1.rs might be test weakness (in the sense of #51025), or an artifact of #47184, or both. In particular, I'm a little nervous about some of the code that is trying to encode some constraint also being clearly unreachable (due to loop { } blocks preceding them).

pnkfelix · 2018-08-14T22:13:38Z

I finished going through the remainder of the list.

I have distributed the entries into the following buckets:

Glaring test weakness (in the sense of #51025); DONE: see PR #53369

For these, the test presumes lexical lifetimes (e.g. it writes let _w = &stuff; or let _w = &mut stuff; early on and assumes that borrows lasts until the end of the block). To fix this for NLL, we usually need to add one or more uses of some borrowed variable. (The use of a leading underscore on such variables is often a dead giveaway...) I've tried to link to the first such occurrence I noticed in the file.

rust/src/test/ui/borrowck/borrow-tuple-fields.rs

Line 21 in a573305

let r = &x.0;

rust/src/test/ui/borrowck/borrowck-borrow-mut-object-twice.rs

Line 22 in a573305

let _y = x.f1();

rust/src/test/ui/borrowck/borrowck-closures-unique-imm.rs

Line 22 in a573305

let p = &this.x;

rust/src/test/ui/borrowck/borrowck-issue-2657-1.rs

Line 18 in a573305

Some(ref _y) => {

rust/src/test/ui/borrowck/borrowck-loan-blocks-move-cc.rs

Line 23 in a573305

let _w = &v;

rust/src/test/ui/borrowck/borrowck-loan-blocks-move.rs

Line 20 in a573305

let _w = &v;

rust/src/test/ui/borrowck/borrowck-move-from-subpath-of-borrowed-path.rs

Line 20 in a573305

let b = &a;

rust/src/test/ui/borrowck/borrowck-move-mut-base-ptr.rs

Line 19 in a573305

let p: &isize = &*t0; // Freezes `*t0`

rust/src/test/ui/borrowck/borrowck-mut-borrow-of-mut-base-ptr.rs

Line 20 in a573305

let p: &isize = &*t0; // Freezes `*t0`

rust/src/test/ui/borrowck/borrowck-object-lifetime.rs

Line 22 in a573305

let _y = x.borrowed();

rust/src/test/ui/borrowck/borrowck-overloaded-index-autoderef.rs

Line 46 in a573305

let _p = &mut f[&s];

rust/src/test/ui/borrowck/borrowck-swap-mut-base-ptr.rs

Line 22 in a573305

let p: &isize = &*t0; // Freezes `*t0`

rust/src/test/ui/borrowck/borrowck-union-borrow-nested.rs

Line 29 in a573305

let ra = &mut u.s.a;

rust/src/test/ui/borrowck/borrowck-uniq-via-lend.rs

Line 44 in a573305

let _w = &mut v;

rust/src/test/ui/methods/method-self-arg-2.rs

Line 24 in a573305

let y = &mut x;

rust/src/test/ui/mut/mut-cant-alias.rs

Line 18 in a573305

let b1 = &mut *b;

Possible test weakness #51025 (e.g., do we need to add some calls of the closures or object methods, or make code reachable?); (almost done, see PR #53407)

Update: adding calls is not needed; it suffices to merely add (no-op) references to the variables that hold the first closure immediately after the second one is defined, and you see the error then.

Update 2: I've covered all the cases except associated-types-subtyping-1.rs in PR #53407

rust/src/test/ui/associated-types/associated-types-subtyping-1.rs

Line 25 in a573305

let a: <T as Trait<'a>>::Type = loop { };

rust/src/test/ui/borrowck/borrowck-closures-two-mut-fail.rs

Line 25 in a573305

let c1 = to_fn_mut(|| x = 4);

rust/src/test/ui/borrowck/borrowck-closures-unique.rs

Line 33 in a573305

let c1 = || get(x);

rust/src/test/ui/hashmap-lifetimes.rs

Line 17 in a573305

let mut it = my_stuff.iter();

rust/src/test/ui/unboxed-closures/unboxed-closure-region.rs

Line 16 in a573305

let _f = {

rust/src/test/ui/unboxed-closures/unboxed-closures-borrow-conflict.rs

Line 18 in a573305

let f = || x += 1;

Fixed (at least somewhat) by NLL

For these cases, we should just add a #[rustc_error] to the fn main() (assuming the existing fn main() is trivial; if the fn main is not trivial, then rename it and add a trivial fn main)

rust/src/test/ui/borrowck/borrowck-lend-flow-if.rs

Lines 31 to 41 in a573305

    
           fn pre_freeze_cond() { 
        
               // In this instance, the freeze is conditional and starts before 
        
               // the mut borrow. 
        
               let mut v: Box<_> = box 3; 
        
               let _w; 
        
               if cond() { 
        
                   _w = &v; 
        
               } 
        
               borrow_mut(&mut *v); //~ ERROR cannot borrow 
        
           }

rust/src/test/ui/borrowck/borrowck-lend-flow.rs

Lines 31 to 37 in a573305

    
           fn pre_freeze() { 
        
               // In this instance, the freeze starts before the mut borrow. 
        
               let mut v: Box<_> = box 3; 
        
               let _w = &v; 
        
               borrow_mut(&mut *v); //~ ERROR cannot borrow 
        
           }

Have our rvalue lifetime rules changed...?

Update The reason this test is passing under NLL is almost certainly due to NLL allows creating variables that are immediately unusable. #53695

I skimmed over this test but I cannot immediately tell whether NLL should have changed anything here... I'll need to read it again with a fresh pair of eyes (and perhaps review some of the doc and/or blog posts discussing our rvalue lifetime rules...)

rust/src/test/ui/cleanup-rvalue-scopes-cf.rs

Lines 37 to 45 in a573305

    
           pub fn main() { 
        
               let _x = arg(&AddFlags(1)); //~ ERROR value does not live long enough 
        
               let _x = AddFlags(1).get(); //~ ERROR value does not live long enough 
        
               let _x = &*arg(&AddFlags(1)); //~ ERROR value does not live long enough 
        
               let ref _x = *arg(&AddFlags(1)); //~ ERROR value does not live long enough 
        
               let &ref _x = arg(&AddFlags(1)); //~ ERROR value does not live long enough 
        
               let _x = AddFlags(1).get(); //~ ERROR value does not live long enough 
        
               let Box { f: _x } = Box { f: AddFlags(1).get() }; //~ ERROR value does not live long enough 
        
           }

Known bug

These cases all fall into some form of #47184

I link to the first obvious example of some type ascription that is currently unchecked:

rust/src/test/ui/associated-types/associated-types-subtyping-1.rs

Line 27 in a573305

let _c: <T as Trait<'a>>::Type = a;

rust/src/test/ui/closure_promotion.rs

Line 18 in a573305

let x: &'static _ = &|| { let z = 3; z }; //~ ERROR does not live long enough

rust/src/test/ui/issues/issue-28848.rs

Line 22 in a573305

Foo::<'a, 'b>::xmute(u) //~ ERROR lifetime bound not satisfied

Finally, all of the tests in regions/ and all of the tests in variance/ use type ascription (in arguably advanced ways) as a way to encode the desired (potentially subtle checks).

I think all of these should be fixed when #47184 is fixed, or potentially sooner as subtasks of that bug are completed.

Already using -Z borrowck=XXX in a different revision.

In this case, the test is already requesting some form of MIR-borrowck via explicit compiler flags and presumably does not need to go through the compare-mode=nll system. HOWEVER, there are instances of this in the test suite where the only reason we used the revision system and compiler flags is that those tests predate the compare-mode=nll. So we should consider trying to update them anyway, if possible.

rust/src/test/ui/issues/issue-5500-1.rs

Line 13 in a573305

//[mir]compile-flags: -Z borrowck=compare

… from rust-lang#53196. See also the bulletpoint list on rust-lang#53351.

pnkfelix · 2018-08-14T23:33:19Z

There isn't that much left to do here in the short term; IMO all of the regions/ and variance/ are effectively blocked on #47184 -- or at least, I do not think we should invest any effort trying to find a different way to encode those tests. All such effort should be spent instead on addressing #47184, and then once that is addressed, we should be able to remove the // ignore-test-compare-nll on those files with relative ease.

I think once you remove those files and the others blocked by #47184, then there's only maybe nine or tests left to be either investigated or updated.

pnkfelix · 2018-08-14T23:33:59Z

(having said that, I'm unassigning myself since I am not sure I will have time during the remainder of this week to investigate the remaining tests here.)

…e-fail-tests-more-robust-wrt-nll, r=davidtwco Make some ported cfail tests robust w.r.t. NLL Updated the most glaring instances of weak tests w.r.t. NLL that came from #53196. See also the bulletpoint list on #53351.

pnkfelix · 2018-08-28T15:02:53Z

It might be good to figure out some way to fold this issue (#53351) into #52663, or open up a new issue that covers the remaining cases in the two issues.

Also, if we do one of those big reviews, we may also want to wait until #53764 is done.

pnkfelix · 2018-09-11T14:06:02Z

assigning to self so that remember to factor this into smaller issues that can be individually triaged.

pnkfelix · 2018-09-24T12:47:29Z

I've decided we need a third big round of review, which is filed as #54528

While it might make sense to try to resolve this issue (#53351) before attacking #54528, it probably makes just as much sense to focus attention fully on #54528 itself.

So I'm closing this issue in favor of #54528.

davidtwco added the A-NLL Area: Non-lexical lifetimes (NLL) label Aug 14, 2018

davidtwco mentioned this issue Aug 14, 2018

Move compile-fail tests to ui #44844

Closed

davidtwco added the NLL-diagnostics Working towards the "diagnostic parity" goal label Aug 14, 2018

pnkfelix self-assigned this Aug 14, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue Aug 14, 2018

Updated the most glaring instances of weak tests w.r.t. NLL that came…

c7041a6

… from rust-lang#53196. See also the bulletpoint list on rust-lang#53351.

pnkfelix mentioned this issue Aug 14, 2018

Make some ported cfail tests robust w.r.t. NLL #53369

Merged

pnkfelix removed their assignment Aug 14, 2018

nikomatsakis added this to the Rust 2018 RC milestone Aug 27, 2018

nikomatsakis added WG-compiler-nll T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed WG-compiler-nll labels Aug 27, 2018

nikomatsakis modified the milestones: Rust 2018 RC, Edition 2018 RC 2 Sep 10, 2018

pnkfelix self-assigned this Sep 11, 2018

pnkfelix mentioned this issue Sep 24, 2018

[NLL] Be more permissive when checking access due to Match #53438

Merged

pnkfelix closed this as completed Sep 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL: Review compile-fail migrated tests #53351

NLL: Review compile-fail migrated tests #53351

davidtwco commented Aug 14, 2018 •

edited by pnkfelix

Loading

pnkfelix commented Aug 14, 2018 •

edited

Loading

pnkfelix commented Aug 14, 2018 •

edited

Loading

pnkfelix commented Aug 14, 2018

pnkfelix commented Aug 14, 2018

pnkfelix commented Aug 28, 2018

pnkfelix commented Sep 11, 2018

pnkfelix commented Sep 24, 2018

NLL: Review compile-fail migrated tests #53351

NLL: Review compile-fail migrated tests #53351

Comments

davidtwco commented Aug 14, 2018 • edited by pnkfelix Loading

borrowck/

regions/

variance/

other

pnkfelix commented Aug 14, 2018 • edited Loading

pnkfelix commented Aug 14, 2018 • edited Loading

Glaring test weakness (in the sense of #51025); DONE: see PR #53369

Possible test weakness #51025 (e.g., do we need to add some calls of the closures or object methods, or make code reachable?); (almost done, see PR #53407)

Fixed (at least somewhat) by NLL

Have our rvalue lifetime rules changed...?

Known bug

Already using -Z borrowck=XXX in a different revision.

pnkfelix commented Aug 14, 2018

pnkfelix commented Aug 14, 2018

pnkfelix commented Aug 28, 2018

pnkfelix commented Sep 11, 2018

pnkfelix commented Sep 24, 2018

davidtwco commented Aug 14, 2018 •

edited by pnkfelix

Loading

pnkfelix commented Aug 14, 2018 •

edited

Loading

pnkfelix commented Aug 14, 2018 •

edited

Loading