FutureExt::shared does not specify potentially important behaviour in docs.

[Vi-Kitten]

The future extension method FutureExt::shared does not specify in its documentation that it does not poll the inner future until the Shared it creates is polled. For people finding or being pointed to this method after looking for a "Thunk" (a construct for lazy evaluation and result sharing) this is incredibly important, as eager evaluation where lazy evaluation is expected can be catastrophic. I know this is rather minor but I think its important to clarify if this behaviour is or is not part of the API, as whilst it can probably be expected, that sort of assumption doesn't seem to be the rust way.

[taiki-e]

it does not poll the inner future until the Shared it creates is polled.

std::future::Future docs say:

Futures alone are inert; they must be actively polled to make progress, meaning that each time the current task is woken up, it should actively re-poll pending futures that it still has an interest in.

Also, must_use messages of std::future::Future and Shared say "do nothing unless you .await or poll them":

futures-rs/futures-util/src/future/future/shared.rs

Line 15 in 7211cb7

#[must_use = "futures do nothing unless you `.await` or poll them"]

[Vi-Kitten]

it does not poll the inner future until the Shared it creates is polled.

^ where is this said explicitly for shared? This is important behaviour.

What your saying about the implementation of the Future trait is all well and good but also not relevant, the shared method doesn't specify that it doesn't, for example, poll it once for you. That would be a really bad implementation, but its still possible given the type signature and lack of specification in the docs.

[taiki-e]

Doing nothing unless polled is the normal behavior of futures, so I don't feel the need to add the same explanation to each individual future about it.

(Or am I misunderstanding your comments? What is the document you specifically want?)

[Vi-Kitten]

Its not about futures in general its about this specific function, there is nothing that stops the implementation of shared from polling your future, it would be very dumb of it, but it could. And considering how important laziness is to thunk functionality I think it would be a good idea to just specify that, and if it would only feel natural to specify that for everything else like map and whatnot, then I think so be it. It's really important functionality that should be explicitly specified instead of implicitly relied upon.

(going to sleep now)

[Vi-Kitten]

Perhaps this will help you understand my perspective on this.

Consider the following god awful code:

struct ThunkAwareFuture<T> {
    thunk: *mut futures::future::Shared<ThunkAwareFuture<T>>,
    _t: PhantomData<T>
    // other fields that would define behaviour
}

impl<T> Future for ThunkAwareFuture<T> {
    type Output = T;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        // on some branch...
        {
            // do something with knowledge that this is a thunk
            let _ = unsafe { self.thunk.as_ref().unwrap_unchecked() }.clone();
        }
        // polling things...
        todo!()
    }
}

let ptr = Box::into_raw(Box::<futures::future::Shared<ThunkAwareFuture<()>>>::new_uninit())
    as *mut futures::future::Shared<ThunkAwareFuture<()>>;

let thunk = ThunkAwareFuture {
    thunk: ptr,
    _t: PhantomData
}.shared();

unsafe { ptr.write(thunk.clone()) };

thunk.await

Putting aside concerns about memory leakage, this is (or at least hopefully demonstrates that one may make similarly) a function that should not produce UB and could be the internals to some safe API.

Now if this is a valid usage of FutureExt::shared which I believe you are saying it is, then because of the lack of specification in the futures API, changing FutureExt::shared to poll one time before giving you the Shared future would not be a breaking change.

This means that a non breaking change would induce UB / other nasties in seemingly safe code!

Now do I think that anyone would change the working of FutureExt::shared to do that? Of cause not that would be stupid, but it is technically valid on the end of the futures library, and something so catastrophic should not be valid even on the most obscure of technicalities.

[seanmonstar]

There is no need to document on every concrete implementation the lazy behavior of Future, which is already described for the trait.

Just as we don't need to document that it doesn't randomly panic, or spawn a thread, or all sorts of other things it doesn't do.