Warn when lifetime parameters are unused.

[thestinger]

UPDATE: Updating this issue to reflect current thinking, which is that inference should issue a warning when a lifetime parameter is inferred to be bivariant (which implies that the parameter is unused).

ORIGINAL: It's sometimes necessary to specify a lifetime on an object that doesn't contain borrowed pointers, because lifetime dependencies can be required for safety outside of Rust's borrowed pointer system. A dummy borrowed pointer member is a workaround, but wasteful.

@nikomatsakis

[catamorphism]

Nominating for milestone 3, feature-complete

[graydon]

accepted for feature-complete milestone

[nikomatsakis]

@thestinger why not just use &'v T in the iterator types, and convert to unsafe pointers as needed? I am nervous about the implicit variance associated with unused lifetime parameters.

[Kimundi]

This seems to work now:

struct Foo<'a>;

[Kimundi]

...but there seem to be issues: #10703

[nikomatsakis]

To expand on my previous comment, which was quite terse --

I see four options:

Option 1. Leave things as they are. This means that unused lifetime parameters are a bit of a footgun, because people seem to expect them to behave in a contravariant fashion (as described by @thestinger and in #10703).

Option 2. If inference finds that a lifetime parameter is bivariant (effectively, unused), default to contravariant, which seems to be what people expect. This means that a lifetime "can only get shorter", much like the lifetime 'a in the type &'a Foo. I am a bit nervous about this in that there is an undocumented assumption -- that the lifetime parameter will behave a certain way -- that can easily be changed.

For example, if I have struct Foo<'a> { x: int } and then I change this to struct Foo<'a> { x: int, y: fn() -> &'a int } then Foo goes from being contravariant with respect to 'a to covariant. This is of course always a danger in inference but somehow it seems dangerous to me that adding additional fields can "flip" the role of a lifetime parameter in this way. I guess it's because it is clear that the user had some kind of role in mind for that lifetime parameter that was represented by the field x but the type checker is not aware of that role -- e.g., the parameter 'a was supposed to be the lifetime of the array that x is an index into or something. This means that the field x "wants" 'a to be contravariant but the type checker doesn't know it. What's scary to me is that the inference will get the desired result up until an additional use 'a is added -- at that point, it should be generated invariant, but it will generate covariant instead.

Option 3. Report an error for bivariant lifetime parameters. This is unfortunate in that one can imagine legitimately wanting a bivariant lifetime parameter in some unusual cases. (The same is true of the previous case)

Option 4. Add the ability to specify variance explicitly, and then report an error for bivariant lifetime parameters. This seems reasonable but requires people to know what "variance" means. I imagine a syntax like +'a (covariant), -'a (contravariant), ='a (invariant), and *'a (bivariant).

All of this applies equally to type parameters, incidentally.

[nikomatsakis]

To be clear, I favor Option 4 at this point.

[pnkfelix]

@nikomatsakis For options 3 and 4, when is the error for bivariant lifetime parameters being reported? Is it an error to declare a bivariant parameter? Is it merely an error to instantiate a bivariant parameter?

It is not clear to me what the value is of having an explicit syntax for an erroneous construct.

Update from IRC: niko clarified that the error would be (I think) from inferring bivariant lifetime parameter. But an explicitly specified bivariant parameter would be allowed.

[nikomatsakis]

Oh, and @pnkfelix says that one wrinkle of Option 4 was not clear: the idea was that it's only an error to we infer bivariance. If the user manually specifies bivariance for a type/lifetime parameter, that would be considered legal.

[pnkfelix]

If we want a syntax that is able to be generalized to type parameters, then we should probably not use any construct that is part of the grammar for type-expressions and trait-bounds, in order to avoid confusion for the users. That would remove *'a (since that overlaps the type *T) and +'a (since that overlaps the trait bound conjunction <A:T1 + T2>)

[thestinger]

I think I'm just in favour of Option 1 at this point. I don't think this is important enough to merit any additional type system complexity. It seems we just add #[lang = "borrowed"] struct Borrowed<'a>; to the standard library and types wanting a lifetime bound without including a 1-word pointer field could use it.

The vector iterator uses raw pointers because the end pointer points one-past-the-end of the vector and the current pointer will end up being equal to end when it's finished. Since privacy can be broken via reflection, maintaining safety requires using raw pointers. I can think of other solutions to this involving only changes to how reflection works.

[nikomatsakis]

@pnkfelix variance annotations don't conflict with the type grammar in any way. They would precede type parameter declarations.

@thestinger I think I am wary of 1 in that it represents a footgun that has come up already twice. People will expect the type system to be giving them guarantees it's not and it's surprising. I might be in favor of rendering bivariance illegal combined with lang items as you describe, but that is pretty close to option 4 (explicit annotation). Variance annotations are not especially complex on the compiler side. Whether it presents a simpler face to the end user is I guess the question -- lang items are bit more obscure somehow. I guess I'm largely indifferent.

[thestinger]

@nikomatsakis: The place where you can screw this up is in code that's already using unsafe, where you already need a solid understanding of the language. Extra annotations may make that simpler, but they'll make things more complex in the safe subset of the language. I don't think most end users should ever need to be touching unsafe code once we have a good library ecosystem.

[pnkfelix]

@nikomatsakis I did say "to avoid confusion for the users" ... but perhaps I am just paranoid.

[nikomatsakis]

@thestinger you cannot violate the type system without unsafe code, but you can be confused about what guarantees you are getting. This is what happened to @Kimundi in #10703

[lilyball]

My general feeling is that Option 4 is the best. I don't think most users are likely to hit this in practice, because most users aren't going to be adding bivariant lifetime parameters to their types (from what I've seen people generally only add lifetime parameters when the compiler yells at them for not having one). Making inferred bivariant parameters illegal and providing a way to explicitly annotate each parameter, while certainly more complex from a syntactical perspective, is also the clearest and least surprising approach.

Incidentally, what's the benefit to allowing explicit bivariant parameters?

[thestinger]

I think lifetimes are already on the boundary of how much syntactical complexity most people will accept in a language. Adding any more qualifiers will push it over the edge. I can understand that it's problematic for the meaning of a lifetime bound to depend on how the type uses it internally, but that's already true of type parameters and whether a type is Send, Freeze or affine. A type Foo<T> where T is affine, non-Send or non-Freeze may or may not have those properties itself.

[lilyball]

@thestinger Good point about type parameters and kinds. I'm worried, though, that there may be cases where someone wants a covariant type parameter without using it in a way that allows covariance to be inferred, if we default to contravariance. For example, FFI bindings may want to use covariant lifetime parameters to enforce restrictions that otherwise can't be inferred (because under the hood they use *).

[glaebhoerl]

I don't have a solid opinion about which option is best, but I share @thestinger's concern about syntax. Pointers and lifetimes are cryptic enough already. If we go with 4 I think the variance annotations should be somewhat longer-form. I think that in general, the rule of thumb should be that verbosity is inversely proportional to ubiquity. If something is used all over the place you're likely to be aware of its meaning, and want it to be compact. If something is used rarely (as here) you don't mind if it's a bit longer, and since you quite possibly haven't ever seen it before, you benefit from it being self-descriptive.

Just spelling out covariant, contraviant, invariant, and bivariant would already be much better than single-character sigils. Maybe there's something even nicer, I dunno.

[nikomatsakis]

I was thinking about this issue and I realized that it's just a specific case of the more general problem of inference being unaware of special constraints that arise in unsafe code, and that we currently have a variety of ad-hoc mechanisms for addressing this problem. For example, we sometimes use annotations to inform the "builtin traits" (#[no_freeze]), except for non-copyable, where we use a special type, etc. I think we should centralize on one mechanism, and I am happy for that to be special marker types like CovariantLifetime<'a> and so on.

[nikomatsakis]

This implies that the current #[no_freeze] annotations would go away in favor of a special Unfreezable type and so on.

[nikomatsakis]

I've added issue #10834 about centralizing on a single mechanism for these various related cases, and updated the title of this issue to indicate that we should at least warn when we infer bivariance.

[nikomatsakis]

Also, unassigning myself.

[pnkfelix]

Assigning P-low.

[reem]

Triage: we now have lifetime markers, which seem like a satisfactory solution to this problem. Not sure if there's still a story here other than we would like to lint or error on unused parameters (if they are inferred to be bivariant).

[nikomatsakis]

(I have a variance RFC coming up. Working on one nit with the implementation, but I should just post the RFC.)

[steveklabnik]

closing in favor of #22212 (the tracking issue for @nikomatsakis 's variance RFC mentioned above)