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: Unsized types #45

Merged
merged 18 commits into from
Aug 21, 2020
Merged

WIP: Unsized types #45

merged 18 commits into from
Aug 21, 2020

Conversation

alekratz
Copy link
Contributor

@alekratz alekratz commented Aug 5, 2020

See #33

This is currently a WIP implementation of support for unsized types in the GC pointers. Here's a checklist of things I've gotten versus what should probably be added before merging:

  • arbitrary Gc<dyn T> will compile
  • creation of DST by converting a Box<T>
  • CoerceUnsized implementation (nightly-only) for Gc<T> and compilation guards
  • tests for everything above

@Others is there anything else you can think of or that I'm missing from my list?

edit: removed TODOs for implementations of ToScan since it has a blanket impl now
edit: removed TODO for coerce_unsized nightly feature, since it's not needed

Copy link
Owner

@Others Others left a comment

Choose a reason for hiding this comment

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

Few little nits. Hopefully adding the blanket impl resolves having to add a ton of manual implementations.

Also I'm okay just having the nightly feature gate, which we'll have to declare unstable anyway

src/collector/alloc.rs Outdated Show resolved Hide resolved
src/scan/mod.rs Outdated Show resolved Hide resolved
src/scan/std_impls.rs Outdated Show resolved Hide resolved
@alekratz
Copy link
Contributor Author

alekratz commented Aug 5, 2020

I also added a small test to the bottom of smart_ptr/mod.rs that just tries to declare a bunch of empty DSTs, but doesn't do anything with them. I was mostly using this to test that unsized types were allowed in Gc. I don't think they're necessary anymore so I can remove them (it doesn't test anything, it just fails to compile the test if unsized types aren't allowed in Gc). The only reason that it's in there is because I forgot to take it out.

@Others
Copy link
Owner

Others commented Aug 5, 2020

I think that test is fine. Feel free to keep it in.

PR looks pretty good to me now, pending the addition of more tests

@alekratz
Copy link
Contributor Author

alekratz commented Aug 6, 2020

@Others currently having trouble implementing a test that uses Gc<RefCell<dyn T>>, I think because ToScan is not implemented for RefCell<dyn T>? I'm thinking it's impossible to get a &dyn Scan out of a RefCell<dyn ToScan>, since borrowing returns a guarded reference. So this is an edge case for unsized types when they may need internal mutability. What are your thoughts on how to handle this?

This is what I'm working with so far, for reference:

use std::cell::RefCell;
use shredder::*;
use once_cell::sync::Lazy;

static TEST_MUTEX: Lazy<parking_lot::Mutex<()>> = Lazy::new(|| parking_lot::Mutex::new(()));

type GcNode = Gc<RefCell<dyn LinkedListNode>>;

trait LinkedListNode: Scan + ToScan {
    fn label(&self) -> &str;
    fn next(&self) -> Option<GcNode>;
    fn set_next(&mut self, next: Option<GcNode>);
}

#[derive(Debug, Scan)]
struct BlueNode {
    next: Option<GcNode>,
}

impl LinkedListNode for BlueNode {
    fn label(&self) -> &str { "blue" }
    fn next(&self) -> Option<GcNode> { self.next.clone() }
    fn set_next(&mut self, next: Option<GcNode>) { self.next = next; }
}

#[test]
fn unlink_middle() {
    let _guard = TEST_MUTEX.lock();
    run_with_gc_cleanup(|| {
        let head_box: Box<RefCell<dyn LinkedListNode>> = Box::new(RefCell::new(BlueNode {next: None}));
        let head = Gc::from_box(head_box);
        //head.set_next(Box::new(RedNode {next: None}));
    });
}

@Others
Copy link
Owner

Others commented Aug 7, 2020

@alekratz Hmmm. I don't see a solution for this. We can't make a &dyn Scan pointer out of RefCell<dyn LinkedListNode>> because it isn't sized. Once the programmer has an unsized type that doesn't implement ToScan they're screwed....

I think the ToScan solution is only useful if the programmer wants to directly store a dyn T (where T: Scan + ToScan) or a dny T + ToScan. That's still an improvement, but your example shows it's flaw--it doesn't seem to work for wrapped unsized types, and it won't work for slices.

@alekratz
Copy link
Contributor Author

@Others Do you know of any other Rust projects that have custom smart pointers that have support for unsized types without falling back to CoerceUnsized on nightly? It may be worth investigating and stealing borrowing some ideas if such a thing exists.

@alekratz
Copy link
Contributor Author

I was only able to find one smart pointer library that supports ?Sized types after a light search of crates.io, and it appears to be using CoerceUnsized as well. This may just be the only solution for wrapped unsized types and slices like you mentioned.

Here's a link to the rust-lang issue on CoerceUnsized: rust-lang/rust#27732 - some of the discussion seems like it's closer than some other features to be on the track to stabilization.

As far as the tests for this feature go, they still need to be written. I'm planning on keeping the current test I'm writing and just adding a conditional for nightly-only compilation, and then writing out some tests with a structure that can compile on stable as well. It's kind of annoying that we can't really make it look and act like the other smart pointers that Rust itself provides, but at this point I think it's as far as we can get until CoerceUnsized lands in stable.

I have added ?Sized to pretty much all T that is garbage collected and
that needs it. However, there are a few structs defined in a rental!
macro that are causing the build to break due to trasmutation rules.
Enforcing a Sized bound causes the GcRef type to require a Sized T,
which doesn't seem right.
…utex, and RefCell

The implementations of RwLock, Mutex, and RefCell all have special
guards for their borrow() (and related) functions that wrap around their
normal standard library guards. These guards are implemented using the
rental crate, which has a special macro that does some magic behind the
scenes involving transmutation of types - which breaks some type rules
when they're possibly unsized.

This now compiles, and the Gc<RwLock>, Gc<Mutex>, and Gc<RefCell> types
do not have access to the ergonomic GcRef type or their special borrow()
family of methods.
These allow a `Gc<T>` to be coerced to a `Gc<dyn R>` when `T` is some
trait `R`. This requires `std::ops::CoerceUnsized` and
`std::marker::Unsize`, which are nightly features.

TODO: Add compile-time bounds for these functions/imports
Unsized types are able to be created by allocating a Box<T>, converting
it to a Box<dyn MyTrait>, and then moving that pointer to the allocator.

* Add `ToScan` trait which forces the value to be converted to a `&dyn
  Scan`, which is required for using any of the new `from_box` methods.
  Also add `impl_to_scan_safe` macro for convenience
* Add Gc::from_box function for converting the boxed value to a GC'd
  value
* Add GcAllocation::from_box function which does the actual convertion
  from the boxed value to a pointer
* Add Colletor::track_boxed_value which tracks a pointer as if it is a
  raw pointer from a `Box<T>`
* Add DeallocationAction::BoxDrop, which tells the collector tracking to
  convert the associated pointer back to the appropriate `Box<T>` before
  dropping it
CoerceUnsized and Unsize are both nightly features. These are now
guarded at compile time so the library will compile on stable Rust
again.
Signed-off-by: Alek Ratzloff <alekratz@gmail.com>
Since types that are adapted from a boxed pointer are already dropped by
the time dealloc() is called, this would result in a double-free which
is !!!UNSAFE!!! and an obvious bug. Now, the deallocation action is
checked before attempting to call dealloc() on a pointer.
@alekratz
Copy link
Contributor Author

Still have to finish tests (and learn how to use git correctly, apparently). I'll update when I have something to show.

It appears my use of `#[cfg(nightly)]` was futile, since that's not an
actual flag that the compiler uses with nightly code. Basically, users
have to opt-in to nightly features themselves.

Now that this has been fixed, a couple of compile-time errors popped up
that are fixed as well.
@Others
Copy link
Owner

Others commented Aug 14, 2020

It might be good to have a CI step that runs the tests on nightly with nightly features enabled.
I can do that in a follow up PR (since I suspect there will be some jank involved). Just thought I'd note that here.

@alekratz
Copy link
Contributor Author

Small update - I learned recently that the Rust compiler doesn't actually have a nightly flag that gets exported, so you have to allow users to opt-in to that via a Cargo feature. I made a new feature called "nightly-features" and I've employed it where necessary. Still working on tests both for coercion and from_box.

FWIW, I think that from_box and ToScan will be obsolete whenever the CoerceUnsized and Unsized types become stable, so keep that in mind as you move forward.

This was preventing basic operation (i.e. transitivity was not being
upheld). Weird errors for sure.
This doesn't really test anything extensive, it mostly demonstrates how
unsized types may be used with linked lists.
@alekratz
Copy link
Contributor Author

@Others I've added a test for the nightly coercion stuff. I only have a single test that makes sure that dynamic types work as expected when plugged in to a Gc<T> pointer. If there are any other tests that you want to have, I'm happy to implement them.

I'm going to also start working on tests for the ToScan and from_box functions on stable, so I hope to land that soon.

@Others
Copy link
Owner

Others commented Aug 15, 2020

Good stuff! Thanks for pushing on this :)

There is only one test in this file, so there are no race conditions to
guard against.
This test ensures that some `T` that is `SomeTrait: Scan + ToScan` can be
put behind a `Gc<dyn SomeTrait>` pointer and shared that way. This tests
`Gc::from_box`, which is the primary way to access this behavior.
@alekratz
Copy link
Contributor Author

@Others Finally got around to writing a test for creating a Gc<dyn Trait> using from_box and can be run on stable.

@Others
Copy link
Owner

Others commented Aug 20, 2020

Cool! Can you fix the formatting for CI?

Will review tonight

Others
Others previously approved these changes Aug 20, 2020
Copy link
Owner

@Others Others left a comment

Choose a reason for hiding this comment

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

Approved, but you'll need to run cargo fmt before I can merge

Note that AtomicGc may need to pick up some ?Sized bounds. (Although it cannot implement CoerceUnsized.) This can just be added as an issue to the backlog though--not needed for this PR :)

tests/coerce.rs Show resolved Hide resolved
@alekratz
Copy link
Contributor Author

@Others Done. Let me know if there's anything else you need from me.

@Others
Copy link
Owner

Others commented Aug 21, 2020

Approved! Great stuff!

@Others Others merged commit 419a34b into Others:master Aug 21, 2020
@Others
Copy link
Owner

Others commented Aug 21, 2020

Oops forgot to remove the WIP tag when I merged! Still, it's in master now!

@Others Others mentioned this pull request Aug 21, 2020
@Dominilk Dominilk mentioned this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants