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

Prevent double boxing in boxed #511

Closed
stepancheg opened this issue Jun 10, 2017 · 9 comments
Closed

Prevent double boxing in boxed #511

stepancheg opened this issue Jun 10, 2017 · 9 comments

Comments

@stepancheg
Copy link
Contributor

stepancheg commented Jun 10, 2017

future.boxed() creates a new box even if future itself is already a Box<Future>.

This can be inconvenient in this use case:

type SomeFuture = ...;
fn make_future() -> SomeFuture { ... }

fn accept_future<F : Future>(f: F) {
    f.boxed() ...
}

accept_future(make_future());

If SomeFuture is an alias for Box<Future> then accept_future makes a Box<Box<Future>>. Which could be worked around (e. g. by creating another function like

fn accept_future_boxed(future: Box<Future>) { ... }

but that would be inconvenient, and still won't prevent accidental wrapping futures in double boxes.

boxed could implemented like this:

fn boxed<F : Future + Sized + 'static>(f: F) -> Box<Future> {
    if TypeId::of::<F>() == TypeId::of::<Box<Future>>() {
        assert!(mem::size_of::<F>() == mem::size_of::<Box<Future>>());
        unsafe {
            let mut r: Box<Future> = mem::uninitialized();
            ptr::copy_nonoverlapping(&f as *const F, &mut r as *mut Box<Future> as *mut u8 as *mut F, 1);
            mem::forget(f);
            r
        }
    } else {
        Box::new(f)
    }
}

PR #512 implements boxed in Future and Stream this way.

@stepancheg stepancheg changed the title Implement boxed in a way it prevents double boxing Prevent double boxing in boxed Jun 10, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
@3Hren
Copy link

3Hren commented Jun 11, 2017

This possibly can be implemented without RTTI using something like Boxed trait using specializations.

@stepancheg
Copy link
Contributor Author

This possibly can be implemented without RTTI using something like Boxed trait using specializations.

First, TypeId::of is not RTTI, because function parameter is a type, not a value. It is compile time type indentification, and condition is optimized away by compiler.

Second, if you know how to implement it with Boxed trait, please show me how. I tried, and failed.

@3Hren
Copy link

3Hren commented Jun 11, 2017

First, TypeId::of is not RTTI, because function parameter is a type, not a value. It is compile time type indentification, and condition is optimized away by compiler.

I'm not sure (but likely so). Docs/ASM proof?

Second, if you know how to implement it with Boxed trait, please show me how. I tried, and failed.

If I understood the issue correctly, here is the proof of concept: #513.

@3Hren
Copy link

3Hren commented Jun 11, 2017

Your tests from #512 passes. Decided not to copy/paste them nevertheless.

@stepancheg
Copy link
Contributor Author

I'm not sure (but likely so). Docs/ASM proof?

https://godbolt.org/g/JdxAes

If I understood the issue correctly, here is the proof of concept: #513.

Thanks. It doesn't work in stable though.

stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 11, 2017
@Kixunil
Copy link
Contributor

Kixunil commented Jun 12, 2017

Specialisation would be much nicer.

@stepancheg
Copy link
Contributor Author

@Kixunil it doesn't work in stable.

Anyway, it doesn't matter how it is implemented internally, from user point view nothing will change when in couple of months implementation will be changed to specialization.

@Kixunil
Copy link
Contributor

Kixunil commented Jun 12, 2017

@stjepang I know. I feel like everything is blocked on specialisation these days... I agree with implementing it with unsafe now and changing it later.

stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 13, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 13, 2017
stepancheg added a commit to stepancheg/futures-rs that referenced this issue Jun 13, 2017
@alexcrichton
Copy link
Member

Closing due to #512 (comment)

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

No branches or pull requests

4 participants