Skip to content

Commit 33a6590

Browse files
authored
Check if deref target implements is_empty for len_zero lint (#13871)
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
2 parents c52740c + 4c9c2cc commit 33a6590

File tree

5 files changed

+95
-32
lines changed

5 files changed

+95
-32
lines changed

clippy_lints/src/eta_reduction.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ fn check_clousure<'tcx>(cx: &LateContext<'tcx>, outer_receiver: Option<&Expr<'tc
182182
// will succeed iff `T: 'static`. But the region of `T` is always erased by `typeck.expr_ty()` when
183183
// T is a generic type. For example, return type of `Option<String>::as_deref()` is a generic.
184184
// So we have a hack like this.
185-
&& generic_args.len() > 0
185+
&& !generic_args.is_empty()
186186
{
187187
return;
188188
}

clippy_lints/src/len_zero.rs

+26-13
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::source::{SpanRangeExt, snippet_with_context};
33
use clippy_utils::sugg::{Sugg, has_enclosing_paren};
4+
use clippy_utils::ty::implements_trait;
45
use clippy_utils::{get_item_name, get_parent_as_impl, is_lint_allowed, is_trait_method, peel_ref_operators};
56
use rustc_ast::ast::LitKind;
67
use rustc_errors::Applicability;
@@ -620,18 +621,30 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
620621
})
621622
}
622623

623-
let ty = &cx.typeck_results().expr_ty(expr).peel_refs();
624-
match ty.kind() {
625-
ty::Dynamic(tt, ..) => tt.principal().is_some_and(|principal| {
626-
let is_empty = sym!(is_empty);
627-
cx.tcx
628-
.associated_items(principal.def_id())
629-
.filter_by_name_unhygienic(is_empty)
630-
.any(|item| is_is_empty(cx, item))
631-
}),
632-
ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id),
633-
ty::Adt(id, _) => has_is_empty_impl(cx, id.did()),
634-
ty::Array(..) | ty::Slice(..) | ty::Str => true,
635-
_ => false,
624+
fn ty_has_is_empty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, depth: usize) -> bool {
625+
match ty.kind() {
626+
ty::Dynamic(tt, ..) => tt.principal().is_some_and(|principal| {
627+
let is_empty = sym!(is_empty);
628+
cx.tcx
629+
.associated_items(principal.def_id())
630+
.filter_by_name_unhygienic(is_empty)
631+
.any(|item| is_is_empty(cx, item))
632+
}),
633+
ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id),
634+
ty::Adt(id, _) => {
635+
has_is_empty_impl(cx, id.did())
636+
|| (cx.tcx.recursion_limit().value_within_limit(depth)
637+
&& cx.tcx.get_diagnostic_item(sym::Deref).is_some_and(|deref_id| {
638+
implements_trait(cx, ty, deref_id, &[])
639+
&& cx
640+
.get_associated_type(ty, deref_id, "Target")
641+
.is_some_and(|deref_ty| ty_has_is_empty(cx, deref_ty, depth + 1))
642+
}))
643+
},
644+
ty::Array(..) | ty::Slice(..) | ty::Str => true,
645+
_ => false,
646+
}
636647
}
648+
649+
ty_has_is_empty(cx, cx.typeck_results().expr_ty(expr).peel_refs(), 0)
637650
}

tests/ui/len_zero.fixed

+22
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ fn main() {
108108
let d2s = DerefToDerefToString {};
109109
println!("{}", (**d2s).is_empty());
110110

111+
println!("{}", std::borrow::Cow::Borrowed("").is_empty());
112+
111113
let y = One;
112114
if y.len() == 0 {
113115
// No error; `One` does not have `.is_empty()`.
@@ -226,3 +228,23 @@ fn binop_with_macros() {
226228

227229
(!has_is_empty.is_empty()).then(|| println!("This can happen."));
228230
}
231+
232+
fn no_infinite_recursion() -> bool {
233+
struct S;
234+
235+
impl Deref for S {
236+
type Target = Self;
237+
fn deref(&self) -> &Self::Target {
238+
self
239+
}
240+
}
241+
242+
impl PartialEq<&'static str> for S {
243+
fn eq(&self, _other: &&'static str) -> bool {
244+
false
245+
}
246+
}
247+
248+
// Do not crash while checking if S implements `.is_empty()`
249+
S == ""
250+
}

tests/ui/len_zero.rs

+22
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ fn main() {
108108
let d2s = DerefToDerefToString {};
109109
println!("{}", &**d2s == "");
110110

111+
println!("{}", std::borrow::Cow::Borrowed("") == "");
112+
111113
let y = One;
112114
if y.len() == 0 {
113115
// No error; `One` does not have `.is_empty()`.
@@ -226,3 +228,23 @@ fn binop_with_macros() {
226228

227229
(compare_to!(0) < has_is_empty.len()).then(|| println!("This can happen."));
228230
}
231+
232+
fn no_infinite_recursion() -> bool {
233+
struct S;
234+
235+
impl Deref for S {
236+
type Target = Self;
237+
fn deref(&self) -> &Self::Target {
238+
self
239+
}
240+
}
241+
242+
impl PartialEq<&'static str> for S {
243+
fn eq(&self, _other: &&'static str) -> bool {
244+
false
245+
}
246+
}
247+
248+
// Do not crash while checking if S implements `.is_empty()`
249+
S == ""
250+
}

tests/ui/len_zero.stderr

+24-18
Original file line numberDiff line numberDiff line change
@@ -58,107 +58,113 @@ error: comparison to empty slice
5858
LL | println!("{}", &**d2s == "");
5959
| ^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `(**d2s).is_empty()`
6060

61+
error: comparison to empty slice
62+
--> tests/ui/len_zero.rs:111:20
63+
|
64+
LL | println!("{}", std::borrow::Cow::Borrowed("") == "");
65+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `std::borrow::Cow::Borrowed("").is_empty()`
66+
6167
error: length comparison to zero
62-
--> tests/ui/len_zero.rs:124:8
68+
--> tests/ui/len_zero.rs:126:8
6369
|
6470
LL | if has_is_empty.len() == 0 {
6571
| ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()`
6672

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

163-
error: aborting due to 26 previous errors
169+
error: aborting due to 27 previous errors
164170

0 commit comments

Comments
 (0)