check bounds to increase accuracy, and add todos

rust-lang · May 16, 2023 · f3b63ba · f3b63ba
1 parent 295b517
commit f3b63ba
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 10 deletions.
diff --git 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<DefId>,
 }
 
+// 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,22 +115,26 @@ 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(
                                 cx,
                                 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
                                     {
                                         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
@@ -70,3 +70,20 @@ impl PartialOrd for E {
         todo!();
     }
 }
+
+// do not lint since ord has more restrictive bounds
+
+#[derive(Eq, PartialEq)]
+struct Uwu<A>(A);
+
+impl<A: std::fmt::Debug + Ord + PartialOrd> Ord for Uwu<A> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        todo!();
+    }
+}
+
+impl<A: Ord + PartialOrd> PartialOrd for Uwu<A> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        todo!();
+    }
+}