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

Tracking issue for RFC 2115: In-band lifetime bindings #44524

Open
aturon opened this Issue Sep 12, 2017 · 126 comments

Comments

Projects
None yet
@aturon
Member

aturon commented Sep 12, 2017

This is a tracking issue for the RFC "In-band lifetime bindings" (rust-lang/rfcs#2115).

Steps:

Unresolved questions:

  • What conventions should we use for lifetime names, and should they be enforced via lint?
  • Can we find an unequivocally-better syntax than '_ for elided lifetimes?
    • These are now stable, so we didn't

Known bugs (possibly incomplete list):

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Sep 12, 2017

@aturon Could we word the unresolved question as something like "what is the appropriate convention, if any" rather than "is this particular convention worthwhile"? Thanks!

@vitiral

This comment has been minimized.

Contributor

vitiral commented Sep 12, 2017

also, that wasn't the only suggested alternative. There is also completely explicit lifetimes using 'self::lifetime_name

@durka

This comment has been minimized.

Contributor

durka commented Sep 12, 2017

$ rg -F "impl<'a>" /rust/src | wc -l
     730

This supposedly ergonomic change is going to require updating a lot of code, including all of our docs and examples. I guess you could just do s/'a/'aa but that seems to miss the point. Could we make some kind of interactive rustfix tool to let you choose new lifetime names and have them automatically updated?

@aturon

This comment has been minimized.

Member

aturon commented Sep 12, 2017

@glaebhoerl Done, thanks!

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 12, 2017

I expanded the unresolved question a bit.

@nikomatsakis nikomatsakis added this to the impl period milestone Sep 15, 2017

@aturon aturon removed this from the impl period milestone Sep 15, 2017

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 18, 2017

Mentoring instructions

I'm not sure 100% the best way to implement this yet, but let me start by giving some pointers. Perhaps @eddyb can elaborate, as he is the last one to touch the relevant code in a serious way, as far as I know.

First thing is a kind of list of what the tasks are in the RFC. This is roughly the order in which I think we should approach them. It may even be worth breaking this out into separate issues.

XXX task list moved into main header

I don't have time for detailed mentoring notes, but here are a few things to start reading into. At present, region resolution etc kind of works like this:

  • During HIR lowering, we insert placeholders for elided lifetimes.
  • After HIR lowering, we run the code in resolve_lifetime.rs.
  • This creates the NamedRegionMap that, for each hir::Lifetime, contains a Region struct indicating what region is being named.
    • This struct is a bit complicated. =)
@eddyb

This comment has been minimized.

Member

eddyb commented Sep 18, 2017

To support '_ we have to do very little:

bors added a commit that referenced this issue Sep 22, 2017

Auto merge of #44691 - cramertj:underscore-lifetimes, r=nikomatsakis
Implement underscore lifetimes

Part of #44524

@nikomatsakis nikomatsakis removed the E-mentor label Sep 25, 2017

bors added a commit that referenced this issue Nov 23, 2017

Auto merge of #46051 - cramertj:in-band-lifetimes, r=nikomatsakis
Implement in-band lifetime bindings

TODO (perhaps in a future PR): Should we ban explicit instantiation of generics with in-band lifetimes, or is it uncontroversial to just append them to the end of the lifetimes list?

Fixes #46042, cc #44524.

r? @nikomatsakis
@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 23, 2017

Lint against single-letter lifetimes not confined to functions

Sorry that I come “after the battle” (this RFC was accepted during the pre-impl-period rush), but as @durka pointed out applying this lint to existing code bases will likely generate a lot of busy work for questionable benefits. I looked for the rationale for this lint but couldn’t find it. https://rust-lang.github.io/rfcs/2115-argument-lifetimes.html#lifetimes-in-impl-headers mentions a “convention”, but doesn’t say why enforcing it is important enough to warn by default. We don’t have such a lint for local variable names or field names, for example.

I assume that proposed lints are intended to warn by default. If they’re silent by default I have no concern.

@rpjohnst

This comment has been minimized.

Contributor

rpjohnst commented Nov 24, 2017

The idea is that there will no longer be <'a> sections in functions, so it will be harder to tell apart lifetime names from impl scope and those from function scope. This makes it tricky to know whether a name used in a function is intended to refer to an impl-scope lifetime, or to define a new one (and should thus be renamed).

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 24, 2017

@rpjohnst Sorry, I have a hard time figuring out if your comment is arguing for or against something, and whether that thing is the lint I mentioned or some other change proposed by this tracking issue.

@rpjohnst

This comment has been minimized.

Contributor

rpjohnst commented Nov 24, 2017

@SimonSapin It's just trying to describe the reasoning behind the lint's existence, not necessarily argue for or against it.

@aturon

This comment has been minimized.

Member

aturon commented Sep 17, 2018

@stjepang I wonder to what degree the problem here is just "bad" syntax -- the <'_> is pretty unwieldy.

As to whether it's obvious whether the types you mention contain references -- I think there are a lot of times where that's not the case, for example when you're working with an unfamiliar code base, or just don't happen to be thinking about that question and hence don't catch the borrowing implied by a signature.

I personally do find a visual signal of some kind to be quite helpful, but am sad that we weren't able to find something more appealing and lightweight.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 18, 2018

@aturon

I wonder to what degree the problem here is just "bad" syntax -- the <'_> is pretty unwieldy.

That's the only problem really: <'_> is far more unwieldy than &.

I personally do find a visual signal of some kind to be quite helpful, but am sad that we weren't able to find something more appealing and lightweight.

We tried using <'_> everywhere in librustc (see #53816), but I'm not sure the end result is great. Signatures like these are not pretty:

pub fn to_dep_node(self, tcx: TyCtxt<'_, '_, '_>, kind: DepKind) -> DepNode;

I appreciate the usefulness of explicit lifetimes here, but considering the verbosity they don't seem like a net win to me. YMMV, though.

My main worry is that the proliferation of lifetimes will be a step back for ergonomics and that we might have to write lifetimes more often in Rust 2018 than in Rust 2015. I'm not sure if that's really true, but it'd surely be unfortunate.

@cramertj

This comment has been minimized.

Member

cramertj commented Sep 18, 2018

I'm of two minds about this: I personally think it's really helpful as a reader to be able to see which types have lifetimes in them, and I like seeing SomeType<'_>. However, I am scared that we're making lifetimes "noisier" which will have the potential to frustrate authors and make them use owned types instead in order to avoid sticking <'_> everywhere.

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 18, 2018

I still think we should make the rule "you have to only write out lifetimes (even just '_) if they're used more than once" (i.e. elision picks them up).

Why? Because only then do they become lifetime relationships.
Otherwise, the function is quite universally quantified over them, which is pretty boring.

@aturon

This comment has been minimized.

Member

aturon commented Sep 18, 2018

@eddyb My concern with that approach is that there's another aspect of ergonomics at play: being able to easily remember what is required where. To me, it seems much easier to remember that Ref always gets a lifetime than to use it only in select circumstances.

While '_ is stable, for the 2018 Edition we could consider scaling back the lint, at least initially.

I'd really love to hear from @nikomatsakis here; he's been pretty outspoken about this in the past.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 18, 2018

I want to speak up in favor of having some signal that there are "hidden lifetimes" in types. The current notation of '_ is not my favorite, but I don't yet have a compelling alternative.

What I find is that I am constantly wanting to know whether a type contains references or not. It tells me a lot about how the type can be used: types without lifetimes can be cloned or copied and stored for arbitrary amounts of type. Types with lifetimes are always temporary in scope and confined to some caller (modulo longer lived arenas like 'tcx in the compiler, which are the exception, and also readily identifiable). In the specific case of a type that appears in the return value, I find it invaluable to know if there is an elided lifetime within, since it tells me how long the borrow on self will last.

I suspect that I derive outsized value from this notation because my intuitions for this stuff are stronger than most (can't imagine why that might be). But I feel like having a consistent syntax that highlights where lifetimes are and where they are not might wind up being very important for helping other users to develop these annotations.

OK, let me give a few examples that I've run across recently. Just yesterday I was reading into the futures code and I saw this:

fn poll(self: PinMut<Self>, cx: &mut task::Context) -> Poll<Self::Output>

Now, I know that a task::Context is an accumulation of bits of state. But I inferred from this signature that this state must not have any references associated with it and hence must be owned by task::Context. This seemed surprising to me. But when I clicked through to the definition, I saw that I was mistaken, and that indeed the Context type carries references:

/// Information about the currently-running task.
///
/// Contexts are always tied to the stack, since they are set up specifically
/// when performing a single `poll` step on a task.
pub struct Context<'a> {
local_waker: &'a LocalWaker,
spawner: &'a mut dyn Spawn,
}

Note: to make matters worse, rustdoc actually hides these references, so it's actually quite hard to figure out what is going on. Anyway, then I kept reading, and I encountered this signature:

pub fn will_wake(&self, other: &Waker) -> bool

Here, I was confused for quite a while, because I misremembered that Waker carried a "hidden" lifetime (in fact, it was Context). I knew that somehow poll was supposed to "store away" a copy of the Waker, so that it could notify the executor when it should be rescheduled, but I couldn't figure out how that was possible, since Waker carried references. It took some time for me to realize that I has misremembered and that Waker does not have references, so you can clone it and not be tied to any active borrow.

Another example comes from the rustc source, which for a long time contained a function like this:

pub fn predecessors_for(&self, bb: BasicBlock) -> MappedReadGuard<Vec<BasicBlock>>

It happens that MappedReadGuard holds a reference into self, so this borrow will last as long as the return value is in use. But could you guess that?

This same principle applies to compiler error messages. There, it's quite nice that we have something we can highlight to show you which reference we are talking about. e.g., we can now give errors like this:

fn foo(x: Blah<'_, T>) { .. }
               ^^ let's call this lifetime `'1`

Here, I imagine that we can find other solutions (and in general I'd like to work on improving these messages in lots of ways), but in general having a visual signal for where references are will always be useful to us, I think.

As far as consistency goes, I think there is value in trying to close the gap between the various ways that you write types in Rust. Right now, types that appear in function signatures and those that appear in struct declarations sometimes look really different (e.g., &Ref<T> vs &'a Ref<'a, T>). If we have '_, then there is at least a signal telling you "when you put this into a struct, it's going to need lifetime parameters". (I would still like to have some way to have a single anonymous lifetime parameter of a struct, so that one could write &Ref<T> as well, but that's neither here nor there.)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 18, 2018

@stjepang

We tried using <'_> everywhere in librustc (see #53816), but I'm not sure the end result is great. Signatures like these are not pretty:

I agree that this is not pretty, but I'm not sure that just a plan TyCtxt is really better. I personally find it a bit "surprising" to see things like TyCtxt without any parameters, since it is relatively unusual, and it makes my mind feel like something is missing. I suspect that for newcomers to rustc it might also be hard to remember what things have lifetimes and which do not, though it's hard for me to judge.

There are also types for which writing lifetimes in parameters is not that helpful:

  • std::pin::PinMut<'_, T>
  • std::cell::Ref<'_, T>
  • std::sync::MutexGuard<'_, T>

Speaking personally, I find the '_ there ugly but I do find it serves a purpose. Without that, when I see Ref<T>, my mind pattern matches it with Rc<T> or other such pointer types, and those two things are quite different.

I think ultimately I agree with @aturon's comment here:

I personally do find a visual signal of some kind to be quite helpful, but am sad that we weren't able to find something more appealing and lightweight.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 18, 2018

I should also add that I appreciate that typing Ref<'_, T> is kind of... challenging. It feels like the sort of thing where I want an IDE that, once I type Ref, just fills in the <'_, _> or something. :)

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 18, 2018

@nikomatsakis Did I miss a part of your comments, or did you not address the specific suggestion of only requiring explicit '_ when there is an implicit relationship (i.e. when elision is involved)?

It happens that MappedReadGuard holds a reference into self, so this borrow will last as long as the return value is in use. But could you guess that?

The elision aspect would cover that. And IMO I find that far more interesting than being able to tell whether a type is parametrized when looking at an universal function.

I'd rather have IDEs and rustdoc provide an expanded version on hover, than have to write in all the lifetimes even in signatures where they're inconsequential.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 18, 2018

@eddyb I did not specifically address that. I agree that is better than the status quo. It might be a good place to start, given the controversy of the more encompassing proposal.

However, I don't find this to be true:

And IMO I find that far more interesting than being able to tell whether a type is parametrized when looking at an universal function.

That is to say, I personally get a lot of value from knowing whether types are parameterized when looking at universal functions. Often, when somebody pings me with some lifetime error and associated source, the first thing I do is to spend a while figuring out "where the lifetimes are". Currently, this requires jumping through to the source of all the various types involved.

I agree though that there is a cost of heavier notation.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 20, 2018

Is there a lint that warns against elided lifetimes in types (I mean everywhere, not just in return position)? I'd like to see how much of an effect that has on a few projects I'm familiar with -- and if it's not too bad, it might change my opinion.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Sep 20, 2018

@stjepang try cargo rustc -- -W elided-lifetimes-in-paths, I think that still has the full behaviour

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 20, 2018

Or even RUSTFLAGS=-Welided-lifetimes-in-paths cargo build (although I don't know if that can affect dependencies, given cargo's "cap lints").

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 20, 2018

Ok, I've just run the lint on all the crossbeam crates, rayon, ripgrep, and hyper. Here's what I learned...

The vast majority of the reported warnings are either missing lifetimes in return position or in fmt::Formatter. There are only a few remaining cases where we need to add lifetimes but it's no big deal, honestly. I'm pleasantly surprised by the low impact of the lint.

Still, the large PR (around 550 lines were changed) submitted on librustc was making me feel uneasy. I thought maybe the code in librustc is a particularly bad example that uses lifetimes heavily.

Then I went on to calculate the ratio of warnings per total lines of code in those projects. Perhaps librustc has a higher ratio of warnings per total lines of code? It turns out it doesn't - the ratio is not significantly higher. It's just that librustc is really big.

I think my opinion has changed so allow me to make a 180 turn and give a 👍 for requiring lifetimes in all types. If anyone else is feeling unconvinced, I encourage you to do the same thing and try to assess the real impact of the lint.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Sep 20, 2018

If anyone else is feeling unconvinced, I encourage you to do the same thing and try to assess the real impact of the lint.

In https://github.com/rust-lang-nursery/futures-rs this results in 511 warnings (from 15028 lines of code according to tokei (including tests which I did not try building with the warning)). Scanning the output I'm relatively confident that every single warning is for an input parameter that is not connected to the output in any way, so would result in no warning for the "explicit lifetimes when elision is used" variant.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 20, 2018

In rust-lang-nursery/futures-rs this results in 511 warnings (from 15028 lines of code according to tokei).

To be fair, futures is a bit of a special case because almost all warnings come from task::Context<'_> and PinMut<'_, Self>. And there's a whole lot of Future impls in there, which you're not supposed to implement by yourself anyway.

Besides, PinMut<'_, Self> will soon become Pin<&mut Self>, which cuts the number of warnings down from 511 to 216.

@cramertj

This comment has been minimized.

Member

cramertj commented Sep 20, 2018

Besides, PinMut<'_, Self> will soon become Pin<&mut Self>, which cuts the number of warnings down from 511 to 216.

And task::Context<'_> will become &LocalWaker, fixing the remaining half (see #54339).

@scottmcm

This comment has been minimized.

Member

scottmcm commented Sep 22, 2018

Signatures like these are not pretty:

pub fn to_dep_node(self, tcx: TyCtxt<'_, '_, '_>, kind: DepKind) -> DepNode;

Undeveloped thought:

pub fn to_dep_node(self, tcx: TyCtxt<'..>, kind: DepKind) -> DepNode;

(By analogy to Foo(_, _, _) => Foo(..) patterns.)

@emilio

This comment has been minimized.

Contributor

emilio commented Sep 23, 2018

Hmm, this lint produces 1514 warnings on Servo's style component alone, and the fix looks a lot more verbose and ugly :(

@emilio

This comment has been minimized.

Contributor

emilio commented Sep 23, 2018

Most of them are actually unused lifetimes, as in, there's no relation with them and the output of the function... I think @eddyb's approach of only warning when elision actually comes at play it's way better than the current warning, getting the usefulness without the annoyance.

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 23, 2018

@scottmcm I frankly disagree we need explicit lifetimes in that case.

bors added a commit that referenced this issue Sep 30, 2018

Auto merge of #53816 - zackmdavis:elided_lifetimes_in_paths_field_day…
…, r=nikomatsakis

don't elide lifetimes in paths in librustc/

In light of the "Apply to rustc" checkbox on #44524 and @nikomatsakis's [recent comment about regularly wanting visual indication of elided lifetimes in types](#44524 (comment)), I was curious to see what it would look like if we turned the `elided_lifetimes_in_path` lint on in at least one crate in the codebase (I chose librustc). Given that I couldn't figure out how to get `cargo fix` work with the build system, this arguably wasn't a very efficient use of my time, but once I started, the conjunction of moral law and the sunk cost fallacy forced me to continue.

This is mostly applying the `<'_>` suggestions issued by the lint, but there were a few places where I named the lifetimes (_e.g._, `<'a, 'gcx, 'tcx>` on `TyCtxt`) in order to match style with surrounding code.

r? @nikomatsakis

bors added a commit that referenced this issue Sep 30, 2018

Auto merge of #53816 - zackmdavis:elided_lifetimes_in_paths_field_day…
…, r=nikomatsakis

don't elide lifetimes in paths in librustc/

In light of the "Apply to rustc" checkbox on #44524 and @nikomatsakis's [recent comment about regularly wanting visual indication of elided lifetimes in types](#44524 (comment)), I was curious to see what it would look like if we turned the `elided_lifetimes_in_path` lint on in at least one crate in the codebase (I chose librustc). Given that I couldn't figure out how to get `cargo fix` work with the build system, this arguably wasn't a very efficient use of my time, but once I started, the conjunction of moral law and the sunk cost fallacy forced me to continue.

This is mostly applying the `<'_>` suggestions issued by the lint, but there were a few places where I named the lifetimes (_e.g._, `<'a, 'gcx, 'tcx>` on `TyCtxt`) in order to match style with surrounding code.

r? @nikomatsakis
@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 30, 2018

Here's a related pet peeve of mine. Currently, this compiles:

struct Foo<'a>(&'a ());
impl Foo<'_> {} // anonymous lifetime

But this doesn't:

struct Foo<'a>(&'a ());
impl Foo {} // elided lifetime

Sometimes requiring <'_> and sometimes not requiring it feels inconsistent. We should pick a side: either require it or don't.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 15, 2018

I wonder, would it be reasonable to stabilize only in-band lifetimes on functions (not methods)? AFAICT, there is no ambiguity about where those lifetimes come from, right?

fn foo(x: &'a usize) -> &'a usize { ... }
@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 15, 2018

To be clear, I mean that we would stabilize a useful subset now as long as it is future-compatible with the rest of in-band lifetimes.

@durka

This comment has been minimized.

Contributor

durka commented Nov 15, 2018

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 15, 2018

Hmm... I hadn't thought about nested functions...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment