Perform lifetime elision (more) syntactically, before type-checking. #39305

eddyb · 2017-01-25T22:13:20Z

The initial goal of this patch was to remove the (contextual) &RegionScope argument passed around rustc_typeck::astconv and allow converting arbitrary (syntactic) hir::Ty to (semantic) Ty.
I've tried to closely match the existing behavior while moving the logic to the earlier resolve_lifetime pass, and the crater report suggests none of the changes broke real code, but I will try to list everything:

There are few cases in lifetime elision that could trip users up due to "hidden knowledge":

type StaticStr = &'static str; // hides 'static
trait WithLifetime<'a> {
    type Output; // can hide 'a
}

// This worked because the type of the first argument contains
// 'static, although StaticStr doesn't even have parameters.
fn foo(x: StaticStr) -> &str { x }

// This worked because the compiler resolved the argument type
// to <T as WithLifetime<'a>>::Output which has the hidden 'a.
fn bar<'a, T: WithLifetime<'a>>(_: T::Output) -> &str { "baz" }

In the two examples above, elision wasn't using lifetimes that were in the source, not even needed by paths in the source, but rather happened to be part of the semantic representation of the types.
To me, this suggests they should have never worked through elision (and they don't with this PR).

Next we have an actual rule with a strange result, that is, the return type here elides to &'x str:

impl<'a, 'b> Trait for Foo<'a, 'b> {
    fn method<'x, 'y>(self: &'x Foo<'a, 'b>, _: Bar<'y>) -> &str {
        &self.name
    }
}

All 3 of 'a, 'b and 'y are being ignored, because the &self elision rule only cares that the first argument is "self by reference". Due implementation considerations (elision running before typeck), I've limited it in this PR to a reference to a primitive/struct/enum/union, but not other types, but I am doing another crater run to assess the impact of limiting it to literally &self and self: &Self (they're identical in HIR).

It's probably ideal to keep an "implicit Self for self" type around and only apply the rule to &self itself, but that would result in more bikeshed, and #21400 suggests some people expect otherwise.
Another decent option is treating self: X, ... -> Y like X -> Y (one unique lifetime in X used for Y).

The remaining changes have to do with "object lifetime defaults" (see RFCs 599 and 1156):

trait Trait {}
struct Ref2<'a, 'b, T: 'a+'b>(&'a T, &'b T);

// These apply specifically within a (fn) body,
// which allows type and lifetime inference:
fn main() {
    // Used to be &'a mut (Trait+'a) - where 'a is one
    // inference variable - &'a mut (Trait+'b) in this PR.
    let _: &mut Trait;

    // Used to be an ambiguity error, but in this PR it's
    // Ref2<'a, 'b, Trait+'c> (3 inference variables).
    let _: Ref2<Trait>;
}

What's happening here is that inference variables are created on the fly by typeck whenever a lifetime has no resolution attached to it - while it would be possible to alter the implementation to reuse inference variables based on decisions made early by resolve_lifetime, not doing that is more flexible and works better - it can compile all testcases from #38624 by not ending up with &'static mut (Trait+'static).

The ambiguity specifically cannot be an early error, because this is only the "default" (typeck can still pick something better based on the definition of Trait and whether it has any lifetime bounds), and having an error at all doesn't help anyone, as we can perfectly infer an appropriate lifetime inside the fn body.

TODO: write tests for the user-visible changes.

cc @nikomatsakis @arielb1

rust-highfive · 2017-01-25T22:13:32Z

r? @nrc

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

arielb1 · 2017-01-25T23:07:06Z

Another decent option is treating self: X, ... -> Y like X -> Y (one unique lifetime in X used for Y).

That's certainly an interesting option with the rule being "resolution-based". I would like either that or to treat "just &self with HIR changes".

If we're changing things here, I would also prefer to change &mut *const Trait to be &mut *const Trait+'static, to be consistent with &mut RawPtr<Trait+'static>.

eddyb · 2017-01-25T23:32:03Z

The second crater report (enabling the &self rule only for &self and self: &Self):

a bunch of breaking libsyntax changes in between crater runs (I rebased)
@joelself's tomllib (failure) is the only true regression

pub fn get_children<S>(self: &Parser<'a>, key: S) -> Option<&Children>

This is really bad because it doesn't fit the "only literally &self" rule, not even the "self: X with exactly one unique lifetime in X" rule, and I'd rather not break anything in practice, at least to start off with.

eddyb · 2017-01-25T23:44:06Z

@alexcrichton I'm getting a travis failure but all I did was move E0106's explanation (including 2 code examples) from src/librustc_typeck/diagnostics.rs to src/librustc/diagnostics.rs - is the former not being checked at all, or is this somehow my bug still? (I checked them locally and the result is the same)

alexcrichton · 2017-01-25T23:51:38Z

@eddyb whoa I can't even interpret that failure at all... I'm not too familiar with error index checking, but others may know more

eddyb · 2017-01-26T00:12:27Z

I checked and the indices do correspond to E0106's code examples (in the combined error code page).
Since it's combined, my earlier question doesn't make sense at all, now that I think about it.

cc @rust-lang/docs

GuillaumeGomez · 2017-01-26T10:49:27Z

@eddyb: I found 9 references to E0106 but they're lost in the middle of other errors. Seems a bit strange and not unexpected, no? Not sure of what is the problem in here.

bors · 2017-01-26T17:45:12Z

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

nikomatsakis · 2017-01-26T20:33:05Z

@arielb1

If we're changing things here, I would also prefer to change &mut *const Trait to be &mut *const Trait+'static, to be consistent with &mut RawPtr<Trait+'static>.

Hmm, this does seem like an oversight in the original RFC. I think the "spirit" of the rules would indeed be that *const T and *mut T "reset" the default, just as Box<T> does.

@eddyb @arielb1

Another decent option is treating self: X, ... -> Y like X -> Y (one unique lifetime in X used for Y).

I agree this is potentially a nice rule but I do not feel comfortable making changes like this to the RFC without at minimum wider discussion. What I had personally envisioned is that we would create a new elision rule which avoids some of the more confusing cases and open an RFC to lint for cases that fall outside of that newer rule. Similarly, I might want to pair such a rule with some improvements that allow for more anonymous or lightweight parameters -- i.e., so that even if you don't have to say nothing at all, you maybe don't have to go all the way towards declaring a named parameter -- but that depends on how many errors we see on crates.io.

A prime example of something I would want to be a lint is this:

struct Ref<'a, T> { ... }
impl Something {
    fn foo(&self) -> Ref<i32>;
}

But I do not want you to have to write fn foo<'a>(&'a self) -> Ref<'a, i32>. I think I would rather you can write something like Ref<&, i32>. I'm sure many will hate this but anyway my point is that while I do think the current rules can be improved, I think this is something we should discuss more broadly, unless there is something clearly broken in current implementation.

nikomatsakis · 2017-01-26T20:34:26Z

In terms of what's actually implemented, then, the main change is that this all happens before typeck and thus has to be driven by what is syntactically written, right? So in particular the two examples you gave no longer "work":

fn foo(x: StaticStr) -> &str { x }
fn bar<'a, T: WithLifetime<'a>>(_: T::Output) -> &str { "baz" }

But any case like &self, self: &Self, or self: &Parser<'a, 'b> continues to work as it did before, right?

nikomatsakis · 2017-01-26T20:35:54Z

Somehow those two changes feel "in the spirit" of the original RFC to me. I'm not sure how I feel about changing the behavior of self: &Parser<'a, 'b>. This seems a bit less clear. I think I am thinking in part of the (not accepted) RFC to disable elision in impls with lifetime parameters, for fear of confusion -- which would have been similar but more extreme.

nikomatsakis · 2017-01-26T20:36:05Z

In any case, I think that this is something that @rust-lang/lang should weigh in on.

eddyb · 2017-01-26T20:44:11Z

I don't want to change any more here, just noting it may be a good idea to do it, so that we can have simpler and more uniform rules (considering we can't just evaluate arbitrary types).

nikomatsakis · 2017-01-26T20:49:27Z

@eddyb

I don't want to change any more here, just noting it may be a good idea to do it, so that we can have simpler and more uniform rules (considering we can't just evaluate arbitrary types).

Agreed. I think the main thing I want to change is that there should never be "invisible" lifetimes. Instead we should have some sufficiently light notation for acknowledging them. So e.g., fn(&T) is ok, but instead of fn(Ref<T>) you might write fn(Ref<&, T>). I feel like I'd prefer something lighter still but haven't figured out what that might be.

nikomatsakis

This looks good, I think, the main thing I would like is more tests, as we discussed on IRC. I'll try to think of some edge cases.

nikomatsakis · 2017-01-26T20:50:32Z

src/librustc/hir/lowering.rs

-        let module = self.lower_mod(&c.module);
-        let attrs = self.lower_attrs(&c.attrs);
-        let exported_macros = c.exported_macros.iter().map(|m| self.lower_macro_def(m)).collect();
+        struct MiscCollector<'lcx, 'interner: 'lcx> {


Why such a generic name? This is basically the "lifetime collector" or something, right?

We'd use this for anything we may need to access out of order.

Seems fine. I'd probably just call it "collector" then but whatever.

nikomatsakis · 2017-01-26T21:26:25Z

src/librustc/hir/mod.rs

@@ -89,6 +89,12 @@ impl fmt::Debug for Lifetime {
    }
 }

+impl Lifetime {
+    pub fn is_elided(&self) -> bool {


I feel like we should leave a comment on the Name field, though I might prefer to make an enum here. Something like

enum LifetimeName { Elided, Present(Name) }

Using a "sentinel" value feels a bit non-obvious.

bors · 2017-01-27T07:32:55Z

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

eddyb · 2017-01-27T14:10:02Z

Oh, these are the failures:

error[E0106]: missing lifetime specifier
 --> <anon>:6:10
  |
6 |     foo: Foo,     // error: expected 1, found 0
  |          ^^^ expected lifetime parameter

thread 'rustc' panicked at 'Some expected error codes were not found: ["E0107"]'

error[E0106]: missing lifetime specifier
 --> <anon>:5:15
  |
5 | impl Quux for Foo { } // error: expected 1, found 0
  |               ^^^ expected lifetime parameter

thread 'rustc' panicked at 'Some expected error codes were not found: ["E0107"]

Looks like this is about E0107 changing to E0106, not E0106 examples themselves.

eddyb · 2017-01-27T18:41:02Z

@bors r=nikomatsakis

bors · 2017-01-27T18:41:03Z

📌 Commit 8fbf336 has been approved by nikomatsakis

bors · 2017-01-28T00:44:29Z

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

…one.

…egion.

eddyb · 2017-01-28T00:57:26Z

@bors r=nikomatsakis

bors · 2017-01-28T00:57:26Z

📌 Commit c0e474d has been approved by nikomatsakis

Rollup of 21 pull requests - Successful merges: #38617, #39284, #39285, #39290, #39302, #39305, #39306, #39307, #39311, #39313, #39314, #39321, #39325, #39332, #39335, #39344, #39345, #39346, #39348, #39350, #39351 - Failed merges:

bors · 2017-01-28T06:21:28Z

⌛ Testing commit c0e474d with merge 0f49616...

@nikomatsakis

Perform lifetime elision (more) syntactically, before type-checking. The *initial* goal of this patch was to remove the (contextual) `&RegionScope` argument passed around `rustc_typeck::astconv` and allow converting arbitrary (syntactic) `hir::Ty` to (semantic) `Ty`. I've tried to closely match the existing behavior while moving the logic to the earlier `resolve_lifetime` pass, and [the crater report](https://gist.github.com/eddyb/4ac5b8516f87c1bfa2de528ed2b7779a) suggests none of the changes broke real code, but I will try to list everything: There are few cases in lifetime elision that could trip users up due to "hidden knowledge": ```rust type StaticStr = &'static str; // hides 'static trait WithLifetime<'a> { type Output; // can hide 'a } // This worked because the type of the first argument contains // 'static, although StaticStr doesn't even have parameters. fn foo(x: StaticStr) -> &str { x } // This worked because the compiler resolved the argument type // to <T as WithLifetime<'a>>::Output which has the hidden 'a. fn bar<'a, T: WithLifetime<'a>>(_: T::Output) -> &str { "baz" } ``` In the two examples above, elision wasn't using lifetimes that were in the source, not even *needed* by paths in the source, but rather *happened* to be part of the semantic representation of the types. To me, this suggests they should have never worked through elision (and they don't with this PR). Next we have an actual rule with a strange result, that is, the return type here elides to `&'x str`: ```rust impl<'a, 'b> Trait for Foo<'a, 'b> { fn method<'x, 'y>(self: &'x Foo<'a, 'b>, _: Bar<'y>) -> &str { &self.name } } ``` All 3 of `'a`, `'b` and `'y` are being ignored, because the `&self` elision rule only cares that the first argument is "`self` by reference". Due implementation considerations (elision running before typeck), I've limited it in this PR to a reference to a primitive/`struct`/`enum`/`union`, but not other types, but I am doing another crater run to assess the impact of limiting it to literally `&self` and `self: &Self` (they're identical in HIR). It's probably ideal to keep an "implicit `Self` for `self`" type around and *only* apply the rule to `&self` itself, but that would result in more bikeshed, and #21400 suggests some people expect otherwise. Another decent option is treating `self: X, ... -> Y` like `X -> Y` (one unique lifetime in `X` used for `Y`). The remaining changes have to do with "object lifetime defaults" (see RFCs [599](https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md) and [1156](https://github.com/rust-lang/rfcs/blob/master/text/1156-adjust-default-object-bounds.md)): ```rust trait Trait {} struct Ref2<'a, 'b, T: 'a+'b>(&'a T, &'b T); // These apply specifically within a (fn) body, // which allows type and lifetime inference: fn main() { // Used to be &'a mut (Trait+'a) - where 'a is one // inference variable - &'a mut (Trait+'b) in this PR. let _: &mut Trait; // Used to be an ambiguity error, but in this PR it's // Ref2<'a, 'b, Trait+'c> (3 inference variables). let _: Ref2<Trait>; } ``` What's happening here is that inference variables are created on the fly by typeck whenever a lifetime has no resolution attached to it - while it would be possible to alter the implementation to reuse inference variables based on decisions made early by `resolve_lifetime`, not doing that is more flexible and works better - it can compile all testcases from #38624 by not ending up with `&'static mut (Trait+'static)`. The ambiguity specifically cannot be an early error, because this is only the "default" (typeck can still pick something better based on the definition of `Trait` and whether it has any lifetime bounds), and having an error at all doesn't help anyone, as we can perfectly infer an appropriate lifetime inside the `fn` body. **TODO**: write tests for the user-visible changes. cc @nikomatsakis @arielb1

bors · 2017-01-28T08:48:49Z

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

rust-highfive assigned nrc Jan 25, 2017

nrc assigned nikomatsakis and unassigned nrc Jan 25, 2017

eddyb force-pushed the synelide branch from e39f9c6 to e8c0ec3 Compare January 26, 2017 18:39

nikomatsakis requested changes Jan 26, 2017

View reviewed changes

eddyb force-pushed the synelide branch from e8c0ec3 to 0495c6c Compare January 27, 2017 15:29

alexcrichton mentioned this pull request Jan 27, 2017

Rollup of 21 pull requests #39353

Merged

eddyb added 2 commits January 28, 2017 02:55

rustc_typeck: force users of RegionScope to get anon_region's one by …

4f559f8

…one.

rustc_typeck: pass all lifetimes through AstConv::opt_ast_region_to_r…

f79feba

…egion.

eddyb added 10 commits January 28, 2017 02:56

rustc: always include elidable lifetimes in HIR types.

7a2a669

rustc: clean up the style of middle::resolve_lifetime.

0682a75

rustc: simplify scope-tracking in resolve_lifetime.

bbc3414

rustc: move most of lifetime elision to resolve_lifetimes.

ba1849d

rustc_typeck: move impl Trait checks out of RegionScope.

9783947

rustc: lower trait type paths as TyTraitObject.

41553d6

rustc: always keep an explicit lifetime in trait objects.

c5befdc

rustc: move object default lifetimes to resolve_lifetimes.

4eac052

rustc: remove unused bounds field from RegionParameterDef.

9a0af16

test: add missing lifetime in recently added test.

c0e474d

eddyb force-pushed the synelide branch from 8fbf336 to c0e474d Compare January 28, 2017 00:57

bors merged commit c0e474d into rust-lang:master Jan 28, 2017

eddyb deleted the synelide branch January 28, 2017 08:49

This was referenced Jan 28, 2017

Add warning for () to ! switch #39009

Merged

Refactoring TyBox -> TyAdt #39230

Merged

Stabilize static lifetime in statics #39265

Merged

eddyb mentioned this pull request Mar 8, 2017

HIR: Move lifetime elision to HIR lowering #28644

Closed

Mark-Simulacrum mentioned this pull request May 31, 2017

undocumented lifetime elision fix from 1.15 -> 1.16 ? #41105

Closed

arielb1 mentioned this pull request Feb 2, 2019

Associated types bug related to trait objects #30833

Closed

QuineDot mentioned this pull request Sep 26, 2023

Default trait object lifetimes section is quite inaccurate rust-lang/reference#1407

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform lifetime elision (more) syntactically, before type-checking. #39305

Perform lifetime elision (more) syntactically, before type-checking. #39305

eddyb commented Jan 25, 2017 •

edited

Loading

rust-highfive commented Jan 25, 2017

arielb1 commented Jan 25, 2017 •

edited

Loading

eddyb commented Jan 25, 2017 •

edited

Loading

eddyb commented Jan 25, 2017

alexcrichton commented Jan 25, 2017

eddyb commented Jan 26, 2017 •

edited

Loading

GuillaumeGomez commented Jan 26, 2017

bors commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

eddyb commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis left a comment

nikomatsakis Jan 26, 2017

eddyb Jan 27, 2017

nikomatsakis Jan 27, 2017

nikomatsakis Jan 26, 2017

bors commented Jan 27, 2017

eddyb commented Jan 27, 2017 •

edited

Loading

eddyb commented Jan 27, 2017

bors commented Jan 27, 2017

bors commented Jan 28, 2017

eddyb commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

Perform lifetime elision (more) syntactically, before type-checking. #39305

Perform lifetime elision (more) syntactically, before type-checking. #39305

Conversation

eddyb commented Jan 25, 2017 • edited Loading

rust-highfive commented Jan 25, 2017

arielb1 commented Jan 25, 2017 • edited Loading

eddyb commented Jan 25, 2017 • edited Loading

eddyb commented Jan 25, 2017

alexcrichton commented Jan 25, 2017

eddyb commented Jan 26, 2017 • edited Loading

GuillaumeGomez commented Jan 26, 2017

bors commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

eddyb commented Jan 26, 2017

nikomatsakis commented Jan 26, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 26, 2017

Choose a reason for hiding this comment

eddyb Jan 27, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 27, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 26, 2017

Choose a reason for hiding this comment

bors commented Jan 27, 2017

eddyb commented Jan 27, 2017 • edited Loading

eddyb commented Jan 27, 2017

bors commented Jan 27, 2017

bors commented Jan 28, 2017

eddyb commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

eddyb commented Jan 25, 2017 •

edited

Loading

arielb1 commented Jan 25, 2017 •

edited

Loading

eddyb commented Jan 25, 2017 •

edited

Loading

eddyb commented Jan 26, 2017 •

edited

Loading

eddyb commented Jan 27, 2017 •

edited

Loading