Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint const_is_empty #12310

Merged
merged 5 commits into from
Mar 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5109,6 +5109,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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,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,
Expand Down
49 changes: 49 additions & 0 deletions clippy_lints/src/methods/is_empty.rs
Original file line number Diff line number Diff line change
@@ -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
}
35 changes: 34 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -4041,6 +4042,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,
Expand Down Expand Up @@ -4089,6 +4115,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,
Expand Down Expand Up @@ -4442,7 +4469,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);
}
Expand Down Expand Up @@ -4616,6 +4643,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),
Expand Down
107 changes: 100 additions & 7 deletions clippy_utils/src/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -303,6 +305,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>,
Expand Down Expand Up @@ -402,7 +410,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)?;
Comment on lines +414 to +415
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the feeling using miri to check if the value is constant can cause annoying false positives. Though, you ran it in 500 crates, and it only generated a single warning, which indicates that this fear might be overly cautions.

I would suggest adding a restriction at this position, to only evaluate the path, if it's a local path, meaning it comes from the same crate. Constant values of dependencies are not always in the control of the user and should therefore be seen as a black box IMO. (Maybe this should be configurable)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run lintcheck on the 1000 most popular crates: no new hit compared to the 1 on 500 crates. But you're right, we might want to err on the side of caution, I've added such a check in an additional commit so that you can have a look at it separately. Tell me if it requires stashing, even though the changes are logically separated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that the likelihood of a crate using external constants highly depends on the type of crate. Crates.io mainly hosts libraries, they would probably provide such constant values but use them rarly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, except constants such as the path separator, but this is now taken care of, so we get no hit in popular libraries.

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() {
Expand Down Expand Up @@ -468,6 +482,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};
Expand Down Expand Up @@ -515,8 +572,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) => {
Expand Down Expand Up @@ -549,9 +609,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,
}
Expand Down Expand Up @@ -742,7 +800,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;
Expand Down Expand Up @@ -788,6 +845,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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like most is_empty functions in rustc's std are already static. Could we maybe make our lives easier and throw the whole <recv>.is_empty() expression into the constant evaluation? That seems easier than this entire thing.

(I haven't worked with const too much, so excuse me, if this suggestion is invalid)

Copy link
Contributor Author

@samueltardieu samueltardieu Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably, if those methods were const they would not trigger a warning but the use of their result might.

Those methods are const already, it is surprising that the use of their result does not trigger a warning. But as far as I can see, no warning is triggered on this code even for the if true:

fn main() {
    if true {
        println!("True");
    }

    if [0u8; 0].is_empty() {
        println!("Is empty");
    }
}

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 {
samueltardieu marked this conversation as resolved.
Show resolved Hide resolved
// (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>,
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/bool_assert_comparison.fixed
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/bool_assert_comparison.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
Loading
Loading