Auto merge of #13421 - lukaslueg:issue11212, r=Manishearth

Initial impl of `unnecessary_first_then_check` Fixes #11212 Checks for `{slice/vec/Box<[]>}.first().is_some()` and suggests replacing the unnecessary `Option`-construct with a direct `{slice/...}.is_empty()`. Other lints guide constructs like `if let Some(_) = v.get(0)` into this, which end up as `!v.is_empty()`. changelog: [`unnecessary_first_then_check`]: Initial implementation
rust-lang · Sep 19, 2024 · 9be28b1 · 9be28b1
2 parents 2e5b680 + 290a01e
commit 9be28b1
Show file tree

Hide file tree

Showing 7 changed files with 182 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6021,6 +6021,7 @@ Released 2018-09-13
 [`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
 [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map
+[`unnecessary_first_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_first_then_check
 [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold
 [`unnecessary_get_then_check`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_get_then_check
 [`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join

diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -467,6 +467,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::methods::UNNECESSARY_FALLIBLE_CONVERSIONS_INFO,
     crate::methods::UNNECESSARY_FILTER_MAP_INFO,
     crate::methods::UNNECESSARY_FIND_MAP_INFO,
+    crate::methods::UNNECESSARY_FIRST_THEN_CHECK_INFO,
     crate::methods::UNNECESSARY_FOLD_INFO,
     crate::methods::UNNECESSARY_GET_THEN_CHECK_INFO,
     crate::methods::UNNECESSARY_JOIN_INFO,

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -111,6 +111,7 @@ mod uninit_assumed_init;
 mod unit_hash;
 mod unnecessary_fallible_conversions;
 mod unnecessary_filter_map;
+mod unnecessary_first_then_check;
 mod unnecessary_fold;
 mod unnecessary_get_then_check;
 mod unnecessary_iter_cloned;
@@ -4137,6 +4138,34 @@ declare_clippy_lint! {
     "use of `map` returning the original item"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks the usage of `.first().is_some()` or `.first().is_none()` to check if a slice is
+    /// empty.
+    ///
+    /// ### Why is this bad?
+    /// Using `.is_empty()` is shorter and better communicates the intention.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let v = vec![1, 2, 3];
+    /// if v.first().is_none() {
+    ///     // The vector is empty...
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let v = vec![1, 2, 3];
+    /// if v.is_empty() {
+    ///     // The vector is empty...
+    /// }
+    /// ```
+    #[clippy::version = "1.83.0"]
+    pub UNNECESSARY_FIRST_THEN_CHECK,
+    complexity,
+    "calling `.first().is_some()` or `.first().is_none()` instead of `.is_empty()`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4294,6 +4323,7 @@ impl_lint_pass!(Methods => [
     UNNECESSARY_RESULT_MAP_OR_ELSE,
     MANUAL_C_STR_LITERALS,
     UNNECESSARY_GET_THEN_CHECK,
+    UNNECESSARY_FIRST_THEN_CHECK,
     NEEDLESS_CHARACTER_ITERATION,
     MANUAL_INSPECT,
     UNNECESSARY_MIN_OR_MAX,
@@ -5066,6 +5096,9 @@ fn check_is_some_is_none(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>,
         Some(("get", f_recv, [arg], _, _)) => {
             unnecessary_get_then_check::check(cx, call_span, recv, f_recv, arg, is_some);
         },
+        Some(("first", f_recv, [], _, _)) => {
+            unnecessary_first_then_check::check(cx, call_span, recv, f_recv, is_some);
+        },
         _ => {},
     }
 }

diff --git a/clippy_lints/src/methods/unnecessary_first_then_check.rs b/clippy_lints/src/methods/unnecessary_first_then_check.rs
@@ -0,0 +1,56 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::SpanRangeExt;
+
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_span::Span;
+
+use super::UNNECESSARY_FIRST_THEN_CHECK;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    call_span: Span,
+    first_call: &Expr<'_>,
+    first_caller: &Expr<'_>,
+    is_some: bool,
+) {
+    if !cx
+        .typeck_results()
+        .expr_ty_adjusted(first_caller)
+        .peel_refs()
+        .is_slice()
+    {
+        return;
+    }
+
+    let ExprKind::MethodCall(_, _, _, first_call_span) = first_call.kind else {
+        return;
+    };
+
+    let both_calls_span = first_call_span.with_hi(call_span.hi());
+    if let Some(both_calls_snippet) = both_calls_span.get_source_text(cx)
+        && let Some(first_caller_snippet) = first_caller.span.get_source_text(cx)
+    {
+        let (sugg_span, suggestion) = if is_some {
+            (
+                first_caller.span.with_hi(call_span.hi()),
+                format!("!{first_caller_snippet}.is_empty()"),
+            )
+        } else {
+            (both_calls_span, "is_empty()".to_owned())
+        };
+        span_lint_and_sugg(
+            cx,
+            UNNECESSARY_FIRST_THEN_CHECK,
+            sugg_span,
+            format!(
+                "unnecessary use of `{both_calls_snippet}` to check if slice {}",
+                if is_some { "is not empty" } else { "is empty" }
+            ),
+            "replace this with",
+            suggestion,
+            Applicability::MaybeIncorrect,
+        );
+    }
+}
diff --git a/tests/ui/unnecessary_first_then_check.fixed b/tests/ui/unnecessary_first_then_check.fixed
@@ -0,0 +1,22 @@
+#![warn(clippy::unnecessary_first_then_check)]
+#![allow(clippy::useless_vec, clippy::const_is_empty)]
+
+fn main() {
+    let s = [1, 2, 3];
+    let _: bool = !s.is_empty();
+    let _: bool = s.is_empty();
+
+    let v = vec![1, 2, 3];
+    let _: bool = !v.is_empty();
+
+    let n = [[1, 2, 3], [4, 5, 6]];
+    let _: bool = !n[0].is_empty();
+    let _: bool = n[0].is_empty();
+
+    struct Foo {
+        bar: &'static [i32],
+    }
+    let f = [Foo { bar: &[] }];
+    let _: bool = !f[0].bar.is_empty();
+    let _: bool = f[0].bar.is_empty();
+}
diff --git a/tests/ui/unnecessary_first_then_check.rs b/tests/ui/unnecessary_first_then_check.rs
@@ -0,0 +1,22 @@
+#![warn(clippy::unnecessary_first_then_check)]
+#![allow(clippy::useless_vec, clippy::const_is_empty)]
+
+fn main() {
+    let s = [1, 2, 3];
+    let _: bool = s.first().is_some();
+    let _: bool = s.first().is_none();
+
+    let v = vec![1, 2, 3];
+    let _: bool = v.first().is_some();
+
+    let n = [[1, 2, 3], [4, 5, 6]];
+    let _: bool = n[0].first().is_some();
+    let _: bool = n[0].first().is_none();
+
+    struct Foo {
+        bar: &'static [i32],
+    }
+    let f = [Foo { bar: &[] }];
+    let _: bool = f[0].bar.first().is_some();
+    let _: bool = f[0].bar.first().is_none();
+}
diff --git a/tests/ui/unnecessary_first_then_check.stderr b/tests/ui/unnecessary_first_then_check.stderr
@@ -0,0 +1,47 @@
+error: unnecessary use of `first().is_some()` to check if slice is not empty
+  --> tests/ui/unnecessary_first_then_check.rs:6:19
+   |
+LL |     let _: bool = s.first().is_some();
+   |                   ^^^^^^^^^^^^^^^^^^^ help: replace this with: `!s.is_empty()`
+   |
+   = note: `-D clippy::unnecessary-first-then-check` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_first_then_check)]`
+
+error: unnecessary use of `first().is_none()` to check if slice is empty
+  --> tests/ui/unnecessary_first_then_check.rs:7:21
+   |
+LL |     let _: bool = s.first().is_none();
+   |                     ^^^^^^^^^^^^^^^^^ help: replace this with: `is_empty()`
+
+error: unnecessary use of `first().is_some()` to check if slice is not empty
+  --> tests/ui/unnecessary_first_then_check.rs:10:19
+   |
+LL |     let _: bool = v.first().is_some();
+   |                   ^^^^^^^^^^^^^^^^^^^ help: replace this with: `!v.is_empty()`
+
+error: unnecessary use of `first().is_some()` to check if slice is not empty
+  --> tests/ui/unnecessary_first_then_check.rs:13:19
+   |
+LL |     let _: bool = n[0].first().is_some();
+   |                   ^^^^^^^^^^^^^^^^^^^^^^ help: replace this with: `!n[0].is_empty()`
+
+error: unnecessary use of `first().is_none()` to check if slice is empty
+  --> tests/ui/unnecessary_first_then_check.rs:14:24
+   |
+LL |     let _: bool = n[0].first().is_none();
+   |                        ^^^^^^^^^^^^^^^^^ help: replace this with: `is_empty()`
+
+error: unnecessary use of `first().is_some()` to check if slice is not empty
+  --> tests/ui/unnecessary_first_then_check.rs:20:19
+   |
+LL |     let _: bool = f[0].bar.first().is_some();
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace this with: `!f[0].bar.is_empty()`
+
+error: unnecessary use of `first().is_none()` to check if slice is empty
+  --> tests/ui/unnecessary_first_then_check.rs:21:28
+   |
+LL |     let _: bool = f[0].bar.first().is_none();
+   |                            ^^^^^^^^^^^^^^^^^ help: replace this with: `is_empty()`
+
+error: aborting due to 7 previous errors
+