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

Consider re-tuning the lifetime elision rules for trait objects #91302

Open
scottmcm opened this issue Nov 27, 2021 · 2 comments
Open

Consider re-tuning the lifetime elision rules for trait objects #91302

scottmcm opened this issue Nov 27, 2021 · 2 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-maybe-future-edition Something we may consider for a future edition. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Nov 27, 2021

The lifetime elision rules for functions are willing to fail sometimes, and that's good because -> &i32 defaulting to -> &'static i32 would not be what people generally want, and having the error say "hey, what lifetime did you mean?" is way better than getting borrowck errors about "that's not 'static".

It might be worth taking inspiration from that to improve the elision rules for dyn Trait, as lints for now and possibly as hard changes in a future edition.

For example, impl dyn Trait is currently impl dyn Trait + 'static, but it's not clear that's good. It might be better to require that the user write impl dyn Trait + '_ or impl dyn Trait + 'static to say which they want. Inspired by this thread: https://users.rust-lang.org/t/why-do-associated-functions-in-impl-dyn-trait-require-static-lifetime/67548/2?u=scottmcm

Similarly, the + 'static default applies even inside a struct with a lifetime. For example, #91292 had

pub struct SectionMut<'a> {
    data: &'a mut Option<Box<dyn SectionData>>,
}

where the implicit + 'static is more restrictive than needed because it's behind the &'a -- but being in the Box hides that from elision.

I don't have any concrete proposal here right now, but I figured I'd open this as a place to track it.


Other examples (feel free to edit this post to add more down here):

@scottmcm scottmcm added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-maybe-future-edition Something we may consider for a future edition. labels Nov 27, 2021
@Yuri6037
Copy link

Yuri6037 commented Dec 1, 2021

If this proposition allows to do

pub type SectionDataType = dyn SectionData + 'static;

and use that type everywhere instead of typing the full type always, I'm in favor of that as it's indeed not always clear that it defaults to 'static and the proof is it doesn't do this for the example in #91292: if the 'static lifetime isn't explicitly written as part of the return type, the compiler infers a different lifetime which fails to compile.

@QuineDot
Copy link

@scottmcm said:

Similarly, the + 'static default applies even inside a struct with a lifetime.

Historical note: It used to work the way you're suggesting under RFC 0599, but was deliberately changed to 'static as part of RFC 1156 even though that was a breaking change to post-1.0 Rust.


Context: for the rest of this comment I'm suggesting changes to the actual behavior and not what the reference says. Some I'd definitely like to see happen and others I'm just throwing out there.

Always infer the elided lifetime in function bodies

In these examples, the elided trait object lifetime is inferred and independent.

// Type with no bounds, trait with no bounds
let _: Box<dyn Trait> = bx;

// Type with one bound, trait with no bounds, parameter is elided
let _: &dyn Trait = rf;

// Type with ambiguous bounds, trait with no bounds
let _: Ambiguous<'a, 'b, dyn Trait> = ambig;

But in these examples, it is not.

// Type with one bound, trait with no bounds, parameter is explicit
// (Elided trait object lifetime is `'a` and cannot be inferred to be `'static` for example)
let _: &'a dyn Trait = rf;

// Trait with bounds (type bounds don't matter)
// (Elided trait object lifetime is `'a` and cannot be inferred to be `'static` for example)
let _: Box<dyn Single<'a>> = bx;
// (Elided trait object lifetime is ambiguous and this throws an error)
let _: Box<dyn Double<'a, 'b>> = bx;

I believe it could be inferred in these cases too, which should only allow more code to compile, and that inference would infer the correct lifetimes where needed. I.e. this change would not be major-breaking if it breaks anything.

I'd like to see this change.

Playground.

Make '_ always a fresh inference variable

This example works exactly like the last two cases above, i.e., '_ doesn't introduce a fresh inference variable in function bodies when there are trait bounds.

// Trait with bounds (type bounds don't matter)
// (Elided trait object lifetime is `'a` and cannot be inferred to be `'static` for example)
let _: Box<dyn Single<'a> + '_> = bx;
// (Elided trait object lifetime is ambiguous and this throws an error)
let _: Box<dyn Double<'a, 'b> + '_> = bx;

I believe this is also non- or minimally-breaking. (Frankly this one feels like an outright bug.)

I'd like to see this change.

Playground.

Make 'static type bounds unambiguous

trait Trait<'a>: 'a + 'static acts like trait Trait<'a>: 'static, but if you do the same thing on a type, it's considered ambiguous.

trait OverBound<'a>: Sync + 'a + 'static {}
impl OverBound<'_> for () {}
// Compiles
static OB: &dyn OverBound<'_> = &();

struct Ambiguous<'a, T: 'a + 'static + ?Sized>(&'a T);
// Errors
static A: Ambiguous<'_, dyn std::fmt::Debug> = Ambiguous(&());

Playground.

Similarly, when there are multiple bounds, one of them being (explicitly/syntactically) 'static makes the trait bounds unambiguous, but not so with type bounds.

trait Double<'a, 'b>: Sync + 'a + 'b {}
// Compiles
impl dyn Double<'static, '_> {}

struct Ambiguous<'a, 'b, T: 'a + 'b + ?Sized>(&'a T, &'b T);
// Fails
impl Ambiguous<'static, '_, dyn std::fmt::Display> {}

Playground.

I would like for these to be consistent.

Generalize 'static behavior to all outlives bounds

The behavior where 'static is unambiguous for trait bounds described immediately above is a syntactical property of 'static specifically. It doesn't apply when 'static must be true:

trait Double<'a: 'static, 'b: 'static>: Sync + 'a + 'b {}
// Fails
impl<'a, 'b> dyn Double<'a, 'b> {}

And it doesn't apply to general outlives requirements:

trait Double<'a: 'b, 'b: 'a>: Sync + 'a + 'b {}
// Fails although `'b` and `'a` must be the same
impl<'a> dyn Double<'a, '_> {}

It would be less surprising if it did. (And for type bounds too.)

Somewhat related subtype soundness issue.

Make default lifetimes in function signatures not rely on late-vs-early bound parameters

The current situation is a mess and it would be better if function signatures just acted like impl headers, say. My instinct says this one will be breaking and perhaps churny though, given how subtle it is and how this can introduce implied bounds you can accidentally rely on.

But perhaps it's not horrible; lifetime-bound traits other than 'static seem uncommon.

I'd like to see this change, but don't know how feasible it is.

Something something type aliases

Using an alias will apply the lifetime defaults based on the alias bounds (or lack of bounds), be they stronger or looser than the aliased type. However, the bounds are not actually enforced and the warning tells you to remove them (which has a semantic impact on the program). See #100270.

This is surprising on its own. On the other hand, you can do this:

#[allow(type_alias_bounds)]
pub type OptBoxMut<'a, T: 'a> = &'a mut Option<Box<T>>;

To address nested lifetime defaults such as those in #91292.

One option to address the surprise would be to make the bounds actually inert. However, I'm not sure that's a good idea and I don't think the decision should be made in a vacuum.

Instead, a coherent design that aligns the allowed-on-stable type aliases with how TAIT works should be considered. Namely, as they exist today, bounds in TAITs are enforced.

#![feature(type_alias_impl_trait)]

pub type Foo<T: Send> = impl Sized;
// Fails to compile
fn foo<T>(t: T) -> Foo<T> { () }

pub type Bar<T: Send> = Vec<T>;
// Compiles
fn bar<T>(t: T) -> Bar<T> { vec![t] }

I couldn't get confirmation on if this is intended or not, though. But it's probably necessary when you're returning a type whose impl Trait obligations rely on a conditional implementation -- those conditionals must be forwarded to callers of your defining function, say.

So perhaps a better change would be to start enforcing bounds in stable-today type aliases (and keep the behavior around default trait object lifetimes). It would be a breaking change.

Desired outcome: I'd like to see TAIT and non-TAIT type aliases be consistent with each other.

Make bounds on GAT type parameters consistent

See #115379.

Treat direct bounds on associated types and GATs as type bounds

Direct lifetime bounds on associated types (and, as far as I know, GATs) also do not effect the default.

Longish playground example.

Making them act like type bounds is an option. It would be a breaking change.

I'm pretty ambivalent on this one so far.

Extreme option 1: Deprecation

Deprecate default lifetimes. I doubt this would fly.

However, if the alias behavior was maintained, Box in particular (and Arc and Rc and some other std types I suppose) could be redefined as an alias with a T: 'static bound to reduce breakage and churn. (I think -- haven't poured much effort into this idea since I doubt it would be entertained.)

Reduced or no, it's a breaking change.

Extreme option 2: Ape struct parameter elision

Make full elision act like '_ everywhere it's allowed, similar to how elided lifetime parameters of structs work.

I want elided_lifetimes_in_paths to be eventually deny or at least warn by default, so I'm not really a fan of this one.

It's a breaking change with a lot of churn. The same aliasing caveat as discussed above probably applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-maybe-future-edition Something we may consider for a future edition. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Needs help: Design
Development

No branches or pull requests

3 participants