Make custom trait object for `Future` generic #51944

MajorBreakfast · 2018-06-30T15:36:45Z

TaskObj -> FutureObj<'static, ()>
The impl From<...> for FutureObj<'a, T> impls are impossible because of the type parameter T. The impl has to live in libstd, but FutureObj<'a, T> is from libcore. Therefore Into<FutureObj<'a, T>> was implemented instead. Edit: This didn‘t compile without warnings. I am now using non-generic Form impls.

Edit: Added lifetime

ngg · 2018-06-30T16:17:17Z

I get the following warnings for the Into implementations:

warning: conflicting implementations of trait `core::convert::Into<core::task::FutureObj<_>>` for type `boxed::PinBox<_>`: (E0119)
   --> liballoc/boxed.rs:953:1
    |
953 | impl<T, F: Future<Output = T> + Send + 'static> Into<FutureObj<T>> for PinBox<F> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: #[warn(incoherent_fundamental_impls)] on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46205 <https://github.com/rust-lang/rust/issues/46205>
    = note: conflicting implementation in crate `core`:
            - impl<T, U> core::convert::Into<U> for T
              where U: core::convert::From<T>;
    = note: downstream crates may implement trait `core::convert::From<boxed::PinBox<_>>` for type `core::task::FutureObj<_>`

Can't we simply remove that impl?

MajorBreakfast · 2018-06-30T17:12:07Z

@nikomatsakis
@aturon
@withoutboats

Can you help? I'm not sure what to do. I can neither implement From nor Into

MajorBreakfast · 2018-06-30T17:27:14Z

Crosslink #46205

MajorBreakfast · 2018-06-30T17:38:57Z

I've removed the Into impls again and reverted to From impls, but just for FutureObj<()> instead of FutureObj<T>

Into impls that I removed

#[unstable(feature = "futures_api", issue = "50547")]
impl<T, F: Future<Output = T> + Send + 'static> Into<FutureObj<T>> for PinBox<F> {
    fn into(self) -> FutureObj<T> {
        FutureObj::new(self)
    }
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<T, F: Future<Output = T> + Send + 'static> Into<FutureObj<T>> for Box<F> {
    fn into(self) -> FutureObj<T> {
        FutureObj::new(PinBox::from(self))
    }
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<T, F: Future<Output = T> + 'static> Into<LocalFutureObj<T>> for PinBox<F> {
    fn into(self) -> LocalFutureObj<T> {
        LocalFutureObj::new(self)
    }
}

#[unstable(feature = "futures_api", issue = "50547")]
impl<T, F: Future<Output = T> + 'static> Into<LocalFutureObj<T>> for Box<F> {
    fn into(self) -> LocalFutureObj<T> {
        LocalFutureObj::new(PinBox::from(self))
    }
}

MajorBreakfast · 2018-06-30T20:06:28Z

Added lifetime
Moved FutureObj into core::future

ngg · 2018-06-30T23:41:23Z

I just tried to use it to create async trait methods (including capturing &self with the generic lifetime), it seems to work great.

MajorBreakfast · 2018-07-01T07:45:47Z

I've added an impl for PinMut. It's possible now that FutureObj can store a lifetime.

@ngg Thats' great to hear!

ngg · 2018-07-01T11:27:28Z

Isn't it unsound for PinMut to implement UnsafeFutureObj?
PinMut doesn't "own" the future, but it would transfer the ownership into UnsafeFutureObj (by calling into_raw()). What would happen in the following case?

let pinbox = PinBox::new(/*...*/);
let obj1 = FutureObj::new(pinbox.as_pin_mut());
let obj2 = FutureObj::new(pinbox);
drop(obj2); // this will drop the future
executor.spawn(obj1); // I think it's a use-after-free

Or does the life-time transfer from PinMut -> UnsafeFutureObj somehow solve this?
Either way I don't really understand the purpose of this, the drop function for this new impl seems like a no-op. What's your usecase where you'd need something like this?

ngg · 2018-07-01T12:17:51Z

Actually it seems to work properly, it may be sound.

MajorBreakfast · 2018-07-01T13:25:10Z

I don't think that it is unsound. It doesn't actually own the future because PinMut is just a reference, so its drop impl doesn't have to destroy anything and the lifetime of FutureObj guarantees that it can't live longer than allowed.
executor.spawn_obj expects a 'static lifetime, so your code example wouldn't compile because your PinMut is not static. PinMuts are almost never static (unless you create one from a static mutable reference xD)
I'm not sure yet where non-static futures can be used. It's quite likely that we'll find some use case, though.

rust-highfive · 2018-07-01T14:02:21Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:03:16]    Compiling rustc_asan v0.0.0 (file:///checkout/src/librustc_asan)
[00:03:19] warning: unused variable: `ptr`
[00:03:19]     --> libcore/mem.rs:1243:20
[00:03:19]      |
[00:03:19] 1243 |     unsafe fn drop(ptr: *mut ()) {}
[00:03:19]      |                    ^^^ help: consider using `_ptr` instead
[00:03:19]      = note: #[warn(unused_variables)] on by default
[00:03:19] 
[00:03:30]    Compiling libc v0.0.0 (file:///checkout/src/rustc/libc_shim)
[00:03:30]    Compiling alloc v0.0.0 (file:///checkout/src/liballoc)
---
[00:21:27]    Compiling rustc_asan v0.0.0 (file:///checkout/src/librustc_asan)
[00:22:00] error: unused variable: `ptr`
[00:22:00]     --> libcore/mem.rs:1243:20
[00:22:00]      |
[00:22:00] 1243 |     unsafe fn drop(ptr: *mut ()) {}
[00:22:00]      |                    ^^^ help: consider using `_ptr` instead
[00:22:00]      = note: `-D unused-variables` implied by `-D warnings`
[00:22:00] 
[00:22:00] 
[00:22:01] Makefile:28: recipe for target 'all' failed
[00:22:01] make: *** [all] Error 1

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

ngg · 2018-07-01T14:18:34Z

For example futures::executor::block_on(obj) doesn't require obj to be 'static.

MajorBreakfast · 2018-07-01T14:58:50Z

@ngg You're right. FutureObj with non-static lifetime is definitely useful with block_on. I've just tested this and it works with the non-static FutureObj I created.

@cramertj

Do you think that a FutureObj impl for PinMut makes sense conceptually?
I think that UnsafeFutureObj should be implemented for Box as well. A PinBox is IMO not necessary because the future doesn't need to be pinned at that point.

MajorBreakfast · 2018-07-01T15:18:38Z

@ngg Thanks BTW for all your testing :)

bors · 2018-07-01T23:06:22Z

☔ The latest upstream changes (presumably #51969) made this pull request unmergeable. Please resolve the merge conflicts.

cramertj · 2018-07-02T16:27:28Z

@mikeyhew What's the timeline on object-safe arbitrary-self-types? I'd really like to avoid these custom objects if we don't need them, but perhaps it's a necessary stopgap?

That said, UnsafeTask was already a temporary hack around not yet having unsized rvalues, so I suppose this isn't much worse?

cramertj · 2018-07-02T16:30:31Z

@MajorBreakfast Thanks as always for this PR!

Do you think that a FutureObj impl for PinMut makes sense conceptually?

Yeah, this is great! It's analogous to PinMut<dyn Future> or PinMut<dyn Future + Send>. I would also add an impl for &mut T where T: Future + Unpin.

cramertj · 2018-07-02T16:31:13Z

r=me with PhantomType nit addressed and &mut impl added.

mikeyhew · 2018-07-02T17:09:23Z

@cramertj they will probably be in nightly within the next month, but not sure when they will be stabilized

cramertj · 2018-07-02T17:12:45Z

@mikeyhew Cool! Thanks again for working on that.

@MajorBreakfast I think with that timeline it's still best for us to push forward with this approach and then we can adjust as necessary-- we need both unsized rvalues and object-safe arbitrary self types before we can totally replace these faked-out trait objects.

MajorBreakfast · 2018-07-02T17:27:52Z

Added an explanation about why FutureObj exists. I felt like this wasn't explained enough. I hope my explanation covers the important bits.
Used where clause to not break 80char limit
Renamed as_future_obj to into_future_obj to follow naming conventions
Implemented the requested changes
Implemented UnsafeFutureObj on Box

cramertj · 2018-07-02T17:52:07Z

Great!

@bors r+ p=1

(p=1 because I'd like to get breaking changes to the futures API out of the way ASAP so that we can push towards a stable-ish nightly release of futures 0.3)

bors · 2018-07-02T17:52:07Z

📌 Commit e666c2b has been approved by cramertj

bors · 2018-07-02T17:52:52Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: HirId-ification, continued #51321

bors · 2018-07-02T17:52:52Z

📌 Commit e666c2b has been approved by cramertj

ngg · 2018-07-02T19:03:15Z

@cramertj Are you sure that PhantomData<T> is really not necessary? This struct does own T in the sense that its Drop implementation will drop T, so I think it's necessary for dropck. (poll_fn should be enough for borrowck)

MajorBreakfast · 2018-07-02T19:36:10Z

@ngg AFAIK PhantomData's only purpose is to avoid error[E0392]: parameter T is never used
@cramertj Correct me if I'm wrong

MajorBreakfast · 2018-07-02T19:44:54Z

@ngg I don't think it is accurate to say that the "struct does own T" because T is only a type and therefore can't be owned because it's not a value that is stored in memory.

ngg · 2018-07-02T19:47:28Z

It will not drop T when dropped, so my comment was kinda meaningless, sry.
@MajorBreakfast PhantomData does lots of other things, it's not just for avoding an error, see https://doc.rust-lang.org/nomicon/phantom-data.html and specifically for dropck uses see https://stackoverflow.com/questions/42708462/why-is-it-useful-to-use-phantomdata-to-inform-the-compiler-that-a-struct-owns-a/42721125#42721125

cramertj · 2018-07-02T19:51:57Z

@ngg PhantomData isn't needed here-- the type T is already used as the output of the poll_fn, so that supplies the variance required. The custom trait object struct itself implements Drop manually (always) regardless of the inner type T, so adding the T here doesn't affect drop checking (AFAIK).

MajorBreakfast · 2018-07-02T20:04:07Z

@cramertj I'm not so sure... I think @ngg is right and we're violating this. I don't think that mentioning the type in the function pointer is enough.

Here's what Vec uses internally:
https://doc.rust-lang.org/1.26.0/src/core/ptr.rs.html#2514-2522

bors · 2018-07-02T20:12:07Z

⌛ Testing commit e666c2b with merge c8df60a...

@cramertj

Make custom trait object for `Future` generic - `TaskObj` -> `FutureObj<'static, ()>` - The `impl From<...> for FutureObj<'a, T>` impls are impossible because of the type parameter `T`. The impl has to live in libstd, but `FutureObj<'a, T>` is from libcore. Therefore `Into<FutureObj<'a, T>>` was implemented instead. Edit: This didn‘t compile without warnings. I am now using non-generic Form impls. See rust-lang/futures-rs#1058 r? @cramertj Edit: Added lifetime

ngg · 2018-07-02T20:13:57Z

Yeah that's what I meant but it wouldn't matter to add PhantomData<T> because it's not T what we own, but the original Future. This whole FutureObj is just emulating Box<Future<Output = T> + 'a> which would contain PhantomData<Future<Output = T> + 'a> so probably that is what we need here. (I'm really not sure about these things, I'm just guessing.. It might be ok now)
edit: It's not possible to have PhantomData<Future<Output = T> + 'a> right now because it's not object-safe due to the arbitrary self type (though it could be allowed by the compiler to have a PhantomData for these even without making them object-safe)

cramertj · 2018-07-02T21:14:24Z

@ngg Yeah, if you could spell PhantomData<dyn UnsafeFutureObj<'a, T>>, that would be correct, but we don't own a T, so PhantomData<T> doesn't do anything for us (unless I'm missing some other weird detail).

bors · 2018-07-02T22:13:11Z

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing c8df60a to master...

rust-highfive assigned cramertj Jun 30, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2018

MajorBreakfast mentioned this pull request Jun 30, 2018

0.3: How can I return a Future from a trait function? rust-lang/futures-rs#1058

Closed

This comment has been minimized.

Sign in to view

MajorBreakfast force-pushed the generic-future-obj branch from 82a498c to 8343be4 Compare June 30, 2018 19:48

This comment has been minimized.

Sign in to view

MajorBreakfast force-pushed the generic-future-obj branch from 64372ed to 1cf9cfb Compare July 1, 2018 07:42

MajorBreakfast force-pushed the generic-future-obj branch from 559f304 to a954f3b Compare July 1, 2018 13:01

MajorBreakfast force-pushed the generic-future-obj branch from 6ba2a8a to 8123dec Compare July 1, 2018 14:19

MajorBreakfast added 5 commits July 2, 2018 13:59

Make custom trait object for Future generic

3d43f82

Use From impls for FutureObj<()>

9f70e7f

Add lifetime to FutureObj

d8bf222

UnsafeFutureObj impl for PinMut

042928f

Improve doc comments for FutureObj

dd3b033

MajorBreakfast added 3 commits July 2, 2018 18:55

Add explanation for custom trait object

cb2c757

Remove unnecessary PhantomData field

5fde8b9

Implement UnsafeFutureObj for &mut Future

ae40894

Implemented UnsafeFutureObj on Box

e666c2b

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 Jul 2, 2018

bors merged commit e666c2b into rust-lang:master Jul 2, 2018

bors mentioned this pull request Jul 2, 2018

Many From implementations are undocumented #51430 #51738

Closed

MajorBreakfast mentioned this pull request Jul 23, 2018

Clarify what a task is #52610

Merged

Make custom trait object for Future generic #51944

Make custom trait object for Future generic #51944

Conversation

MajorBreakfast commented Jun 30, 2018 • edited

This comment has been minimized.

ngg commented Jun 30, 2018

MajorBreakfast commented Jun 30, 2018 • edited

MajorBreakfast commented Jun 30, 2018

MajorBreakfast commented Jun 30, 2018

This comment has been minimized.

MajorBreakfast commented Jun 30, 2018 • edited

ngg commented Jun 30, 2018

This comment has been minimized.

MajorBreakfast commented Jul 1, 2018

ngg commented Jul 1, 2018

ngg commented Jul 1, 2018

MajorBreakfast commented Jul 1, 2018 • edited

rust-highfive commented Jul 1, 2018

ngg commented Jul 1, 2018

MajorBreakfast commented Jul 1, 2018 • edited

MajorBreakfast commented Jul 1, 2018

bors commented Jul 1, 2018

cramertj commented Jul 2, 2018

cramertj commented Jul 2, 2018 • edited

cramertj commented Jul 2, 2018

mikeyhew commented Jul 2, 2018

cramertj commented Jul 2, 2018

MajorBreakfast commented Jul 2, 2018

cramertj commented Jul 2, 2018 • edited

bors commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 2, 2018

ngg commented Jul 2, 2018

MajorBreakfast commented Jul 2, 2018 • edited

MajorBreakfast commented Jul 2, 2018 • edited

ngg commented Jul 2, 2018

cramertj commented Jul 2, 2018 • edited

MajorBreakfast commented Jul 2, 2018 • edited

bors commented Jul 2, 2018

ngg commented Jul 2, 2018 • edited

cramertj commented Jul 2, 2018 • edited

bors commented Jul 2, 2018

Make custom trait object for `Future` generic #51944

Make custom trait object for `Future` generic #51944

MajorBreakfast commented Jun 30, 2018 •

edited

MajorBreakfast commented Jun 30, 2018 •

edited

MajorBreakfast commented Jun 30, 2018 •

edited

MajorBreakfast commented Jul 1, 2018 •

edited

MajorBreakfast commented Jul 1, 2018 •

edited

cramertj commented Jul 2, 2018 •

edited

cramertj commented Jul 2, 2018 •

edited

MajorBreakfast commented Jul 2, 2018 •

edited

MajorBreakfast commented Jul 2, 2018 •

edited

cramertj commented Jul 2, 2018 •

edited

MajorBreakfast commented Jul 2, 2018 •

edited

ngg commented Jul 2, 2018 •

edited

cramertj commented Jul 2, 2018 •

edited