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

It is hard to see the return types of async functions #69781

Open
NickeZ opened this issue Mar 6, 2020 · 15 comments
Open

It is hard to see the return types of async functions #69781

NickeZ opened this issue Mar 6, 2020 · 15 comments
Labels
A-async-await AsyncAwait-Triaged C-enhancement E-medium P-medium T-rustdoc

Comments

@NickeZ
Copy link

NickeZ commented Mar 6, 2020

In the documentation it is hard to distinguish async functions from normal functions.

Take for example warp::Server. Both run and bind returns futures. But when I was reading the docs I thought only bind was asynchronous. And I had a lot of problems getting my code to work due to wrong assumptions about the types...

My suggestion is that async functions are listed in a separate listing, have another color, use the correct return type (Future) or in some other way stand out.

ref: https://www.reddit.com/r/rust/comments/fef3zh/minor_rant_about_async_syntax_and_documentation/

@jonas-schievink jonas-schievink added C-enhancement T-rustdoc labels Mar 6, 2020
@CodesInChaos
Copy link

CodesInChaos commented Mar 7, 2020

IMO a function returning a future being implemented via async vs the same function being implemented manually is an implementation detail that shouldn't be visible in the documentation. So I'm for showing the Future return type and hiding the async keyword.

@jonas-schievink jonas-schievink added the A-async-await label Mar 7, 2020
@NickeZ
Copy link
Author

NickeZ commented Mar 7, 2020

Why would I care as a consumer of the API if the function is implemented using async or if it is implemented with returning a Future?

@tmandry
Copy link
Contributor

tmandry commented Mar 17, 2020

One idea is to leave the output as-is, but add an async "badge" next to any function which returns impl Future (either because it is async fn or because it's explicit in the return type). This would help async functions stand out more visually.

And maybe you could click/hover over the badge to see the desugared signature of async fns?

@tmandry tmandry added AsyncAwait-Triaged E-medium P-medium labels Mar 17, 2020
@NickeZ
Copy link
Author

NickeZ commented Mar 17, 2020

I just think it is important that it stands out more. I would prefer if it is clearly visible on the right side because that is where the return type is. But first I guess we should decide if this is something we want to solve at all. Maybe nobody else has this problem.

Functions implemented with returning Futures are not a problem, you see immediately that they return a future by looking at their return type.

The problem isn't figuring out if it is async. This I could do by looking at the async keyword. The problem is that I'm fooling myself that the function returns something other than a Future because that is what it looks like. So the solution has to stand out so much so that I don't mistake the function for a non-async function.

I'm not really convinced by hover-effects. I don't hover every line I read.

@ragnese
Copy link

ragnese commented Mar 18, 2020

I really believe the most sane and obvious answer is to remove the async keyword from the front and always display the "true" return type as impl Future<[...]>.

async and .await are syntactic sugar after all, and downstream consumers of your fabulous library really don't care if you chose to use that syntax or not. Just like they don't care if you used an if-else or a match inside your function.

@joshtriplett
Copy link
Member

joshtriplett commented Apr 8, 2020

I would like to see this as well: show the real return type, and optionally add an async badge to any function returning an impl Future.

@davidbarsky
Copy link

davidbarsky commented Apr 14, 2020

How would this apply to #[async_trait]-generated docs, if at all? Personally, I'd love to have a toggle in Rustdoc that showed the "pretty"/pre-macro expansion "type signature" in Rustdoc. Without that toggle, the documentation is pretty noisy, and arguably, a hindrance to accessibility and learning. For instance, see rusoto_dynamodb's documentation.

@NickeZ
Copy link
Author

NickeZ commented Apr 14, 2020

How would this apply to #[async_trait]-generated docs, if at all? Personally, I'd love to have a toggle in Rustdoc that showed the "pretty"/pre-macro expansion "type signature" in Rustdoc. Without that toggle, the documentation is pretty noisy, and arguably, a hindrance to accessibility and learning. For instance, see rusoto_dynamodb's documentation.

the function you link to returns a boxed pin. if all async functions does that I've completely misunderstood what a async desugares to. then it is even more important that the real type is in the signature.

@davidbarsky
Copy link

davidbarsky commented Apr 14, 2020

the function you link to returns a boxed pin. if all async functions does that I've completely misunderstood what a async desugares to. then it is even more important that the real type is in the signature.

My belief is that once you understand that an async fn desugars to either an impl Future (or some trait object that derefs to Future, in the case of the example I linked), defaulting to rendering async is preferable. While I like @tmandry's and @joshtriplett's suggestion to provide an async badge, I'd also make the async badge a button that toggles between the two representations of an async function or method. Does that seem reasonable?

@NickeZ
Copy link
Author

NickeZ commented Apr 14, 2020

There are probably many reasonable ways to document async functions. The best IMO is to list the real types by default, impl Future, and have a toggle button to switch to the pretty versions. More importantly it would be nice if it was consistent and not like today where some functions show async before the function and some show impl Future as return value. The inconsistency is right now the reason of my inability to comprehend the docs.

@jyn514
Copy link
Member

jyn514 commented Sep 24, 2020

One thing @Nemo157 mentioned is that there is a difference between fn f() -> impl Future and async fn f(): the first can perform blocking operations before returning a future and the second can't.

That said this does seem useful. Maybe it could be opt-in, with #[doc(async_fn)] or something like that?

@jyn514
Copy link
Member

jyn514 commented Sep 24, 2020

Without that toggle, the documentation is pretty noisy, and arguably, a hindrance to accessibility and learning. For instance, see rusoto_dynamodb's documentation.

I think #63037 would help with this (and could be done automatically, it's just a bug fix in rustdoc).

@jyn514
Copy link
Member

jyn514 commented Dec 22, 2020

Personally, I'd love to have a toggle in Rustdoc that showed the "pretty"/pre-macro expansion "type signature" in Rustdoc.

This isn't really feasible, rustdoc runs after macro expansion: #1998

@tmandry
Copy link
Contributor

tmandry commented Dec 29, 2020

Rewriting an async fn signature to the desugared form seems like a worthwhile option to have, but would also be somewhat surprising to me as default behavior. As far as I know we decided on async fn syntax to make signatures easier both to read and write (especially when it comes to that pesky + 'a bound).

I think adding a toggle button might be a good solution.

@betamos
Copy link

betamos commented Jan 7, 2021

I agree with the premise but would rephrase it as identifying asynchronous functions is unnecessarily complex. However, I wouldn't automatically assume that drilling into the details of the return type, and exposing them, is what the users want and need.

There are three ways to declare an "asynchronous function" that I know of:

async fn foo() { } // keyword
fn bar() -> impl Future<..> { .. } // impl
fn baz() -> SomeCustomFut { .. } // custom

Whichever style is more frequent in the user's projects might influence their way of identifying asynchronous functions. Hence, I'm hesitant to giving special status to one syntax and claiming it's "less confusing". I think the declaration should reflect the way the code was written, just like it is in other cases.

A badge for any type that implements Future is both simple and effective -- this lets the user know (a) they need to await it, and (b) the function is part of the async API, as opposed to the sync API (for types with dual sync/async APIs).

Hovering does not work on touch screens. Clicking for more info sounds a bit undiscoverable, but yeah perhaps. In either case, an interactive solution requires a default, which still puts us in a value-judgment position of choosing the "least confusing" option. I don't think this is necessary to solve OPs problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await AsyncAwait-Triaged C-enhancement E-medium P-medium T-rustdoc
Projects
None yet
Development

No branches or pull requests

9 participants