From a6f80fccb19a52305ff88d960b4bb4e74df3d8b2 Mon Sep 17 00:00:00 2001 From: bors Date: Sun, 9 Jan 2022 14:27:36 +0000 Subject: [PATCH] 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 | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_restriction.rs | 1 + clippy_lints/src/lib.rs | 2 + .../src/single_char_lifetime_names.rs | 63 +++++++++++++++++++ tests/ui/single_char_lifetime_names.rs | 43 +++++++++++++ tests/ui/single_char_lifetime_names.stderr | 43 +++++++++++++ 7 files changed, 154 insertions(+) create mode 100644 clippy_lints/src/single_char_lifetime_names.rs create mode 100644 tests/ui/single_char_lifetime_names.rs create mode 100644 tests/ui/single_char_lifetime_names.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 27bac4718b6c..83fd8396bc72 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 002122793f3b..f9241d943b2c 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index eab389a9bd89..e7e2798da7da 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/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), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4687a1e2879..c31acb5f4ef8 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/single_char_lifetime_names.rs b/clippy_lints/src/single_char_lifetime_names.rs new file mode 100644 index 000000000000..ee82666b5aff --- /dev/null +++ b/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", + ); + } + } + } +} diff --git a/tests/ui/single_char_lifetime_names.rs b/tests/ui/single_char_lifetime_names.rs new file mode 100644 index 000000000000..261d8bc7260c --- /dev/null +++ b/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); +} diff --git a/tests/ui/single_char_lifetime_names.stderr b/tests/ui/single_char_lifetime_names.stderr new file mode 100644 index 000000000000..013b64f46a8c --- /dev/null +++ b/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 +