Auto merge of #5294 - tmiasko:trait-ptr-cmp, r=Manishearth

Lint unnamed address comparisons

Functions and vtables have an insignificant address. Attempts to compare such addresses will lead to very surprising behaviour. For example: addresses of different functions could compare equal; two trait object pointers representing the same object and the same type could be unequal.

Lint against unnamed address comparisons to avoid issues like those in rust-lang/rust#69757 and rust-lang/rust#54685.

Changelog: New lints: [`fn_address_comparisons`] [#5294](#5294), [`vtable_address_comparisons`] [#5294](#5294)

Author: bors

Diff for: CHANGELOG.md

@@ -1271,6 +1271,7 @@ Released 2018-09-13
 [`float_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_arithmetic
 [`float_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp
 [`float_cmp_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp_const
+[`fn_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_address_comparisons
 [`fn_params_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_params_excessive_bools
 [`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast
 [`fn_to_numeric_cast_with_truncation`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_with_truncation
@@ -1540,6 +1541,7 @@ Released 2018-09-13
 [`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box
 [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask
 [`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads
+[`vtable_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons
 [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition
 [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop
 [`while_let_on_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_on_iterator

Diff for: README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 361 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 363 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

Diff for: clippy_lints/src/lib.rs

@@ -309,6 +309,7 @@ pub mod trivially_copy_pass_by_ref;
 pub mod try_err;
 pub mod types;
 pub mod unicode;
+pub mod unnamed_address;
 pub mod unsafe_removed_from_name;
 pub mod unused_io_amount;
 pub mod unused_self;
@@ -818,6 +819,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &unicode::NON_ASCII_LITERAL,
         &unicode::UNICODE_NOT_NFC,
         &unicode::ZERO_WIDTH_SPACE,
+        &unnamed_address::FN_ADDRESS_COMPARISONS,
+        &unnamed_address::VTABLE_ADDRESS_COMPARISONS,
         &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
         &unused_io_amount::UNUSED_IO_AMOUNT,
         &unused_self::UNUSED_SELF,
@@ -1027,6 +1030,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| box macro_use::MacroUseImports);
     store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
+    store.register_late_pass(|| box unnamed_address::UnnamedAddress);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1378,6 +1382,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&types::UNNECESSARY_CAST),
         LintId::of(&types::VEC_BOX),
         LintId::of(&unicode::ZERO_WIDTH_SPACE),
+        LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS),
+        LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS),
         LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
         LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
         LintId::of(&unwrap::PANICKING_UNWRAP),
@@ -1631,6 +1637,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&types::CAST_REF_TO_MUT),
         LintId::of(&types::UNIT_CMP),
         LintId::of(&unicode::ZERO_WIDTH_SPACE),
+        LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS),
+        LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS),
         LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
         LintId::of(&unwrap::PANICKING_UNWRAP),
     ]);

Diff for: clippy_lints/src/unnamed_address.rs

@@ -0,0 +1,133 @@
+use crate::utils::{match_def_path, paths, span_lint, span_lint_and_help};
+use if_chain::if_chain;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for comparisons with an address of a function item.
+    ///
+    /// **Why is this bad?** Function item address is not guaranteed to be unique and could vary
+    /// between different code generation units. Furthermore different function items could have
+    /// the same address after being merged together.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// type F = fn();
+    /// fn a() {}
+    /// let f: F = a;
+    /// if f == a {
+    ///     // ...
+    /// }
+    /// ```
+    pub FN_ADDRESS_COMPARISONS,
+    correctness,
+    "comparison with an address of a function item"
+}
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for comparisons with an address of a trait vtable.
+    ///
+    /// **Why is this bad?** Comparing trait objects pointers compares an vtable addresses which
+    /// are not guaranteed to be unique and could vary between different code generation units.
+    /// Furthermore vtables for different types could have the same address after being merged
+    /// together.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust,ignore
+    /// let a: Rc<dyn Trait> = ...
+    /// let b: Rc<dyn Trait> = ...
+    /// if Rc::ptr_eq(&a, &b) {
+    ///     ...
+    /// }
+    /// ```
+    pub VTABLE_ADDRESS_COMPARISONS,
+    correctness,
+    "comparison with an address of a trait vtable"
+}
+
+declare_lint_pass!(UnnamedAddress => [FN_ADDRESS_COMPARISONS, VTABLE_ADDRESS_COMPARISONS]);
+
+impl LateLintPass<'_, '_> for UnnamedAddress {
+    fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
+        fn is_comparison(binop: BinOpKind) -> bool {
+            match binop {
+                BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Ge | BinOpKind::Gt => true,
+                _ => false,
+            }
+        }
+
+        fn is_trait_ptr(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+            match cx.tables.expr_ty_adjusted(expr).kind {
+                ty::RawPtr(ty::TypeAndMut { ty, .. }) => ty.is_trait(),
+                _ => false,
+            }
+        }
+
+        fn is_fn_def(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+            if let ty::FnDef(..) = cx.tables.expr_ty(expr).kind {
+                true
+            } else {
+                false
+            }
+        }
+
+        if_chain! {
+            if let ExprKind::Binary(binop, ref left, ref right) = expr.kind;
+            if is_comparison(binop.node);
+            if is_trait_ptr(cx, left) && is_trait_ptr(cx, right);
+            then {
+                span_lint_and_help(
+                    cx,
+                    VTABLE_ADDRESS_COMPARISONS,
+                    expr.span,
+                    "comparing trait object pointers compares a non-unique vtable address",
+                    "consider extracting and comparing data pointers only",
+                );
+            }
+        }
+
+        if_chain! {
+            if let ExprKind::Call(ref func, [ref _left, ref _right]) = expr.kind;
+            if let ExprKind::Path(ref func_qpath) = func.kind;
+            if let Some(def_id) = cx.tables.qpath_res(func_qpath, func.hir_id).opt_def_id();
+            if match_def_path(cx, def_id, &paths::PTR_EQ) ||
+                match_def_path(cx, def_id, &paths::RC_PTR_EQ) ||
+                match_def_path(cx, def_id, &paths::ARC_PTR_EQ);
+            let ty_param = cx.tables.node_substs(func.hir_id).type_at(0);
+            if ty_param.is_trait();
+            then {
+                span_lint_and_help(
+                    cx,
+                    VTABLE_ADDRESS_COMPARISONS,
+                    expr.span,
+                    "comparing trait object pointers compares a non-unique vtable address",
+                    "consider extracting and comparing data pointers only",
+                );
+            }
+        }
+
+        if_chain! {
+            if let ExprKind::Binary(binop, ref left, ref right) = expr.kind;
+            if is_comparison(binop.node);
+            if cx.tables.expr_ty_adjusted(left).is_fn_ptr() &&
+                cx.tables.expr_ty_adjusted(right).is_fn_ptr();
+            if is_fn_def(cx, left) || is_fn_def(cx, right);
+            then {
+                span_lint(
+                    cx,
+                    FN_ADDRESS_COMPARISONS,
+                    expr.span,
+                    "comparing with a non-unique address of a function item",
+                );
+            }
+        }
+    }
+}

Diff for: clippy_lints/src/utils/paths.rs

@@ -3,6 +3,7 @@
 
 pub const ANY_TRAIT: [&str; 3] = ["std", "any", "Any"];
 pub const ARC: [&str; 3] = ["alloc", "sync", "Arc"];
+pub const ARC_PTR_EQ: [&str; 4] = ["alloc", "sync", "Arc", "ptr_eq"];
 pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"];
 pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"];
 pub const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"];
@@ -74,6 +75,7 @@ pub const PATH: [&str; 3] = ["std", "path", "Path"];
 pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"];
 pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];
 pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
+pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
 pub const PTR_NULL: [&str; 2] = ["ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"];
 pub const RANGE: [&str; 3] = ["core", "ops", "Range"];
@@ -90,6 +92,7 @@ pub const RANGE_TO_INCLUSIVE: [&str; 3] = ["core", "ops", "RangeToInclusive"];
 pub const RANGE_TO_INCLUSIVE_STD: [&str; 3] = ["std", "ops", "RangeToInclusive"];
 pub const RANGE_TO_STD: [&str; 3] = ["std", "ops", "RangeTo"];
 pub const RC: [&str; 3] = ["alloc", "rc", "Rc"];
+pub const RC_PTR_EQ: [&str; 4] = ["alloc", "rc", "Rc", "ptr_eq"];
 pub const RECEIVER: [&str; 4] = ["std", "sync", "mpsc", "Receiver"];
 pub const REGEX: [&str; 3] = ["regex", "re_unicode", "Regex"];
 pub const REGEX_BUILDER_NEW: [&str; 5] = ["regex", "re_builder", "unicode", "RegexBuilder", "new"];

Diff for: src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 361] = [
+pub const ALL_LINTS: [Lint; 363] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -623,6 +623,13 @@ pub const ALL_LINTS: [Lint; 361] = [
         deprecation: None,
         module: "misc",
     },
+    Lint {
+        name: "fn_address_comparisons",
+        group: "correctness",
+        desc: "comparison with an address of a function item",
+        deprecation: None,
+        module: "unnamed_address",
+    },
     Lint {
         name: "fn_params_excessive_bools",
         group: "pedantic",
@@ -2408,6 +2415,13 @@ pub const ALL_LINTS: [Lint; 361] = [
         deprecation: None,
         module: "verbose_file_reads",
     },
+    Lint {
+        name: "vtable_address_comparisons",
+        group: "correctness",
+        desc: "comparison with an address of a trait vtable",
+        deprecation: None,
+        module: "unnamed_address",
+    },
     Lint {
         name: "while_immutable_condition",
         group: "correctness",

Diff for: tests/ui/fn_address_comparisons.rs

@@ -0,0 +1,20 @@
+use std::fmt::Debug;
+use std::ptr;
+use std::rc::Rc;
+use std::sync::Arc;
+
+fn a() {}
+
+#[warn(clippy::fn_address_comparisons)]
+fn main() {
+    type F = fn();
+    let f: F = a;
+    let g: F = f;
+
+    // These should fail:
+    let _ = f == a;
+    let _ = f != a;
+
+    // These should be fine:
+    let _ = f == g;
+}

Diff for: tests/ui/fn_address_comparisons.stderr

@@ -0,0 +1,16 @@
+error: comparing with a non-unique address of a function item
+  --> $DIR/fn_address_comparisons.rs:15:13
+   |
+LL |     let _ = f == a;
+   |             ^^^^^^
+   |
+   = note: `-D clippy::fn-address-comparisons` implied by `-D warnings`
+
+error: comparing with a non-unique address of a function item
+  --> $DIR/fn_address_comparisons.rs:16:13
+   |
+LL |     let _ = f != a;
+   |             ^^^^^^
+
+error: aborting due to 2 previous errors
+

Diff for: tests/ui/vtable_address_comparisons.rs

@@ -0,0 +1,42 @@
+use std::fmt::Debug;
+use std::ptr;
+use std::rc::Rc;
+use std::sync::Arc;
+
+#[warn(clippy::vtable_address_comparisons)]
+fn main() {
+    let a: *const dyn Debug = &1 as &dyn Debug;
+    let b: *const dyn Debug = &1 as &dyn Debug;
+
+    // These should fail:
+    let _ = a == b;
+    let _ = a != b;
+    let _ = a < b;
+    let _ = a <= b;
+    let _ = a > b;
+    let _ = a >= b;
+    ptr::eq(a, b);
+
+    let a = &1 as &dyn Debug;
+    let b = &1 as &dyn Debug;
+    ptr::eq(a, b);
+
+    let a: Rc<dyn Debug> = Rc::new(1);
+    Rc::ptr_eq(&a, &a);
+
+    let a: Arc<dyn Debug> = Arc::new(1);
+    Arc::ptr_eq(&a, &a);
+
+    // These should be fine:
+    let a = &1;
+    ptr::eq(a, a);
+
+    let a = Rc::new(1);
+    Rc::ptr_eq(&a, &a);
+
+    let a = Arc::new(1);
+    Arc::ptr_eq(&a, &a);
+
+    let a: &[u8] = b"";
+    ptr::eq(a, a);
+}