diff --git a/CHANGELOG.md b/CHANGELOG.md index 81a1e276a880..f3b1073988b0 100644 --- a/CHANGELOG.md +++ b/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 --git a/README.md b/README.md index fefbb3486fdc..856058e58b02 100644 --- a/README.md +++ b/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 --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 73c05607b0fb..5d5b5d9a6da5 100644 --- a/clippy_lints/src/lib.rs +++ b/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 --git a/clippy_lints/src/unnamed_address.rs b/clippy_lints/src/unnamed_address.rs new file mode 100644 index 000000000000..b6473fc594ee --- /dev/null +++ b/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 = ... + /// let b: Rc = ... + /// 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 --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 6cb1f694fd5e..4a4ee5baf002 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/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 --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 5777e7d90e58..fa51af156efd 100644 --- a/src/lintlist/mod.rs +++ b/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 --git a/tests/ui/fn_address_comparisons.rs b/tests/ui/fn_address_comparisons.rs new file mode 100644 index 000000000000..362dcb4fd80c --- /dev/null +++ b/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 --git a/tests/ui/fn_address_comparisons.stderr b/tests/ui/fn_address_comparisons.stderr new file mode 100644 index 000000000000..9c1b5419a431 --- /dev/null +++ b/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 --git a/tests/ui/vtable_address_comparisons.rs b/tests/ui/vtable_address_comparisons.rs new file mode 100644 index 000000000000..c91d96ee18a3 --- /dev/null +++ b/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 = Rc::new(1); + Rc::ptr_eq(&a, &a); + + let a: Arc = 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); +} diff --git a/tests/ui/vtable_address_comparisons.stderr b/tests/ui/vtable_address_comparisons.stderr new file mode 100644 index 000000000000..76bd57217d78 --- /dev/null +++ b/tests/ui/vtable_address_comparisons.stderr @@ -0,0 +1,83 @@ +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:12:13 + | +LL | let _ = a == b; + | ^^^^^^ + | + = note: `-D clippy::vtable-address-comparisons` implied by `-D warnings` + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:13:13 + | +LL | let _ = a != b; + | ^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:14:13 + | +LL | let _ = a < b; + | ^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:15:13 + | +LL | let _ = a <= b; + | ^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:16:13 + | +LL | let _ = a > b; + | ^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:17:13 + | +LL | let _ = a >= b; + | ^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:18:5 + | +LL | ptr::eq(a, b); + | ^^^^^^^^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:22:5 + | +LL | ptr::eq(a, b); + | ^^^^^^^^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:25:5 + | +LL | Rc::ptr_eq(&a, &a); + | ^^^^^^^^^^^^^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: comparing trait object pointers compares a non-unique vtable address + --> $DIR/vtable_address_comparisons.rs:28:5 + | +LL | Arc::ptr_eq(&a, &a); + | ^^^^^^^^^^^^^^^^^^^ + | + = help: consider extracting and comparing data pointers only + +error: aborting due to 10 previous errors +