-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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)
- Loading branch information
Showing
10 changed files
with
323 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } |
Oops, something went wrong.