structured suggestions for single-use lifetime lint on fns and methods

It would be nice to demonstrate the shining correctness here with more run-rustfix tests than this, but unfortunately, that doesn't work with multipart suggestions yet (rust-lang#53934). While we're here, reword the zero-use lifetime suggestion to "elide the unused lifetime" instead of "remove it". (It's classier.)
zackmdavis · Oct 28, 2018 · fd28753 · fd28753
1 parent 1982f18
commit fd28753
Show file tree

Hide file tree

Showing 14 changed files with 168 additions and 36 deletions.
diff --git a/src/librustc/middle/resolve_lifetime.rs b/src/librustc/middle/resolve_lifetime.rs
@@ -1443,23 +1443,101 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
     /// helper method to determine the span to remove when suggesting the
     /// deletion of a lifetime
     fn lifetime_deletion_span(&self, name: ast::Ident, generics: &hir::Generics) -> Option<Span> {
-        if generics.params.len() == 1 {
-            // if sole lifetime, remove the `<>` brackets
-            Some(generics.span)
-        } else {
-            generics.params.iter().enumerate().find_map(|(i, param)| {
-                if param.name.ident() == name {
-                    // We also want to delete a leading or trailing comma
-                    // as appropriate
-                    if i >= generics.params.len() - 1 {
-                        Some(generics.params[i - 1].span.shrink_to_hi().to(param.span))
-                    } else {
-                        Some(param.span.to(generics.params[i + 1].span.shrink_to_lo()))
+        generics.params.iter().enumerate().find_map(|(i, param)| {
+            if param.name.ident() == name {
+                let mut in_band = false;
+                if let hir::GenericParamKind::Lifetime { kind } = param.kind {
+                    if let hir::LifetimeParamKind::InBand = kind {
+                        in_band = true;
                     }
+                }
+                if in_band {
+                    Some(param.span)
                 } else {
-                    None
+                    if generics.params.len() == 1 {
+                        // if sole lifetime, remove the entire `<>` brackets
+                        Some(generics.span)
+                    } else {
+                        // if removing within `<>` brackets, we also want to
+                        // delete a leading or trailing comma as appropriate
+                        if i >= generics.params.len() - 1 {
+                            Some(generics.params[i - 1].span.shrink_to_hi().to(param.span))
+                        } else {
+                            Some(param.span.to(generics.params[i + 1].span.shrink_to_lo()))
+                        }
+                    }
                 }
-            })
+            } else {
+                None
+            }
+        })
+    }
+
+    // helper method to issue suggestions from `fn rah<'a>(&'a T)` to `fn rah(&T)`
+    fn suggest_eliding_single_use_lifetime(
+        &self, err: &mut DiagnosticBuilder<'_>, def_id: DefId, lifetime: &hir::Lifetime
+    ) {
+        // FIXME: future work: also suggest `impl Foo<'_>` for `impl<'a> Foo<'a>`
+        let name = lifetime.name.ident();
+        let mut remove_decl = None;
+        if let Some(parent_def_id) = self.tcx.parent(def_id) {
+            if let Some(generics) = self.tcx.hir.get_generics(parent_def_id) {
+                remove_decl = self.lifetime_deletion_span(name, generics);
+            }
+        }
+
+        let mut remove_use = None;
+        let mut find_arg_use_span = |inputs: &hir::HirVec<hir::Ty>| {
+            for input in inputs {
+                if let hir::TyKind::Rptr(lt, _) = input.node {
+                    if lt.name.ident() == name {
+                        // include the trailing whitespace between the ampersand and the type name
+                        let lt_through_ty_span = lifetime.span.to(input.span.shrink_to_hi());
+                        remove_use = Some(
+                            self.tcx.sess.source_map()
+                                .span_until_non_whitespace(lt_through_ty_span)
+                        );
+                        break;
+                    }
+                }
+            }
+        };
+        if let Node::Lifetime(hir_lifetime) = self.tcx.hir.get(lifetime.id) {
+            if let Some(parent) = self.tcx.hir.find(self.tcx.hir.get_parent(hir_lifetime.id)) {
+                match parent {
+                    Node::Item(item) => {
+                        if let hir::ItemKind::Fn(decl, _, _, _) = &item.node {
+                            find_arg_use_span(&decl.inputs);
+                        }
+                    },
+                    Node::ImplItem(impl_item) => {
+                        if let hir::ImplItemKind::Method(sig, _) = &impl_item.node {
+                            find_arg_use_span(&sig.decl.inputs);
+                        }
+                    }
+                    _ => {}
+                }
+            }
+        }
+
+        if let (Some(decl_span), Some(use_span)) = (remove_decl, remove_use) {
+            // if both declaration and use deletion spans start at the same
+            // place ("start at" because the latter includes trailing
+            // whitespace), then this is an in-band lifetime
+            if decl_span.shrink_to_lo() == use_span.shrink_to_lo() {
+                err.span_suggestion_with_applicability(
+                    use_span,
+                    "elide the single-use lifetime",
+                    String::new(),
+                    Applicability::MachineApplicable,
+                );
+            } else {
+                err.multipart_suggestion_with_applicability(
+                    "elide the single-use lifetime",
+                    vec![(decl_span, String::new()), (use_span, String::new())],
+                    Applicability::MachineApplicable,
+                );
+            }
         }
     }
 
@@ -1521,8 +1599,15 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
                             span,
                             &format!("lifetime parameter `{}` only used once", name),
                         );
-                        err.span_label(span, "this lifetime...");
-                        err.span_label(lifetime.span, "...is used only here");
+
+                        if span == lifetime.span {
+                            // spans are the same for in-band lifetime declarations
+                            err.span_label(span, "this lifetime is only used here");
+                        } else {
+                            err.span_label(span, "this lifetime...");
+                            err.span_label(lifetime.span, "...is used only here");
+                        }
+                        self.suggest_eliding_single_use_lifetime(&mut err, def_id, lifetime);
                         err.emit();
                     }
                 }
@@ -1555,7 +1640,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
                                 if let Some(span) = unused_lt_span {
                                     err.span_suggestion_with_applicability(
                                         span,
-                                        "remove it",
+                                        "elide the unused lifetime",
                                         String::new(),
                                         Applicability::MachineApplicable,
                                     );

diff --git a/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.fixed b/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.fixed
@@ -0,0 +1,28 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// run-rustfix
+
+#![feature(in_band_lifetimes)]
+#![deny(single_use_lifetimes)]
+#![allow(dead_code)]
+#![allow(unused_variables)]
+
+// Test that we DO warn when lifetime name is used only
+// once in a fn argument, even with in band lifetimes.
+
+fn a(x: &u32, y: &u32) {
+    //~^ ERROR `'a` only used once
+    //~| ERROR `'b` only used once
+    //~| HELP elide the single-use lifetime
+    //~| HELP elide the single-use lifetime
+}
+
+fn main() { }
diff --git a/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.rs b/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.rs
@@ -8,6 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+// run-rustfix
+
 #![feature(in_band_lifetimes)]
 #![deny(single_use_lifetimes)]
 #![allow(dead_code)]
@@ -19,6 +21,8 @@
 fn a(x: &'a u32, y: &'b u32) {
     //~^ ERROR `'a` only used once
     //~| ERROR `'b` only used once
+    //~| HELP elide the single-use lifetime
+    //~| HELP elide the single-use lifetime
 }
 
 fn main() { }
diff --git a/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.stderr b/src/test/ui/single-use-lifetime/one-use-in-fn-argument-in-band.stderr
@@ -1,26 +1,26 @@
 error: lifetime parameter `'a` only used once
-  --> $DIR/one-use-in-fn-argument-in-band.rs:19:10
+  --> $DIR/one-use-in-fn-argument-in-band.rs:21:10
    |
 LL | fn a(x: &'a u32, y: &'b u32) {
-   |          ^^
+   |          ^^-
    |          |
-   |          this lifetime...
-   |          ...is used only here
+   |          this lifetime is only used here
+   |          help: elide the single-use lifetime
    |
 note: lint level defined here
-  --> $DIR/one-use-in-fn-argument-in-band.rs:12:9
+  --> $DIR/one-use-in-fn-argument-in-band.rs:14:9
    |
 LL | #![deny(single_use_lifetimes)]
    |         ^^^^^^^^^^^^^^^^^^^^
 
 error: lifetime parameter `'b` only used once
-  --> $DIR/one-use-in-fn-argument-in-band.rs:19:22
+  --> $DIR/one-use-in-fn-argument-in-band.rs:21:22
    |
 LL | fn a(x: &'a u32, y: &'b u32) {
-   |                      ^^
+   |                      ^^-
    |                      |
-   |                      this lifetime...
-   |                      ...is used only here
+   |                      this lifetime is only used here
+   |                      help: elide the single-use lifetime
 
 error: aborting due to 2 previous errors
 
diff --git a/src/test/ui/single-use-lifetime/one-use-in-fn-argument.rs b/src/test/ui/single-use-lifetime/one-use-in-fn-argument.rs
@@ -16,6 +16,7 @@
 // once in a fn argument.
 
 fn a<'a>(x: &'a u32) { //~ ERROR `'a` only used once
+    //~^ HELP elide the single-use lifetime
 }
 
 fn main() { }
diff --git a/src/test/ui/single-use-lifetime/one-use-in-fn-argument.stderr b/src/test/ui/single-use-lifetime/one-use-in-fn-argument.stderr
@@ -11,6 +11,10 @@ note: lint level defined here
    |
 LL | #![deny(single_use_lifetimes)]
    |         ^^^^^^^^^^^^^^^^^^^^
+help: elide the single-use lifetime
+   |
+LL | fn a(x: &u32) { //~ ERROR `'a` only used once
+   |    --   --
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/single-use-lifetime/one-use-in-inherent-method-argument.rs b/src/test/ui/single-use-lifetime/one-use-in-inherent-method-argument.rs
@@ -20,6 +20,7 @@ struct Foo<'f> {
 
 impl<'f> Foo<'f> { //~ ERROR `'f` only used once
     fn inherent_a<'a>(&self, data: &'a u32) { //~ ERROR `'a` only used once
+        //~^ HELP elide the single-use lifetime
     }
 }
 

diff --git a/src/test/ui/single-use-lifetime/one-use-in-inherent-method-argument.stderr b/src/test/ui/single-use-lifetime/one-use-in-inherent-method-argument.stderr
@@ -11,6 +11,10 @@ note: lint level defined here
    |
 LL | #![deny(single_use_lifetimes)]
    |         ^^^^^^^^^^^^^^^^^^^^
+help: elide the single-use lifetime
+   |
+LL |     fn inherent_a(&self, data: &u32) { //~ ERROR `'a` only used once
+   |                 --             --
 
 error: lifetime parameter `'f` only used once
   --> $DIR/one-use-in-inherent-method-argument.rs:21:6

diff --git a/src/test/ui/single-use-lifetime/one-use-in-trait-method-argument.rs b/src/test/ui/single-use-lifetime/one-use-in-trait-method-argument.rs
@@ -23,6 +23,7 @@ impl<'f> Iterator for Foo<'f> {
     type Item = &'f u32;
 
     fn next<'g>(&'g mut self) -> Option<Self::Item> { //~ ERROR `'g` only used once
+        //~^ HELP elide the single-use lifetime
         None
     }
 }

diff --git a/src/test/ui/single-use-lifetime/one-use-in-trait-method-argument.stderr b/src/test/ui/single-use-lifetime/one-use-in-trait-method-argument.stderr
@@ -11,6 +11,10 @@ note: lint level defined here
    |
 LL | #![deny(single_use_lifetimes)]
    |         ^^^^^^^^^^^^^^^^^^^^
+help: elide the single-use lifetime
+   |
+LL |     fn next(&mut self) -> Option<Self::Item> { //~ ERROR `'g` only used once
+   |           ----
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/single-use-lifetime/zero-uses-in-fn.fixed b/src/test/ui/single-use-lifetime/zero-uses-in-fn.fixed
@@ -7,17 +7,17 @@
 
 fn september() {}
 //~^ ERROR lifetime parameter `'a` never used
-//~| HELP remove it
+//~| HELP elide the unused lifetime
 
 fn october<'b, T>(s: &'b T) -> &'b T {
     //~^ ERROR lifetime parameter `'a` never used
-    //~| HELP remove it
+    //~| HELP elide the unused lifetime
     s
 }
 
 fn november<'a>(s: &'a str) -> (&'a str) {
     //~^ ERROR lifetime parameter `'b` never used
-    //~| HELP remove it
+    //~| HELP elide the unused lifetime
     s
 }
 

diff --git a/src/test/ui/single-use-lifetime/zero-uses-in-fn.rs b/src/test/ui/single-use-lifetime/zero-uses-in-fn.rs
@@ -7,17 +7,17 @@
 
 fn september<'a>() {}
 //~^ ERROR lifetime parameter `'a` never used
-//~| HELP remove it
+//~| HELP elide the unused lifetime
 
 fn october<'a, 'b, T>(s: &'b T) -> &'b T {
     //~^ ERROR lifetime parameter `'a` never used
-    //~| HELP remove it
+    //~| HELP elide the unused lifetime
     s
 }
 
 fn november<'a, 'b>(s: &'a str) -> (&'a str) {
     //~^ ERROR lifetime parameter `'b` never used
-    //~| HELP remove it
+    //~| HELP elide the unused lifetime
     s
 }
 

diff --git a/src/test/ui/single-use-lifetime/zero-uses-in-fn.stderr b/src/test/ui/single-use-lifetime/zero-uses-in-fn.stderr
@@ -2,7 +2,7 @@ error: lifetime parameter `'a` never used
   --> $DIR/zero-uses-in-fn.rs:8:14
    |
 LL | fn september<'a>() {}
-   |             -^^- help: remove it
+   |             -^^- help: elide the unused lifetime
    |
 note: lint level defined here
   --> $DIR/zero-uses-in-fn.rs:5:9
@@ -16,15 +16,15 @@ error: lifetime parameter `'a` never used
 LL | fn october<'a, 'b, T>(s: &'b T) -> &'b T {
    |            ^^--
    |            |
-   |            help: remove it
+   |            help: elide the unused lifetime
 
 error: lifetime parameter `'b` never used
   --> $DIR/zero-uses-in-fn.rs:18:17
    |
 LL | fn november<'a, 'b>(s: &'a str) -> (&'a str) {
    |               --^^
    |               |
-   |               help: remove it
+   |               help: elide the unused lifetime
 
 error: aborting due to 3 previous errors
 
diff --git a/src/test/ui/single-use-lifetime/zero-uses-in-impl.stderr b/src/test/ui/single-use-lifetime/zero-uses-in-impl.stderr
@@ -2,7 +2,7 @@ error: lifetime parameter `'a` never used
   --> $DIR/zero-uses-in-impl.rs:8:6
    |
 LL | impl<'a> Foo {} //~ ERROR `'a` never used
-   |     -^^- help: remove it
+   |     -^^- help: elide the unused lifetime
    |
 note: lint level defined here
   --> $DIR/zero-uses-in-impl.rs:3:9
-Original file line number
+Diff line change
@@ Expand Up / @@ -20,6 +20,7 @@ struct Foo<'f> { @@
     impl<'f> Foo<'f> { //~ ERROR `'f` only used once
         fn inherent_a<'a>(&self, data: &'a u32) { //~ ERROR `'a` only used once
+            //~^ HELP elide the single-use lifetime
         }
     }
@@ Expand Down @@