Compare only the data part of fat pointers

[rom1v]

Two fat pointers may point to the same data, but with different vtables (the compiler do not guarantee that vtables are unique).

Such pointers should be considered equal by std::ptr::eq(), so cast them to thin pointers to compare only their data part.

See #48795.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BurntSushi (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[petrochenkov]

Slices are also fat pointers and array[0 .. 10] and array[0 .. 11] are certainly different slices despite the same data pointer.

[rom1v]

@petrochenkov good point, but even in that case, shouldn't they be considered equal by std::ptr::eq()?

[oli-obk]

The docs state

This can be used to compare &T references (which coerce to *const T implicitly) by their address rather than comparing the values they point to (which is what the PartialEq for &T implementation does).

Which does not talk about fat pointers at all, but only about addresses.

The addresses of slices produced by array[0..10] and array[0..11] are the same, so if the result is that of comparing their addresses, then this change is correct imo.

[kennytm]

This is a breaking change. We'll need to crater this before accepting.

cc #36497 (original tracking issue)

[emilyalbini]

Added to the crater queue. There are two other PRs waiting in queue before this one.

@bors try

[bors]

⌛ Trying commit 28feb4c with merge 0a34025aebcc507297260f39eadf2f890a098bed...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[aidanhs]

Crater run started.

[aidanhs]

Hi @pietroalbini (crater requester), @BurntSushi (reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-48814/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

[shepmaster]

Ping from triage, @rust-lang/libs ! We haven't heard from @BurntSushi in a while, so I'm going to randomly reassign to...

r? @withoutboats

[SimonSapin]

As its doc-comments say, the reason ptr::eq exist is to make it easier (in terms of type inference and implicit coercion) to call <*const _ as PartialEq>::eq. By definition it should be implemented as a == b, I’m opposed to changing that.

If the meaning of raw pointer equality is to be changed, it should be changed in == / PartialEq (with std::ptr::eq being “naturally” affected as well). This would arguably also be a decision for @rust-lang/lang, not just @rust-lang/libs.

Or maybe a less disruptive change would be to add a new API? (Potentially as a method rather than a free function.)

[joshtriplett]

I don't think this makes sense, especially not with the change to slice behavior.

[mbrubeck]

This makes the implementation of Hash for *const T incorrect (c.f. #45483). It would require a corresponding change to that impl.

Edit: Oops, my comment above was incorrect. I mistakenly thought this was changing PartialEq::eq rather than just core::ptr::eq.

[shepmaster]

Ping from triage, @rust-lang/libs !

We haven't heard from @BurntSushi in a while, so I'm going to randomly reassign to...

We haven't heard from @withoutboats in a while, so I'm going to deliberately reassign to...

r? @SimonSapin

[SimonSapin]

I’ve already said how the PR as-is is insufficient if we want to make any change here. I hear the argument that the current behavior is broken, but changing the meaning of equality for a primitive type seems like a significant enough change that it should probably go through the RFC process.

@rom1v, how do you feel about closing this PR for now and writing an RFC?

[rom1v]

how do you feel about […] writing an RFC?

I think this is the right thing to do. 👍

However, I never did this and have currently no time to get into this.

If someone is interested in writing an RFC, please go ahead. Otherwise, if nothing happens for too long, I will possibly work on it in the future…

[shepmaster]

how do you feel about […] writing an RFC?

I think this is the right thing to do. 👍

Given this, I'm going to go ahead and close this PR for now to keep things tidy. Thank you very much for your hard work, @rom1v !

[rom1v]

In fact, comparing the data part only would be unsound in some cases:

structX{}implSomeTraitforX{ ... }structY{x:X}implSomeTraitforY{ ... }let y = Y{x:X{}}let a = &y as*constSomeTrait;let b = &y.xas*constSomeTrait;println!("{:?} == {:?} : {}", a, b, a == b);

(from here)

Here, a and b have the same data part, but they should not be equal. The current behavior is correct:

0x7ffe1131c0e0 == 0x7ffe1131c0e0 : false