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

Lifetime regression in kick 0.1.2 #38390

Open
brson opened this Issue Dec 15, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@brson
Copy link
Contributor

brson commented Dec 15, 2016

Original report.

error[E0309]: the parameter type `T` may not live long enough

   --> src/signal/variable.rs:114:5

    |

114 |     fn listen<F>(&self, f: F) -> Self::Listen


    |     ^

    |

    = help: consider adding an explicit lifetime bound `T: 'a`...

note: ...so that the type `T` will meet its required lifetime bounds

   --> src/signal/variable.rs:114:5

    |

114 |     fn listen<F>(&self, f: F) -> Self::Listen


    |     ^


error: aborting due to previous error

It's not clear what the change was here but @nagisa remembers something about:

09:28 < nagisa> so, the error for kick seems legit to me
09:28 < acrichto> brson: ok kaws is benign
09:28 < acrichto> nondeterministic cargo crate graph resolution
09:28 < acrichto> first build just happened to get a working crate graph
09:28 < acrichto> second build didn't
09:28 < nagisa> it has `where F: Into<Slot<'a, 'a, T, Tr>>` and no `T:'a`
09:29 < acrichto> brson: tl;dr; kaws is a standard legit cargo error, no regression
09:29 < nagisa> I mildly remember a pr or something along the lines
09:29 <~brson> ugh
09:30 <~brson> nagisa: that's for kick?
09:30 < nagisa> yes
09:30 < nagisa> though I might be remembering incorrectly too
09:30 < nagisa> would be best to ask nmatsakis if he wasn’t on a vacation
09:31  * brson still filing bugs
09:32 < nagisa> I would fill a bug for kick too, pink nmatsakis and revisit a week later

cc @nikomatsakis @rust-lang/lang

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Dec 15, 2016

Namely the signature of failing function looks like this;

pub enum Slot<'a, 'b: 'a, T: 'a, Tr: Transaction<'b> + 'b> {
    Nothing,
    Ref(&'a SlotFn<'a, 'b, T, Tr>),
    SRef(Rc<SlotFn<'a, 'b, T, Tr>>),
    WRef(Weak<SlotFn<'a, 'b, T, Tr>>),
    Mut(&'a mut SlotFnMut<'a, 'b, T, Tr>),
    Boxed(Box<SlotFnMut<'a, 'b, T, Tr>>),

    #[cfg(feature="nightly")]
    Once(Box<SlotFnBox<'a, 'b, T, Tr>>),
}

impl<'a, T, Tr: Transaction<'a> + 'a> Stream<'a, Tr> for Variable<'a, T, Tr>
    where T: Clone
{
    type Output = T;
    type Listen = <Event<'a, T, Tr> as Stream<'a, Tr>>::Listen;

    fn listen<F>(&self, f: F) -> Self::Listen
        where F: Into<Slot<'a, 'a, T, Tr>>
    {
        self.updates.listen(f)
    }
}

Note the T:'a bound on Slot but lack of any such requirement for the listen bound (or I’m not seeing one, anyway).

@nikomatsakis nikomatsakis added T-compiler and removed T-lang labels Dec 22, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 22, 2016

triage: P-high

@rust-highfive rust-highfive added P-high and removed I-nominated labels Dec 22, 2016

@nikomatsakis nikomatsakis self-assigned this Dec 22, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 22, 2016

I'll investigate, but seems like it is likely expected behavior.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2017

I agree with @nagisa that this is expected behavior, though I'm not sure what caused it to change.

@brson brson added the E-needstest label Jan 3, 2017

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Jan 3, 2017

Seems like we just need to add the test case.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2017

Yeah, I'll have to try and minimize it though.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 4, 2017

If this is expected behavior and just needs a test case, I'll also remove the regression tag.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 4, 2017

Can anyone verify that this used to work? It seems like if we are going to make a test case, we'd also want to verify that the test case used to pass...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.