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 :)

[iluuu1994]

The changes need to be made in parse_item_implementation. These are all the fn modifiers currently accepted by the parser:

visibility extern fn ...
visibility const fn ...
visibility async [unsafe] fn ...
visibility unsafe [extern] fn ...

These are all the other things parsed in parse_item_implementation:

visibility use ...
visibility extern crate ...
visibility extern { ... }
visibility static ...
visibility const ...
visibility unsafe [auto] trait ...
visibility [unsafe] impl ...
visibility mod ...
visibility type ...
visibility struct ...
visibility enum ...
visibility macro ...

Not sure how we should proceed. We could move the parsing of the modifiers into a separate function, make it check the order of the modifiers and also check for duplicates. That would, however, still not allow for things like fn pub .... In that case each branch would have to try to parse modifiers again and print an error in case any are found.

Furthermore each branch would have to make sure that only modifiers are set that make sense for the particular branch. Seems like this would complicate things quite a bit.

Should I proceed with that strategy or does anyone have a better suggestion?

[whizsid]

I would like to deal with this issue. But #76447 addresses the same issue.

[jyn514]

@whizsid that PR hasn't seen activity in a few days, I think it would be reasonable to update it with the fixes mentioned in the review comments yourself. cc @pickfire

[pickfire]

There is one last thing in the patch #76447 (comment), but it doesn't seemed to be near the stuff I do I believe, it may need some tweak.

[rylev]

Triage: this no longer seems to be an issue.

The code:

/// Blah blah blah
/// Blah
async pub fn foo() -> impl Future<()> {todo!()}

yields this error:

error: expected one of `extern`, `fn`, or `unsafe`, found keyword `pub`
 --> src/lib.rs:3:7
  |
3 | async pub fn foo() -> impl Future<()> {todo!()}
  | ------^^^
  | |     |
  | |     expected one of `extern`, `fn`, or `unsafe`
  | help: visibility `pub` must come before `async`: `pub async`