Skip to content

test IntoFuture before stabilization #687

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

Closed
wants to merge 12 commits into from

Conversation

yoshuawuyts
Copy link
Contributor

DO NOT MERGE

This PR removes the .into_future calls with the intention of providing a testable environment for #![feature(into_future)] in the Rust compiler prior to stabilization. We're between nightlies right now, and we'll need to rebase on top of #686 for this to continue to work. But I want to validate our test suite works with the feature enabled so that we can propose the stabilization of the feature soon. Thanks!

@yoshuawuyts yoshuawuyts marked this pull request as draft March 14, 2022 13:38
@yoshuawuyts yoshuawuyts changed the title remove .into_future() calls test IntoFuture before stabilization Mar 14, 2022
@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Mar 18, 2022

This seems to be working. We have one instance where we had to keep the into_future call on the caller's side. And once instance where we could not implement IntoFuture, and had to keep using our own manual into_future method.

into_future and timeouts

In order to call FutureExt::timeout we first have to finalize our futures builder, and get an actual future:

let future = client.create_database("my_database");
match future.into_future().timeout_at(deadline).await {

This pattern matches how IntoIterator is used as well, requiring into_iter to be called before we can call methods on Iterator. In that sense it's nothing unexpected, and if we want to address this issue someday in the Rust language/library we can do so for both traits.

IntoFuture and GATs

edit (2022-03-21): This has been solved!

The only instance where we couldn't implement the std::future::IntoFuture trait was for our GetDocumentBuilder type:

let response: GetDocumentResponse<MySampleStruct> = client
.document_client(id.clone(), &id)?
.get_document()
.consistency_level(&response)
.into_future()
.await?;

Attempting to implement IntoFuture yields an error:

impl<T> IntoFuture for GetDocumentBuilder {
    type Output = GetDocumentResponse<T>;

    type IntoFuture = GetDocument<T>;

    fn into_future(self) -> Self::IntoFuture {
        self.into_future()
    }
}
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> sdk\data_cosmos\src\operations\get_document.rs:73:6       
   |
73 | impl<T> IntoFuture for GetDocumentBuilder {
   |      ^ unconstrained type parameter

The issue is that we cannot define generics solely for use generics in our associated types here, which will hopefully be solvable by adding Generic Associated Types (GATs). This problem is not unique to the IntoFuture trait, and resolving this for all traits would resolve this for IntoFuture too.

Conclusion

In this PR we attempted to convert 31 distinct APIs in the Azure Cosmos SDK to use the std::future::IntoFuture trait, and rely on the new IntoFuture desugaring. Of those 31 APIs, we were able to convert 30 to use IntoFuture. And on the calling side we had one other instance where an into_future annotation was warranted.

The only instance where we couldn't adopt the IntoFuture trait is where we were had a need for GATs, which is not a limitation affecting many traits. We haven't found any issues unique to IntoFuture, and based on this experiment we would recommend moving forward with the stabilization of IntoFuture.

@tmandry
Copy link

tmandry commented Mar 19, 2022

The issue is that we cannot define generics solely for use generics in our associated types here, which will hopefully be solvable by adding Generic Associated Types (GATs). This problem is not unique to the IntoFuture trait, and resolving this for all traits would resolve this for IntoFuture too.

Perhaps I misunderstand, but GATs would require modifying the trait itself. Can you add the type parameter to GetDocumentBuilder using e.g. PhantomData?

@yoshuawuyts
Copy link
Contributor Author

Perhaps I misunderstand, but GATs would require modifying the trait itself. Can you add the type parameter to GetDocumentBuilder using e.g. PhantomData?

Oh, yeah I trust your understanding of GATs more than I trust my own. I tried implementing this using PhantomData, and that indeed did the trick! -- yes, no GATs needed. Thank you!

@cataggar cataggar mentioned this pull request Jul 19, 2022
@yoshuawuyts yoshuawuyts closed this Aug 9, 2022
@yoshuawuyts yoshuawuyts deleted the try-into-future branch August 9, 2022 18:26
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