Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Oct 27, 2022
Merged

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

merged 2 commits into from
Oct 27, 2022

Conversation

RalfJung
Copy link
Member

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 rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 26, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2022

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
Copy link
Collaborator

r? @thomcc

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2022
dtolnay
dtolnay approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Oct 26, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 26, 2022

📌 Commit 99a74af has been approved by dtolnay

It is now in the queue for this repository.

@bors 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
@RalfJung
Copy link
Member Author

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

@bors 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 rust-highfive assigned dtolnay and unassigned thomcc Oct 26, 2022
@dtolnay
Copy link
Member

dtolnay commented Oct 26, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2022

📌 Commit 1946a18 has been approved by dtolnay

It is now in the queue for this repository.

@bors 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 added a commit to compiler-errors/rust that referenced this pull request Oct 26, 2022
@compiler-errors
Rollup merge of rust-lang#103567 - RalfJung:ptr-eq-dyn-trait, r=dtolnay
f619f01 
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
@compiler-errors compiler-errors mentioned this pull request Oct 26, 2022
Closed
@JohnTitor JohnTitor mentioned this pull request Oct 26, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 26, 2022
@bors
Auto merge of rust-lang#103604 - JohnTitor:rollup-q4ns2gh, r=JohnTitor
df57f7b 
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
@bors bors merged commit 26ad51f into rust-lang:master Oct 27, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 27, 2022
@RalfJung RalfJung deleted the ptr-eq-dyn-trait branch October 27, 2022 13:07
@RalfJung RalfJung mentioned this pull request Nov 5, 2022
Closed
@rib
Copy link

rib commented Nov 5, 2022

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
mzji reviewed Dec 8, 2022
View reviewed changes
library/core/src/ptr/mod.rs
@@ -1733,6 +1733,12 @@ pub(crate) unsafe fn align_offset<T: Sized>(p: *const T, a: usize) -> usize {
/// by their address rather than comparing the values they point to
/// (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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: unrealiable -> unreliable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! #105474

@Amanieu Amanieu mentioned this pull request Jan 4, 2023
Closed
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#103604 - JohnTitor:rollup-q4ns2gh, r=JohnTitor
1e0b113 
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
pietroalbini added a commit to ferrocene/sphinx-shared-resources that referenced this pull request Mar 22, 2023
@bors-ferrocene @Veykril
@pietroalbini @DropDemBits @bors
Merge #600
3f42cd2 
600: Pull changes from upstream `master` r=kirtchev-adacore a=pietroalbini

* rust-lang/rust#103605
* rust-lang/rust#103604
  * rust-lang/rust#103598
  * rust-lang/rust#103596
  * rust-lang/rust#103580
  * rust-lang/rust#103579
  * rust-lang/rust#103567
  * rust-lang/rust#103558
  * rust-lang/rust#103549
  * rust-lang/rust#103537
  * rust-lang/rust#103526
  * rust-lang/rust#103432
* rust-lang/rust#103571
* rust-lang/rust#103492
* rust-lang/rust#103572
  * rust-lang/rust#103554
  * rust-lang/rust#103546
  * rust-lang/rust#103543
  * rust-lang/rust#103428
  * rust-lang/rust#102706
  * rust-lang/rust#95710
* rust-lang/rust#103284
* rust-lang/rust#103562
  * rust-lang/rust#103542
  * rust-lang/rust#103536
  * rust-lang/rust#103533
  * rust-lang/rust#103520
  * rust-lang/rust#103444
  * rust-lang/rust#103430
  * rust-lang/rust#103416
  * rust-lang/rust#103287
  * rust-lang/rust#103209
  * rust-lang/rust#102951
* rust-lang/rust#103279
* rust-lang/rust#103158



Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
Co-authored-by: Pietro Albini <pietro.albini@ferrous-systems.com>
Co-authored-by: DropDemBits <r3usrlnd@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Pietro Albini <pietro@pietroalbini.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzji mzji mzji left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
8 participants
@RalfJung @rustbot @rust-highfive @dtolnay @bors @rib @mzji @thomcc