Skip to content

Commit

Permalink
Check if deref target implements is_empty for len_zero lint (#13871)
Browse files Browse the repository at this point in the history
In this case, the lint can be triggered as well as `is_empty()` will be
found on the target type.
One such case was found in Clippy sources (first commit)

changelog: [`len_zero`]: trigger if deref target implements `is_empty()`

Close #13861
  • Loading branch information
blyxyas authored Jan 1, 2025
2 parents c52740c + 4c9c2cc commit 33a6590
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 32 deletions.
2 changes: 1 addition & 1 deletion clippy_lints/src/eta_reduction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ fn check_clousure<'tcx>(cx: &LateContext<'tcx>, outer_receiver: Option<&Expr<'tc
// will succeed iff `T: 'static`. But the region of `T` is always erased by `typeck.expr_ty()` when
// T is a generic type. For example, return type of `Option<String>::as_deref()` is a generic.
// So we have a hack like this.
&& generic_args.len() > 0
&& !generic_args.is_empty()
{
return;
}
Expand Down
39 changes: 26 additions & 13 deletions clippy_lints/src/len_zero.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::{SpanRangeExt, snippet_with_context};
use clippy_utils::sugg::{Sugg, has_enclosing_paren};
use clippy_utils::ty::implements_trait;
use clippy_utils::{get_item_name, get_parent_as_impl, is_lint_allowed, is_trait_method, peel_ref_operators};
use rustc_ast::ast::LitKind;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -620,18 +621,30 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
})
}

let ty = &cx.typeck_results().expr_ty(expr).peel_refs();
match ty.kind() {
ty::Dynamic(tt, ..) => tt.principal().is_some_and(|principal| {
let is_empty = sym!(is_empty);
cx.tcx
.associated_items(principal.def_id())
.filter_by_name_unhygienic(is_empty)
.any(|item| is_is_empty(cx, item))
}),
ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id),
ty::Adt(id, _) => has_is_empty_impl(cx, id.did()),
ty::Array(..) | ty::Slice(..) | ty::Str => true,
_ => false,
fn ty_has_is_empty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, depth: usize) -> bool {
match ty.kind() {
ty::Dynamic(tt, ..) => tt.principal().is_some_and(|principal| {
let is_empty = sym!(is_empty);
cx.tcx
.associated_items(principal.def_id())
.filter_by_name_unhygienic(is_empty)
.any(|item| is_is_empty(cx, item))
}),
ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id),
ty::Adt(id, _) => {
has_is_empty_impl(cx, id.did())
|| (cx.tcx.recursion_limit().value_within_limit(depth)
&& cx.tcx.get_diagnostic_item(sym::Deref).is_some_and(|deref_id| {
implements_trait(cx, ty, deref_id, &[])
&& cx
.get_associated_type(ty, deref_id, "Target")
.is_some_and(|deref_ty| ty_has_is_empty(cx, deref_ty, depth + 1))
}))
},
ty::Array(..) | ty::Slice(..) | ty::Str => true,
_ => false,
}
}

ty_has_is_empty(cx, cx.typeck_results().expr_ty(expr).peel_refs(), 0)
}
22 changes: 22 additions & 0 deletions tests/ui/len_zero.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ fn main() {
let d2s = DerefToDerefToString {};
println!("{}", (**d2s).is_empty());

println!("{}", std::borrow::Cow::Borrowed("").is_empty());

let y = One;
if y.len() == 0 {
// No error; `One` does not have `.is_empty()`.
Expand Down Expand Up @@ -226,3 +228,23 @@ fn binop_with_macros() {

(!has_is_empty.is_empty()).then(|| println!("This can happen."));
}

fn no_infinite_recursion() -> bool {
struct S;

impl Deref for S {
type Target = Self;
fn deref(&self) -> &Self::Target {
self
}
}

impl PartialEq<&'static str> for S {
fn eq(&self, _other: &&'static str) -> bool {
false
}
}

// Do not crash while checking if S implements `.is_empty()`
S == ""
}
22 changes: 22 additions & 0 deletions tests/ui/len_zero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ fn main() {
let d2s = DerefToDerefToString {};
println!("{}", &**d2s == "");

println!("{}", std::borrow::Cow::Borrowed("") == "");

let y = One;
if y.len() == 0 {
// No error; `One` does not have `.is_empty()`.
Expand Down Expand Up @@ -226,3 +228,23 @@ fn binop_with_macros() {

(compare_to!(0) < has_is_empty.len()).then(|| println!("This can happen."));
}

fn no_infinite_recursion() -> bool {
struct S;

impl Deref for S {
type Target = Self;
fn deref(&self) -> &Self::Target {
self
}
}

impl PartialEq<&'static str> for S {
fn eq(&self, _other: &&'static str) -> bool {
false
}
}

// Do not crash while checking if S implements `.is_empty()`
S == ""
}
42 changes: 24 additions & 18 deletions tests/ui/len_zero.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -58,107 +58,113 @@ error: comparison to empty slice
LL | println!("{}", &**d2s == "");
| ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `(**d2s).is_empty()`

error: comparison to empty slice
--> tests/ui/len_zero.rs:111:20
|
LL | println!("{}", std::borrow::Cow::Borrowed("") == "");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `std::borrow::Cow::Borrowed("").is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:124:8
--> tests/ui/len_zero.rs:126:8
|
LL | if has_is_empty.len() == 0 {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:127:8
--> tests/ui/len_zero.rs:129:8
|
LL | if has_is_empty.len() != 0 {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:130:8
--> tests/ui/len_zero.rs:132:8
|
LL | if has_is_empty.len() > 0 {
| ^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to one
--> tests/ui/len_zero.rs:133:8
--> tests/ui/len_zero.rs:135:8
|
LL | if has_is_empty.len() < 1 {
| ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to one
--> tests/ui/len_zero.rs:136:8
--> tests/ui/len_zero.rs:138:8
|
LL | if has_is_empty.len() >= 1 {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:147:8
--> tests/ui/len_zero.rs:149:8
|
LL | if 0 == has_is_empty.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:150:8
--> tests/ui/len_zero.rs:152:8
|
LL | if 0 != has_is_empty.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:153:8
--> tests/ui/len_zero.rs:155:8
|
LL | if 0 < has_is_empty.len() {
| ^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to one
--> tests/ui/len_zero.rs:156:8
--> tests/ui/len_zero.rs:158:8
|
LL | if 1 <= has_is_empty.len() {
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to one
--> tests/ui/len_zero.rs:159:8
--> tests/ui/len_zero.rs:161:8
|
LL | if 1 > has_is_empty.len() {
| ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:173:8
--> tests/ui/len_zero.rs:175:8
|
LL | if with_is_empty.len() == 0 {
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `with_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:185:6
--> tests/ui/len_zero.rs:187:6
|
LL | (has_is_empty.len() > 0).then(|| println!("This can happen."));
| ^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:186:6
--> tests/ui/len_zero.rs:188:6
|
LL | (has_is_empty.len() == 0).then(|| println!("Or this!"));
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:190:8
--> tests/ui/len_zero.rs:192:8
|
LL | if b.len() != 0 {}
| ^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!b.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:224:8
--> tests/ui/len_zero.rs:226:8
|
LL | if has_is_empty.len() == compare_to!(0) {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:225:8
--> tests/ui/len_zero.rs:227:8
|
LL | if has_is_empty.len() == zero!() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`

error: length comparison to zero
--> tests/ui/len_zero.rs:227:6
--> tests/ui/len_zero.rs:229:6
|
LL | (compare_to!(0) < has_is_empty.len()).then(|| println!("This can happen."));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()`

error: aborting due to 26 previous errors
error: aborting due to 27 previous errors

0 comments on commit 33a6590

Please sign in to comment.