Stream: t-compiler/wg-nll

Topic: remaining-ignore-compare-mode-tests


Matthew Jasper (Oct 22 2018 at 19:50, on Zulip):

So, there are 2 tests which have ignore-compare-mode-nll which don't use revisions or have a comment. https://github.com/rust-lang/rust/blob/master/src/test/ui/cleanup-rvalue-scopes-cf.rs and https://github.com/rust-lang/rust/blob/master/src/test/ui/issues/issue-28848.rs

Matthew Jasper (Oct 22 2018 at 19:52, on Zulip):

The second one has the same behaviour for both AST and migrate borrow check, and if the explicit lifetimes are removed it compiles, so doesn't seem like a huge priority.

davidtwco (Oct 22 2018 at 19:53, on Zulip):

Those are likely tests that failed on NLL compare mode when I was doing the compile-fail migration. They were listed in #53351.

Matthew Jasper (Oct 22 2018 at 19:54, on Zulip):

They still do (if compare mode used still used -Zborrowck=mir)

Matthew Jasper (Oct 22 2018 at 19:56, on Zulip):

The first one is more immediately unusable variables, but it's not as clear here that it should fail to compile. Since the temporaries are dropped after the assignment (this is observable using a pattern that partially moves the value)

Matthew Jasper (Oct 22 2018 at 19:57, on Zulip):

And there's still the argument that it has to suppress lints to compile without warning

nikomatsakis (Oct 22 2018 at 19:59, on Zulip):

hmm

nikomatsakis (Oct 22 2018 at 20:00, on Zulip):
#![feature(nll)]

struct Foo<'a, 'b: 'a>(&'a &'b ());

impl<'a, 'b> Foo<'a, 'b> {
    fn xmute(a: &'b ()) -> &'a () {
        unreachable!()
    }
}

pub fn foo<'a, 'b>(u: &'b ()) -> &'a () {
    Foo::<'a, 'b>::xmute(u) //~ ERROR lifetime bound not satisfied
}

fn main() {}

that is the "other one" you are referring to?

nikomatsakis (Oct 22 2018 at 20:01, on Zulip):

i.e., issue-28848.rs?

nikomatsakis (Oct 22 2018 at 20:01, on Zulip):

that looks like a user type annot bug, and one that I'm a bit surprised to see is not fixed

Matthew Jasper (Oct 22 2018 at 20:03, on Zulip):

I don't think we check Self is a well formed type when checking typeof(Self::method) is. (AST borrowck accepts this code if the lifetimes are removed)

nikomatsakis (Oct 22 2018 at 20:05, on Zulip):

hmm

nikomatsakis (Oct 22 2018 at 20:05, on Zulip):

seems like an easy fix

nikomatsakis (Oct 22 2018 at 20:05, on Zulip):

I agree that is likely the problem

nikomatsakis (Oct 22 2018 at 20:05, on Zulip):

I could maybe throw into my yet-to-be-merged PR

Matthew Jasper (Oct 22 2018 at 20:11, on Zulip):

Well, you have until bors recovers, so you should have time.

pnkfelix (Oct 22 2018 at 20:14, on Zulip):

We should remove the // ignore-compare-mode-nll from the tests that work under migrate mode

pnkfelix (Oct 22 2018 at 20:14, on Zulip):

(in my opinion)

pnkfelix (Oct 22 2018 at 20:14, on Zulip):

keep our effort focused in short term of the things that would plague the edition

pnkfelix (Oct 22 2018 at 20:19, on Zulip):

re: cleanup-rvalue-scopes-cf.rs, you will get errors if you add drop(_x) after each let ...

Matthew Jasper (Oct 22 2018 at 20:19, on Zulip):

yes

pnkfelix (Oct 22 2018 at 20:19, on Zulip):

Hmm. Didn't we add something that would read a variable right after it was written? I wonder where those reads fell, relative to the scope of such temporaries as these

Matthew Jasper (Oct 22 2018 at 20:20, on Zulip):

The order is assign, fake read, temporaries droped

pnkfelix (Oct 22 2018 at 20:21, on Zulip):

should we ... consider ... inverting the last two steps there ... ?

pnkfelix (Oct 22 2018 at 20:21, on Zulip):

or should we just change the test here by adding drop(_x); after each let ?

Matthew Jasper (Oct 22 2018 at 20:22, on Zulip):

I tried that, it changes drop order in some cases and requires some hacks to ensure that the outermost temporary is dropped after the assignment.

Matthew Jasper (Oct 22 2018 at 20:22, on Zulip):

or should we just change the test here by adding drop(_x); after each let ?

Probably this

pnkfelix (Oct 22 2018 at 20:22, on Zulip):

Okay. I think that to preserve the spirit of the test in the face of NLL, we are fine just ading drop(_x); after each let

pnkfelix (Oct 22 2018 at 20:23, on Zulip):

@nikomatsakis : are you cool with ^ ?

nikomatsakis (Oct 23 2018 at 12:45, on Zulip):

I tried that, it changes drop order in some cases and requires some hacks to ensure that the outermost temporary is dropped after the assignment.

how can it change drop order?

nikomatsakis (Oct 23 2018 at 12:46, on Zulip):

I'm fine with adding drop to preserve the spirit of the test, though I'd prefer to rename from _x to x =)

nikomatsakis (Oct 23 2018 at 12:46, on Zulip):

but I don't fully understand yet why we can't change the order :)

nikomatsakis (Oct 23 2018 at 12:46, on Zulip):

Well, you have until bors recovers, so you should have time.

trying now

Matthew Jasper (Oct 23 2018 at 19:31, on Zulip):

OK, so currently this compiles and prints drop(0) drop(1) drop(-1): first we drop (what remains of) the RHS temporary, then the temporary for D(1), then v.

struct D(i32);
impl Drop for D {
    fn drop(&mut self) {
        print!("drop({}) ", self.0);
    }
}

fn main() {
    let (v, ..) = (D(-1), D(0), D(1).0);
}
Matthew Jasper (Oct 23 2018 at 19:32, on Zulip):

If we want to drop D(1) before the assignment to v, then it has to be dropped before the RHS temporary, since the RHS temporary is needed for the assignment.

Matthew Jasper (Oct 23 2018 at 19:33, on Zulip):

We could add a fake read after all of the temporaries are dropped, but now this read isn't really representing anything in the source code.

Last update: Nov 22 2019 at 00:15UTC