Implement underscore lifetimes #44691

cramertj · 2017-09-19T05:58:32Z

rust-highfive · 2017-09-19T05:58:43Z

(rust_highfive has picked a reviewer for you, use r? to override)

cramertj · 2017-09-19T06:01:02Z

src/librustc_passes/ast_validation.rs

+                self.err_handler()
+                    .span_err(lt_def.lifetime.span,
+                              &format!("invalid lifetime name `{}`", lt_def.lifetime.ident));
+            }


Feel free to bikeshed this error message (it shows up in code that tries to explicitly bind '_ in the generics of an item declaration).

An equivalent error is already issued for 'static, so the wording can be reused:

invalid lifetime parameter name: `'static`

arielb1 · 2017-09-19T08:47:19Z

r? @eddyb

kennytm · 2017-09-19T09:08:40Z

src/libsyntax/feature_gate.rs

@@ -383,6 +383,9 @@ declare_features! (

    // allow '|' at beginning of match arms (RFC 1925)
    (active, match_beginning_vert, "1.21.0", Some(44101)),
+
+    // allow `'_` placeholder lifetimes
+    (active, underscore_lifetimes, "1.21.0", Some(44524)),


Should be 1.22.0? (BTW match_beginning_vert should also be 1.22.0?)

I never know what to actually put here.

eddyb · 2017-09-19T09:11:25Z

r=me with build & nit fixed

petrochenkov · 2017-09-19T13:19:42Z

src/librustc_passes/ast_validation.rs

@@ -318,6 +310,13 @@ impl<'a> Visitor<'a> for AstValidator<'a> {

    fn visit_generics(&mut self, g: &'a Generics) {
        let mut seen_default = None;
+        for lt_def in &g.lifetimes {


Lifetime definitions can happen in for<'a, 'b, 'c> in fn pointers, trait objects, etc.
It's better to place the error in fn visit_lifetime_def.

petrochenkov · 2017-09-19T13:24:38Z

@cramertj
What happens when '_ is written in some place where elided lifetimes are not supported currently?
For example bounds 'a: '_ + 'b, impl headers, struct definitions.
Could you add tests for these cases?

nikomatsakis · 2017-09-19T20:06:45Z

src/librustc/hir/mod.rs

@@ -159,7 +159,8 @@ impl fmt::Debug for Lifetime {

 impl Lifetime {
    pub fn is_elided(&self) -> bool {
-        self.name == keywords::Invalid.name()
+        self.name == keywords::Invalid.name() ||
+        self.name == "'_"


So I've never been crazy about using a "sentinel value" here rather than an enum -- now that we've got more than one sentinel, I feel even less good. Can't we change this to something like

enum LifetimeName { Elided, Underscore, Static, Name(Token) // or whatever this is }

It's a Symbol / ast::Name I think.

nikomatsakis · 2017-09-19T20:07:31Z

src/librustc/middle/resolve_lifetime.rs

@@ -1422,7 +1422,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
            let lifetime_i = &lifetimes[i];

            for lifetime in lifetimes {
-                if lifetime.lifetime.is_static() {
+                if lifetime.lifetime.is_static() || lifetime.lifetime.name == "'_"  {


...if we were using an enum, this code would be rather cleaner

nikomatsakis · 2017-09-19T20:49:28Z

src/test/run-pass/underscore-lifetimes.rs

+
+struct Foo<'a>(&'a u8);
+
+fn foo(x: &u8) -> Foo<'_> {


can we have some tests that use multiple lifetime parameters in one function? For example:

fn foo(x: &'_ u8, y: &'_ u8) -> &'_ u8 { } // ERROR from elision fn foo(x: &mut Vec<&'_ u8>, y: &'_ u8) { x.push(y); // ERROR } struct Foo<'a, 'b> { a: &'a u32, b: &'b u32 } fn foo_a<'b>(foo: Foo<'_, 'b>) -> &'b u32 { &foo.a } // ERROR fn foo_b<'b>(foo: Foo<'_, 'b>) -> &'b u32 { &foo.b } // OK

nikomatsakis · 2017-09-19T20:50:02Z

@cramertj thanks for jumping on this btw -- so excited to see it land =)

petrochenkov · 2017-09-19T23:52:13Z

src/librustc/hir/mod.rs

+            Elided => keywords::Invalid.name(),
+            Underscore => Symbol::intern("'_"),
+            Static => Symbol::intern("'static"),
+            Name(name) => name,


Nooooo.
This LifetimeName over Name is pure over-engineering, N extra lines of boilerplate just to avoid one string comparison.

Heh. I don't have any strong opinion here. Take it up with @nikomatsakis 😄
I left it as its own commit so it's easy to pull out if that's the decision.

I'd better turn '_ into a weak keyword.

I disagree. Sentinel values mean you wind up with if-else-if chains like if foo.is_elided() { ... } which are easily forgotten. Having an enum means we can use exhaustive matching, so that when we add variants in the future, the compiler helps us catch cases that are missing etc.

Plus, there isn't a lot of boilerplate here.

@nikomatsakis
Ok, not a big deal.
(I'll revert this when you are on vacation, ha-ha.)

cramertj · 2017-09-20T00:45:32Z

Gah! I didn't try building rustdoc and the others...

eddyb · 2017-09-20T16:33:06Z

cc @Mark-Simulacrum this is why I have rustbuild in my build setup.

petrochenkov · 2017-09-20T17:31:13Z

src/librustc/hir/mod.rs

+        match *self {
+            Elided => keywords::Invalid.name(),
+            Underscore => Symbol::intern("'_"),
+            Static => Symbol::intern("'static"),


Symbol::intern("'static") => keywords::StaticLifetime.name()

petrochenkov · 2017-09-20T21:22:05Z

@cramertj
Could you add tests for '_ in impl headers and struct definitions? (But not as bounds or lifetime definitions.)

bors · 2017-09-21T02:59:54Z

☔ The latest upstream changes (presumably #44551) made this pull request unmergeable. Please resolve the merge conflicts.

petrochenkov · 2017-09-21T08:34:22Z

src/librustc/hir/mod.rs

+    Underscore,
+    Static,
+    Name(Name),
+}


@cramertj
Actually, can you tweak the naming slightly.

enum LifetimeKind { // This is not a name Implicit, // `Elided` is not correct, because implicit can be both elided and inferred, but we don't discern between them yet when lowering into HIR Underscore, Static, Named(Name), // Named lifetime }

(The variants are going to change anyway when lifetimes are first resolved, then lowered into HIR.)

I think what we want is name resolution to give us a Def, and the existing resolve_lifetimes pass to do elision and compute the semantic representation from Def::Lifetime(DefId).

@petrochenkov I don't object to calling "Implicit" for sure, but I'm curious: what does "elided" mean to you? To me, it means "not written". I would then say that the behavior around elided lifetimes depends on context: in function signatures, we use defaulting rules (as specified by the lifetime elision RFCs plus the object-lifetime RFCs), and in function bodies, we use inference.

Using "elided" to mean literally "not written" may be correct wrt the English language but then "elision rules" doesn't mean much anymore and we have to say "default lifetime rules" I guess? And "lifetime defaulting" for the process of picking a lifetime.

So personally, when I explain to people, I say something like "sometimes you can elide lifetimes. The meaning of this depends on context. In a function signature, we use the following system of defaults. etc". In other words, I think I do use the terms the way you described. =) But also I rely on context. That is, I might say "elision rules" when it's clear we're talking about function signatures, in which case it just means "defaulting rules" (but including, naturally, the object defaults).

In any case, it seems clear that calling things elided is ripe for confusion. I prefer either "implicit", "not written", or "not present", all of which seem reasonably clear.

@nikomatsakis
Well, I usually associate "elided" with "lifetime elision RFCs", i.e. "defaulting", not inference.
It's useful to have some separate words for "not-written-in-signature" and "not-written-in-body", and "elided" and "inferred" already kinda serve this purpose given that "lifetime elision" is commonly used to describe what happens in signatures.

That makes some sense. I guess I prefer to have words that match their English meanings to the extent possible, though, and to me "elided" just means "not written" (whereas "defaulted" seems more specific -- we assigned it a default value). I admit that the distinction between defaulted (based on some simple rules) and inferred (based on complex analysis) is a subtle one as well. =)

Regardless, the question at hand is whether to use elided here with the broader meaning, right? In which case, "Implicit" or "Not written" seems fine.

…ions

nikomatsakis · 2017-09-21T17:33:07Z

@bors r+

bors · 2017-09-21T17:33:08Z

📌 Commit f5505d1 has been approved by nikomatsakis

bors · 2017-09-22T14:05:22Z

⌛ Testing commit f5505d1 with merge 3eb19bf...

Implement underscore lifetimes Part of #44524

bors · 2017-09-22T17:17:26Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3eb19bf to master...

rust-highfive assigned nikomatsakis Sep 19, 2017

cramertj commented Sep 19, 2017

View reviewed changes

kennytm reviewed Sep 19, 2017

View reviewed changes

petrochenkov reviewed Sep 19, 2017

View reviewed changes

cramertj force-pushed the underscore-lifetimes branch 2 times, most recently from 0c3b9ef to 942a85c Compare September 19, 2017 18:11

nikomatsakis requested changes Sep 19, 2017

View reviewed changes

arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 19, 2017

cramertj force-pushed the underscore-lifetimes branch from ac16bad to c91a1a7 Compare September 19, 2017 23:46

petrochenkov reviewed Sep 19, 2017

View reviewed changes

cramertj force-pushed the underscore-lifetimes branch from c91a1a7 to 051b5cf Compare September 20, 2017 16:56

petrochenkov reviewed Sep 20, 2017

View reviewed changes

cramertj force-pushed the underscore-lifetimes branch from 051b5cf to 71c8ad4 Compare September 20, 2017 18:13

Implement underscore lifetimes

64314e3

cramertj force-pushed the underscore-lifetimes branch from 8f5f35c to 31fd099 Compare September 21, 2017 06:46

petrochenkov reviewed Sep 21, 2017

View reviewed changes

nikomatsakis approved these changes Sep 21, 2017

View reviewed changes

cramertj added 3 commits September 21, 2017 10:19

Refactor lifetime name into an enum

f64af7a

Add tests for multiple underscore and non-underscore lifetimes

06926b6

Add tests for underscore lifetimes in impl headers and struct definit…

f5505d1

…ions

cramertj force-pushed the underscore-lifetimes branch from 31fd099 to f5505d1 Compare September 21, 2017 17:19

This was referenced Sep 21, 2017

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

Closed

In-band lifetimes: Lint against single-use lifetime names #44752

Open

nikomatsakis added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2017

bors added a commit that referenced this pull request Sep 22, 2017

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

3eb19bf

Implement underscore lifetimes Part of #44524

bors merged commit f5505d1 into rust-lang:master Sep 22, 2017

This was referenced Sep 22, 2017

Initial support for ..= syntax #44709

Merged

Friendlier error message for closure argument type mismatch #44735

Merged

cramertj deleted the underscore-lifetimes branch September 22, 2017 17:22

Frederick888 mentioned this pull request Sep 23, 2017

Failed to build with rustc 1.22.0-nightly (14039a42a 2017-09-22) rust-lang/rust-clippy#2080

Closed

Implement underscore lifetimes #44691

Implement underscore lifetimes #44691

Conversation

cramertj commented Sep 19, 2017

rust-highfive commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 19, 2017

Choose a reason for hiding this comment

petrochenkov commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Sep 19, 2017

Choose a reason for hiding this comment

cramertj Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Sep 20, 2017

eddyb commented Sep 20, 2017

Choose a reason for hiding this comment

petrochenkov commented Sep 20, 2017

bors commented Sep 21, 2017

petrochenkov Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Sep 21, 2017

bors commented Sep 21, 2017

bors commented Sep 22, 2017

bors commented Sep 22, 2017

cramertj Sep 19, 2017 •

edited

Loading

petrochenkov Sep 21, 2017 •

edited

Loading

petrochenkov Sep 21, 2017 •

edited

Loading