Add lint against ambiguous wide pointer comparisons

[Urgau]

This PR is the resolution of #106447 decided in #117717 by T-lang.

ambiguous_wide_pointer_comparisons

warn-by-default

The ambiguous_wide_pointer_comparisons lint checks comparison of *const/*mut ?Sized as the operands.

Example

let ab = (A, B);
let a = &ab.0 as *const dyn T;
let b = &ab.1 as *const dyn T;

let _ = a == b;

Explanation

The comparison includes metadata which may not be expected.

---

This PR also drops clippy::vtable_address_comparisons which is superseded by this one.

One thing: is the current naming right? invalid seems a bit too much.

Fixes #117717

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[asquared31415]

#117717 says that the lint should be on "non-thin pointers" where #106447 (comment) says specifically for "dyn trait pointer comparison".

I believe that the intent was only to lint on dyn trait, because it could be useful to compare the metadata portion of slice raw pointers. This does appear to be what is implemented, but I think it would be good to get clarification on exactly what was agreed on.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[asquared31415]

Naming bikeshed since you mentioned that "invalid" seems strong: unreliable_dyn_pointer_comparisons.

I think the name should communicate that the comparison may spuriously return true or false and cannot be relied upon for correctness.

[Urgau]

@scottmcm could you clarify the unclear-ness around the lint intention #117758 (comment) ?

As far as naming goes, it will depend on an answer to the previous question.

[scottmcm]

Hmm, I think my personal vision of this was something like unclear_fat_pointer_comparisons that would lint on *const T for any T: ?Sized too, even without knowing what specific kind of DST it was. My thought was that even if you did intend to compare both (say because it's a slice and that's what you meant), it might still be good to use ptr::eq to help emphasize that you're not accidentally comparing the length too (like say if you forgot it was a slice pointer and not an item pointer).

But I'm not sure if that's what other people from the triage meeting were thinking. cc @Amanieu @rust-lang/lang @rust-lang/lang-advisors so others can chime in if they were thinking that this would check dyn only.

(Do people frequently compare slice pointers as a way of checking the length too? I don't know that I've ever seen code doing that, vs working in https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.len + data pointer separately...)

[asquared31415]

The Reference says

When comparing raw pointers they are compared by their address, rather than by what they point to. When comparing raw pointers to dynamically sized types they also have their additional data compared.

The following code also executes without panicking:

fn main() {
    let arr = [1_u32, 2, 3, 4];
    let ptr = arr.as_ptr();
    let p1 = core::ptr::slice_from_raw_parts(ptr, 4);
    let p2 = core::ptr::slice_from_raw_parts(ptr, 3);
    // addresses are equal
    assert!(p1.cast::<()>() as usize == p2.cast::<()>() as usize);
    // pointers are not equal
    assert!(p1 != p2);
}

I think that it's reasonable to rely on the metadata being compared. The issue arises because *const dyn Trait metadata isn't consistent, so when the metadata is compared, the comparison might be meaningless for such types.

[asquared31415]

Also there's not a stable way to get the length metadata of a slice pointer without asserting validity as a reference to 0 bytes (you can make a &[()] and access its length but only if the pointer is to a valid object), so I doubt that many users are using that method that you linked at all.

[scottmcm]

I think the name should communicate that the comparison may spuriously return true or false and cannot be relied upon for correctness.

Hmm, so I specifically wasn't thinking that this lint would be about "hey, vtable metadata comparison is unreliable", because if that's the goal, it needs to lint calls to https://doc.rust-lang.org/std/ptr/fn.eq.html too. But ptr::eq is explicitly documented to talk about that unreliability:

When comparing wide pointers, both the address and the metadata are tested for equality. However, note that comparing trait object pointers (*const dyn Trait) is unreliable: pointers to values of the same underlying type can compare inequal (because vtables are duplicated in multiple codegen units), and pointers to values of different underlying type can compare equal (since identical vtables can be deduplicated within a codegen unit).

And a "hey, vtable comparison doesn't do what you probably want" feels more like a clippy lint to me, because the only (stable) way past it is to #[allow] it if that is what you meant to do.

I see this lint as being about saying "hey, that's a fat pointer, and we know that fat pointer comparison can be surprising. Please think about what you meant -- maybe you meant to look at the metadata too, maybe you only meant the data address -- and use the function that corresponds to that so it's clear to the reader. We're fine with whichever one you meant, but would like you to clarify."

(Sortof like the direction we're going for things like u32::max_value as u64, where we want to be able to say "that's sketchy, please say which thing you meant, but if it's the address of the function as a u64 that's fine, we just want it to be obvious to the reader that that's actually what you meant".)

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[Urgau]

Hmm, I think my personal vision of this was something like unclear_fat_pointer_comparisons that would lint on *const T for any T: ?Sized

In the light of this, I've adjusted the lint accordingly and renamed it as you suggested.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[Urgau]

@davidtwco Since you reviewed this PR, the scope of the lint grew a bit to lint on all unsized pointers as such the implementation changed quite a bit, in particular the second commit 5630540.

I would appreciate it if you could review those changes or delegate me bors permissions for this PR.

[davidtwco]

@bors r+

[bors]

📌 Commit 5e1bfb5 has been approved by davidtwco

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5e1bfb5 with merge 8a37655...

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 8a37655 to master...

[rust-timer]

Finished benchmarking commit (8a37655): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.2%, 3.6%]	2
Improvements ✅ (primary)	-5.6%	[-5.6%, -5.6%]	1
Improvements ✅ (secondary)	-1.7%	[-2.1%, -1.3%]	2
All ❌✅ (primary)	-5.6%	[-5.6%, -5.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.24s -> 672.509s (0.19%)
Artifact size: 314.20 MiB -> 314.21 MiB (0.00%)

[notriddle]

This seems to be failing CI for unrelated pull requests?

#118402 (comment)

#118623 (comment)

#118833 (comment)

Testing GCC stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished release [optimized] target(s) in 1.43s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --no-default-features --mini-tests --std-tests`
Using system GCC
Using system GCC
error: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
##[error]   --> example/mini_core.rs:344:9
    |
    |
344 |         *self == *other
    |
    |
    = note: `-D ambiguous-wide-pointer-comparisons` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(ambiguous_wide_pointer_comparisons)]`
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
344 |         std::ptr::addr_eq(*self, *other)
help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
    |
344 |         std::ptr::eq(*self, *other)

error: ambiguous wide pointer comparison, the comparison includes metadata which may not be expected
##[error]   --> example/mini_core.rs:347:9
    |
    |
347 |         *self != *other
    |
help: use `std::ptr::addr_eq` or untyped pointers to only compare their addresses
    |
    |
347 |         !std::ptr::addr_eq(*self, *other)
help: use explicit `std::ptr::eq` method to compare metadata and addresses
    |
    |
347 |         !std::ptr::eq(*self, *other)

error: aborting due to 2 previous errors

[RalfJung]

Testing GCC stage1

@GuillaumeGomez @antoyo any idea why this would land but then start failing?

[GuillaumeGomez]

@Urgau proposed a theory about this here. I'm still surprised though.