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

Infer T: 'x outlives requirements on structs #2093

Merged
merged 5 commits into from
Sep 11, 2017

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Aug 2, 2017

Remove the need for explicit T: 'x annotations on structs. We will
infer their presence based on the fields of the struct. In short, if
the struct contains a reference (directly or indirectly) to T with
lifetime 'x, then we will infer that T: 'x is a requirement:

struct Foo<'x, T> {
  // inferred: `T: 'x`
  field: &'x T
}

Explicit annotations remain as an option used to control trait object
lifetime defaults, and simply for backwards compatibility.

Rendered

@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 2, 2017
implied bound that `T: 'a` holds.

This situation is considerd unlikely: typically, if a struct has a
lifetime parameter (such as the `Iterator` struct), then the fact that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean Iter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, fixed.

therefore requires an explicit `where T: 'a` declaration on the
struct; in fact, such explicit declarations can be thought of as
existing primarily for the purpose of informing trait object lifetime
defaults, since they are typically not needed otherwise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is my understanding correct that it would be a backwards-compatible change to add an explicit where clause, but that removing a where clause (and leaving it to be inferred) would be a breaking change? In the first case, you're providing extra information to trait object lifetime defaulting, so it seems "better" from a user's perspective. If a library author sees that users are having to frequently specify the extra lifetime bound, they may want to add a where clause in order to enable lifetime defaulting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your understanding is correct, and that seems like a good point to include in the RFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note though that removing a where clause (and leaving it to be inferred) is only a breaking change if you have T: ?Sized types.

Due to the implied bounds rules, it is currently the case that
removing `where T: 'a` annotations is potentially a breaking
change. After this RFC, the rule is a bit more subtle: removing a
field of type `&'a T` could affect the results of inference, and hence
Copy link
Member

@cramertj cramertj Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a field could also be a breaking change. Consider this struct:

struct Foo<'a, T> {
    x: Box<Debug + 'a>,
    y: T,
}

Here, no where T: 'a bound is inferred. Consider adding a &'a T field (or another type with similar WF requirements):

struct Foo<'a, T> {
    x: Box<Debug + 'a>,
    y: T,
    z: &'a T,
}

By adding the z field, we've caused where T: 'a to be inferred, which is a somewhat non-obvious breaking change to our type. It's especially non-obvious when you consider that &'a T could be replaced by CustomRef<'a, T>, a type which may or may not require T: 'a. You'd have to examine the definition of CustomRef to know for sure, and even then it may be non-obvious, as CustomRef might have its own where clause inferred.

This doesn't feel like a huge problem-- I'm certain we could find ways to help users avoid making these kinds of mistakes, but I think it is good to keep in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, it could in some scenarios -- notably around impls, as I mentioned -- be a breaking change.

@steveklabnik
Copy link
Member

I'm a big 👍 on this change; this would eliminate some boilerplate that's not very useful, IMO.

structs, we must be able to do associated type normalization and hence
trait solving, but we would have to do before we know the full WF
requirements for each struct. The current setup avoids this
complication.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "might make try to" has one verb too many and "we would have to do before we know" is missing a verb.

@scottmcm
Copy link
Member

scottmcm commented Aug 3, 2017

I noticed that every example has one lifetime parameter and one type parameter. Since the reference section mentions inferring outlives between lifetimes, it might be nice to have an example like

struct Foo<'a,'b,T>
  // now inferred:  where 'b: 'a, T: 'b
{
    x: &'a &'b T
}

That said, I wonder how useful, comparatively, the multiple-lifetime cases are. It could be that Foo<'a, 'b, T, U, V> is better annotated as where T:'a+'b, V:'b than inferring that.

@Centril
Copy link
Contributor

Centril commented Aug 8, 2017

Great RFC! This will really help in making the language easier for newcomers and lower the bar for entry. <3

@eira-fransham
Copy link

eira-fransham commented Aug 12, 2017

I think this is in danger of making lifetime parameters more, not less, complicated. I think for both this and for functions with inferred parameters it would be good for any lifetime error messages to write the fully-inferred bounds as a note:. The user on IRC had a function that looked something like this:

struct Bar<'a, T: 'a>(&'a mut T);
struct Foo<'a, T: 'a>(&'a mut Bar<'a, T>);

fn make_foo<'a, T>(m: &'a mut Bar<T>) -> Foo<'a, T> {
    Foo(m)
}

It was giving "cannot infer appropriate lifetime" errors. See the problem? I didn't see it instantly and I've been programming Rust for 2 years. If the error message said:

note: fully inferred function definition
      fn make_foo<'a, 'anon0, T>(m: &'a mut Bar<'anon0, T>) -> Foo<'a, T> { ... }

It would be a lot more obvious. If we're thinking of adding more lifetime elision we should definitely consider what this does for teachability. I wouldn't get rid of lifetime elision for the world - it really encourages keeping your code zero-copy - but it's easy to fall into the trap of making this more obvious for the already-obvious cases and less obvious for the non-obvious ones.

@withoutboats withoutboats added the Ergonomics Initiative Part of the ergonomics initiative label Aug 14, 2017
@Ixrec
Copy link
Contributor

Ixrec commented Aug 20, 2017

@Vurich I think that example isn't really relevant to this RFC, and what it's actually demonstrating is that allowing struct lifetime parameters to be omitted is more harmful than helpful, but that's already widely considered a mistake and #2115 proposes to deprecate it.

If you know of an example where an implicit T: 'x bound would make error messages harder to understand and fix (in a way that's not solvable by simply saying "we implied T: 'x" in the error), I'd be very interested in that. So far, in all the code I've ever seen that had a T: 'x bound that this RFC would make implicit, that T: 'x was not providing any useful information and felt like a "you know what I mean, just compile this" annotation.

I think for both this and for functions with inferred parameters it would be good for any lifetime error messages to write the fully-inferred bounds as a note:.

When the error has something to do with the inferred parts of the signature, I 100% agree. Hopefully we'll be reworking lifetime error messages a lot as NLL and these other proposals make progress.

@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

This RFC has been open for 20 days, with no real objections to its content. I think we should merge!

PS, @scottmcm, I agree with you that some extended examples would be useful and will happily add them in a bit.

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 23, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Aug 23, 2017
@nikomatsakis
Copy link
Contributor Author

@Vurich you may be interested in rust-lang/rust#42516, which is tracking some of the improvements to lifetime errors that we are making. We are looking specifically at cases like these! I'd appreciate you adding comments to the issue as well.

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 30, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Aug 30, 2017
@scottmcm
Copy link
Member

@nikomatsakis Friendly ping for extended examples.

@frankmcsherry
Copy link

frankmcsherry commented Sep 6, 2017

This is a minor comment, but I wanted to contrast the position here with that in the implied bounds RFC. The proposal in this RFC seems to support implied bounds in struct definition, but not for impl blocks. The proposal in the linked RFC seems to support implied bounds in impl blocks but not struct definitions.

I understand most of the details locally, how they are different, and e.g. why we shouldn't (can't) imply the bounds in impl blocks (they might not hold). Nonetheless, it seems like this could make it a pain to sort out where the root of truth about implied constraints lives? This worries me as especially frustrating when it comes to debugging lifetime mis-inference (or, correct inference but why !#@$!@ is it !#$!@#!).

how often do you get an error that is solved by writing where T: 'a or where 'a: 'b outside of a struct definition?

I've had this multiple times, not lots, but the only other solution was "delete the code, because I can't understand what Rust is doing in its lifetime inference/unification". Actually, once was "post on irc and have eddyb spot the problem in 5s", but I haven't gotten that ability yet.

One of the scarier things about lifetime inference, for me, is that I don't know the idioms to use to debug lifetime reasoning. I'm more familiar with this for trait implementation, and it is usually fine if a type implements more traits than I realize (i.e. if I misunderstand the breadth of implied bounds for traits). For lifetimes, it is just all more complicated and terrifying; I don't know that I'll understand the implications of further lifetime bounds / unification.

Edit: as a thought experiment: would the implied bounds for traits be quite as appealing if there were pervasive blanket trait implementations based on negative trait constraints? It seems similarly terrifying to try and understand why you don't have an impl you think you should have. That is how reasoning about lifetime errors usually strikes me. My understanding from peers (limited) is that the scary thing about lifetimes in Rust isn't that you have to type 'a a lot, it is that when you get them wrong you are often !@#$%d.

@vitiral
Copy link

vitiral commented Sep 7, 2017

I'm a proponent of this change and have been bitten by issues like this when I was a newbie with lifetimes.

However, one of the reasons I was bitten is because at no point did the compiler ever tell me about "outlives annotations" -- this after doing the full rust by example tutorial and reading the old rust book.

As we have the compiler be better and better at inferring things like this in most cases, I want to make sure there is an issue for the following items:

  • "outlives" syntax in the rust-book: it looks like this exists
  • suggesting reading a document on outlives syntax as a possible solution whenever the compiler can't determine lifetimes
  • as @Vurich suggested, showing the full inferred where clause in compiler errors for lifetimes

I think these would go a long way in improving the ergonomics for corner cases of lifetime elision failure.

@nikomatsakis
Copy link
Contributor Author

I agree with the spirit of these last two comments -- in particular, our error reporting can be better. In fact, we've been hard at work on this. But I don't see this as an either-or thing -- and having fewer errors in the first place will certainly help us to focus on the ones that remain. In fact, I think we've been making good progress on this lately, and more improvements are underway. The general tracking issue for this sort of thing is rust-lang/rust#42516, though it's due for an update with the status and overall strategy.

@frankmcsherry
Copy link

I've read through that and I totally support it (for what that's worth). The error reporting will become much more important, the more magic gets sprinkled on top of things. :)

I might have a different experience, and may miss the point of the proposal, but to my experience Rust has been great about telling me which lifetime bounds to add (and doing so has not resulted in errors), and maybe less great about telling me about lifetime unification I implied (e.g. by returning a borrow taken in a loop body).

It seems like the proposal is not about reducing errors, but about removing boilerplate at the risk of increasing errors, with the hope that there will be a net increase in happiness. I can't do that calculation, but just wanted to make sure you all felt me that there might be two sides to the equation; that optimizing the representation of correct programs is great, but if it comes at the expense of increasing the time to arrive at them...

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Sep 8, 2017

It seems like the proposal is not about reducing errors, but about removing boilerplate at the risk of increasing errors...

That's not how I think of it -- I think this proposal strictly reduces errors (while also reducing boilerplate). Certainly, any program that type-checks today is intended to continue type-checking (if you have a possible counter-example, I'd love to hear it!), so the number of errors reported can only be going down. Moreover, I don't think that this change will make any errors harder to explain or understand, as I tried to explain in the RFC under the section on long-range errors. But I do think we should be on the watch for this, and pay extra attention to explaining such errors.

@frankmcsherry
Copy link

frankmcsherry commented Sep 8, 2017

Moreover, I don't think that this change will make any errors harder to explain or understand, [...]

This was the part I wasn't personally convinced of, but I think you've spent a lot more time introducing people to Rust than I have. I didn't mean "increasing errors" in terms of "number of programs that don't compile" so much as "time spent looking at a program that doesn't compile" or "programming attempts that do not eventually compile".

But I do think we should be on the watch for this, and pay extra attention to explaining such errors.

This part I am totally convinced of. I'll happily report on them if I get bit while this is in nightly (not in the "malicious glee" way, to be clear ;)).

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 9, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Sep 11, 2017

This RFC has been merged! Tracking issue.

@aturon aturon merged commit aa6c09b into rust-lang:master Sep 11, 2017
@vitiral
Copy link

vitiral commented Sep 13, 2017

@nikomatsakis the rendered link is now broken

bors added a commit to rust-lang/rust that referenced this pull request May 26, 2018
2093 infer outlives requirements

Tracking issue:  #44493
RFC: rust-lang/rfcs#2093

- [x] add `rustc_attrs` flag
- [x] use `RequirePredicates` type
- [x]  handle explicit predicates on `dyn` Trait
- [x] handle explicit predicates on projections
- [x] more tests
- [x]  remove `unused`, `dead_code` and etc..
- [x]  documentation
@Centril Centril added A-typesystem Type system related proposals & ideas A-inference Type inference related proposals & ideas A-borrowck Borrow checker related proposals & ideas labels Nov 23, 2018
tonytonyjan added a commit to tonytonyjan/rust-book that referenced this pull request Feb 24, 2019
reasons:

1. This annotation is no longer needed since it can be inferred from the fields present in the definitions in Rust 2018.
2. `T: 'a` annotation is never mentioned in previous chapters, this would make beinners confused.

references:

rust-lang/rfcs#2093
rust-lang/rust#44493
https://rust-lang-nursery.github.io/edition-guide/rust-2018/ownership-and-lifetimes/inference-in-structs.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrowck Borrow checker related proposals & ideas A-inference Type inference related proposals & ideas A-typesystem Type system related proposals & ideas Ergonomics Initiative Part of the ergonomics initiative final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet