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

std::rc::Rc should accept DST #18248

Closed
little-arhat opened this issue Oct 23, 2014 · 14 comments
Closed

std::rc::Rc should accept DST #18248

little-arhat opened this issue Oct 23, 2014 · 14 comments
Labels
A-DSTs Area: Dynamically-sized types (DSTs)

Comments

@little-arhat
Copy link

It's currently impossible to store DST (Trait object, for example) inside RC:

let x:std::rc::Rc<Reader>;

gives you: the trait 'core::kinds::Sized' must be implemented because it is required by 'alloc::rc::Rc'. However, you can store trait object inside Box. Rc uses Box internally, so it seems inconsistent that one cannot use refcounted box as replacement for Box.

One could work around this issue by storing Box<Trait> inside Rc, but that would incur double indirection.

Moreover, one couldn't implement their own Rc because of related ICE: #17959 .

(Maybe related to: #16918 )

@brandonson
Copy link
Contributor

No idea whether we even want to change things before #16918 gets resolved, but I'll work on this just to give me an excuse to dig into the libs again.

@little-arhat
Copy link
Author

@brandonson FYI, I've tried to fix it by changing RcBox to:

struct RcBox<Sized? T> {
    strong: Cell<uint>,
    weak: Cell<uint>,
    value T
}

but that didn't work out because of ICE from #17959 .

@brandonson
Copy link
Contributor

Quick update, my attempts have halted for the moment. The way I did it appears to have made compiling std cause rustc to enter an infinite loop, and I had added an extra pointer in any case. We may just want to wait for that ICE to be fixed.

@steveklabnik
Copy link
Member

This also impacts using closures, since they're trait objects.

@milibopp
Copy link
Contributor

Shouldn't this include std::sync::Arc? Or would that be a separate PR?

@steveklabnik
Copy link
Member

It could be the same, the root cause is the same.

@nrc
Copy link
Member

nrc commented Mar 16, 2015

There are two pieces missing here: we need coercions of deeply DST objects in order to create Rc<T> and we need to handle drop - the latter uses a bunch of functions, mainly size_of which don't accept ?Sized type params. The rest is just adding ?Sized in a bunch of places (which I have on a branch).

I'm working on spec'ing then implementing the DST coercions atm. I guess #19063 will cover everything that needs doing to implement drop, but I haven't checked that.

Arc has exactly the same issues as Rc (drop and coercions).

@japaric
Copy link
Member

japaric commented Apr 2, 2015

FWIW, I have an out of tree implementation of Rc that works with DST. I've mainly tested Rc<str> and Rc<Fn(A, B) -> C> and valgrind seems to be happy with it.

My implementation only supports strong references [1] and uses a different data layout than the one in libstd, which results in a simple implementation of the constructor and the destructor. I have also documented the (re)allocations that each constructor/conversion (e.g. String -> Rc<str>) may require.

[1] I didn't implement weak references because (I think) I don't need them (yet) for what I'm using the Rc for (I don't have to deal with cyclic structures, or have RefCells in my code which AFAIK are required to create them)

@huonw
Copy link
Member

huonw commented Apr 2, 2015

@japaric cool! Although, fwiw, that sort of pointer (with separate allocation for the refcounts) almost exists today via Rc<Box<T>>.

@japaric
Copy link
Member

japaric commented Apr 2, 2015

that sort of pointer (with separate allocation for the refcounts) almost exists today via Rc<Box>.

Yes, but that one has double indirection, it's like Box<Box<Trait>> and Box<String>. Each call to deref has to go through one layer of indirection.

@huonw
Copy link
Member

huonw commented Apr 2, 2015

Yeah, hence "almost". :)

@ftxqxd
Copy link
Contributor

ftxqxd commented May 15, 2015

I believe this was fixed in #24619.

@alexcrichton
Copy link
Member

Yay!

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 3, 2017

There is a FIXME related to this issue,
https://github.com/rust-lang/rust/blob/master/src/libcore/tests/hash/mod.rs#L72
Now that the issue is closed can the FIXME be fix, or made more specific?

nivkner added a commit to nivkner/rust that referenced this issue Sep 30, 2017
remove FIXME(rust-lang#13101) since `assert_receiver_is_total_eq` stays.
remove FIXME(rust-lang#19649) now that stability markers render.
remove FIXME(rust-lang#13642) now the benchmarks were moved.
remove FIXME(rust-lang#6220) now that floating points can be formatted.
remove FIXME(rust-lang#18248) and write tests for `Rc<str>` and `Rc<[u8]>`
remove reference to irelevent issues in FIXME(rust-lang#1697, rust-lang#2178...)
update FIXME(rust-lang#5516) to point to getopts issue 7
update FIXME(rust-lang#7771) to point to RFC 628
update FIXME(rust-lang#19839) to point to issue 26925
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-DSTs Area: Dynamically-sized types (DSTs)
Projects
None yet
Development

No branches or pull requests

10 participants