Add warning for use of lifetime parameter with 'static bound #40734

adamransom · 2017-03-22T12:48:56Z

Previously a 'static lifetime bound would result in an undeclared lifetime error when compiling, even though it could be considered valid.

However, it is unnecessary to use it as a lifetime bound so we present the user with a warning instead and suggest using the 'static lifetime directly, in place of the lifetime parameter. We can change this to an error (or warning with lint) if that's decided to be more appropriate.

Example output:

warning: unnecessary lifetime parameter `'a`
 --> ../static-lifetime-bound.rs:3:10
  |
3 | fn f<'a: 'static>(val: &'a i32) {
  |      ^^^^^^^^^^^
  |
  = help: you can use the `'static` lifetime directly, in place `'a`

Fixes #40661

r? @jseyfried

Simply move the test for `keywords::StaticLifetime` into the `Lifetime` impl, to match how elision is checked.

rust-highfive · 2017-03-22T12:49:03Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jseyfried (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jseyfried

Looks good!

jseyfried · 2017-03-23T01:35:39Z

src/librustc/middle/resolve_lifetime.rs

            self.insert_lifetime(lifetime_ref, Region::Static);
            return;
        }
-        self.resolve_lifetime_ref(lifetime_ref);
+        self.resolve_lifetime_ref(lifetime_ref, lifetime_ref);


I would make the second parameter Option<&hir::Lifetime> and use None here to make it clear that the second argument won't get used.

Yes, I was thinking about this also. I'll add that.

jseyfried · 2017-03-23T01:36:43Z

src/librustc/middle/resolve_lifetime.rs

+            // The only way we end up here is if there is a static lifetime bound (due to early
+            // return on `visit_lifetime()`)
+            self.insert_lifetime(lifetime_ref, Region::Static);
+            let full_span = mk_sp(context_ref.span.lo, lifetime_ref.span.hi);


This would become context_ref.unwrap().span.lo, enforcing the invariant described in the above comment.

jseyfried · 2017-03-23T01:38:12Z

src/librustc/middle/resolve_lifetime.rs

+            self.sess.struct_span_warn(full_span,
+                &format!("unnecessary lifetime parameter `{}`", context_ref.name))
+                .help(&format!("you can use a `'static` lifetime directly, in place of \
+                                the lifetime parameter `{}`", context_ref.name))


I would reword: you can use the lifetime `'static` directly in place of `{}` .

Yeah, that's better. I was also wondering if it's worth adding a note? Something like:

note: due to use of `'static` bound on `'a`

or whether the help message alone is enough?

The help message is sufficient, imo.

jseyfried · 2017-03-23T01:45:30Z

Nominating for @rust-lang/lang decision: should we allow, warn, or forbid a 'static bound on a lifetime parameter, e.g. fn f<'a: 'static>(_: &'a ()) {}?

Today, this causes a confusing error.

nikomatsakis · 2017-03-24T18:52:06Z

@jseyfried conclusion from @rust-lang/lang meeting was that we should accept it, but warn (as I believe this PR does)

eddyb · 2017-03-24T19:01:49Z

src/librustc/middle/resolve_lifetime.rs

-    fn resolve_lifetime_ref(&mut self, lifetime_ref: &hir::Lifetime) {
+    fn resolve_lifetime_ref(&mut self,
+                            lifetime_ref: &hir::Lifetime,
+                            context_ref: Option<&hir::Lifetime>) {


The terminology here is wrong IMO. The intention of _ref, AFAICT, is to mean "lifetime reference", as in, one use of (referring to) a lifetime. So this extra argument should be maybe_def or some such, but even then, I would just bypass this whole function from check_lifetime_defs, for 'static, i.e. handle it there instead of here.

Yes, I agree that's a better place to handle it. Will make that change!

Previously a `'static` lifetime bound would result in an `undeclared lifetime` error when compiling, even though it could be considered valid. However, it is unnecessary to use it as a lifetime bound so we present the user with a warning instead and suggest using the `'static` lifetime directly, in place of the lifetime parameter.

adamransom · 2017-03-25T02:04:52Z

Updated with latest suggestions.

adamransom · 2017-03-25T02:05:49Z

src/librustc/middle/resolve_lifetime.rs

@@ -1464,7 +1464,17 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
            self.check_lifetime_def_for_shadowing(old_scope, &lifetime_i.lifetime);

            for bound in &lifetime_i.bounds {
-                self.resolve_lifetime_ref(bound);
+                if !bound.is_static() {


I chose to put the common path first, even though its a negated condition. I can change if this is not preferred style though.

Looks good!

jseyfried · 2017-03-25T04:14:53Z

@adamransom Thanks!
@bors r+

bors · 2017-03-25T04:14:54Z

📌 Commit e7949d0 has been approved by jseyfried

bors · 2017-03-25T12:16:16Z

⌛ Testing commit e7949d0 with merge e425597...

bors · 2017-03-25T12:22:08Z

💔 Test failed - status-travis

arielb1 · 2017-03-25T12:31:14Z

try to fix the build on emscripten #40821
@bors retry

bors · 2017-03-25T14:10:14Z

⌛ Testing commit e7949d0 with merge d73c258...

bors · 2017-03-25T14:16:08Z

💔 Test failed - status-travis

arielb1 · 2017-03-25T14:39:44Z

try to fix the build on emscripten #40821
@bors retry

@jseyfried

Add warning for use of lifetime parameter with 'static bound Previously a `'static` lifetime bound would result in an `undeclared lifetime` error when compiling, even though it could be considered valid. However, it is unnecessary to use it as a lifetime bound so we present the user with a warning instead and suggest using the `'static` lifetime directly, in place of the lifetime parameter. We can change this to an error (or warning with lint) if that's decided to be more appropriate. Example output: ``` warning: unnecessary lifetime parameter `'a` --> ../static-lifetime-bound.rs:3:10 | 3 | fn f<'a: 'static>(val: &'a i32) { | ^^^^^^^^^^^ | = help: you can use the `'static` lifetime directly, in place `'a` ``` Fixes rust-lang#40661 r? @jseyfried

@jseyfried

Add warning for use of lifetime parameter with 'static bound Previously a `'static` lifetime bound would result in an `undeclared lifetime` error when compiling, even though it could be considered valid. However, it is unnecessary to use it as a lifetime bound so we present the user with a warning instead and suggest using the `'static` lifetime directly, in place of the lifetime parameter. We can change this to an error (or warning with lint) if that's decided to be more appropriate. Example output: ``` warning: unnecessary lifetime parameter `'a` --> ../static-lifetime-bound.rs:3:10 | 3 | fn f<'a: 'static>(val: &'a i32) { | ^^^^^^^^^^^ | = help: you can use the `'static` lifetime directly, in place `'a` ``` Fixes rust-lang#40661 r? @jseyfried

arielb1 · 2017-03-25T19:37:05Z

try to fix the build on emscripten #40821
@bors retry

Rollup of 7 pull requests - Successful merges: #40642, #40734, #40740, #40771, #40807, #40820, #40821 - Failed merges:

Refactor checking if a Lifetime is static

1ae1a19

Simply move the test for `keywords::StaticLifetime` into the `Lifetime` impl, to match how elision is checked.

rust-highfive assigned jseyfried Mar 22, 2017

jseyfried reviewed Mar 23, 2017

View reviewed changes

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

adamransom force-pushed the fix/40661 branch from 4d04ed9 to 2d03dfb Compare March 23, 2017 10:19

nikomatsakis removed the I-nominated label Mar 24, 2017

eddyb reviewed Mar 24, 2017

View reviewed changes

adamransom force-pushed the fix/40661 branch 2 times, most recently from a9b9fa6 to e7949d0 Compare March 25, 2017 01:46

adamransom commented Mar 25, 2017

View reviewed changes

frewsxcv mentioned this pull request Mar 25, 2017

Rollup of 6 pull requests #40825

Closed

frewsxcv mentioned this pull request Mar 25, 2017

Rollup of 7 pull requests #40826

Merged

bors added a commit that referenced this pull request Mar 26, 2017

Auto merge of #40826 - frewsxcv:rollup, r=frewsxcv

7846dbe

Rollup of 7 pull requests - Successful merges: #40642, #40734, #40740, #40771, #40807, #40820, #40821 - Failed merges:

bors merged commit e7949d0 into rust-lang:master Mar 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for use of lifetime parameter with 'static bound #40734

Add warning for use of lifetime parameter with 'static bound #40734

adamransom commented Mar 22, 2017 •

edited

Loading

rust-highfive commented Mar 22, 2017

jseyfried left a comment

jseyfried Mar 23, 2017

adamransom Mar 23, 2017

jseyfried Mar 23, 2017

jseyfried Mar 23, 2017

adamransom Mar 23, 2017

jseyfried Mar 23, 2017

jseyfried commented Mar 23, 2017 •

edited

Loading

nikomatsakis commented Mar 24, 2017

eddyb Mar 24, 2017

adamransom Mar 25, 2017

adamransom commented Mar 25, 2017

adamransom Mar 25, 2017

eddyb Mar 25, 2017

jseyfried commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

arielb1 commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

arielb1 commented Mar 25, 2017

arielb1 commented Mar 25, 2017

Add warning for use of lifetime parameter with 'static bound #40734

Add warning for use of lifetime parameter with 'static bound #40734

Conversation

adamransom commented Mar 22, 2017 • edited Loading

rust-highfive commented Mar 22, 2017

jseyfried left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Mar 23, 2017 • edited Loading

nikomatsakis commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamransom commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

arielb1 commented Mar 25, 2017

bors commented Mar 25, 2017

bors commented Mar 25, 2017

arielb1 commented Mar 25, 2017

arielb1 commented Mar 25, 2017

adamransom commented Mar 22, 2017 •

edited

Loading

jseyfried commented Mar 23, 2017 •

edited

Loading