From a27bd22519f4b2acf3b2e05091250f0ce7b3f569 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Fri, 5 Dec 2025 12:45:58 +0100 Subject: [PATCH 1/2] clean-up This mostly qualifies variable names with either `is_empty_` or `len_`, to make it easier to understand which of the two methods they relate to --- clippy_lints/src/len_zero.rs | 163 +++++++++++++++++------------------ 1 file changed, 79 insertions(+), 84 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 877bd34a732b..7c1d62951eaf 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -156,8 +156,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { && let Some(ty_id) = cx.qpath_res(ty_path, imp.self_ty.hir_id).opt_def_id() && let Some(local_id) = ty_id.as_local() && let ty_hir_id = cx.tcx.local_def_id_to_hir_id(local_id) - && let Some(output) = - parse_len_output(cx, cx.tcx.fn_sig(item.owner_id).instantiate_identity().skip_binder()) + && let Some(output) = LenOutput::new(cx, cx.tcx.fn_sig(item.owner_id).instantiate_identity().skip_binder()) { let (name, kind) = match cx.tcx.hir_node(ty_hir_id) { Node::ForeignItem(x) => (x.ident.name, "extern type"), @@ -401,13 +400,6 @@ fn check_trait_items(cx: &LateContext<'_>, visited_trait: &Item<'_>, ident: Iden } } -#[derive(Debug, Clone, Copy)] -enum LenOutput { - Integral, - Option(DefId), - Result(DefId), -} - fn extract_future_output<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'tcx PathSegment<'tcx>> { if let ty::Alias(_, alias_ty) = ty.kind() && let Some(Node::OpaqueTy(opaque)) = cx.tcx.hir_get_if_local(alias_ty.def_id) @@ -439,42 +431,49 @@ fn is_first_generic_integral<'tcx>(segment: &'tcx PathSegment<'tcx>) -> bool { } } -fn parse_len_output<'tcx>(cx: &LateContext<'tcx>, sig: FnSig<'tcx>) -> Option { - if let Some(segment) = extract_future_output(cx, sig.output()) { - let res = segment.res; +#[derive(Debug, Clone, Copy)] +enum LenOutput { + Integral, + Option(DefId), + Result(DefId), +} - if matches!(res, Res::PrimTy(PrimTy::Uint(_) | PrimTy::Int(_))) { - return Some(LenOutput::Integral); - } +impl LenOutput { + fn new<'tcx>(cx: &LateContext<'tcx>, sig: FnSig<'tcx>) -> Option { + if let Some(segment) = extract_future_output(cx, sig.output()) { + let res = segment.res; - if let Res::Def(_, def_id) = res - && let Some(res) = match cx.tcx.get_diagnostic_name(def_id) { - Some(sym::Option) => Some(LenOutput::Option(def_id)), - Some(sym::Result) => Some(LenOutput::Result(def_id)), - _ => None, + if matches!(res, Res::PrimTy(PrimTy::Uint(_) | PrimTy::Int(_))) { + return Some(Self::Integral); } - && is_first_generic_integral(segment) - { - return Some(res); - } - return None; - } + if let Res::Def(_, def_id) = res + && let Some(res) = match cx.tcx.get_diagnostic_name(def_id) { + Some(sym::Option) => Some(Self::Option(def_id)), + Some(sym::Result) => Some(Self::Result(def_id)), + _ => None, + } + && is_first_generic_integral(segment) + { + return Some(res); + } + + return None; + } - match *sig.output().kind() { - ty::Int(_) | ty::Uint(_) => Some(LenOutput::Integral), - ty::Adt(adt, subs) => match cx.tcx.get_diagnostic_name(adt.did()) { - Some(sym::Option) => subs.type_at(0).is_integral().then(|| LenOutput::Option(adt.did())), - Some(sym::Result) => subs.type_at(0).is_integral().then(|| LenOutput::Result(adt.did())), + match *sig.output().kind() { + ty::Int(_) | ty::Uint(_) => Some(Self::Integral), + ty::Adt(adt, subs) => match cx.tcx.get_diagnostic_name(adt.did()) { + Some(sym::Option) => subs.type_at(0).is_integral().then(|| Self::Option(adt.did())), + Some(sym::Result) => subs.type_at(0).is_integral().then(|| Self::Result(adt.did())), + _ => None, + }, _ => None, - }, - _ => None, + } } -} -impl LenOutput { - fn matches_is_empty_output<'tcx>(self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { - if let Some(segment) = extract_future_output(cx, ty) { + fn matches_is_empty_output<'tcx>(self, cx: &LateContext<'tcx>, is_empty_output: Ty<'tcx>) -> bool { + if let Some(segment) = extract_future_output(cx, is_empty_output) { return match (self, segment.res) { (_, Res::PrimTy(PrimTy::Bool)) => true, (Self::Option(_), Res::Def(_, def_id)) if cx.tcx.is_diagnostic_item(sym::Option, def_id) => true, @@ -483,48 +482,51 @@ impl LenOutput { }; } - match (self, ty.kind()) { + match (self, is_empty_output.kind()) { (_, &ty::Bool) => true, (Self::Option(id), &ty::Adt(adt, subs)) if id == adt.did() => subs.type_at(0).is_bool(), (Self::Result(id), &ty::Adt(adt, subs)) if id == adt.did() => subs.type_at(0).is_bool(), _ => false, } } +} - fn expected_sig(self, self_kind: ImplicitSelfKind) -> String { - let self_ref = match self_kind { - ImplicitSelfKind::RefImm => "&", - ImplicitSelfKind::RefMut => "&mut ", - _ => "", - }; - match self { - Self::Integral => format!("expected signature: `({self_ref}self) -> bool`"), - Self::Option(_) => { - format!("expected signature: `({self_ref}self) -> bool` or `({self_ref}self) -> Option") - }, - Self::Result(..) => { - format!("expected signature: `({self_ref}self) -> bool` or `({self_ref}self) -> Result") - }, - } +/// The expected signature of `is_empty`, based on that of `len` +fn expected_is_empty_sig(len_output: LenOutput, len_self_kind: ImplicitSelfKind) -> String { + let self_ref = match len_self_kind { + ImplicitSelfKind::RefImm => "&", + ImplicitSelfKind::RefMut => "&mut ", + _ => "", + }; + match len_output { + LenOutput::Integral => format!("expected signature: `({self_ref}self) -> bool`"), + LenOutput::Option(_) => { + format!("expected signature: `({self_ref}self) -> bool` or `({self_ref}self) -> Option") + }, + LenOutput::Result(..) => { + format!("expected signature: `({self_ref}self) -> bool` or `({self_ref}self) -> Result") + }, } } /// Checks if the given signature matches the expectations for `is_empty` fn check_is_empty_sig<'tcx>( cx: &LateContext<'tcx>, - sig: FnSig<'tcx>, - self_kind: ImplicitSelfKind, + is_empty_sig: FnSig<'tcx>, + len_self_kind: ImplicitSelfKind, len_output: LenOutput, ) -> bool { - match &**sig.inputs_and_output { - [arg, res] if len_output.matches_is_empty_output(cx, *res) => { - matches!( - (arg.kind(), self_kind), - (ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::RefImm) - | (ty::Ref(_, _, Mutability::Mut), ImplicitSelfKind::RefMut) - ) || (!arg.is_ref() && matches!(self_kind, ImplicitSelfKind::Imm | ImplicitSelfKind::Mut)) - }, - _ => false, + if let [is_empty_self_arg, is_empty_output] = &**is_empty_sig.inputs_and_output + && len_output.matches_is_empty_output(cx, *is_empty_output) + { + match (is_empty_self_arg.kind(), len_self_kind) { + (ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::RefImm) + | (ty::Ref(_, _, Mutability::Mut), ImplicitSelfKind::RefMut) => true, + (_, ImplicitSelfKind::Imm | ImplicitSelfKind::Mut) if !is_empty_self_arg.is_ref() => true, + _ => false, + } + } else { + false } } @@ -532,9 +534,9 @@ fn check_is_empty_sig<'tcx>( #[expect(clippy::too_many_arguments)] fn check_for_is_empty( cx: &LateContext<'_>, - span: Span, - self_kind: ImplicitSelfKind, - output: LenOutput, + len_span: Span, + len_self_kind: ImplicitSelfKind, + len_output: LenOutput, impl_ty: DefId, item_name: Symbol, item_kind: &str, @@ -556,20 +558,14 @@ fn check_for_is_empty( .flat_map(|&id| cx.tcx.associated_items(id).filter_by_name_unhygienic(sym::is_empty)) .find(|item| item.is_fn()); - let (msg, is_empty_span, self_kind) = match is_empty { + let (msg, is_empty_span, is_empty_expected_sig) = match is_empty { None => ( - format!( - "{item_kind} `{}` has a public `len` method, but no `is_empty` method", - item_name.as_str(), - ), + format!("{item_kind} `{item_name}` has a public `len` method, but no `is_empty` method"), None, None, ), Some(is_empty) if !cx.effective_visibilities.is_exported(is_empty.def_id.expect_local()) => ( - format!( - "{item_kind} `{}` has a public `len` method, but a private `is_empty` method", - item_name.as_str(), - ), + format!("{item_kind} `{item_name}` has a public `len` method, but a private `is_empty` method"), Some(cx.tcx.def_span(is_empty.def_id)), None, ), @@ -578,29 +574,28 @@ fn check_for_is_empty( && check_is_empty_sig( cx, cx.tcx.fn_sig(is_empty.def_id).instantiate_identity().skip_binder(), - self_kind, - output, + len_self_kind, + len_output, )) => { ( format!( - "{item_kind} `{}` has a public `len` method, but the `is_empty` method has an unexpected signature", - item_name.as_str(), + "{item_kind} `{item_name}` has a public `len` method, but the `is_empty` method has an unexpected signature", ), Some(cx.tcx.def_span(is_empty.def_id)), - Some(self_kind), + Some(expected_is_empty_sig(len_output, len_self_kind)), ) }, Some(_) => return, }; if !fulfill_or_allowed(cx, LEN_WITHOUT_IS_EMPTY, [len_method_hir_id, ty_decl_hir_id]) { - span_lint_and_then(cx, LEN_WITHOUT_IS_EMPTY, span, msg, |db| { + span_lint_and_then(cx, LEN_WITHOUT_IS_EMPTY, len_span, msg, |db| { if let Some(span) = is_empty_span { db.span_note(span, "`is_empty` defined here"); } - if let Some(self_kind) = self_kind { - db.note(output.expected_sig(self_kind)); + if let Some(expected_sig) = is_empty_expected_sig { + db.note(expected_sig); } }); } From d6b561376e932fc136f2443cc733d830ec160b6d Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Fri, 5 Dec 2025 13:53:02 +0100 Subject: [PATCH 2/2] fix(len_without_is_empty): allow `is_empty(&self)` with `len(&mut self)` --- clippy_lints/src/len_zero.rs | 8 ++++++-- tests/ui/len_without_is_empty.rs | 12 ++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 7c1d62951eaf..f5a832f3adfd 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -495,7 +495,7 @@ impl LenOutput { fn expected_is_empty_sig(len_output: LenOutput, len_self_kind: ImplicitSelfKind) -> String { let self_ref = match len_self_kind { ImplicitSelfKind::RefImm => "&", - ImplicitSelfKind::RefMut => "&mut ", + ImplicitSelfKind::RefMut => "&(mut) ", _ => "", }; match len_output { @@ -520,8 +520,12 @@ fn check_is_empty_sig<'tcx>( && len_output.matches_is_empty_output(cx, *is_empty_output) { match (is_empty_self_arg.kind(), len_self_kind) { + // if `len` takes `&self`, `is_empty` should do so as well (ty::Ref(_, _, Mutability::Not), ImplicitSelfKind::RefImm) - | (ty::Ref(_, _, Mutability::Mut), ImplicitSelfKind::RefMut) => true, + // if `len` takes `&mut self`, `is_empty` may take that _or_ `&self` (#16190) + | (ty::Ref(_, _, Mutability::Mut | Mutability::Not), ImplicitSelfKind::RefMut) => true, + // if len takes `self`, `is_empty` should do so as well + // XXX: we might want to relax this to allow `&self` and `&mut self` (_, ImplicitSelfKind::Imm | ImplicitSelfKind::Mut) if !is_empty_self_arg.is_ref() => true, _ => false, } diff --git a/tests/ui/len_without_is_empty.rs b/tests/ui/len_without_is_empty.rs index 011833072d76..509348628dd6 100644 --- a/tests/ui/len_without_is_empty.rs +++ b/tests/ui/len_without_is_empty.rs @@ -473,4 +473,16 @@ impl Alias2 { } } +// Issue #16190 +pub struct RefMutLenButRefIsEmpty; +impl RefMutLenButRefIsEmpty { + pub fn len(&mut self) -> usize { + todo!() + } + + pub fn is_empty(&self) -> bool { + todo!() + } +} + fn main() {}