Skip to content

NLL: named lifetimes in closures can be unrelated to those in parent function #98589

Description

@aliemjay

Update: I originally thought it's a soundness issue, but it may not be, see comment #98589 (comment).

This code compiles after #95565 although it shouldn't: (playground)

fn lives_as_long<'a, T>() where T: 'a, {}

fn test<'a, T>()
where
    &'a (): Sized, // any predicate containing `'a`
{
    || {
        lives_as_long::<'a, T>();
    };
}

@rustbot label C-bug T-types regression-from-stable-to-nightly I-unsound A-NLL

Activity

  1. added
    A-NLLArea: Non-lexical lifetimes (NLL)
    C-bugCategory: This is a bug.
    I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
    T-typesRelevant to the types team, which will review and decide on the PR/issue.
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jun 27, 2022
  2. apiraino commented on Jun 27, 2022

    @apiraino
    Contributor

    WG-prioritization assigning priority (Zulip discussion).

    @rustbot label -I-prioritize +P-critical

  3. added and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jun 27, 2022
  4. steffahn commented on Jun 27, 2022

    @steffahn
    Member

    I have yet to find a way to exploit this code for actual unsound behavior. Perhaps you know a way @aliemjay? Preliminary experiments make it seem … possibly … more as if the lives_as_long::<'a, T>() call kinda ignores the explicit 'a and infers a shorter lifetime instead.

  5. steffahn commented on Jun 27, 2022

    @steffahn
    Member

    E.g. this works

    use std::fmt::Display;
    
    fn lives_as_long<'a, T: Display>(x: T) -> Box<dyn Display + 'a>
    where
        T: 'a,
    {
        Box::new(x)
    }
    
    fn test<'a, T: Display>(x: T)
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        let f = |x: T| -> Box<dyn Display + 'a> { lives_as_long::<'a, T>(x) };
        let r: Box<dyn Display + '_> = f(x); // <- inferred short lifetime for '_
    }

    but this doesn’t

    use std::fmt::Display;
    
    fn lives_as_long<'a, T: Display>(x: T) -> Box<dyn Display + 'a>
    where
        T: 'a,
    {
        Box::new(x)
    }
    
    fn test<'a, T: Display>(x: T)
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        let f = |x: T| -> Box<dyn Display + 'a> { lives_as_long::<'a, T>(x) };
        let r: Box<dyn Display + 'a> = f(x); // <- explicit lifetime 'a from the function signature
    }
    error[E0309]: the parameter type `T` may not live long enough
      --> src/lib.rs:14:13
       |
    14 |     let f = |x: T| -> Box<dyn Display + 'a> { lives_as_long::<'a, T>(x) };
       |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...so that the type `T` will meet its required lifetime bounds
       |
    help: consider adding an explicit lifetime bound...
       |
    10 | fn test<'a, T: Display + 'a>(x: T)
       |                        ++++
    

    Same story with

    fn test<'a, T: Display>(x: T)
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        let mut r: Option<Box<dyn Display + '_>> = None; // <- inferred short lifetime for '_
        let f = |x: T| {
            r = Some(lives_as_long::<'a, T>(x));
        };
    }

    vs

    fn test<'a, T: Display>(x: T)
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        let mut r: Option<Box<dyn Display + 'a>> = None; // <- explicit lifetime 'a from the function signature
        let f = |x: T| {
            r = Some(lives_as_long::<'a, T>(x));
        };
    }
    error[E0309]: the parameter type `T` may not live long enough
      --> src/lib.rs:15:13
       |
    15 |       let f = |x: T| {
       |  _____________^
    16 | |         r = Some(lives_as_long::<'a, T>(x));
    17 | |     };
       | |_____^ ...so that the type `T` will meet its required lifetime bounds
       |
    help: consider adding an explicit lifetime bound...
       |
    10 | fn test<'a, T: Display + 'a>(x: T)
       |                        ++++
    
  6. steffahn commented on Jun 27, 2022

    @steffahn
    Member

    I feel like the “predicate containing 'a” makes 'a early-bound instead of late-bound which is probably what makes “a difference” here in terms of compiler behavior.

  7. jackh726 commented on Jun 27, 2022

    @jackh726
    Member

    Early-bound vs late-bound makes sense for the change in behavior

  8. steffahn commented on Jun 27, 2022

    @steffahn
    Member

    I’m increasingly convinced that this is not really a soundness issue. The behavior is confusing though, so it is an issue. It’s almost as if the closure struct implementing the closure (which would be necessarily generic over the lifetimes it “captures” if implemented manually) did not get instantiated with the right lifetimes, but instead those were left inferred. I.e.

    fn lives_as_long<'a, T>() where T: 'a, {}
    
    fn test<'a, T>()
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        || {
            lives_as_long::<'a, T>();
        };
    }

    would turn into something like

    fn lives_as_long<'a, T>() where T: 'a, {}
    
    fn test<'a, T>()
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        struct Closure<'a, T>(…) where T: 'a;
        impl<'a, T> FnOnce<…> for Closure<'a, T> { … } // and also FnMut and Fn
    
        Closure::<'a, T>(…);
    }

    if implemented by hand, but it currently behaves as if instead of Closure::<'a, T>(…); something like Closure::<'_, T>(…); is used, leaving the lifetime inferred/unspecified, and meaning that an error due to the T: 'a constraint only appears if the calls (or other usages of the closure) put bounds on that inferred lifetime that are in conflict with the fact that T: 'a is not true.

  9. aliemjay commented on Jun 27, 2022

    @aliemjay
    ContributorAuthor

    I agree that this is most likely not a soundness issue, but it's still confusing and fixing it in the future would be a breaking change.

    By looking closer at the source, the named lifetime 'a that appears in the closure is indeed a free inference variable independent from the 'a found in the defining scope. It behaves similarly to how @steffahn theorized in the previous comment.

    Perhaps this counts an unintended stabilization. Re-requesting prioritization in light of this.

    @rustbot label -I-unsound +I-prioritize

  10. added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    and removed
    I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
    on Jun 27, 2022
  11. changed the title [-]NLL: unsound propagation of region constraints from closures[/-] [+]NLL: named lifetimes in closures can be unrelated to those in parent function[/+] on Jun 27, 2022
  12. apiraino commented on Jun 28, 2022

    @apiraino
    Contributor

    Prioritization assessment P-critical still holds (discussion on Zulip discussion).

    @rustbot label -I-prioritize

  13. removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jun 28, 2022
  14. steffahn commented on Jun 30, 2022

    @steffahn
    Member

    I haven’t more thoroughly tested it, but adding (): Dummy<'a> bounds for all early-bound lifetime arguments of a function for a trait

    trait Dummy<'a> {}
    impl<'a> Dummy<'a> for () {}

    as in:

    trait Dummy<'a> {}
    impl<'a> Dummy<'a> for () {}
    
    fn lives_as_long<'a, T>() where T: 'a, {}
    
    fn test<'a, T>()
    where
        &'a (): Sized, // any predicate containing `'a`
        (): Dummy<'a>, // <- this is new; added for every early-bound lifetime parameter
    {
        || {
            lives_as_long::<'a, T>();
        };
    }

    seems to “fix” the problem (of incorrectly accepting this function). If code after this transformation does indeed behave as desired, all that would be necessary is to identify what needs to be changed so it behaves as if such a dummy trait bound was present.


    Edit: Nevermind, this breaks down quickly, e.g. even for this example, being systematic about the thing as in

    trait Dummy<'a> {}
    impl<'a> Dummy<'a> for () {}
    
    fn lives_as_long<'a, T>()
    where
        T: 'a,
        (): Dummy<'a>, // <- added for every early-bound lifetime parameter on every function
    {
    }
    
    fn test<'a, T>()
    where
        &'a (): Sized, // any predicate containing `'a`
        (): Dummy<'a>, // <- added for every early-bound lifetime parameter on every function
    {
        || {
            lives_as_long::<'_, T>(); // no longer 'a
        };
    }

    will not compile, despite the fact that the original code

    trait Dummy<'a> {}
    impl<'a> Dummy<'a> for () {}
    
    fn lives_as_long<'a, T>()
    where
        T: 'a,
    {
    }
    
    fn test<'a, T>()
    where
        &'a (): Sized, // any predicate containing `'a`
    {
        || {
            lives_as_long::<'_, T>(); // no longer 'a
        };
    }

    does (and should) compile.


    Nonetheless, I still think it’s an interesting observation that adding such a trait bound (only to test so it is not even used for calling lives_as_long) somehow makes the code (“successfully”) fail again.

  15. added a commit that references this issue on Aug 3, 2022
    88e9417
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-NLLArea: Non-lexical lifetimes (NLL)C-bugCategory: This is a bug.P-criticalCritical priorityT-typesRelevant to the types team, which will review and decide on the PR/issue.regression-from-stable-to-nightlyPerformance or correctness regression from stable to nightly.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions