Support generic lifetime arguments in method calls #42492

petrochenkov · 2017-06-06T21:59:12Z

Fixes #42403
Fixes #42115
Lifetimes in a method call x.f::<'a, 'b, T, U>() are treated exactly like lifetimes in the equivalent UFCS call X::f::<'a, 'b, T, U>.
In addition, if the method has late bound lifetime parameters (explicit or implicit), then explicitly specifying lifetime arguments is not permitted (guarded by a compatibility lint).
[breaking-change] because previously lifetimes in method calls were accepted unconditionally.

r? @eddyb

eddyb · 2017-06-06T22:28:41Z

cc @nikomatsakis Whom I've discussed this with previously.

So I'm not sure I still have them somewhere, but I've done a crater run on a stricter version of this (just banning lifetimes) and found a few problems, mostly related to the fact that late-bound lifetimes do not appear in ty::Generics. I'm not even sure it's a good idea to expose the distinction.

There were some ideas to prevent explicit lifetimes when any ended up being late-bound, or outright support them (after #42417 we could start playing with that idea).

nikomatsakis · 2017-06-09T19:32:13Z

So I am pretty nervous about exposing only the early-bound lifetimes. This ties in to that other issue, where we do allow such regions sometimes. I really would prefer it we only allow users to manually specify regions when they are all early-bound, so we can do it realiably and in a clearly forwards compatible way.

petrochenkov · 2017-06-10T10:28:16Z

only allow users to manually specify regions when they are all early-bound, so we can do it realiably and in a clearly forwards compatible way

I'll implement this then for both method calls and UFCS as a deny-by-default lint for cratering.

nikomatsakis · 2017-06-15T21:13:13Z

Discussed in the @rust-lang/lang meeting -- we agree that restricting to 100% early-bound is the right thing here.

alexcrichton · 2017-06-22T14:41:43Z

ping @petrochenkov, just wanted to keep this on your radar!

bors · 2017-06-23T16:20:56Z

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

petrochenkov · 2017-06-23T23:03:06Z

I've almost submitted an update implementing #42492 (comment), but then realized that late bound lifetime parameters can be defined implicitly through lifetime elision (e.g. fn f(_: &u8) {}).
@eddyb Is there an easy way to detect presence of such implicit parameters during type checking? I haven't investigated yet.

eddyb · 2017-06-24T05:35:25Z

You can detect them but I wouldn't consider them parameters for the purposes of paths, as I wouldn't an impl Trait somewhere in an argument type.

petrochenkov · 2017-06-24T11:55:23Z

@eddyb

I wouldn't consider them parameters for the purposes of paths

It's just strange that these two functions are identical

fn late_explicit<'early, 'late>(_: &'late u8) -> &'early u8 { loop {} }
fn late_implicit<'early       >(_: &      u8) -> &'early u8 { loop {} }

, but late_implicit::<'static> would be legal and late_explicit::<'static> would be not. Writing out the late bound lifetime explicitly would be a breaking change then.

(I suppose turning impl Trait into an explicit parameter shouldn't be a breaking change as well, because it's going to happen all the time.)

eddyb · 2017-06-24T12:11:44Z

IMO there is no other real choice if we want the explicitly written parameters to mean anything.

petrochenkov · 2017-06-24T12:50:03Z

if we want the explicitly written parameters to mean anything.

I'm not sure what do you mean.

(I'm still going to check for implicit parameters, at least to look at what crater will say.)

eddyb · 2017-06-24T13:05:02Z

@petrochenkov I meant that I don't think we can reasonably let you write anything other than what is present between <...> syntactically in the definition of the item, in a path to that item.

But I suppose I'm okay with going the even more conservative route of banning explicit parameters of a certain kind when there are implicit ones of that kind which could be later made explicit.

petrochenkov · 2017-06-24T23:49:48Z

I believe I've extracted all the necessary logic to catch late bound parameters from resolve_lifetime.rs.
This should be ready for crater now.

(There's also an interesting case with lifetimes in struct/variant constructors which are currently treated as early bound (#30904), but I haven't touched it, only added a test.)

eddyb · 2017-06-25T10:06:04Z

Started crater run.

eddyb · 2017-06-25T11:44:24Z

Crater run failed due to the inclusion of rustfmt in the build.

cc @rust-lang/dev-tools

eddyb · 2017-07-15T21:29:48Z

@petrochenkov Started another crater run.

eddyb · 2017-07-16T10:06:41Z

Third crater report shows 4 regressions.
Since the information was never used, I'm not sure why the authors kept the lifetimes (but really, it's our fault for parsing and ignoring them, some of them could be just "documentation").
There is at least one regression in unsafe code, which could potentially mean an invariant wasn't really enforced, but function signatures should prevent a real problem from spreading.

Mark-Simulacrum · 2017-07-17T00:27:06Z

I guess this is waiting on review? Or perhaps a decision from the lang team?

nikomatsakis · 2017-07-17T12:42:45Z

I think the relevant decisions have all been made -- we want to limit explicit lifetimes to cases that have only early-bound lifetimes for now (which are clearly backwards compatible). However, I'm not sure of the current back-and-forth between @eddyb and @petrochenkov on the state of the PR itself, it seems like they are iterating on crater runs.

As an aside, @brson has been complaining to me that people still use crater, and suggesting that we should try to do some cargobomb runs for better coverage. @brson-- who should be ping to try and get that going?

petrochenkov · 2017-07-17T12:55:42Z

@nikomatsakis

However, I'm not sure of the current back-and-forth between @eddyb and @petrochenkov on the state of the PR itself, it seems like they are iterating on crater runs.

The iteration is complete, all found regressions and mistakes are fixed.

nikomatsakis

I skimmed mostly, given that there has been so much back and forth, but this looks good to me. r=me once we change to warn-by-default (I expect this is what was intended, right?)

nikomatsakis · 2017-07-17T13:50:19Z

src/librustc/lint/builtin.rs

@@ -205,6 +205,12 @@ declare_lint! {
 }

 declare_lint! {
+    pub LATE_BOUND_LIFETIME_ARGUMENTS,
+    Deny,


Do we really want to default to Deny here?

… parameters

Fix treatment of lifetimes defined in nested types during detection of late bound regions in signatures. Do not replace substs with inference variables when "cannot specify lifetime arguments explicitly..." is reported as a lint.

petrochenkov · 2017-07-17T21:35:39Z

The lint level is changed to warn-by-default.
@bors r=nikomatsakis

bors · 2017-07-17T21:35:40Z

📌 Commit 39114f9 has been approved by nikomatsakis

bors · 2017-07-18T02:34:18Z

⌛ Testing commit 39114f9 with merge 7c758abdad01aaa219c2cb14e4ed6878fb69c236...

bors · 2017-07-18T03:08:11Z

💔 Test failed - status-travis

alexcrichton · 2017-07-18T03:11:38Z

@bors: retry

curl failure

bors · 2017-07-18T03:22:26Z

⌛ Testing commit 39114f9 with merge 29d60228266412b6bee73098d00ef185d781a8f3...

bors · 2017-07-18T03:30:12Z

💔 Test failed - status-travis

est31 · 2017-07-18T06:50:19Z

@bors retry

bors · 2017-07-18T10:20:21Z

⌛ Testing commit 39114f9 with merge 83c659e...

@eddyb

Support generic lifetime arguments in method calls Fixes #42403 Fixes #42115 Lifetimes in a method call `x.f::<'a, 'b, T, U>()` are treated exactly like lifetimes in the equivalent UFCS call `X::f::<'a, 'b, T, U>`. In addition, if the method has late bound lifetime parameters (explicit or implicit), then explicitly specifying lifetime arguments is not permitted (guarded by a compatibility lint). [breaking-change] because previously lifetimes in method calls were accepted unconditionally. r? @eddyb

bors · 2017-07-18T12:46:09Z

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

rust-highfive assigned eddyb Jun 6, 2017

eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 6, 2017

eddyb approved these changes Jun 6, 2017

View reviewed changes

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2017

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2017

nikomatsakis removed the I-nominated label Jun 15, 2017

nikomatsakis mentioned this pull request Jun 15, 2017

Lifetime parameters are silently deleted #42115

Closed

petrochenkov force-pushed the methlife branch from 517934c to db5df59 Compare June 24, 2017 12:11

petrochenkov force-pushed the methlife branch from db5df59 to c00ebdd Compare June 24, 2017 23:39

petrochenkov mentioned this pull request Jun 25, 2017

Tracking issue for future-incompatibility lint late_bound_lifetime_arguments #42868

Open

3 tasks

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 16, 2017

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2017

nikomatsakis requested changes Jul 17, 2017

View reviewed changes

petrochenkov added 5 commits July 18, 2017 00:12

Support generic lifetime arguments in method calls

9967e9e

Prohibit lifetime arguments in path segments with late bound lifetime…

7ca378b

… parameters

Detect implicitly defined late bound lifetime parameters as well

e40cedb

Fix incorrect subst index

46f427b

Fix treatment of lifetimes defined in nested types during detection of late bound regions in signatures. Do not replace substs with inference variables when "cannot specify lifetime arguments explicitly..." is reported as a lint.

Make late_bound_lifetime_arguments lint warn-by-default

39114f9

petrochenkov force-pushed the methlife branch from 5637f89 to 39114f9 Compare July 17, 2017 21:34

bors merged commit 39114f9 into rust-lang:master Jul 18, 2017

petrochenkov mentioned this pull request Aug 20, 2017

Make fields of Span private #43968

Merged

petrochenkov deleted the methlife branch August 26, 2017 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support generic lifetime arguments in method calls #42492

Support generic lifetime arguments in method calls #42492

petrochenkov commented Jun 6, 2017 •

edited

Loading

eddyb commented Jun 6, 2017

nikomatsakis commented Jun 9, 2017

petrochenkov commented Jun 10, 2017

nikomatsakis commented Jun 15, 2017

alexcrichton commented Jun 22, 2017

bors commented Jun 23, 2017

petrochenkov commented Jun 23, 2017

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017 •

edited

Loading

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017

eddyb commented Jun 25, 2017

eddyb commented Jun 25, 2017

eddyb commented Jul 15, 2017

eddyb commented Jul 16, 2017

Mark-Simulacrum commented Jul 17, 2017

nikomatsakis commented Jul 17, 2017

petrochenkov commented Jul 17, 2017

nikomatsakis left a comment

nikomatsakis Jul 17, 2017

petrochenkov commented Jul 17, 2017

bors commented Jul 17, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

est31 commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

Support generic lifetime arguments in method calls #42492

Support generic lifetime arguments in method calls #42492

Conversation

petrochenkov commented Jun 6, 2017 • edited Loading

eddyb commented Jun 6, 2017

nikomatsakis commented Jun 9, 2017

petrochenkov commented Jun 10, 2017

nikomatsakis commented Jun 15, 2017

alexcrichton commented Jun 22, 2017

bors commented Jun 23, 2017

petrochenkov commented Jun 23, 2017

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017 • edited Loading

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017

eddyb commented Jun 24, 2017

petrochenkov commented Jun 24, 2017

eddyb commented Jun 25, 2017

eddyb commented Jun 25, 2017

eddyb commented Jul 15, 2017

eddyb commented Jul 16, 2017

Mark-Simulacrum commented Jul 17, 2017

nikomatsakis commented Jul 17, 2017

petrochenkov commented Jul 17, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jul 17, 2017

Choose a reason for hiding this comment

petrochenkov commented Jul 17, 2017

bors commented Jul 17, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

alexcrichton commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

est31 commented Jul 18, 2017

bors commented Jul 18, 2017

bors commented Jul 18, 2017

petrochenkov commented Jun 6, 2017 •

edited

Loading

petrochenkov commented Jun 24, 2017 •

edited

Loading