pub/async/doc comment parse error

[benbrittain]

When the async and pub identifiers are swapped next to a doc comment:

/// Build the ControlImpl to interact with fidl messages
/// State is stored in the HostDispatcher object
async pub fn make_control_service(hd: Arc<RwLock<HostDispatcher>>, chan: fasync::Channel) -> impl Future<Item = (), Error = Never> {

a confusing error is returned:

error: expected item after doc comment
  --> src/control_service.rs:23:1
   |
23 | /// State is stored in the HostDispatcher object
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

instead of making it clear pub should be first

[benbrittain]

cc @cramertj

[estebank]

This can be fixed by handling async pub fn foo() -> impl Future<..> {} in parser as a valid item emitting a custom error for that case. While we're at it we could also consider handling fn pub foo() as well.

The doc comment error should also point to the following non-item with a secondary span as well.

[JohnTitor]

@estebank I'd like to work on this, how we should handle async pub fn foo() -> impl Future<..> {}?

[cramertj]

The issue here is just that we don't recognize out-of-order fn modifiers as items.

[estebank]

The work needs to happen in the parser in a couple of places (trait fn items and bare fns are parsed in different places). If you look at parse_impl_item_ you have to tentatively consume an async token before parse_visibility in such a way that advances the parser but doesn't break the generated item while at the same time emitting an error. Same in parse_item_implementation.

[JohnTitor]

Thank you @estebank! I'm sorry for the late response. I looked at parse_impl_item_, what does before parse_visibility mean? Should I add a custom error before this line?
Uh, I may need time and advice. If it’s too much, I will leave this issue.

[estebank]

It would be before doing self.eat(&token::Async); before this line, and if consumed, emitting a custom error and change the asyncness of the FnHeader in the Method returned here so that type checking does the right thing afterwards.

[JohnTitor]

Hmm, I wasn't able to find token::Async, can I use self.eat_keyword(keywords::Async) instead of this?

[estebank]

@JohnTitor, yes, I was writing it off the top of my head, the correct code is the one you came up with :)

[JohnTitor]

@estebank Okay! But I don't know how change asyncness. Can I change parse_impl_method, or need not do?

[JohnTitor]

@estebank Uh, could I open a draft PR and ask you to review?

[estebank]

@JohnTitor

Can I change parse_impl_method, or need not do?

I'll have to look at the code in detail, but you'll probably will have to make the returned ImplItemKind mutable and modify the asyncness field of FnHeader of its MethodSig. A bit convoluted :$

could I open a draft PR and ask you to review?

Of course! Go ahead :)