Add warn-by-default lint against unpredictable fn pointer comparisons

Author: Urgau

compiler/rustc_lint/messages.ftl

@@ -882,6 +882,12 @@ lint_unnameable_test_items = cannot test inner items
 lint_unnecessary_qualification = unnecessary qualification
     .suggestion = remove the unnecessary path segments
 
+lint_unpredictable_fn_pointer_comparisons = function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
+    .note_duplicated_fn = the address of the same function can vary between different codegen units
+    .note_deduplicated_fn = furthermore, different functions could have the same address after being merged together
+    .note_visit_fn_addr_eq = for more information visit <https://doc.rust-lang.org/nightly/core/ptr/fn.fn_addr_eq.html>
+    .fn_addr_eq_suggestion = refactor your code, or use `std::ptr::fn_addr_eq` to suppress the lint
+
 lint_unqualified_local_imports = `use` of a local item without leading `self::`, `super::`, or `crate::`
 
 lint_unsafe_attr_outside_unsafe = unsafe attribute used without unsafe

compiler/rustc_lint/src/lints.rs

@@ -1810,6 +1810,42 @@ pub(crate) enum AmbiguousWidePointerComparisonsAddrSuggestion<'a> {
     },
 }
 
+#[derive(LintDiagnostic)]
+pub(crate) enum UnpredictableFunctionPointerComparisons<'a> {
+    #[diag(lint_unpredictable_fn_pointer_comparisons)]
+    #[note(lint_note_duplicated_fn)]
+    #[note(lint_note_deduplicated_fn)]
+    #[note(lint_note_visit_fn_addr_eq)]
+    Suggestion {
+        #[subdiagnostic]
+        sugg: UnpredictableFunctionPointerComparisonsSuggestion<'a>,
+    },
+    #[diag(lint_unpredictable_fn_pointer_comparisons)]
+    #[note(lint_note_duplicated_fn)]
+    #[note(lint_note_deduplicated_fn)]
+    #[note(lint_note_visit_fn_addr_eq)]
+    Warn,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(
+    lint_fn_addr_eq_suggestion,
+    style = "verbose",
+    applicability = "maybe-incorrect"
+)]
+pub(crate) struct UnpredictableFunctionPointerComparisonsSuggestion<'a> {
+    pub ne: &'a str,
+    pub cast_right: String,
+    pub deref_left: &'a str,
+    pub deref_right: &'a str,
+    #[suggestion_part(code = "{ne}std::ptr::fn_addr_eq({deref_left}")]
+    pub left: Span,
+    #[suggestion_part(code = ", {deref_right}")]
+    pub middle: Span,
+    #[suggestion_part(code = "{cast_right})")]
+    pub right: Span,
+}
+
 pub(crate) struct ImproperCTypes<'a> {
     pub ty: Ty<'a>,
     pub desc: &'a str,

compiler/rustc_lint/src/types.rs

@@ -23,7 +23,9 @@ use crate::lints::{
     AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion,
     AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad,
     AtomicOrderingStore, ImproperCTypes, InvalidAtomicOrderingDiag, InvalidNanComparisons,
-    InvalidNanComparisonsSuggestion, UnusedComparisons, VariantSizeDifferencesDiag,
+    InvalidNanComparisonsSuggestion, UnpredictableFunctionPointerComparisons,
+    UnpredictableFunctionPointerComparisonsSuggestion, UnusedComparisons,
+    VariantSizeDifferencesDiag,
 };
 use crate::{LateContext, LateLintPass, LintContext, fluent_generated as fluent};
 
@@ -166,6 +168,35 @@ declare_lint! {
     "detects ambiguous wide pointer comparisons"
 }
 
+declare_lint! {
+    /// The `unpredictable_function_pointer_comparisons` lint checks comparison
+    /// of function pointer as the operands.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// fn a() {}
+    /// fn b() {}
+    ///
+    /// let f: fn() = a;
+    /// let g: fn() = b;
+    ///
+    /// let _ = f == g;
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Function pointers comparisons do not produce meaningful result since
+    /// they are never guaranteed to be unique and could vary between different
+    /// code generation units. Furthermore, different functions could have the
+    /// same address after being merged together.
+    UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
+    Warn,
+    "detects unpredictable function pointer comparisons"
+}
+
 #[derive(Copy, Clone, Default)]
 pub(crate) struct TypeLimits {
     /// Id of the last visited negated expression
@@ -178,7 +209,8 @@ impl_lint_pass!(TypeLimits => [
     UNUSED_COMPARISONS,
     OVERFLOWING_LITERALS,
     INVALID_NAN_COMPARISONS,
-    AMBIGUOUS_WIDE_POINTER_COMPARISONS
+    AMBIGUOUS_WIDE_POINTER_COMPARISONS,
+    UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS
 ]);
 
 impl TypeLimits {
@@ -255,7 +287,7 @@ fn lint_nan<'tcx>(
     cx.emit_span_lint(INVALID_NAN_COMPARISONS, e.span, lint);
 }
 
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Copy, Clone)]
 enum ComparisonOp {
     BinOp(hir::BinOpKind),
     Other,
@@ -383,6 +415,100 @@ fn lint_wide_pointer<'tcx>(
     );
 }
 
+fn lint_fn_pointer<'tcx>(
+    cx: &LateContext<'tcx>,
+    e: &'tcx hir::Expr<'tcx>,
+    cmpop: ComparisonOp,
+    l: &'tcx hir::Expr<'tcx>,
+    r: &'tcx hir::Expr<'tcx>,
+) {
+    let peel_refs = |mut ty: Ty<'tcx>| -> (Ty<'tcx>, usize) {
+        let mut refs = 0;
+
+        while let ty::Ref(_, inner_ty, _) = ty.kind() {
+            ty = *inner_ty;
+            refs += 1;
+        }
+
+        (ty, refs)
+    };
+
+    // Left and right operands can have borrows, remove them
+    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 };
+
+    // Remove any references as `==` will deref through them (and count the
+    // number of references removed, for latter).
+    let (l_ty, l_ty_refs) = peel_refs(l_ty);
+    let (r_ty, r_ty_refs) = peel_refs(r_ty);
+
+    if !l_ty.is_fn() || !r_ty.is_fn() {
+        return;
+    }
+
+    // Let's try to suggest `ptr::fn_addr_eq` if/when possible.
+
+    let is_eq_ne = matches!(cmpop, ComparisonOp::BinOp(hir::BinOpKind::Eq | hir::BinOpKind::Ne));
+
+    if !is_eq_ne {
+        // Neither `==` nor `!=`, we can't suggest `ptr::fn_addr_eq`, just show the warning.
+        return cx.emit_span_lint(
+            UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
+            e.span,
+            UnpredictableFunctionPointerComparisons::Warn,
+        );
+    }
+
+    let (Some(l_span), Some(r_span)) =
+        (l.span.find_ancestor_inside(e.span), r.span.find_ancestor_inside(e.span))
+    else {
+        // No appropriate spans for the left and right operands, just show the warning.
+        return cx.emit_span_lint(
+            UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
+            e.span,
+            UnpredictableFunctionPointerComparisons::Warn,
+        );
+    };
+
+    let ne = if cmpop == ComparisonOp::BinOp(hir::BinOpKind::Ne) { "!" } else { "" };
+
+    // `ptr::fn_addr_eq` only works with raw pointer, deref any references.
+    let deref_left = &*"*".repeat(l_ty_refs);
+    let deref_right = &*"*".repeat(r_ty_refs);
+
+    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());
+
+    // We only check for a right cast as `FnDef` == `FnPtr` is not possible,
+    // only `FnPtr == FnDef` is possible.
+    let cast_right = if !r_ty.is_fn_ptr() {
+        let fn_sig = r_ty.fn_sig(cx.tcx);
+        format!(" as {fn_sig}")
+    } else {
+        String::new()
+    };
+
+    cx.emit_span_lint(
+        UNPREDICTABLE_FUNCTION_POINTER_COMPARISONS,
+        e.span,
+        UnpredictableFunctionPointerComparisons::Suggestion {
+            sugg: UnpredictableFunctionPointerComparisonsSuggestion {
+                ne,
+                deref_left,
+                deref_right,
+                left,
+                middle,
+                right,
+                cast_right,
+            },
+        },
+    );
+}
+
 impl<'tcx> LateLintPass<'tcx> for TypeLimits {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx hir::Expr<'tcx>) {
         match e.kind {
@@ -399,7 +525,9 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
                         cx.emit_span_lint(UNUSED_COMPARISONS, e.span, UnusedComparisons);
                     } else {
                         lint_nan(cx, e, binop, l, r);
-                        lint_wide_pointer(cx, e, ComparisonOp::BinOp(binop.node), l, r);
+                        let cmpop = ComparisonOp::BinOp(binop.node);
+                        lint_wide_pointer(cx, e, cmpop, l, r);
+                        lint_fn_pointer(cx, e, cmpop, l, r);
                     }
                 }
             }
@@ -411,13 +539,15 @@ impl<'tcx> LateLintPass<'tcx> for TypeLimits {
                     && let Some(cmpop) = diag_item_cmpop(diag_item) =>
             {
                 lint_wide_pointer(cx, e, cmpop, l, r);
+                lint_fn_pointer(cx, e, cmpop, 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(cmpop) = diag_item_cmpop(diag_item) =>
             {
                 lint_wide_pointer(cx, e, cmpop, l, r);
+                lint_fn_pointer(cx, e, cmpop, l, r);
             }
             _ => {}
         };

tests/ui/lint/fn-ptr-comparisons-weird.rs

@@ -0,0 +1,15 @@
+//@ check-pass
+
+fn main() {
+    let f: fn() = main;
+    let g: fn() = main;
+
+    let _ = f > g;
+    //~^ WARN function pointer comparisons
+    let _ = f >= g;
+    //~^ WARN function pointer comparisons
+    let _ = f <= g;
+    //~^ WARN function pointer comparisons
+    let _ = f < g;
+    //~^ WARN function pointer comparisons
+}

tests/ui/lint/fn-ptr-comparisons-weird.stderr

@@ -0,0 +1,43 @@
+warning: function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
+  --> $DIR/fn-ptr-comparisons-weird.rs:7:13
+   |
+LL |     let _ = f > g;
+   |             ^^^^^
+   |
+   = note: the address of the same function can vary between different codegen units
+   = note: furthermore, different functions could have the same address after being merged together
+   = note: for more information visit <https://doc.rust-lang.org/nightly/core/ptr/fn.fn_addr_eq.html>
+   = note: `#[warn(unpredictable_function_pointer_comparisons)]` on by default
+
+warning: function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
+  --> $DIR/fn-ptr-comparisons-weird.rs:9:13
+   |
+LL |     let _ = f >= g;
+   |             ^^^^^^
+   |
+   = note: the address of the same function can vary between different codegen units
+   = note: furthermore, different functions could have the same address after being merged together
+   = note: for more information visit <https://doc.rust-lang.org/nightly/core/ptr/fn.fn_addr_eq.html>
+
+warning: function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
+  --> $DIR/fn-ptr-comparisons-weird.rs:11:13
+   |
+LL |     let _ = f <= g;
+   |             ^^^^^^
+   |
+   = note: the address of the same function can vary between different codegen units
+   = note: furthermore, different functions could have the same address after being merged together
+   = note: for more information visit <https://doc.rust-lang.org/nightly/core/ptr/fn.fn_addr_eq.html>
+
+warning: function pointer comparisons do not produce meaningful results since their addresses are not guaranteed to be unique
+  --> $DIR/fn-ptr-comparisons-weird.rs:13:13
+   |
+LL |     let _ = f < g;
+   |             ^^^^^
+   |
+   = note: the address of the same function can vary between different codegen units
+   = note: furthermore, different functions could have the same address after being merged together
+   = note: for more information visit <https://doc.rust-lang.org/nightly/core/ptr/fn.fn_addr_eq.html>
+
+warning: 4 warnings emitted
+

tests/ui/lint/fn-ptr-comparisons.fixed

@@ -0,0 +1,58 @@
+//@ check-pass
+//@ run-rustfix
+
+extern "C" {
+    fn test();
+}
+
+fn a() {}
+
+extern "C" fn c() {}
+
+extern "C" fn args(_a: i32) -> i32 { 0 }
+
+#[derive(PartialEq, Eq)]
+struct A {
+    f: fn(),
+}
+
+fn main() {
+    let f: fn() = a;
+    let g: fn() = f;
+
+    let a1 = A { f };
+    let a2 = A { f };
+
+    let _ = std::ptr::fn_addr_eq(f, a as fn());
+    //~^ WARN function pointer comparisons
+    let _ = !std::ptr::fn_addr_eq(f, a as fn());
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(f, g);
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(f, f);
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(g, g);
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(g, g);
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(g, g);
+    //~^ WARN function pointer comparisons
+    let _ = std::ptr::fn_addr_eq(a as fn(), g);
+    //~^ WARN function pointer comparisons
+
+    let cfn: extern "C" fn() = c;
+    let _ = std::ptr::fn_addr_eq(cfn, c as extern "C" fn());
+    //~^ WARN function pointer comparisons
+
+    let argsfn: extern "C" fn(i32) -> i32 = args;
+    let _ = std::ptr::fn_addr_eq(argsfn, args as extern "C" fn(i32) -> i32);
+    //~^ WARN function pointer comparisons
+
+    let t: unsafe extern "C" fn() = test;
+    let _ = std::ptr::fn_addr_eq(t, test as unsafe extern "C" fn());
+    //~^ WARN function pointer comparisons
+
+    let _ = a1 == a2; // should not warn
+    let _ = std::ptr::fn_addr_eq(a1.f, a2.f);
+    //~^ WARN function pointer comparisons
+}