Auto merge of #117758 - Urgau:lint_pointer_trait_comparisons, r=davidtwco

Add lint against ambiguous wide pointer comparisons

This PR is the resolution of #106447 decided in #117717 by T-lang.

## `ambiguous_wide_pointer_comparisons`

*warn-by-default*

The `ambiguous_wide_pointer_comparisons` lint checks comparison of `*const/*mut ?Sized` as the operands.

### Example

```rust
let ab = (A, B);
let a = &ab.0 as *const dyn T;
let b = &ab.1 as *const dyn T;

let _ = a == b;
```

### Explanation

The comparison includes metadata which may not be expected.

-------

This PR also drops `clippy::vtable_address_comparisons` which is superseded by this one.

~~One thing: is the current naming right? `invalid` seems a bit too much.~~

Fixes #117717

Author: bors

compiler/rustc_codegen_cranelift/example/mini_core.rs

@@ -11,7 +11,7 @@
     thread_local
 )]
 #![no_core]
-#![allow(dead_code, internal_features)]
+#![allow(dead_code, internal_features, ambiguous_wide_pointer_comparisons)]
 
 #[lang = "sized"]
 pub trait Sized {}

compiler/rustc_lint/messages.ftl

@@ -1,3 +1,7 @@
+lint_ambiguous_wide_pointer_comparisons = ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
+    .addr_metadata_suggestion = use explicit `std::ptr::eq` method to compare metadata and addresses
+    .addr_suggestion = use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
+
 lint_array_into_iter =
     this method call resolves to `<&{$target} as IntoIterator>::into_iter` (due to backwards compatibility), but will resolve to <{$target} as IntoIterator>::into_iter in Rust 2021
     .use_iter_suggestion = use `.iter()` instead of `.into_iter()` to avoid ambiguity

compiler/rustc_lint/src/lints.rs

@@ -1574,6 +1574,76 @@ pub enum InvalidNanComparisonsSuggestion {
     Spanless,
 }
 
+#[derive(LintDiagnostic)]
+pub enum AmbiguousWidePointerComparisons<'a> {
+    #[diag(lint_ambiguous_wide_pointer_comparisons)]
+    Spanful {
+        #[subdiagnostic]
+        addr_suggestion: AmbiguousWidePointerComparisonsAddrSuggestion<'a>,
+        #[subdiagnostic]
+        addr_metadata_suggestion: Option<AmbiguousWidePointerComparisonsAddrMetadataSuggestion<'a>>,
+    },
+    #[diag(lint_ambiguous_wide_pointer_comparisons)]
+    #[help(lint_addr_metadata_suggestion)]
+    #[help(lint_addr_suggestion)]
+    Spanless,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(
+    lint_addr_metadata_suggestion,
+    style = "verbose",
+    applicability = "machine-applicable"
+)]
+pub struct AmbiguousWidePointerComparisonsAddrMetadataSuggestion<'a> {
+    pub ne: &'a str,
+    pub deref_left: &'a str,
+    pub deref_right: &'a str,
+    #[suggestion_part(code = "{ne}std::ptr::eq({deref_left}")]
+    pub left: Span,
+    #[suggestion_part(code = ", {deref_right}")]
+    pub middle: Span,
+    #[suggestion_part(code = ")")]
+    pub right: Span,
+}
+
+#[derive(Subdiagnostic)]
+pub enum AmbiguousWidePointerComparisonsAddrSuggestion<'a> {
+    #[multipart_suggestion(
+        lint_addr_suggestion,
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    AddrEq {
+        ne: &'a str,
+        deref_left: &'a str,
+        deref_right: &'a str,
+        #[suggestion_part(code = "{ne}std::ptr::addr_eq({deref_left}")]
+        left: Span,
+        #[suggestion_part(code = ", {deref_right}")]
+        middle: Span,
+        #[suggestion_part(code = ")")]
+        right: Span,
+    },
+    #[multipart_suggestion(
+        lint_addr_suggestion,
+        style = "verbose",
+        applicability = "machine-applicable"
+    )]
+    Cast {
+        deref_left: &'a str,
+        deref_right: &'a str,
+        #[suggestion_part(code = "{deref_left}")]
+        left_before: Option<Span>,
+        #[suggestion_part(code = " as *const ()")]
+        left: Span,
+        #[suggestion_part(code = "{deref_right}")]
+        right_before: Option<Span>,
+        #[suggestion_part(code = " as *const ()")]
+        right: Span,
+    },
+}
+
 pub struct ImproperCTypes<'a> {
     pub ty: Ty<'a>,
     pub desc: &'a str,

compiler/rustc_lint/src/types.rs

@@ -1,12 +1,13 @@
 use crate::{
     fluent_generated as fluent,
     lints::{
-        AtomicOrderingFence, AtomicOrderingLoad, AtomicOrderingStore, ImproperCTypes,
-        InvalidAtomicOrderingDiag, InvalidNanComparisons, InvalidNanComparisonsSuggestion,
-        OnlyCastu8ToChar, OverflowingBinHex, OverflowingBinHexSign, OverflowingBinHexSignBitSub,
-        OverflowingBinHexSub, OverflowingInt, OverflowingIntHelp, OverflowingLiteral,
-        OverflowingUInt, RangeEndpointOutOfRange, UnusedComparisons, UseInclusiveRange,
-        VariantSizeDifferencesDiag,
+        AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion,
+        AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad,
+        AtomicOrderingStore, ImproperCTypes, InvalidAtomicOrderingDiag, InvalidNanComparisons,
+        InvalidNanComparisonsSuggestion, OnlyCastu8ToChar, OverflowingBinHex,
+        OverflowingBinHexSign, OverflowingBinHexSignBitSub, OverflowingBinHexSub, OverflowingInt,
+        OverflowingIntHelp, OverflowingLiteral, OverflowingUInt, RangeEndpointOutOfRange,
+        UnusedComparisons, UseInclusiveRange, VariantSizeDifferencesDiag,
     },
 };
 use crate::{LateContext, LateLintPass, LintContext};
@@ -17,17 +18,18 @@ use rustc_errors::DiagnosticMessage;
 use rustc_hir as hir;
 use rustc_hir::{is_range_literal, Expr, ExprKind, Node};
 use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton};
-use rustc_middle::ty::GenericArgsRef;
 use rustc_middle::ty::{
     self, AdtKind, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
 };
+use rustc_middle::ty::{GenericArgsRef, TypeAndMut};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::source_map;
 use rustc_span::symbol::sym;
 use rustc_span::{Span, Symbol};
 use rustc_target::abi::{Abi, Size, WrappingRange};
 use rustc_target::abi::{Integer, TagEncoding, Variants};
 use rustc_target::spec::abi::Abi as SpecAbi;
+use rustc_type_ir::DynKind;
 
 use std::iter;
 use std::ops::ControlFlow;
@@ -136,6 +138,37 @@ declare_lint! {
     "detects invalid floating point NaN comparisons"
 }
 
+declare_lint! {
+    /// The `ambiguous_wide_pointer_comparisons` lint checks comparison
+    /// of `*const/*mut ?Sized` as the operands.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// # struct A;
+    /// # struct B;
+    ///
+    /// # trait T {}
+    /// # impl T for A {}
+    /// # impl T for B {}
+    ///
+    /// let ab = (A, B);
+    /// let a = &ab.0 as *const dyn T;
+    /// let b = &ab.1 as *const dyn T;
+    ///
+    /// let _ = a == b;
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// The comparison includes metadata which may not be expected.
+    AMBIGUOUS_WIDE_POINTER_COMPARISONS,
+    Warn,
+    "detects ambiguous wide pointer comparisons"
+}
+
 #[derive(Copy, Clone)]
 pub struct TypeLimits {
     /// Id of the last visited negated expression
@@ -144,7 +177,12 @@ pub struct TypeLimits {
     negated_expr_span: Option<Span>,
 }
 
-impl_lint_pass!(TypeLimits => [UNUSED_COMPARISONS, OVERFLOWING_LITERALS, INVALID_NAN_COMPARISONS]);
+impl_lint_pass!(TypeLimits => [
+    UNUSED_COMPARISONS,
+    OVERFLOWING_LITERALS,
+    INVALID_NAN_COMPARISONS,
+    AMBIGUOUS_WIDE_POINTER_COMPARISONS
+]);
 
 impl TypeLimits {
     pub fn new() -> TypeLimits {
@@ -620,6 +658,106 @@ fn lint_nan<'tcx>(
     cx.emit_spanned_lint(INVALID_NAN_COMPARISONS, e.span, lint);
 }
 
+fn lint_wide_pointer<'tcx>(
+    cx: &LateContext<'tcx>,
+    e: &'tcx hir::Expr<'tcx>,
+    binop: hir::BinOpKind,
+    l: &'tcx hir::Expr<'tcx>,
+    r: &'tcx hir::Expr<'tcx>,
+) {
+    let ptr_unsized = |mut ty: Ty<'tcx>| -> Option<(usize, bool)> {
+        let mut refs = 0;
+        // here we remove any "implicit" references and count the number
+        // of them to correctly suggest the right number of deref
+        while let ty::Ref(_, inner_ty, _) = ty.kind() {
+            ty = *inner_ty;
+            refs += 1;
+        }
+        match ty.kind() {
+            ty::RawPtr(TypeAndMut { mutbl: _, ty }) => (!ty.is_sized(cx.tcx, cx.param_env))
+                .then(|| (refs, matches!(ty.kind(), ty::Dynamic(_, _, DynKind::Dyn)))),
+            _ => None,
+        }
+    };
+
+    // PartialEq::{eq,ne} takes references, remove any explicit references
+    let l = l.peel_borrows();
+    let r = r.peel_borrows();
+
+    let Some(l_ty) = cx.typeck_results().expr_ty_opt(l) else {
+        return;
+    };
+    let Some(r_ty) = cx.typeck_results().expr_ty_opt(r) else {
+        return;
+    };
+
+    let Some((l_ty_refs, l_inner_ty_is_dyn)) = ptr_unsized(l_ty) else {
+        return;
+    };
+    let Some((r_ty_refs, r_inner_ty_is_dyn)) = ptr_unsized(r_ty) else {
+        return;
+    };
+
+    let (Some(l_span), Some(r_span)) =
+        (l.span.find_ancestor_inside(e.span), r.span.find_ancestor_inside(e.span))
+    else {
+        return cx.emit_spanned_lint(
+            AMBIGUOUS_WIDE_POINTER_COMPARISONS,
+            e.span,
+            AmbiguousWidePointerComparisons::Spanless,
+        );
+    };
+
+    let ne = if binop == hir::BinOpKind::Ne { "!" } else { "" };
+    let is_eq_ne = matches!(binop, hir::BinOpKind::Eq | hir::BinOpKind::Ne);
+    let is_dyn_comparison = l_inner_ty_is_dyn && r_inner_ty_is_dyn;
+
+    let left = e.span.shrink_to_lo().until(l_span.shrink_to_lo());
+    let middle = l_span.shrink_to_hi().until(r_span.shrink_to_lo());
+    let right = r_span.shrink_to_hi().until(e.span.shrink_to_hi());
+
+    let deref_left = &*"*".repeat(l_ty_refs);
+    let deref_right = &*"*".repeat(r_ty_refs);
+
+    cx.emit_spanned_lint(
+        AMBIGUOUS_WIDE_POINTER_COMPARISONS,
+        e.span,
+        AmbiguousWidePointerComparisons::Spanful {
+            addr_metadata_suggestion: (is_eq_ne && !is_dyn_comparison).then(|| {
+                AmbiguousWidePointerComparisonsAddrMetadataSuggestion {
+                    ne,
+                    deref_left,
+                    deref_right,
+                    left,
+                    middle,
+                    right,
+                }
+            }),
+            addr_suggestion: if is_eq_ne {
+                AmbiguousWidePointerComparisonsAddrSuggestion::AddrEq {
+                    ne,
+                    deref_left,
+                    deref_right,
+                    left,
+                    middle,
+                    right,
+                }
+            } else {
+                AmbiguousWidePointerComparisonsAddrSuggestion::Cast {
+                    deref_left,
+                    deref_right,
+                    // those two Options are required for correctness as having
+                    // an empty span and an empty suggestion is not permitted
+                    left_before: (l_ty_refs != 0).then_some(left),
+                    right_before: (r_ty_refs != 0).then(|| r_span.shrink_to_lo()),
+                    left: l_span.shrink_to_hi(),
+                    right,
+                }
+            },
+        },
+    );
+}
+
 impl<'tcx> LateLintPass<'tcx> for TypeLimits {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
         match e.kind {
@@ -636,10 +774,26 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
                         cx.emit_spanned_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
                     } else {
                         lint_nan(cx, e, binop, l, r);
+                        lint_wide_pointer(cx, e, binop.node, l, r);
                     }
                 }
             }
             hir::ExprKind::Lit(lit) => lint_literal(cx, self, e, lit),
+            hir::ExprKind::Call(path, [l, r])
+                if let ExprKind::Path(ref qpath) = path.kind
+                    && let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
+                    && let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
+                    && let Some(binop) = partialeq_binop(diag_item) =>
+            {
+                lint_wide_pointer(cx, e, binop, l, r);
+            }
+            hir::ExprKind::MethodCall(_, l, [r], _)
+                if let Some(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
+                    && let Some(diag_item) = cx.tcx.get_diagnostic_name(def_id)
+                    && let Some(binop) = partialeq_binop(diag_item) =>
+            {
+                lint_wide_pointer(cx, e, binop, l, r);
+            }
             _ => {}
         };
 
@@ -722,6 +876,16 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
                     | hir::BinOpKind::Gt
             )
         }
+
+        fn partialeq_binop(diag_item: Symbol) -> Option<hir::BinOpKind> {
+            if diag_item == sym::cmp_partialeq_eq {
+                Some(hir::BinOpKind::Eq)
+            } else if diag_item == sym::cmp_partialeq_ne {
+                Some(hir::BinOpKind::Ne)
+            } else {
+                None
+            }
+        }
     }
 }
 

compiler/rustc_span/src/symbol.rs

@@ -520,6 +520,8 @@ symbols! {
         cmp,
         cmp_max,
         cmp_min,
+        cmp_partialeq_eq,
+        cmp_partialeq_ne,
         cmpxchg16b_target_feature,
         cmse_nonsecure_entry,
         coerce_unsized,

library/alloc/tests/vec.rs

@@ -1980,7 +1980,7 @@ fn vec_macro_repeating_null_raw_fat_pointer() {
 
     let vec = vec![null_raw_dyn; 1];
     dbg!(ptr_metadata(vec[0]));
-    assert!(vec[0] == null_raw_dyn);
+    assert!(std::ptr::eq(vec[0], null_raw_dyn));
 
     // Polyfill for https://github.com/rust-lang/rfcs/pull/2580
 

library/core/src/cmp.rs

@@ -230,13 +230,15 @@ pub trait PartialEq<Rhs: ?Sized = Self> {
     /// by `==`.
     #[must_use]
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialeq_eq")]
     fn eq(&self, other: &Rhs) -> bool;
 
     /// This method tests for `!=`. The default implementation is almost always
     /// sufficient, and should not be overridden without very good reason.
     #[inline]
     #[must_use]
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_diagnostic_item = "cmp_partialeq_ne")]
     fn ne(&self, other: &Rhs) -> bool {
         !self.eq(other)
     }