Auto merge of #12310 - samueltardieu:issue-12307, r=xFrednet

New lint `const_is_empty`

This lint detects calls to `.is_empty()` on an entity initialized from a string literal and flag them as suspicious. To avoid triggering on macros called from generated code, it checks that the `.is_empty()` receiver, the call itself and the initialization come from the same context.

Fixes #12307

changelog: [`const_is_empty`]: new lint

Author: bors

CHANGELOG.md

@@ -5110,6 +5110,7 @@ Released 2018-09-13
 [`collection_is_never_read`]: https://rust-lang.github.io/rust-clippy/master/index.html#collection_is_never_read
 [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
 [`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
+[`const_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty
 [`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime
 [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
 [`crate_in_macro_def`]: https://rust-lang.github.io/rust-clippy/master/index.html#crate_in_macro_def

clippy_lints/src/declared_lints.rs

@@ -353,6 +353,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::CLONE_ON_COPY_INFO,
     crate::methods::CLONE_ON_REF_PTR_INFO,
     crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
+    crate::methods::CONST_IS_EMPTY_INFO,
     crate::methods::DRAIN_COLLECT_INFO,
     crate::methods::ERR_EXPECT_INFO,
     crate::methods::EXPECT_FUN_CALL_INFO,

clippy_lints/src/methods/is_empty.rs

@@ -0,0 +1,49 @@
+use clippy_utils::consts::constant_is_empty;
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::{find_binding_init, path_to_local};
+use rustc_hir::{Expr, HirId};
+use rustc_lint::{LateContext, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_span::sym;
+
+use super::CONST_IS_EMPTY;
+
+/// Expression whose initialization depend on a constant conditioned by a `#[cfg(…)]` directive will
+/// not trigger the lint.
+pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, receiver: &Expr<'_>) {
+    if in_external_macro(cx.sess(), expr.span) || !receiver.span.eq_ctxt(expr.span) {
+        return;
+    }
+    let init_expr = expr_or_init(cx, receiver);
+    if !receiver.span.eq_ctxt(init_expr.span) {
+        return;
+    }
+    if let Some(init_is_empty) = constant_is_empty(cx, init_expr) {
+        span_lint(
+            cx,
+            CONST_IS_EMPTY,
+            expr.span,
+            &format!("this expression always evaluates to {init_is_empty:?}"),
+        );
+    }
+}
+
+fn is_under_cfg(cx: &LateContext<'_>, id: HirId) -> bool {
+    cx.tcx
+        .hir()
+        .parent_id_iter(id)
+        .any(|id| cx.tcx.hir().attrs(id).iter().any(|attr| attr.has_name(sym::cfg)))
+}
+
+/// Similar to [`clippy_utils::expr_or_init`], but does not go up the chain if the initialization
+/// value depends on a `#[cfg(…)]` directive.
+fn expr_or_init<'a, 'b, 'tcx: 'b>(cx: &LateContext<'tcx>, mut expr: &'a Expr<'b>) -> &'a Expr<'b> {
+    while let Some(init) = path_to_local(expr)
+        .and_then(|id| find_binding_init(cx, id))
+        .filter(|init| cx.typeck_results().expr_adjustments(init).is_empty())
+        .filter(|init| !is_under_cfg(cx, init.hir_id))
+    {
+        expr = init;
+    }
+    expr
+}

clippy_lints/src/methods/mod.rs

@@ -36,6 +36,7 @@ mod inefficient_to_string;
 mod inspect_for_each;
 mod into_iter_on_ref;
 mod is_digit_ascii_radix;
+mod is_empty;
 mod iter_cloned_collect;
 mod iter_count;
 mod iter_filter;
@@ -4044,6 +4045,31 @@ declare_clippy_lint! {
     "calling `.get().is_some()` or `.get().is_none()` instead of `.contains()` or `.contains_key()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// It identifies calls to `.is_empty()` on constant values.
+    ///
+    /// ### Why is this bad?
+    /// String literals and constant values are known at compile time. Checking if they
+    /// are empty will always return the same value. This might not be the intention of
+    /// the expression.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let value = "";
+    /// if value.is_empty() {
+    ///     println!("the string is empty");
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// println!("the string is empty");
+    /// ```
+    #[clippy::version = "1.78.0"]
+    pub CONST_IS_EMPTY,
+    suspicious,
+    "is_empty() called on strings known at compile time"
+}
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4092,6 +4118,7 @@ impl_lint_pass!(Methods => [
     CLONE_ON_COPY,
     CLONE_ON_REF_PTR,
     COLLAPSIBLE_STR_REPLACE,
+    CONST_IS_EMPTY,
     ITER_OVEREAGER_CLONED,
     CLONED_INSTEAD_OF_COPIED,
     FLAT_MAP_OPTION,
@@ -4445,7 +4472,7 @@ impl Methods {
                 ("as_deref" | "as_deref_mut", []) => {
                     needless_option_as_deref::check(cx, expr, recv, name);
                 },
-                ("as_bytes" | "is_empty", []) => {
+                ("as_bytes", []) => {
                     if let Some(("as_str", recv, [], as_str_span, _)) = method_call(recv) {
                         redundant_as_str::check(cx, expr, recv, as_str_span, span);
                     }
@@ -4619,6 +4646,12 @@ impl Methods {
                 ("hash", [arg]) => {
                     unit_hash::check(cx, expr, recv, arg);
                 },
+                ("is_empty", []) => {
+                    if let Some(("as_str", recv, [], as_str_span, _)) = method_call(recv) {
+                        redundant_as_str::check(cx, expr, recv, as_str_span, span);
+                    }
+                    is_empty::check(cx, expr, recv);
+                },
                 ("is_file", []) => filetype_is_file::check(cx, expr, recv),
                 ("is_digit", [radix]) => is_digit_ascii_radix::check(cx, expr, recv, radix, &self.msrv),
                 ("is_none", []) => check_is_some_is_none(cx, expr, recv, call_span, false),

clippy_utils/src/consts.rs

@@ -10,8 +10,10 @@ use rustc_hir::{BinOp, BinOpKind, Block, ConstBlock, Expr, ExprKind, HirId, Item
 use rustc_lexer::tokenize;
 use rustc_lint::LateContext;
 use rustc_middle::mir::interpret::{alloc_range, Scalar};
+use rustc_middle::mir::ConstValue;
 use rustc_middle::ty::{self, EarlyBinder, FloatTy, GenericArgsRef, IntTy, List, ScalarInt, Ty, TyCtxt, UintTy};
 use rustc_middle::{bug, mir, span_bug};
+use rustc_span::def_id::DefId;
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::SyntaxContext;
 use rustc_target::abi::Size;
@@ -307,6 +309,12 @@ impl ConstantSource {
     }
 }
 
+/// Attempts to check whether the expression is a constant representing an empty slice, str, array,
+/// etc…
+pub fn constant_is_empty(lcx: &LateContext<'_>, e: &Expr<'_>) -> Option<bool> {
+    ConstEvalLateContext::new(lcx, lcx.typeck_results()).expr_is_empty(e)
+}
+
 /// Attempts to evaluate the expression as a constant.
 pub fn constant<'tcx>(
     lcx: &LateContext<'tcx>,
@@ -406,7 +414,13 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
         match e.kind {
             ExprKind::ConstBlock(ConstBlock { body, .. }) => self.expr(self.lcx.tcx.hir().body(body).value),
             ExprKind::DropTemps(e) => self.expr(e),
-            ExprKind::Path(ref qpath) => self.fetch_path(qpath, e.hir_id, self.typeck_results.expr_ty(e)),
+            ExprKind::Path(ref qpath) => {
+                self.fetch_path_and_apply(qpath, e.hir_id, self.typeck_results.expr_ty(e), |this, result| {
+                    let result = mir_to_const(this.lcx, result)?;
+                    this.source = ConstantSource::Constant;
+                    Some(result)
+                })
+            },
             ExprKind::Block(block, _) => self.block(block),
             ExprKind::Lit(lit) => {
                 if is_direct_expn_of(e.span, "cfg").is_some() {
@@ -472,6 +486,49 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
         }
     }
 
+    /// Simple constant folding to determine if an expression is an empty slice, str, array, …
+    /// `None` will be returned if the constness cannot be determined, or if the resolution
+    /// leaves the local crate.
+    pub fn expr_is_empty(&mut self, e: &Expr<'_>) -> Option<bool> {
+        match e.kind {
+            ExprKind::ConstBlock(ConstBlock { body, .. }) => self.expr_is_empty(self.lcx.tcx.hir().body(body).value),
+            ExprKind::DropTemps(e) => self.expr_is_empty(e),
+            ExprKind::Path(ref qpath) => {
+                if !self
+                    .typeck_results
+                    .qpath_res(qpath, e.hir_id)
+                    .opt_def_id()
+                    .is_some_and(DefId::is_local)
+                {
+                    return None;
+                }
+                self.fetch_path_and_apply(qpath, e.hir_id, self.typeck_results.expr_ty(e), |this, result| {
+                    mir_is_empty(this.lcx, result)
+                })
+            },
+            ExprKind::Lit(lit) => {
+                if is_direct_expn_of(e.span, "cfg").is_some() {
+                    None
+                } else {
+                    match &lit.node {
+                        LitKind::Str(is, _) => Some(is.is_empty()),
+                        LitKind::ByteStr(s, _) | LitKind::CStr(s, _) => Some(s.is_empty()),
+                        _ => None,
+                    }
+                }
+            },
+            ExprKind::Array(vec) => self.multi(vec).map(|v| v.is_empty()),
+            ExprKind::Repeat(..) => {
+                if let ty::Array(_, n) = self.typeck_results.expr_ty(e).kind() {
+                    Some(n.try_eval_target_usize(self.lcx.tcx, self.lcx.param_env)? == 0)
+                } else {
+                    span_bug!(e.span, "typeck error");
+                }
+            },
+            _ => None,
+        }
+    }
+
     #[expect(clippy::cast_possible_wrap)]
     fn constant_not(&self, o: &Constant<'tcx>, ty: Ty<'_>) -> Option<Constant<'tcx>> {
         use self::Constant::{Bool, Int};
@@ -519,8 +576,11 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
         vec.iter().map(|elem| self.expr(elem)).collect::<Option<_>>()
     }
 
-    /// Lookup a possibly constant expression from an `ExprKind::Path`.
-    fn fetch_path(&mut self, qpath: &QPath<'_>, id: HirId, ty: Ty<'tcx>) -> Option<Constant<'tcx>> {
+    /// Lookup a possibly constant expression from an `ExprKind::Path` and apply a function on it.
+    fn fetch_path_and_apply<T, F>(&mut self, qpath: &QPath<'_>, id: HirId, ty: Ty<'tcx>, f: F) -> Option<T>
+    where
+        F: FnOnce(&mut Self, rustc_middle::mir::Const<'tcx>) -> Option<T>,
+    {
         let res = self.typeck_results.qpath_res(qpath, id);
         match res {
             Res::Def(DefKind::Const | DefKind::AssocConst, def_id) => {
@@ -553,9 +613,7 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
                     .const_eval_resolve(self.param_env, mir::UnevaluatedConst::new(def_id, args), None)
                     .ok()
                     .map(|val| rustc_middle::mir::Const::from_value(val, ty))?;
-                let result = mir_to_const(self.lcx, result)?;
-                self.source = ConstantSource::Constant;
-                Some(result)
+                f(self, result)
             },
             _ => None,
         }
@@ -746,7 +804,6 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
 }
 
 pub fn mir_to_const<'tcx>(lcx: &LateContext<'tcx>, result: mir::Const<'tcx>) -> Option<Constant<'tcx>> {
-    use rustc_middle::mir::ConstValue;
     let mir::Const::Val(val, _) = result else {
         // We only work on evaluated consts.
         return None;
@@ -794,6 +851,42 @@ pub fn mir_to_const<'tcx>(lcx: &LateContext<'tcx>, result: mir::Const<'tcx>) ->
     }
 }
 
+fn mir_is_empty<'tcx>(lcx: &LateContext<'tcx>, result: mir::Const<'tcx>) -> Option<bool> {
+    let mir::Const::Val(val, _) = result else {
+        // We only work on evaluated consts.
+        return None;
+    };
+    match (val, result.ty().kind()) {
+        (_, ty::Ref(_, inner_ty, _)) => match inner_ty.kind() {
+            ty::Str | ty::Slice(_) => {
+                if let ConstValue::Indirect { alloc_id, offset } = val {
+                    // Get the length from the slice, using the same formula as
+                    // [`ConstValue::try_get_slice_bytes_for_diagnostics`].
+                    let a = lcx.tcx.global_alloc(alloc_id).unwrap_memory().inner();
+                    let ptr_size = lcx.tcx.data_layout.pointer_size;
+                    if a.size() < offset + 2 * ptr_size {
+                        // (partially) dangling reference
+                        return None;
+                    }
+                    let len = a
+                        .read_scalar(&lcx.tcx, alloc_range(offset + ptr_size, ptr_size), false)
+                        .ok()?
+                        .to_target_usize(&lcx.tcx)
+                        .ok()?;
+                    Some(len == 0)
+                } else {
+                    None
+                }
+            },
+            ty::Array(_, len) => Some(len.try_to_target_usize(lcx.tcx)? == 0),
+            _ => None,
+        },
+        (ConstValue::Indirect { .. }, ty::Array(_, len)) => Some(len.try_to_target_usize(lcx.tcx)? == 0),
+        (ConstValue::ZeroSized, _) => Some(true),
+        _ => None,
+    }
+}
+
 fn field_of_struct<'tcx>(
     adt_def: ty::AdtDef<'tcx>,
     lcx: &LateContext<'tcx>,

tests/ui/bool_assert_comparison.fixed

@@ -1,4 +1,4 @@
-#![allow(unused, clippy::assertions_on_constants)]
+#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty)]
 #![warn(clippy::bool_assert_comparison)]
 
 use std::ops::Not;

tests/ui/bool_assert_comparison.rs

@@ -1,4 +1,4 @@
-#![allow(unused, clippy::assertions_on_constants)]
+#![allow(unused, clippy::assertions_on_constants, clippy::const_is_empty)]
 #![warn(clippy::bool_assert_comparison)]
 
 use std::ops::Not;