Incorrect suggestion for ptr_eq warning leads to vtable_address_comparisons error

[andersk]

Suppose we start with the following code:

use std::rc::Rc;
pub trait Trait {}
pub fn eq(a: Rc<dyn Trait>, b: Rc<dyn Trait>) -> bool {
    Rc::ptr_eq(&a, &b)
}

error: comparing trait object pointers compares a non-unique vtable address
 --> src/lib.rs:4:5
  |
4 |     Rc::ptr_eq(&a, &b)
  |     ^^^^^^^^^^^^^^^^^^
  |
  = note: `#[deny(clippy::vtable_address_comparisons)]` on by default
  = help: consider extracting and comparing data pointers only
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons

Okay, let’s do that:

pub fn eq(a: Rc<dyn Trait>, b: Rc<dyn Trait>) -> bool {
    &*a as *const dyn Trait as *const u8 == &*b as *const dyn Trait as *const u8
}

warning: use `std::ptr::eq` when comparing raw pointers
 --> src/lib.rs:4:5
  |
4 |     &*a as *const dyn Trait as *const u8 == &*b as *const dyn Trait as *const u8
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: try: `std::ptr::eq(&*a as *const dyn Trait, &*b as *const dyn Trait)`
  |
  = note: `#[warn(clippy::ptr_eq)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq

warning: 1 warning emitted

This suggestion is wrong. The cast to *const u8 is important, and removing it has led back to the first error:

pub fn eq(a: Rc<dyn Trait>, b: Rc<dyn Trait>) -> bool {
    std::ptr::eq(&*a as *const dyn Trait, &*b as *const dyn Trait)
}

error: comparing trait object pointers compares a non-unique vtable address
 --> src/lib.rs:4:5
  |
4 |     std::ptr::eq(&*a as *const dyn Trait, &*b as *const dyn Trait)
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[deny(clippy::vtable_address_comparisons)]` on by default
  = help: consider extracting and comparing data pointers only
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons

Meta

• cargo clippy -V: clippy 0.0.212 (0b644e4 2020-12-26)
• rustc -Vv:

  rustc 1.51.0-nightly (0b644e419 2020-12-26)
  binary: rustc
  commit-hash: 0b644e419681835bd0f5871c3bfbd648aa04f157
  commit-date: 2020-12-26
  host: x86_64-unknown-linux-gnu
  release: 1.51.0-nightly
  

[camsteffen]

Okay, let’s do that:

Do what? Clippy didn't make a suggestion.

The lint docs say:

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.

Does this not apply to your example?

[andersk]

Do what? Clippy didn't make a suggestion.

Clippy’s suggestion for Rc::ptr_eq(&a, &b) was “consider extracting and comparing data pointers only”. That is a good suggestion (at least until rust-lang/rust#80505 is merged), and that is what I did (correctly) in &*a as *const dyn Trait as *const u8 == &*b as *const dyn Trait as *const u8. But then Clippy made a further incorrect suggestion for that.

[camsteffen]

Okay, so there are two issues here. Correct me if I'm mistaken in any way since I am not very comfortable with pointers in Rust.

1. The clippy::vtable_address_comparisons lint is correct now, but it is no longer correct once Ignore vtables in {Rc, Arc, Weak}::ptr_eq rust#80505 is merged.
2. The clippy::ptr_eq lint should not fire if the pointer is cast to any type other than usize.

[andersk]

1. The clippy::vtable_address_comparisons lint is correct now, but it is no longer correct once rust-lang/rust#80505 is merged.

Right; I’ll open a separate issue for that when it becomes relevant.

2. The clippy::ptr_eq lint should not fire if the pointer is cast to any type other than usize.

The ideal behavior may be a little more context-dependent: we can suggest replacing ref-to-pointer casts with ptr::eq, but not fat-pointer-to-thin-pointer casts:

fn f(a: &u8, b: &u8) -> bool {
    a as *const u8 == b as *const u8
    // should suggest std::ptr::eq(a, b)
}
fn g(a: *const dyn Trait, b: *const dyn Trait) -> bool {
    a as *const u8 == b as *const u8
    // should not suggest std::ptr::eq(a, b)
}

We probably need to exclude all pointer-to-pointer casts, since some of them currently lead to a suggestion that doesn’t even type-check:

pub fn h(a: *const u16, b: *const u32) -> bool {
    a as *const u8 == b as *const u8
    // should not suggest str::ptr::eq(a, b)
}

[camsteffen]

Right; I’ll open a separate issue for that when it becomes relevant.

Great!

we can suggest replacing ref-to-pointer casts with ptr::eq, but not fat-pointer-to-thin-pointer casts

That should be possible.

[MarijnS95]

Seeing that this hasn't received a reply since 2021, it looks like the intent of rust-lang/rust#80505 has now been merged as:

rust-lang/rust#103763
rust-lang/rust#106450

Thus, this lint listed at https://rust-lang.github.io/rust-clippy/master/index.html#/vtable_address_comparisons is now stale/wrong (since Rust 1.72) for Rc and Arc. It is also no longer raised because that PR already updated the clippy condition to only trigger on raw core::ptr::eq(). Can the example be updated to reflect this?

Also thanks @rib for back-linking to this issue, otherwise I probably wouldn't have found that it got fixed in 1.72 and ended up in a wild chase implementing target_ptr comparison by hand 😬

[stevenengler]

I think this issue can be closed now since vtable_address_comparisons has been removed as of a2ea760? It seems like the rust ambiguous_wide_pointer_comparisons lint is the approximate replacement, and it does not trigger on Rc::ptr_eq or Arc::ptr_eq.