diff --git a/clippy_lints/src/manual_partial_ord_and_ord_impl.rs b/clippy_lints/src/manual_partial_ord_and_ord_impl.rs index 8c7c7b9f7820..e9346a1bc907 100644 --- a/clippy_lints/src/manual_partial_ord_and_ord_impl.rs +++ b/clippy_lints/src/manual_partial_ord_and_ord_impl.rs @@ -2,9 +2,10 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::{get_trait_def_id, match_qpath, path_res, ty::implements_trait}; use rustc_errors::Applicability; use rustc_hir::def::Res; -use rustc_hir::{Expr, ExprKind, Impl, ImplItemKind, Item, ItemKind, PatKind, QPath}; +use rustc_hir::{Expr, ExprKind, Impl, ImplItemKind, Item, ItemKind, PatKind}; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{EarlyBinder, TraitRef}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::def_id::DefId; use rustc_span::sym; @@ -65,33 +66,46 @@ pub struct ManualPartialOrdAndOrdImpl { pub ord_def_id: OnceCell, } +// TODO: The number of if_chain! calls makes this is a bit hard to follow, can that be reduced? +// or more generally, can this be done cleaner? + impl LateLintPass<'_> for ManualPartialOrdAndOrdImpl { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { if_chain! { if let ItemKind::Impl(imp) = &item.kind; - if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id); - let partial_ord_def_id = impl_trait_ref.skip_binder().def_id; + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(item.owner_id).map(EarlyBinder::skip_binder); + let partial_ord_def_id = impl_trait_ref.def_id; if cx.tcx.is_diagnostic_item(sym::PartialOrd, partial_ord_def_id); if !cx.tcx.is_automatically_derived(item.owner_id.to_def_id()); then { - lint_impl_body(self, cx, imp, item); + lint_impl_body(self, cx, imp, item, impl_trait_ref); } } } } #[allow(clippy::unnecessary_def_path)] // ??? -// ^ line 91, that can't be changed without causing compilation to fail -fn lint_impl_body(conf: &mut ManualPartialOrdAndOrdImpl, cx: &LateContext<'_>, imp: &Impl<'_>, item: &Item<'_>) { +fn lint_impl_body<'tcx>( + conf: &mut ManualPartialOrdAndOrdImpl, + cx: &LateContext<'tcx>, + imp: &Impl<'_>, + item: &Item<'_>, + impl_trait_ref: TraitRef<'tcx>, +) { for imp_item in imp.items { if_chain! { if imp_item.ident.name == sym::partial_cmp; if let ImplItemKind::Fn(_, id) = cx.tcx.hir().impl_item(imp_item.id).kind; then { let body = cx.tcx.hir().body(id); + // TODO: This can't be changed without causing compilation to fail let ord_def_id = conf.ord_def_id.get_or_init(|| get_trait_def_id(cx, &["core", "cmp", "Ord"]).unwrap()); - if let ExprKind::Block(block, ..) - = body.value.kind && implements_trait(cx, hir_ty_to_ty(cx.tcx, imp.self_ty), *ord_def_id, &[]) + if let ExprKind::Block(block, ..) = body.value.kind && implements_trait( + cx, + hir_ty_to_ty(cx.tcx, imp.self_ty), + *ord_def_id, + impl_trait_ref.substs, + ) { if_chain! { if block.stmts.is_empty(); @@ -101,6 +115,7 @@ fn lint_impl_body(conf: &mut ManualPartialOrdAndOrdImpl, cx: &LateContext<'_>, i if let ExprKind::MethodCall(cmp_path, _, [other_expr], ..) = cmp_expr.kind; if cmp_path.ident.name == sym::cmp; if let Res::Local(..) = path_res(cx, other_expr); + // TODO: Self explanatory then {} else { span_lint_and_then( @@ -108,6 +123,9 @@ fn lint_impl_body(conf: &mut ManualPartialOrdAndOrdImpl, cx: &LateContext<'_>, i MANUAL_PARTIAL_ORD_AND_ORD_IMPL, item.span, "manual implementation of `PartialOrd` when `Ord` is already implemented", + // TODO: Is this how this should be done? Should we instead suggest + // changing the entire block, including the name + // of (by default) other if we can't find it? |diag| { if let Some(param) = body.params.get(1) && let PatKind::Binding(_, _, param_ident, ..) = param.pat.kind @@ -115,8 +133,8 @@ fn lint_impl_body(conf: &mut ManualPartialOrdAndOrdImpl, cx: &LateContext<'_>, i diag.span_suggestion( block.span, "change this to", - format!("{{ Some(self.cmp({})) }}", - param_ident.name), + format!("{{ Some(self.cmp({})) }}", param_ident.name), + // TODO: This is always correct afaik. Applicability::MaybeIncorrect ); } else { diff --git a/tests/ui/manual_partial_ord_and_ord_impl.rs b/tests/ui/manual_partial_ord_and_ord_impl.rs index 2dd20b67de8a..436ac1a05550 100644 --- a/tests/ui/manual_partial_ord_and_ord_impl.rs +++ b/tests/ui/manual_partial_ord_and_ord_impl.rs @@ -70,3 +70,20 @@ impl PartialOrd for E { todo!(); } } + +// do not lint since ord has more restrictive bounds + +#[derive(Eq, PartialEq)] +struct Uwu(A); + +impl Ord for Uwu { + fn cmp(&self, other: &Self) -> Ordering { + todo!(); + } +} + +impl PartialOrd for Uwu { + fn partial_cmp(&self, other: &Self) -> Option { + todo!(); + } +}