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

[WIP] Experiment with a safe adoption API #47

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open

Conversation

lopopolo
Copy link
Member

This PR experiments with a safe Adopt::adopt API.

The API requires that the inner T of and Rc<T> implement a new trait, Trace. Trace has a single required method: Trace::mark. Rc::<T>::adopt is only available when T impls Trace.

Trace::mark allows a T to use internal iteration to mark all of the Rc<T>s that are immediately reachable. Rc::<T>::adopt determines whether an adoption of other is safe based on whether T yields a &mut Rc<T> that points to the same allocation as the given other Rc to adopt.

By yielding a &mut Rc<T>, T proves that it owns an Rc<T> that points to the same allocation as other, which satisfies the safety invariants of Adopt::adopt_unchecked.

Practically, Ts that implement Trace must use some sort of interior mutability, since Trace::mark takes &self.

The doubly linked list integration test is updated to use this new safe adopt API and impls Trace. The orphan rule for trait impls complicates implementing Trace for Rc<RefCell<T>> and requires the introduction of a wrapper type. The doubly linked list test now has #![forbid(unsafe_code)].

r? @tummychow

@lopopolo lopopolo added A-adopt Area: Adopt and unadopt API. S-wip Status: This pull request is a work in progress. A-unsafe-code Area: Unsafe code, be careful when reviewing. labels Jun 15, 2021
src/adopt.rs Show resolved Hide resolved
src/adopt.rs Outdated
@@ -34,6 +35,14 @@ mod sealed {
/// [`adopt_unchecked`]: Adopt::adopt_unchecked
/// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
pub unsafe trait Adopt: sealed::Sealed {
Copy link
Member Author

Choose a reason for hiding this comment

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

adopt's trait-level documentation will need to be significantly reworked.

src/trace.rs Outdated
/// TODO: document me!
fn yield_owned_rcs<F>(&self, mark: F)
where
F: for<'a> FnMut(&'a mut Rc<Self>);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if the higher ranked lifetime bound is correct here. Do we want to enforce that the lifetime of the yielded &mut Rc<T> is the same as &self? Removing the for<'a> would disallow non-static T that have &mut Rc<T> fields which would be a good thing.

src/adopt.rs Outdated
type Inner = T;

/// TODO: document me!
fn adopt(this: &mut Self, other: &Self)
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe adopt should return Result<(), &Self> where the error variant contains other if the adoption failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe there is try_adopt and adopt which unwraps try_adopt.

Copy link

Choose a reason for hiding this comment

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

  1. The E type in Result should implement Error, otherwise it won't compose well with existing error-handling mechanisms, such as the ? sugar.
  2. try_adopt that returns a Result and adopt that unwraps the former is indeed a good, ergonomic API. The standard library does that a lot too, I think.
  3. Why would you want to return other in case of a failure, if it's a reference and the callee already has it anyway?

src/adopt.rs Show resolved Hide resolved
…_rcs

This ensures that yielded Rcs have the same lifetime as self, which is what
we want to assert that self owns the Rc.
@LoganDark

This comment was marked as off-topic.

@lopopolo

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-adopt Area: Adopt and unadopt API. A-unsafe-code Area: Unsafe code, be careful when reviewing. S-wip Status: This pull request is a work in progress.
Development

Successfully merging this pull request may close these issues.

3 participants