ptr::eq: clarify that comparing dyn Trait is fragile#103567

RalfJung · 2022-10-26T09:16:35Z

Also remove the dyn trait example from ptr::eq since those tests are not actually guaranteed to pass due to how unstable vtable comparison is.

Cc @rust-lang/libs-api
Cc discussion following #80505

rustbot · 2022-10-26T09:16:39Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-26T09:16:39Z

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

dtolnay

Thanks!

dtolnay · 2022-10-26T12:15:09Z

@bors r+ rollup

bors · 2022-10-26T12:15:11Z

📌 Commit 99a74af has been approved by dtolnay

It is now in the queue for this repository.

RalfJung · 2022-10-26T12:21:53Z

Looking at #59390 which introduced these examples, we should probably have a line explicitly stating the wide pointer behavior. So I added that. Sorry for the late addition.

@bors r-
r? @dtolnay

dtolnay · 2022-10-26T12:25:35Z

@bors r+

bors · 2022-10-26T12:25:37Z

📌 Commit 1946a18 has been approved by dtolnay

It is now in the queue for this repository.

ptr::eq: clarify that comparing dyn Trait is fragile Also remove the dyn trait example from `ptr::eq` since those tests are not actually guaranteed to pass due to how unstable vtable comparison is. Cc `@rust-lang/libs-api` Cc discussion following rust-lang#80505

Rollup of 10 pull requests Successful merges: - rust-lang#103432 (rustdoc: don't mark Box<T> as Iterator, Read, etc) - rust-lang#103526 (More dupe typos again) - rust-lang#103537 (rustdoc: combine shared CSS between `.*-line-numbers`) - rust-lang#103549 (llvm-16: Don't initialize removed legacy passes) - rust-lang#103558 (Update cargo) - rust-lang#103567 (ptr::eq: clarify that comparing dyn Trait is fragile) - rust-lang#103579 (:arrow_up: rust-analyzer) - rust-lang#103580 (Fix typo in docs for `guaranteed_ne`) - rust-lang#103596 (thread::set_name: debug-assert that things went well) - rust-lang#103598 (rustc_lexer::TokenKind improve docs) Failed merges: - rust-lang#103585 (Migrate source line numbers CSS to CSS variables) r? `@ghost` `@rustbot` modify labels: rollup

rib · 2022-11-05T17:16:23Z

I wonder if some of the documentation changes here for Arc could do with further clarification and it's possible even that these changes make the semantics for Arc::ptr_eq more ambiguous now.

This PR references #80505 which notably appeared to agree that Arc::ptr_eq should return true when two Arcs point to the same allocation that's owned by the same counters.

In other words Arc::ptr_eq shouldn't conceptually compare the pointers to the wrapped values and so the warning about how dyn Trait wide pointers are compared may be misplaced and instead throws into doubt what "allocation" refers to.

Since pull request #80505 was closed (I think just because the patch wasn't updated according to feedback) I recently opened #103763 to keep track of the original issue about the Arc::ptr_eq semantics.

If the same conclusion as from #80505 is confirmed for #103763 then it would probably make sense to further iterate the Arc and Rc documentation to somehow clarify that the ptr::eq semantics for dyn Trait comparisons isn't something that should be a concern for users of Arc::ptr_eq or Rc::ptr_eq.

On the other hand if it's decided that Arc::ptr_eq is supposed to compare the wrapped value pointers (including their meta data) then it could also make sense to explicitly state that it is equivalent to Arc::as_ptr(&a) == Arc::as_ptr(&b) to try remove the uncertainty about what "allocation" refers to and why the semantics of dyn Trait wide pointer comparisons is relevant.

mzji · 2022-12-08T01:36:08Z

library/core/src/ptr/mod.rs

 /// (which is what the `PartialEq for &T` implementation does).
 ///
+/// 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 unrealiable: pointers


typo: unrealiable -> unreliable

Good catch! #105474

Rollup of 10 pull requests Successful merges: - rust-lang#103432 (rustdoc: don't mark Box<T> as Iterator, Read, etc) - rust-lang#103526 (More dupe typos again) - rust-lang#103537 (rustdoc: combine shared CSS between `.*-line-numbers`) - rust-lang#103549 (llvm-16: Don't initialize removed legacy passes) - rust-lang#103558 (Update cargo) - rust-lang#103567 (ptr::eq: clarify that comparing dyn Trait is fragile) - rust-lang#103579 (:arrow_up: rust-analyzer) - rust-lang#103580 (Fix typo in docs for `guaranteed_ne`) - rust-lang#103596 (thread::set_name: debug-assert that things went well) - rust-lang#103598 (rustc_lexer::TokenKind improve docs) Failed merges: - rust-lang#103585 (Migrate source line numbers CSS to CSS variables) r? `@ghost` `@rustbot` modify labels: rollup

ptr::eq: clarify that comparing dyn Trait is fragile

99a74af

rust-highfive assigned thomcc Oct 26, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2022

dtolnay approved these changes Oct 26, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 26, 2022

explicitly mention that both components of wide prts are compared

1946a18

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 26, 2022

rust-highfive assigned dtolnay and unassigned thomcc Oct 26, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2022

compiler-errors mentioned this pull request Oct 26, 2022

Rollup of 11 pull requests #103597

Closed

JohnTitor mentioned this pull request Oct 26, 2022

Rollup of 10 pull requests #103604

Merged

bors merged commit 26ad51f into rust-lang:master Oct 27, 2022

rustbot added this to the 1.66.0 milestone Oct 27, 2022

RalfJung deleted the ptr-eq-dyn-trait branch October 27, 2022 13:07

RalfJung mentioned this pull request Nov 5, 2022

Arc::ptr_eq does not always return "true if the two Arcs point to the same allocation" as documented #103763

Closed

mzji reviewed Dec 8, 2022

View reviewed changes

Amanieu mentioned this pull request Jan 4, 2023

dyn Trait comparison should not include the vtable pointer #106447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ptr::eq: clarify that comparing dyn Trait is fragile#103567