Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nll rejects this code #48238

Closed
d653 opened this issue Feb 15, 2018 · 8 comments
Closed

nll rejects this code #48238

d653 opened this issue Feb 15, 2018 · 8 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-complete Working towards the "valid code works" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@d653
Copy link

d653 commented Feb 15, 2018

UPDATE: Turns out this code should not work, so we just need to add a negative test. See this comment but also the mentoring instructions in this tracking bug.


This code compiles, but it does not compile with nll enabled.

#![feature(nll)]
use std::io::{BufRead, BufReader};

fn main() {
    let v = BufReader::new("aaa.bbb\nccc.ddd\neee.fff.ggg\n".as_bytes());
    let _ = v.lines().map(|x:Result<String,_>|{
        let x = x.unwrap();
        move||{x.rsplit(".")}
    });
}
@pietroalbini pietroalbini added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Feb 15, 2018
@pietroalbini
Copy link
Member

cc @nikomatsakis

@nikomatsakis nikomatsakis added this to the NLL: Valid code works milestone Feb 15, 2018
@Aaron1011
Copy link
Member

I'd like to work on this.

@Aaron1011
Copy link
Member

Reduced version:

#![feature(nll)]

fn main() {
    let a = "Hello".to_string();
    move || {
        a.rsplit("")
    };
}

@Aaron1011
Copy link
Member

An even simpler reduction:

#![feature(nll)]

fn use_val<'a>(val: &'a u8) -> &'a u8 {
    val
} 

fn main() {
    let orig: u8 = 5;
    move || {
        use_val(&orig)
    };
}

@Aaron1011
Copy link
Member

Looking at this some more, I think the error is actually correct. The move closure takes ownership of orig, but then tries to return a reference to it. With ast-borrowck, the closure expression itself compiles, but any attempt to call it results in a similar lifetime error.

It seems to me that the error message itself could be improved, however. In the absence of explicit lifetime parameters, temporary lifetimes like '_#1r and regions without a proper name shouldn't show up in errors.

@daboross
Copy link
Contributor

daboross commented Feb 15, 2018

This code compiles on stable, though. It's still a regression, right? @Aaron1011

Your simple example fails to compile on stable if you run the closure, but the original example given by @gw653 compiles and runs completely fine on stable, and it is running the closure.

Nevermind that, I realized what the code was doing. it's the closure inside the closure that's failing. Changing let _ = to let mut a = and adding (a.next().unwrap())(); causes the original to also fail to compile

@Aaron1011
Copy link
Member

@daboross: Given that it's impossible to actually call the closure, I don't feel that this is really a regression. As written, the original example shouldn't be valid - NLL just gives the error 'earlier' than ast-borrowck.

@nikomatsakis: Thoughts?

@nikomatsakis
Copy link
Contributor

@Aaron1011 is correct. If you try to think about the desugaring of the closure expression to the Fn traits, it turns out that there is no legal desugaring of this example. The problem is that the closure C is attempting to return a reference to data owned by self; but the Fn trait looks like this:

trait Fn<A> {
    type Output;
    fn call<'a>(&'a self, args: A) -> Self::Output;
}

In particular, the return type we would need here is &'a u8, but the lifetime 'a is used only in &self and cannot appear in the return type (Self::Output).

The AST-based type-checker has several known flaws in this area, so I'm not surprised to be uncovering bugs in this area. Seems like one we should add to #47366.

@nikomatsakis nikomatsakis added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Feb 21, 2018
@nikomatsakis nikomatsakis added the NLL-complete Working towards the "valid code works" goal label Mar 14, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 23, 2018
Add test for issue-48238

Fixes rust-lang#48238
test case made from comments in rust-lang#48238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. NLL-complete Working towards the "valid code works" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants