Skip to content

Commit dcee00d

Browse files
borsbors
committed
Auto merge of rust-lang#6980 - Jarcho:len_without_is_empty_sig, r=llogiq
`len_without_is_empty` improvements fixes: rust-lang#6958 fixes: rust-lang#6972 changelog: Check the return type of `len`. Only integral types, or an `Option` or `Result` wrapping one. changelog: Ensure the return type of `is_empty` matches. e.g. `Option<usize>` -> `Option<bool>`
2 parents 6f2a6fe + 12985af commit dcee00d

File tree

3 files changed

+231
-25
lines changed

3 files changed

+231
-25
lines changed

‎clippy_lints/src/len_zero.rs

+64-14
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ use rustc_hir::{
1010
ItemKind, Mutability, Node, TraitItemRef, TyKind,
1111
};
1212
use rustc_lint::{LateContext, LateLintPass};
13-
use rustc_middle::ty::{self, AssocKind, FnSig};
13+
use rustc_middle::ty::{self, AssocKind, FnSig, Ty, TyS};
1414
use rustc_session::{declare_lint_pass, declare_tool_lint};
15-
use rustc_span::source_map::{Span, Spanned, Symbol};
15+
use rustc_span::{
16+
source_map::{Span, Spanned, Symbol},
17+
symbol::sym,
18+
};
1619

1720
declare_clippy_lint! {
1821
/// **What it does:** Checks for getting the length of something via `.len()`
@@ -137,6 +140,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
137140
if let Some(local_id) = ty_id.as_local();
138141
let ty_hir_id = cx.tcx.hir().local_def_id_to_hir_id(local_id);
139142
if !is_allowed(cx, LEN_WITHOUT_IS_EMPTY, ty_hir_id);
143+
if let Some(output) = parse_len_output(cx, cx.tcx.fn_sig(item.def_id).skip_binder());
140144
then {
141145
let (name, kind) = match cx.tcx.hir().find(ty_hir_id) {
142146
Some(Node::ForeignItem(x)) => (x.ident.name, "extern type"),
@@ -148,7 +152,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
148152
}
149153
_ => return,
150154
};
151-
check_for_is_empty(cx, sig.span, sig.decl.implicit_self, ty_id, name, kind)
155+
check_for_is_empty(cx, sig.span, sig.decl.implicit_self, output, ty_id, name, kind)
152156
}
153157
}
154158
}
@@ -231,10 +235,62 @@ fn check_trait_items(cx: &LateContext<'_>, visited_trait: &Item<'_>, trait_items
231235
}
232236
}
233237

238+
#[derive(Debug, Clone, Copy)]
239+
enum LenOutput<'tcx> {
240+
Integral,
241+
Option(DefId),
242+
Result(DefId, Ty<'tcx>),
243+
}
244+
fn parse_len_output(cx: &LateContext<'_>, sig: FnSig<'tcx>) -> Option<LenOutput<'tcx>> {
245+
match *sig.output().kind() {
246+
ty::Int(_) | ty::Uint(_) => Some(LenOutput::Integral),
247+
ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::option_type, adt.did) => {
248+
subs.type_at(0).is_integral().then(|| LenOutput::Option(adt.did))
249+
},
250+
ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::result_type, adt.did) => subs
251+
.type_at(0)
252+
.is_integral()
253+
.then(|| LenOutput::Result(adt.did, subs.type_at(1))),
254+
_ => None,
255+
}
256+
}
257+
258+
impl LenOutput<'_> {
259+
fn matches_is_empty_output(self, ty: Ty<'_>) -> bool {
260+
match (self, ty.kind()) {
261+
(_, &ty::Bool) => true,
262+
(Self::Option(id), &ty::Adt(adt, subs)) if id == adt.did => subs.type_at(0).is_bool(),
263+
(Self::Result(id, err_ty), &ty::Adt(adt, subs)) if id == adt.did => {
264+
subs.type_at(0).is_bool() && TyS::same_type(subs.type_at(1), err_ty)
265+
},
266+
_ => false,
267+
}
268+
}
269+
270+
fn expected_sig(self, self_kind: ImplicitSelfKind) -> String {
271+
let self_ref = match self_kind {
272+
ImplicitSelfKind::ImmRef => "&",
273+
ImplicitSelfKind::MutRef => "&mut ",
274+
_ => "",
275+
};
276+
match self {
277+
Self::Integral => format!("expected signature: `({}self) -> bool`", self_ref),
278+
Self::Option(_) => format!(
279+
"expected signature: `({}self) -> bool` or `({}self) -> Option<bool>",
280+
self_ref, self_ref
281+
),
282+
Self::Result(..) => format!(
283+
"expected signature: `({}self) -> bool` or `({}self) -> Result<bool>",
284+
self_ref, self_ref
285+
),
286+
}
287+
}
288+
}
289+
234290
/// Checks if the given signature matches the expectations for `is_empty`
235-
fn check_is_empty_sig(cx: &LateContext<'_>, sig: FnSig<'_>, self_kind: ImplicitSelfKind) -> bool {
291+
fn check_is_empty_sig(sig: FnSig<'_>, self_kind: ImplicitSelfKind, len_output: LenOutput<'_>) -> bool {
236292
match &**sig.inputs_and_output {
237-
[arg, res] if *res == cx.tcx.types.bool => {
293+
[arg, res] if len_output.matches_is_empty_output(res) => {
238294
matches!(
239295
(arg.kind(), self_kind),
240296
(ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::ImmRef)
@@ -250,6 +306,7 @@ fn check_for_is_empty(
250306
cx: &LateContext<'_>,
251307
span: Span,
252308
self_kind: ImplicitSelfKind,
309+
output: LenOutput<'_>,
253310
impl_ty: DefId,
254311
item_name: Symbol,
255312
item_kind: &str,
@@ -289,7 +346,7 @@ fn check_for_is_empty(
289346
},
290347
Some(is_empty)
291348
if !(is_empty.fn_has_self_parameter
292-
&& check_is_empty_sig(cx, cx.tcx.fn_sig(is_empty.def_id).skip_binder(), self_kind)) =>
349+
&& check_is_empty_sig(cx.tcx.fn_sig(is_empty.def_id).skip_binder(), self_kind, output)) =>
293350
{
294351
(
295352
format!(
@@ -309,14 +366,7 @@ fn check_for_is_empty(
309366
db.span_note(span, "`is_empty` defined here");
310367
}
311368
if let Some(self_kind) = self_kind {
312-
db.note(&format!(
313-
"expected signature: `({}self) -> bool`",
314-
match self_kind {
315-
ImplicitSelfKind::ImmRef => "&",
316-
ImplicitSelfKind::MutRef => "&mut ",
317-
_ => "",
318-
}
319-
));
369+
db.note(&output.expected_sig(self_kind));
320370
}
321371
});
322372
}

‎tests/ui/len_without_is_empty.rs

+98-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// edition:2018
2+
13
#![warn(clippy::len_without_is_empty)]
24
#![allow(dead_code, unused)]
35

@@ -172,9 +174,9 @@ pub trait DependsOnFoo: Foo {
172174
fn len(&mut self) -> usize;
173175
}
174176

177+
// issue #1562
175178
pub struct MultipleImpls;
176179

177-
// issue #1562
178180
impl MultipleImpls {
179181
pub fn len(&self) -> usize {
180182
1
@@ -187,4 +189,99 @@ impl MultipleImpls {
187189
}
188190
}
189191

192+
// issue #6958
193+
pub struct OptionalLen;
194+
195+
impl OptionalLen {
196+
pub fn len(&self) -> Option<usize> {
197+
Some(0)
198+
}
199+
200+
pub fn is_empty(&self) -> Option<bool> {
201+
Some(true)
202+
}
203+
}
204+
205+
pub struct OptionalLen2;
206+
impl OptionalLen2 {
207+
pub fn len(&self) -> Option<usize> {
208+
Some(0)
209+
}
210+
211+
pub fn is_empty(&self) -> bool {
212+
true
213+
}
214+
}
215+
216+
pub struct OptionalLen3;
217+
impl OptionalLen3 {
218+
pub fn len(&self) -> usize {
219+
0
220+
}
221+
222+
// should lint, len is not an option
223+
pub fn is_empty(&self) -> Option<bool> {
224+
None
225+
}
226+
}
227+
228+
pub struct ResultLen;
229+
impl ResultLen {
230+
pub fn len(&self) -> Result<usize, ()> {
231+
Ok(0)
232+
}
233+
234+
// Differing result types
235+
pub fn is_empty(&self) -> Option<bool> {
236+
Some(true)
237+
}
238+
}
239+
240+
pub struct ResultLen2;
241+
impl ResultLen2 {
242+
pub fn len(&self) -> Result<usize, ()> {
243+
Ok(0)
244+
}
245+
246+
pub fn is_empty(&self) -> Result<bool, ()> {
247+
Ok(true)
248+
}
249+
}
250+
251+
pub struct ResultLen3;
252+
impl ResultLen3 {
253+
pub fn len(&self) -> Result<usize, ()> {
254+
Ok(0)
255+
}
256+
257+
// Non-fallible result is ok.
258+
pub fn is_empty(&self) -> bool {
259+
true
260+
}
261+
}
262+
263+
pub struct OddLenSig;
264+
impl OddLenSig {
265+
// don't lint
266+
pub fn len(&self) -> bool {
267+
true
268+
}
269+
}
270+
271+
// issue #6958
272+
pub struct AsyncLen;
273+
impl AsyncLen {
274+
async fn async_task(&self) -> bool {
275+
true
276+
}
277+
278+
pub async fn len(&self) -> usize {
279+
if self.async_task().await { 0 } else { 1 }
280+
}
281+
282+
pub async fn is_empty(&self) -> bool {
283+
self.len().await == 0
284+
}
285+
}
286+
190287
fn main() {}

‎tests/ui/len_without_is_empty.stderr

+69-10
Original file line numberDiff line numberDiff line change
@@ -1,64 +1,123 @@
11
error: struct `PubOne` has a public `len` method, but no `is_empty` method
2-
--> $DIR/len_without_is_empty.rs:7:5
2+
--> $DIR/len_without_is_empty.rs:9:5
33
|
44
LL | pub fn len(&self) -> isize {
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::len-without-is-empty` implied by `-D warnings`
88

99
error: trait `PubTraitsToo` has a `len` method but no (possibly inherited) `is_empty` method
10-
--> $DIR/len_without_is_empty.rs:55:1
10+
--> $DIR/len_without_is_empty.rs:57:1
1111
|
1212
LL | / pub trait PubTraitsToo {
1313
LL | | fn len(&self) -> isize;
1414
LL | | }
1515
| |_^
1616

1717
error: struct `HasIsEmpty` has a public `len` method, but a private `is_empty` method
18-
--> $DIR/len_without_is_empty.rs:68:5
18+
--> $DIR/len_without_is_empty.rs:70:5
1919
|
2020
LL | pub fn len(&self) -> isize {
2121
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
2222
|
2323
note: `is_empty` defined here
24-
--> $DIR/len_without_is_empty.rs:72:5
24+
--> $DIR/len_without_is_empty.rs:74:5
2525
|
2626
LL | fn is_empty(&self) -> bool {
2727
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
2828

2929
error: struct `HasWrongIsEmpty` has a public `len` method, but the `is_empty` method has an unexpected signature
30-
--> $DIR/len_without_is_empty.rs:80:5
30+
--> $DIR/len_without_is_empty.rs:82:5
3131
|
3232
LL | pub fn len(&self) -> isize {
3333
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
3434
|
3535
note: `is_empty` defined here
36-
--> $DIR/len_without_is_empty.rs:84:5
36+
--> $DIR/len_without_is_empty.rs:86:5
3737
|
3838
LL | pub fn is_empty(&self, x: u32) -> bool {
3939
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4040
= note: expected signature: `(&self) -> bool`
4141

4242
error: struct `MismatchedSelf` has a public `len` method, but the `is_empty` method has an unexpected signature
43-
--> $DIR/len_without_is_empty.rs:92:5
43+
--> $DIR/len_without_is_empty.rs:94:5
4444
|
4545
LL | pub fn len(self) -> isize {
4646
| ^^^^^^^^^^^^^^^^^^^^^^^^^
4747
|
4848
note: `is_empty` defined here
49-
--> $DIR/len_without_is_empty.rs:96:5
49+
--> $DIR/len_without_is_empty.rs:98:5
5050
|
5151
LL | pub fn is_empty(&self) -> bool {
5252
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5353
= note: expected signature: `(self) -> bool`
5454

5555
error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method
56-
--> $DIR/len_without_is_empty.rs:171:1
56+
--> $DIR/len_without_is_empty.rs:173:1
5757
|
5858
LL | / pub trait DependsOnFoo: Foo {
5959
LL | | fn len(&mut self) -> usize;
6060
LL | | }
6161
| |_^
6262

63-
error: aborting due to 6 previous errors
63+
error: struct `OptionalLen3` has a public `len` method, but the `is_empty` method has an unexpected signature
64+
--> $DIR/len_without_is_empty.rs:218:5
65+
|
66+
LL | pub fn len(&self) -> usize {
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
68+
|
69+
note: `is_empty` defined here
70+
--> $DIR/len_without_is_empty.rs:223:5
71+
|
72+
LL | pub fn is_empty(&self) -> Option<bool> {
73+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
74+
= note: expected signature: `(&self) -> bool`
75+
76+
error: struct `ResultLen` has a public `len` method, but the `is_empty` method has an unexpected signature
77+
--> $DIR/len_without_is_empty.rs:230:5
78+
|
79+
LL | pub fn len(&self) -> Result<usize, ()> {
80+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
81+
|
82+
note: `is_empty` defined here
83+
--> $DIR/len_without_is_empty.rs:235:5
84+
|
85+
LL | pub fn is_empty(&self) -> Option<bool> {
86+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
87+
= note: expected signature: `(&self) -> bool` or `(&self) -> Result<bool>
88+
89+
error: this returns a `Result<_, ()>
90+
--> $DIR/len_without_is_empty.rs:230:5
91+
|
92+
LL | pub fn len(&self) -> Result<usize, ()> {
93+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
94+
|
95+
= note: `-D clippy::result-unit-err` implied by `-D warnings`
96+
= help: use a custom Error type instead
97+
98+
error: this returns a `Result<_, ()>
99+
--> $DIR/len_without_is_empty.rs:242:5
100+
|
101+
LL | pub fn len(&self) -> Result<usize, ()> {
102+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
103+
|
104+
= help: use a custom Error type instead
105+
106+
error: this returns a `Result<_, ()>
107+
--> $DIR/len_without_is_empty.rs:246:5
108+
|
109+
LL | pub fn is_empty(&self) -> Result<bool, ()> {
110+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
111+
|
112+
= help: use a custom Error type instead
113+
114+
error: this returns a `Result<_, ()>
115+
--> $DIR/len_without_is_empty.rs:253:5
116+
|
117+
LL | pub fn len(&self) -> Result<usize, ()> {
118+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
119+
|
120+
= help: use a custom Error type instead
121+
122+
error: aborting due to 12 previous errors
64123

0 commit comments

Comments
 (0)