Auto merge of #8236 - PatchMixolydic:single_char_lifetime_names, r=ll…

…ogiq

new lint: `single_char_lifetime_names`

This pull request adds a lint against single character lifetime names, as they might not divulge enough information about the purpose of the lifetime. This can make code harder to understand. I placed this in `restriction` rather than `pedantic` (as suggested in #8233) since most of the Rust ecosystem already uses single character lifetime names (to my knowledge, at least) and since single character lifetime names aren't incorrect. I'd be happy to change this upon request, however. Fixes #8233.

- [x] Followed lint naming conventions
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `cargo dev update_lints`
- [x] Added lint documentation
- [x] Run `cargo dev fmt`

changelog: new lint: [`single_char_lifetime_names`]

CHANGELOG.md

@@ -3253,6 +3253,7 @@ Released 2018-09-13
 [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait
 [`similar_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#similar_names
 [`single_char_add_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
+[`single_char_lifetime_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_lifetime_names
 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
 [`single_component_path_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports
 [`single_element_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop

clippy_lints/src/lib.register_lints.rs

@@ -435,6 +435,7 @@ store.register_lints(&[
     shadow::SHADOW_REUSE,
     shadow::SHADOW_SAME,
     shadow::SHADOW_UNRELATED,
+    single_char_lifetime_names::SINGLE_CHAR_LIFETIME_NAMES,
     single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
     size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT,
     slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,

clippy_lints/src/lib.register_restriction.rs

@@ -54,6 +54,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(shadow::SHADOW_REUSE),
     LintId::of(shadow::SHADOW_SAME),
     LintId::of(shadow::SHADOW_UNRELATED),
+    LintId::of(single_char_lifetime_names::SINGLE_CHAR_LIFETIME_NAMES),
     LintId::of(strings::STRING_ADD),
     LintId::of(strings::STRING_SLICE),
     LintId::of(strings::STRING_TO_STRING),

clippy_lints/src/lib.rs

@@ -351,6 +351,7 @@ mod self_named_constructors;
 mod semicolon_if_nothing_returned;
 mod serde_api;
 mod shadow;
+mod single_char_lifetime_names;
 mod single_component_path_imports;
 mod size_of_in_element_count;
 mod slow_vector_initialization;
@@ -858,6 +859,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit));
     store.register_late_pass(|| Box::new(return_self_not_must_use::ReturnSelfNotMustUse));
     store.register_late_pass(|| Box::new(init_numbered_fields::NumberedFields));
+    store.register_early_pass(|| Box::new(single_char_lifetime_names::SingleCharLifetimeNames));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/single_char_lifetime_names.rs

@@ -0,0 +1,63 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use rustc_ast::ast::{GenericParam, GenericParamKind};
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for lifetimes with names which are one character
+    /// long.
+    ///
+    /// ### Why is this bad?
+    /// A single character is likely not enough to express the
+    /// purpose of a lifetime. Using a longer name can make code
+    /// easier to understand, especially for those who are new to
+    /// Rust.
+    ///
+    /// ### Known problems
+    /// Rust programmers and learning resources tend to use single
+    /// character lifetimes, so this lint is at odds with the
+    /// ecosystem at large. In addition, the lifetime's purpose may
+    /// be obvious or, rarely, expressible in one character.
+    ///
+    /// ### Example
+    /// ```rust
+    /// struct DiagnosticCtx<'a> {
+    ///     source: &'a str,
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// struct DiagnosticCtx<'src> {
+    ///     source: &'src str,
+    /// }
+    /// ```
+    #[clippy::version = "1.59.0"]
+    pub SINGLE_CHAR_LIFETIME_NAMES,
+    restriction,
+    "warns against single-character lifetime names"
+}
+
+declare_lint_pass!(SingleCharLifetimeNames => [SINGLE_CHAR_LIFETIME_NAMES]);
+
+impl EarlyLintPass for SingleCharLifetimeNames {
+    fn check_generic_param(&mut self, ctx: &EarlyContext<'_>, param: &GenericParam) {
+        if in_external_macro(ctx.sess, param.ident.span) {
+            return;
+        }
+
+        if let GenericParamKind::Lifetime = param.kind {
+            if !param.is_placeholder && param.ident.as_str().len() <= 2 {
+                span_lint_and_help(
+                    ctx,
+                    SINGLE_CHAR_LIFETIME_NAMES,
+                    param.ident.span,
+                    "single-character lifetime names are likely uninformative",
+                    None,
+                    "use a more informative name",
+                );
+            }
+        }
+    }
+}

tests/ui/single_char_lifetime_names.rs

@@ -0,0 +1,43 @@
+#![warn(clippy::single_char_lifetime_names)]
+
+// Lifetimes should only be linted when they're introduced
+struct DiagnosticCtx<'a, 'b>
+where
+    'a: 'b,
+{
+    _source: &'a str,
+    _unit: &'b (),
+}
+
+// Only the lifetimes on the `impl`'s generics should be linted
+impl<'a, 'b> DiagnosticCtx<'a, 'b> {
+    fn new(source: &'a str, unit: &'b ()) -> DiagnosticCtx<'a, 'b> {
+        Self {
+            _source: source,
+            _unit: unit,
+        }
+    }
+}
+
+// No lifetimes should be linted here
+impl<'src, 'unit> DiagnosticCtx<'src, 'unit> {
+    fn new_pass(source: &'src str, unit: &'unit ()) -> DiagnosticCtx<'src, 'unit> {
+        Self {
+            _source: source,
+            _unit: unit,
+        }
+    }
+}
+
+// Only 'a should be linted here
+fn split_once<'a>(base: &'a str, other: &'_ str) -> (&'a str, Option<&'a str>) {
+    base.split_once(other)
+        .map(|(left, right)| (left, Some(right)))
+        .unwrap_or((base, None))
+}
+
+fn main() {
+    let src = "loop {}";
+    let unit = ();
+    DiagnosticCtx::new(src, &unit);
+}

tests/ui/single_char_lifetime_names.stderr

@@ -0,0 +1,43 @@
+error: single-character lifetime names are likely uninformative
+  --> $DIR/single_char_lifetime_names.rs:4:22
+   |
+LL | struct DiagnosticCtx<'a, 'b>
+   |                      ^^
+   |
+   = note: `-D clippy::single-char-lifetime-names` implied by `-D warnings`
+   = help: use a more informative name
+
+error: single-character lifetime names are likely uninformative
+  --> $DIR/single_char_lifetime_names.rs:4:26
+   |
+LL | struct DiagnosticCtx<'a, 'b>
+   |                          ^^
+   |
+   = help: use a more informative name
+
+error: single-character lifetime names are likely uninformative
+  --> $DIR/single_char_lifetime_names.rs:13:6
+   |
+LL | impl<'a, 'b> DiagnosticCtx<'a, 'b> {
+   |      ^^
+   |
+   = help: use a more informative name
+
+error: single-character lifetime names are likely uninformative
+  --> $DIR/single_char_lifetime_names.rs:13:10
+   |
+LL | impl<'a, 'b> DiagnosticCtx<'a, 'b> {
+   |          ^^
+   |
+   = help: use a more informative name
+
+error: single-character lifetime names are likely uninformative
+  --> $DIR/single_char_lifetime_names.rs:33:15
+   |
+LL | fn split_once<'a>(base: &'a str, other: &'_ str) -> (&'a str, Option<&'a str>) {
+   |               ^^
+   |
+   = help: use a more informative name
+
+error: aborting due to 5 previous errors
+