Suggest replacing named lifetime with `'static` by estebank · Pull Request #51513 · rust-lang/rust

estebank · 2018-06-12T05:01:06Z

warning: unnecessary lifetime parameter `'a`
  --> file.rs:11:14
   |
11 | fn foo<'c, 'a: 'static, 'b, 'd>(_x: &'a str) -> &'a str {
   |            ^^^^^^^^^^^
help: you can use the `'static` lifetime directly, in place of `'a`
   |
11 | fn foo<'c, 'b, 'd>(_x: &'static str) -> &'static str {
   |           --            ^^^^^^^          ^^^^^^^

warning: unnecessary lifetime parameter `'a`
  --> file.rs:14:9
   |
14 | fn bar<'a: 'static>(_x: &'a str) -> &'a str {
   |        ^^^^^^^^^^^
help: you can use the `'static` lifetime directly, in place of `'a`
   |
14 | fn bar(_x: &'static str) -> &'static str {
   |      --     ^^^^^^^          ^^^^^^^

cc #36179

rust-highfive · 2018-06-12T05:01:16Z

r? @michaelwoerister

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

``` warning: unnecessary lifetime parameter `'a` --> file.rs:11:14 | 11 | fn foo<'c, 'a: 'static, 'b, 'd>(_x: &'a str) -> &'a str { | ^^^^^^^^^^^ help: you can use the `'static` lifetime directly, in place of `'a` | 11 | fn foo<'c, 'b, 'd>(_x: &'static str) -> &'static str { | -- ^^^^^^^ ^^^^^^^ warning: unnecessary lifetime parameter `'a` --> file.rs:14:9 | 14 | fn bar<'a: 'static>(_x: &'a str) -> &'a str { | ^^^^^^^^^^^ help: you can use the `'static` lifetime directly, in place of `'a` | 14 | fn bar(_x: &'static str) -> &'static str { | -- ^^^^^^^ ^^^^^^^ ```

estebank · 2018-06-12T05:21:45Z

r? @nikomatsakis

estebank · 2018-06-14T21:14:07Z

emilyalbini · 2018-06-18T09:42:51Z

Ping from triage @nikomatsakis! This PR needs your review.

estebank · 2018-06-20T20:04:14Z

Ping.

nikomatsakis · 2018-06-20T20:13:26Z

@estebank I'm not entirely sure how this is related to #36179 -- it seems like a distinct problem. Do we think that it's common to write 'a: 'static? I do it sometimes as a kind of type-system hack, but I'm not sure if it is something that arises "in the wild".

nikomatsakis · 2018-06-20T20:13:43Z

Overall, this seems like a bit more code than I would have expected; it feels like there should be a lighterweight way to detect this condition.

nikomatsakis · 2018-06-20T20:14:07Z

Well, I guess that making the edit is part of the hard part...

estebank · 2018-06-20T20:18:06Z

This code can probably be cleaned quite a bit, but wanted to get some feedback on wether this is worthwhile to begin with.

This doesn't fix #36179, but by providing a suggestion for lifetimes the risk of someone writing '&str is much lower. I used this case in particular to start experimenting on how suggestions for changing multiple lifetimes would look like. If we're ok with the output (and the increased amount of code to be able to get and operate over the lifetime spans), this would only be the first of many other suggestions being reworked from pure text to structured suggestion.

nikomatsakis · 2018-06-20T19:39:35Z

src/librustc/hir/mod.rs

 }

+impl Ty {
+    pub fn lifetimes(&self) -> Vec<Lifetime> {


Can't we use a visitor for this?

Also, what is this used for? It seems like it doesn't handle shadowing (which, I guess, we disallow at present, so maybe that's ok).

(I was thinking of types like for<'a> fn(&'a u32), which mention 'a, but not the same 'a as might be in scope outside... but as I said at present we do disallow such shadowing)

It's only used to gather all the lifetime spans in the argument and return tys that match the name being checked.

nikomatsakis · 2018-06-21T18:35:44Z

I see. I think the output looks good. I'm not sure if there is a more light-weight way to get it.

nikomatsakis · 2018-06-21T18:35:56Z

I made one suggestion that would help to remove a lot of the boilerplate (using a visitor)

estebank · 2018-06-21T23:03:28Z

CC #44506 (same mechanism can be used to suggest the proposed output).

bors · 2018-06-21T23:10:35Z

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

nikomatsakis

OK, @estebank -- now that I understand better what you were going for, I think this looks pretty nice. It seems like we should (A) rewrite to use a visitor, (B) make the vector construction lazy, and (C) add a helper fn, but otherwise this seems like a nice POC. (We could/should fix shadowing too but that is less urgent since it is currently illegal anyway)

nikomatsakis · 2018-06-25T21:40:12Z

src/librustc/middle/resolve_lifetime.rs


        let next_early_index = index + generics.ty_params().count() as u32;
+        let mut arg_lifetimes = vec![];
+        for input in &decl.inputs {


This vector is only used in the case of lint/error, right? I'm not sure how I feel about building it eagerly -- maybe we can convert this into a closure or a trait or something?

Makes sense, I'll see what I can do about it.

nikomatsakis · 2018-06-25T21:41:27Z

src/librustc/middle/resolve_lifetime.rs

-                        self.resolve_lifetime_ref(bound);
+                            );
+
+                            // all the use spans for this lifetime in arguments to be replaced


we should extract this into a helper fn, but the basic idea seems pretty good 💯

nikomatsakis · 2018-06-25T21:42:55Z

As an example where shadowing could be relevant though:

fn foo<'a: 'static>(x: for<'a> fn(&'a u32)) { }

estebank · 2018-06-26T19:30:10Z

The output for

fn foo<'a: 'static>(x: for<'a> fn(&'a u32)) { }

is

warning: unnecessary lifetime parameter `'a`
 --> file.rs:1:8
  |
1 | fn foo<'a: 'static>(x: for<'a> fn(&'a u32)) { }
  |        ^^^^^^^^^^^
help: you can use the `'static` lifetime directly, in place of `'a`
  |
1 | fn foo(x: for<'a> fn(&'static u32)) { }
  |      --               ^^^^^^^

error[E0496]: lifetime name `'a` shadows a lifetime name that is already in scope
 --> file.rs:1:28
  |
1 | fn foo<'a: 'static>(x: for<'a> fn(&'a u32)) { }
  |        --                  ^^ lifetime 'a already in scope
  |        |
  |        first declared here

nikomatsakis · 2018-06-26T21:55:07Z

@estebank

The output for...

hmm, that seems suboptimal, do you agree? I think it'd be pretty easy to fix -- in the lifetimes() function that gathers up the lifetimes that appear in a type, when we encounter types with for<'a> binders, we should remove the things that appear in the binders from the result. (i.e., we only want the "free" lifetimes of the type)

estebank · 2018-06-28T00:51:23Z

Agree with your comments. I tried rebasing to see how hard it would be to include the feedback and the codebase diverged significantly. I'll get back to this later.

nikomatsakis · 2018-06-28T15:54:07Z

@estebank should we close the PR in the mean time?

estebank · 2018-06-28T18:24:46Z

Closing until I get back to this.

rust-highfive assigned michaelwoerister Jun 12, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2018

estebank force-pushed the use-static branch from d679331 to 46704fd Compare June 12, 2018 05:21

estebank changed the title ~~Suggest replacing named lifetime with 'static [wip]~~ Suggest replacing named lifetime with 'static Jun 12, 2018

rust-highfive assigned nikomatsakis and unassigned michaelwoerister Jun 12, 2018

This comment has been minimized.

Sign in to view

Extend lifetime substitution to all possible cases

2e3e383

nikomatsakis reviewed Jun 21, 2018

View reviewed changes

estebank 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 21, 2018

nikomatsakis requested changes Jun 25, 2018

View reviewed changes

estebank closed this Jun 28, 2018

Uh oh!

Conversation

estebank commented Jun 12, 2018

Uh oh!

rust-highfive commented Jun 12, 2018

Uh oh!

estebank commented Jun 12, 2018

Uh oh!

This comment has been minimized.

estebank commented Jun 14, 2018

Uh oh!

emilyalbini commented Jun 18, 2018

Uh oh!

estebank commented Jun 20, 2018

Uh oh!

nikomatsakis commented Jun 20, 2018

Uh oh!

nikomatsakis commented Jun 20, 2018

Uh oh!

nikomatsakis commented Jun 20, 2018

Uh oh!

estebank commented Jun 20, 2018

Uh oh!

nikomatsakis Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 21, 2018

Uh oh!

nikomatsakis commented Jun 21, 2018

Uh oh!

estebank commented Jun 21, 2018

Uh oh!

bors commented Jun 21, 2018

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

estebank Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jun 25, 2018

Uh oh!

estebank commented Jun 26, 2018

Uh oh!

nikomatsakis commented Jun 26, 2018

Uh oh!

estebank commented Jun 28, 2018

Uh oh!

nikomatsakis commented Jun 28, 2018

Uh oh!

estebank commented Jun 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants