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.

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

Lift T: Sized bounds from some strict_provenance pointer methods #103702

Conversation

WaffleLapkin
Copy link
Member

This PR removes requirement for T (pointee type) to be Sized to call pointer::{addr, expose_addr, with_addr, map_addr}. These functions don't use T's size, so there is no reason for them to require this. Updated public API:

cc @Gankra, #95228
r? libs-api

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 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

@WaffleLapkin WaffleLapkin force-pushed the lift-sized-bounds-from-pointer-methods-where-applicable branch from d42428b to 1f34e11 Compare October 28, 2022 19:36
@ghost
Copy link

ghost commented Nov 6, 2022

This should probably also change the similar methods on NonNull.

@WaffleLapkin
Copy link
Member Author

@H4x5 good catch! Weirdly enough there is no NonNull::expose_addr, but I've removed the bound from all other methods.

@dtolnay
Copy link
Member

dtolnay commented Dec 23, 2022

Reassigning yaahc's reviews as she has stepped down from the review rotation.

r? rust-lang/libs-api

@rustbot rustbot assigned Amanieu and unassigned yaahc Dec 23, 2022
@Amanieu
Copy link
Member

Amanieu commented Dec 29, 2022

This was previously discussed in the original PR here. Is there any specific use case that allowing this would enable?

Alternatively, @Gankra do you have any objections to removing these bounds since you added them in the first place?

I personally can't see any harm in removing these bounds for addr and expose_addr, but doing so for with_addr and map_addr seem much more questionable since we currently have no existing API for manually modifying the address part of a fat pointer without touching the metadata.

@jedel1043
Copy link
Contributor

@Amanieu The gc crate currently uses kind of a linked list of NonNull<dyn Trace> trait objects to track the managed data. Every pointer to a garbage collected object is tagged with a "root" bit to determine if a pointer is currently on the stack or not. It currently uses as usize, but this PR will enable using addr directly:

https://github.com/Manishearth/rust-gc/blob/a243a107694c636d539856495a2ac0341a616428/gc/src/lib.rs#L119-L121

However, there's no way to easily modify the address of a pointer to a dynamic object, so tagging a NonNull<dyn Trace> pointer as rooted is done using a hack:

https://github.com/Manishearth/rust-gc/blob/a243a107694c636d539856495a2ac0341a616428/gc/src/lib.rs#L1001-L1010

Having map_addr would really help in this case.

@WaffleLapkin
Copy link
Member Author

@Amanieu I can't recall the exact use case, but I wanted some of these methods when working on Arc re-implementation.

but doing so for with_addr and map_addr seem much more questionable since we currently have no existing API for manually modifying the address part of a fat pointer without touching the metadata.

That is actually a shame and exposing at least some option here would be really nice (the perfect solution would be to stabilize byte_offset, but anyway...), for example offsetting a pointer to unsized values is required to fully support Arc-like structures that hold a pointer to (DataStructureMeta, T).


In general extending our support of un-Sized values is really valuable IMO since it's currently lacking while it could support a lot of interesting use cases (my opinion, I'm a bit obsessed with DST lol).

@Amanieu Amanieu added I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. and removed I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. labels Jan 13, 2023
@Amanieu
Copy link
Member

Amanieu commented Jan 13, 2023

We already have an (unstable) API for fat pointer manipulation: #81513

I'm tending towards accepting this PR, but I'd like to hear more opinions from the libs-api team on how these APIs interact with each other.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 17, 2023

Discussed in the libs-api meeting. No concerns from us.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit 662f1f2 has been approved by m-ou-se

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 Jan 17, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 18, 2023
…m-pointer-methods-where-applicable, r=m-ou-se

Lift `T: Sized` bounds from some `strict_provenance` pointer methods

This PR removes requirement for `T` (pointee type) to be `Sized` to call `pointer::{addr, expose_addr, with_addr, map_addr}`. These functions don't use `T`'s size, so there is no reason for them to require this. Updated public API:

cc `@Gankra,` rust-lang#95228
r? libs-api
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#103702 (Lift `T: Sized` bounds from some `strict_provenance` pointer methods)
 - rust-lang#106441 (relax reference requirement on SocketAddrExt::from_abstract_name)
 - rust-lang#106718 (finish trait solver skeleton work)
 - rust-lang#106950 (Don't do pointer arithmetic on pointers to deallocated memory)
 - rust-lang#107014 (rustdoc: remove deprecated / unused code from main.js)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d6ea99d into rust-lang:master Jan 18, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 18, 2023
@dtolnay dtolnay removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants