squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Auto merge of #6980 - Jarcho:len_without_is_empty_sig, r=llogiq

Browse source 
`len_without_is_empty` improvements

fixes: #6958
fixes: #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>`
This commit is contained in:
bors 2021-03-27 13:10:43 +00:00
commit dcee00d64b
3 changed files with 231 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

78
clippy_lints/src/len_zero.rs
View file

@ -10,9 +10,12 @@ use rustc_hir::{
ItemKind, Mutability, Node, TraitItemRef, TyKind,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, AssocKind, FnSig};
use rustc_middle::ty::{self, AssocKind, FnSig, Ty, TyS};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::{Span, Spanned, Symbol};
use rustc_span::{
source_map::{Span, Spanned, Symbol},
symbol::sym,
};
declare_clippy_lint! {
/// **What it does:** Checks for getting the length of something via `.len()`
@ -137,6 +140,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
if let Some(local_id) = ty_id.as_local();
let ty_hir_id = cx.tcx.hir().local_def_id_to_hir_id(local_id);
if !is_allowed(cx, LEN_WITHOUT_IS_EMPTY, ty_hir_id);
if let Some(output) = parse_len_output(cx, cx.tcx.fn_sig(item.def_id).skip_binder());
then {
let (name, kind) = match cx.tcx.hir().find(ty_hir_id) {
Some(Node::ForeignItem(x)) => (x.ident.name, "extern type"),
@ -148,7 +152,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero {
}
_ => return,
};
check_for_is_empty(cx, sig.span, sig.decl.implicit_self, ty_id, name, kind)
check_for_is_empty(cx, sig.span, sig.decl.implicit_self, output, ty_id, name, kind)
}
}
}
@ -231,10 +235,62 @@ fn check_trait_items(cx: &LateContext<'_>, visited_trait: &Item<'_>, trait_items
}
}
#[derive(Debug, Clone, Copy)]
enum LenOutput<'tcx> {
Integral,
Option(DefId),
Result(DefId, Ty<'tcx>),
}
fn parse_len_output(cx: &LateContext<'_>, sig: FnSig<'tcx>) -> Option<LenOutput<'tcx>> {
match *sig.output().kind() {
ty::Int(_) | ty::Uint(_) => Some(LenOutput::Integral),
ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::option_type, adt.did) => {
subs.type_at(0).is_integral().then(|| LenOutput::Option(adt.did))
},
ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::result_type, adt.did) => subs
.type_at(0)
.is_integral()
.then(|| LenOutput::Result(adt.did, subs.type_at(1))),
_ => None,
}
}
impl LenOutput<'_> {
fn matches_is_empty_output(self, ty: Ty<'_>) -> bool {
match (self, ty.kind()) {
(_, &ty::Bool) => true,
(Self::Option(id), &ty::Adt(adt, subs)) if id == adt.did => subs.type_at(0).is_bool(),
(Self::Result(id, err_ty), &ty::Adt(adt, subs)) if id == adt.did => {
subs.type_at(0).is_bool() && TyS::same_type(subs.type_at(1), err_ty)
},
_ => false,
}
}
fn expected_sig(self, self_kind: ImplicitSelfKind) -> String {
let self_ref = match self_kind {
ImplicitSelfKind::ImmRef => "&",
ImplicitSelfKind::MutRef => "&mut ",
_ => "",
};
match self {
Self::Integral => format!("expected signature: `({}self) -> bool`", self_ref),
Self::Option(_) => format!(
"expected signature: `({}self) -> bool` or `({}self) -> Option<bool>",
self_ref, self_ref
),
Self::Result(..) => format!(
"expected signature: `({}self) -> bool` or `({}self) -> Result<bool>",
self_ref, self_ref
),
}
}
}
/// Checks if the given signature matches the expectations for `is_empty`
fn check_is_empty_sig(cx: &LateContext<'_>, sig: FnSig<'_>, self_kind: ImplicitSelfKind) -> bool {
fn check_is_empty_sig(sig: FnSig<'_>, self_kind: ImplicitSelfKind, len_output: LenOutput<'_>) -> bool {
match &**sig.inputs_and_output {
[arg, res] if *res == cx.tcx.types.bool => {
[arg, res] if len_output.matches_is_empty_output(res) => {
matches!(
(arg.kind(), self_kind),
(ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::ImmRef)
@ -250,6 +306,7 @@ fn check_for_is_empty(
cx: &LateContext<'_>,
span: Span,
self_kind: ImplicitSelfKind,
output: LenOutput<'_>,
impl_ty: DefId,
item_name: Symbol,
item_kind: &str,
@ -289,7 +346,7 @@ fn check_for_is_empty(
},
Some(is_empty)
if !(is_empty.fn_has_self_parameter
&& check_is_empty_sig(cx, cx.tcx.fn_sig(is_empty.def_id).skip_binder(), self_kind)) =>
&& check_is_empty_sig(cx.tcx.fn_sig(is_empty.def_id).skip_binder(), self_kind, output)) =>
{
(
format!(
@ -309,14 +366,7 @@ fn check_for_is_empty(
db.span_note(span, "`is_empty` defined here");
}
if let Some(self_kind) = self_kind {
db.note(&format!(
"expected signature: `({}self) -> bool`",
match self_kind {
ImplicitSelfKind::ImmRef => "&",
ImplicitSelfKind::MutRef => "&mut ",
_ => "",
}
));
db.note(&output.expected_sig(self_kind));
}
});
}

99
tests/ui/len_without_is_empty.rs
View file

@ -1,3 +1,5 @@
// edition:2018
#![warn(clippy::len_without_is_empty)]
#![allow(dead_code, unused)]
@ -172,9 +174,9 @@ pub trait DependsOnFoo: Foo {
fn len(&mut self) -> usize;
}
// issue #1562
pub struct MultipleImpls;
// issue #1562
impl MultipleImpls {
pub fn len(&self) -> usize {
1
@ -187,4 +189,99 @@ impl MultipleImpls {
}
}
// issue #6958
pub struct OptionalLen;
impl OptionalLen {
pub fn len(&self) -> Option<usize> {
Some(0)
}
pub fn is_empty(&self) -> Option<bool> {
Some(true)
}
}
pub struct OptionalLen2;
impl OptionalLen2 {
pub fn len(&self) -> Option<usize> {
Some(0)
}
pub fn is_empty(&self) -> bool {
true
}
}
pub struct OptionalLen3;
impl OptionalLen3 {
pub fn len(&self) -> usize {
0
}
// should lint, len is not an option
pub fn is_empty(&self) -> Option<bool> {
None
}
}
pub struct ResultLen;
impl ResultLen {
pub fn len(&self) -> Result<usize, ()> {
Ok(0)
}
// Differing result types
pub fn is_empty(&self) -> Option<bool> {
Some(true)
}
}
pub struct ResultLen2;
impl ResultLen2 {
pub fn len(&self) -> Result<usize, ()> {
Ok(0)
}
pub fn is_empty(&self) -> Result<bool, ()> {
Ok(true)
}
}
pub struct ResultLen3;
impl ResultLen3 {
pub fn len(&self) -> Result<usize, ()> {
Ok(0)
}
// Non-fallible result is ok.
pub fn is_empty(&self) -> bool {
true
}
}
pub struct OddLenSig;
impl OddLenSig {
// don't lint
pub fn len(&self) -> bool {
true
}
}
// issue #6958
pub struct AsyncLen;
impl AsyncLen {
async fn async_task(&self) -> bool {
true
}
pub async fn len(&self) -> usize {
if self.async_task().await { 0 } else { 1 }
}
pub async fn is_empty(&self) -> bool {
self.len().await == 0
}
}
fn main() {}

79
tests/ui/len_without_is_empty.stderr
View file

@ -1,5 +1,5 @@
error: struct `PubOne` has a public `len` method, but no `is_empty` method
--> $DIR/len_without_is_empty.rs:7:5
--> $DIR/len_without_is_empty.rs:9:5
|
LL | pub fn len(&self) -> isize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -7,7 +7,7 @@ LL | pub fn len(&self) -> isize {
= note: `-D clippy::len-without-is-empty` implied by `-D warnings`
error: trait `PubTraitsToo` has a `len` method but no (possibly inherited) `is_empty` method
--> $DIR/len_without_is_empty.rs:55:1
--> $DIR/len_without_is_empty.rs:57:1
|
LL | / pub trait PubTraitsToo {
LL | | fn len(&self) -> isize;
@ -15,50 +15,109 @@ LL | | }
| |_^
error: struct `HasIsEmpty` has a public `len` method, but a private `is_empty` method
--> $DIR/len_without_is_empty.rs:68:5
--> $DIR/len_without_is_empty.rs:70:5
|
LL | pub fn len(&self) -> isize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: `is_empty` defined here
--> $DIR/len_without_is_empty.rs:72:5
--> $DIR/len_without_is_empty.rs:74:5
|
LL | fn is_empty(&self) -> bool {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
error: struct `HasWrongIsEmpty` has a public `len` method, but the `is_empty` method has an unexpected signature
--> $DIR/len_without_is_empty.rs:80:5
--> $DIR/len_without_is_empty.rs:82:5
|
LL | pub fn len(&self) -> isize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: `is_empty` defined here
--> $DIR/len_without_is_empty.rs:84:5
--> $DIR/len_without_is_empty.rs:86:5
|
LL | pub fn is_empty(&self, x: u32) -> bool {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected signature: `(&self) -> bool`
error: struct `MismatchedSelf` has a public `len` method, but the `is_empty` method has an unexpected signature
--> $DIR/len_without_is_empty.rs:92:5
--> $DIR/len_without_is_empty.rs:94:5
|
LL | pub fn len(self) -> isize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: `is_empty` defined here
--> $DIR/len_without_is_empty.rs:96:5
--> $DIR/len_without_is_empty.rs:98:5
|
LL | pub fn is_empty(&self) -> bool {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected signature: `(self) -> bool`
error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method
--> $DIR/len_without_is_empty.rs:171:1
--> $DIR/len_without_is_empty.rs:173:1
|
LL | / pub trait DependsOnFoo: Foo {
LL | | fn len(&mut self) -> usize;
LL | | }
| |_^
error: aborting due to 6 previous errors
error: struct `OptionalLen3` has a public `len` method, but the `is_empty` method has an unexpected signature
--> $DIR/len_without_is_empty.rs:218:5
|
LL | pub fn len(&self) -> usize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: `is_empty` defined here
--> $DIR/len_without_is_empty.rs:223:5
|
LL | pub fn is_empty(&self) -> Option<bool> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected signature: `(&self) -> bool`
error: struct `ResultLen` has a public `len` method, but the `is_empty` method has an unexpected signature
--> $DIR/len_without_is_empty.rs:230:5
|
LL | pub fn len(&self) -> Result<usize, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: `is_empty` defined here
--> $DIR/len_without_is_empty.rs:235:5
|
LL | pub fn is_empty(&self) -> Option<bool> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: expected signature: `(&self) -> bool` or `(&self) -> Result<bool>
error: this returns a `Result<_, ()>
--> $DIR/len_without_is_empty.rs:230:5
|
LL | pub fn len(&self) -> Result<usize, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::result-unit-err` implied by `-D warnings`
= help: use a custom Error type instead
error: this returns a `Result<_, ()>
--> $DIR/len_without_is_empty.rs:242:5
|
LL | pub fn len(&self) -> Result<usize, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: use a custom Error type instead
error: this returns a `Result<_, ()>
--> $DIR/len_without_is_empty.rs:246:5
|
LL | pub fn is_empty(&self) -> Result<bool, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: use a custom Error type instead
error: this returns a `Result<_, ()>
--> $DIR/len_without_is_empty.rs:253:5
|
LL | pub fn len(&self) -> Result<usize, ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: use a custom Error type instead
error: aborting due to 12 previous errors