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

rustc fails to produce lifetime and closure related compiler error. #29793

Closed
mitchmindtree opened this issue Nov 12, 2015 · 8 comments
Closed
Assignees
Labels
A-lifetimes Area: lifetime related I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mitchmindtree
Copy link
Contributor

We ran into this in conrod, where one of the widgets would produce different behaviour depending on whether it was compiled in debug or release.

The related conrod issue is here.

This comment describes the strange behaviour more precisely.

This comment demonstrates the fix used.

@huonw and @Aatch helped to point out that the difference in behaviour seemed to be due to the fact that rustc was failing to produce an error relating to the lifetimes of the demo, col and row values.

@Aatch shared a simplified example of what error we should have received here.

Edit: I should mention, this bug was found in:

  • rustc 1.4.0 (8ab8581f6 2015-10-27)
  • rustc 1.6.0-nightly (5b4986fa5 2015-11-08)
  • rustc 1.5.0-beta.2 (b0f440163 2015-10-28)

on both windows and osx.

@huonw huonw added A-lifetimes Area: lifetime related I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Nov 12, 2015
@Aatch
Copy link
Contributor

Aatch commented Nov 12, 2015

I'm looking into this, trying to get a reasonably accurate reproduction and have come across this:

fn main() {
    let x = Some(|x: usize, y: usize| {
        |t: bool| if t { x } else { y }
    });

    // If the following line is uncommented, it doesn't compile
    //use_(x);
}

fn use_<G, F: FnMut(usize, usize) -> G>(f: Option<F>) {
    if let Some(mut f) = f {
        let _ = f(1, 2);
    }
}

https://play.rust-lang.org/?gist=b4a25867408e0056c8f3&version=stable

Which I believe is related.

@Aatch
Copy link
Contributor

Aatch commented Nov 12, 2015

Woo! Took me a while, but here is a minimal(-ish) reproduction (while still being similar to the original example):

struct WrapA<F>(Option<F>);

impl<F> WrapA<F> {
    fn new() -> WrapA<F> {
        WrapA(None)
    }
    fn set(mut self, f: F) -> Self {
        self.0 = Some(f);
        self
    }
}

struct WrapB<F>(Option<F>);

impl<F> WrapB<F> {
    fn new() -> WrapB<F> {
        WrapB(None)
    }
    fn set(mut self, f: F) -> Self {
        self.0 = Some(f);
        self
    }
}

trait DoStuff : Sized {
    fn handle(self);
}

impl<F, T> DoStuff for WrapA<F> 
where F: FnMut(usize, usize) -> T, T: DoStuff {
    fn handle(mut self) {
        if let Some(ref mut f) = self.0 {
            let x = f(1, 2);
            let _foo = [0usize; 16];
            x.handle();
        }
    }
}

impl<F> DoStuff for WrapB<F> where F: FnMut(bool) -> usize {
    fn handle(mut self) {
        if let Some(ref mut f) = self.0 {
            println!("{}", f(true));
        }
    }
}

impl<F, T> WrapA<F> 
where F: FnMut(usize, usize) -> T, T: DoStuff {
    fn handle_ref(&mut self) {
        if let Some(ref mut f) = self.0 {
            let x = f(1, 2);
        }
    }
}

fn main() {
    let mut w = WrapA::new().set(|x: usize, y: usize| {
        WrapB::new().set(|t: bool| if t { x } else { y })
    })
    w.handle(); // This works
    // w.handle_ref(); // This doesn't
}

https://play.rust-lang.org/?gist=330b4ee92fecd1c8f499&version=stable

I think the move of the wrapping type has something to do with it, given that handle_ref doesn't work, while handle does.

Nope, just double-checked and it's actually inherent vs. trait methods. Making an identical handle method on WrapA directly works (as in, it causes the expected error) just fine. No idea why that would be the case though.

@Aatch
Copy link
Contributor

Aatch commented Nov 12, 2015

I decided to go back to my simple case:

fn main() {
    let _x = |x: usize, y: usize| {
        |t: bool| if t { x } else { y }
    };

This compiles (and probably shouldn't), while the following doesn't (which is expected):

fn main() {
    let _x = |x: usize, y: usize| {
        let x = x;
        let y = y;
        |t: bool| if t { x } else { y }
    };

This suggests to me that the lifetimes associated with the x and y arguments are slightly too long.

Also /cc @rust-lang/compiler

@nikomatsakis
Copy link
Contributor

agreed, seems wrong. sigh.

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Nov 12, 2015
@nikomatsakis nikomatsakis self-assigned this Nov 12, 2015
@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2015

The issue is that:
* Closure signatures are not required to be well-formed.
* That would not be regularly a problem, as regions in closure signatures are supposed to always outlive the closure.
* However, parameters are regarded as being "outside" the closure, so they pass this check.

I am not sure of the best way to fix this. Maybe add an actual "root" region wrapping a closure, instead of using the DestructionScope?

@pnkfelix
Copy link
Member

assigning to self to try to help niko with his plate of stuff

@pnkfelix pnkfelix assigned pnkfelix and unassigned nikomatsakis Nov 19, 2015
@pnkfelix
Copy link
Member

Okay, I've done something much like what @arielb1 described, namely adding a new Scope variant that outlives the ParameterScope and then using that for these lifetimes, rather than DestructionScope.

I should have a PR up shortly, assuming my make check run passes...

bors added a commit that referenced this issue Dec 16, 2015
Ensure borrows of fn/closure params do not outlive invocations.

Does this by adding a new CallSiteScope to the region (or rather code extent) hierarchy, which outlives even the ParameterScope (which in turn outlives the DestructionScope of a fn/closure's body).

Fix #29793

r? @nikomatsakis
arielb1 added a commit to arielb1/rust that referenced this issue Dec 6, 2017
This fixes the tests for issue rust-lang#29793
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: lifetime related I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority 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

7 participants