Detect async visibility wrong order, async pub

[pickfire]

Partially address #76437.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[pickfire]

niko is probably the wrong reviewer for this maybe

r? @estebank

[estebank]

I would prefer to make this change to parse_fn_front_matter:

rust/compiler/rustc_parse/src/parser/item.rs

Lines 1566 to 1594 in 0e2c128

	/// Parses all the "front matter" (or "qualifiers") for a `fn` declaration,
	/// up to and including the `fn` keyword. The formal grammar is:
	///
	/// ```
	/// Extern = "extern" StringLit ;
	/// FnQual = "const"? "async"? "unsafe"? Extern? ;
	/// FnFrontMatter = FnQual? "fn" ;
	/// ```
	pub(super) fn parse_fn_front_matter(&mut self) -> PResult<'a, FnHeader> {
	let constness = self.parse_constness();
	let asyncness = self.parse_asyncness();
	let unsafety = self.parse_unsafety();
	let ext = self.parse_extern()?;
	if let Async::Yes { span, .. } = asyncness {
	self.ban_async_in_2015(span);
	}
	if !self.eat_keyword(kw::Fn) {
	// It is possible for `expect_one_of` to recover given the contents of
	// `self.expected_tokens`, therefore, do not use `self.unexpected()` which doesn't
	// account for this.
	if !self.expect_one_of(&[], &[])? {
	unreachable!()
	}
	}
	Ok(FnHeader { constness, unsafety, asyncness, ext })
	}

That way the parser can recover more gracefully and you also have the spans for each element to work with more easily. You could also check for other permutations, like async const pub fn and unsafe async fn.

[pickfire]

This is weird, but at least users can know that visibility is duplicated. We could add a check but I probably don't want to do it in this patch.

[pickfire]

What's left for another patch

extern "C" pub

error: expected `{`, found keyword `pub`
 --> test.rs:1:16
  |
1 |     extern "C" pub fn t() {}
  |                ^^^ expected `{`

error: aborting due to previous error

unsafe const and the other permutations

error: expected one of `extern` or `fn`, found keyword `const`
 --> test.rs:1:12
  |
1 |     unsafe const pub fn t() {}
  |            ^^^^^ expected one of `extern` or `fn`

error: aborting due to previous error

Duplicate visibility (this one is obvious unlike the rest)

error: expected one of `(`, `async`, `const`, `default`, `extern`, `fn`, `pub`, `unsafe`, or `use`, found keyword `pub`
  --> $DIR/duplicate-visibility.rs:4:9
   |
LL | extern {
   |        - while parsing this item list starting here
LL |     pub pub fn foo();
   |         ^^^
   |         |
   |         expected one of 9 possible tokens
   |         help: visibility `pub` must come before `pub pub`: `pub pub pub`
LL |
LL | }
   | - the item list ends here

error: aborting due to previous error

[pickfire]

tidy error: /checkout/src/test/ui/parser/default.rs:24: line longer than 100 chars
tidy error: /checkout/src/test/ui/parser/duplicate-visibility.rs:5: line longer than 100 chars

I don't know how to fix this without using // ignore tidy-line-length, asking in another group.

[pickfire]

@estebank Do you know how should I fix this?

[jyn514]

I would add the // ignore-tidy-linelength if it's just a test, rustdoc does this for all the intra-doc link tests.

[pickfire]

@estebank Can you please help to review again?

[estebank]

Only one nitpick: this suggestion is incorrect. The span should point at default pub and the suggestion content should be pub default.

[pickfire]

pub default? Can you show me an example output you mentioned? I don't think default pub should be there, it should be pub only right?

[estebank]

Actually, it should be only default, as pub is implied, but yes the correct syntactic order is pub default fn foo().

[pickfire]

How should I check that? It doesn't seemed to be part of parse_fn_front_matter but rather in parse_fn_kind, I think it is related to

} else if self.check_fn_front_matter() {
    // FUNCTION ITEM
    let (ident, sig, generics, body) = self.parse_fn(attrs, req_name, lo)?;
    (ident, ItemKind::Fn(def(), sig, generics, body))

It feels weird that defaultness is not parsed as part of parse_fn_front_matter.

I feel lazy to do this, requires cleaning on other parts, @estebank do you want to do it?

[pickfire]

This should be fixed. I didn't know this is a regression created by the patch I made.

[JohnCSimon]

ping from triage:
@pickfire this conversation seems to have stalled out, can you address the comments or ping the reviewer for clarification?

[pickfire]

I haven't get myself to work on the last thing the reviewer mentioned.

I think it may be a bit different from where I worked on this, it would be good if @estebank could point out where needs to be changed, I feel less motivated to work on the remaining parts either since this may need changes in other parts. If @estebank want to work on this then he can take this up, otherwise I can take a look at it this weekend.

[Dylan-DPC-zz]

@pickfire can you resolve the conflict ? will be faster as we can get a review and then merge it quickly

[pickfire]

@Dylan-DPC done

[bors]

☔ The latest upstream changes (presumably #82517) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

triage: @pickfire unfortunately there's another merge conflict.
Can you please resolve this and set it to S-waiting-on-review ? Thank you.

[JohnCSimon]

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[pickfire]

@JohnCSimon Do I have to keep on updating it? I am thinking of getting a review first before updating it, I have updated a few times in the past for merge conflict but still no review.

[Dylan-DPC-zz]

@pickfire better to, makes it easier to merge if the review is green after that

[estebank]

@pickfire my apologies. I believed I could get around this to point you towards an alternative way of doing this, but there's no reason not to merge this as is and potentially implement my idea later.

So it isn't lost to the corners of my mind, my thinking here was to make the parser advance through tokens that can be part of the fn front matter (fn, pub, async, etc) and keep track of whether they've been found and whether they are in the right order. If not, construct an appropriate front matter string and suggest that in the error. Doing that would get us for free support for fixing things like async fn pub async foo() without any extra effort.

@bors r+

[bors]

📌 Commit 21c1574 has been approved by estebank

[bors]

⌛ Testing commit 21c1574 with merge 81c1d7a...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 81c1d7a to master...

[pickfire]

So it isn't lost to the corners of my mind, my thinking here was to make the parser advance through tokens that can be part of the fn front matter (fn, pub, async, etc) and keep track of whether they've been found and whether they are in the right order. If not, construct an appropriate front matter string and suggest that in the error. Doing that would get us for free support for fixing things like async fn pub async foo() without any extra effort.

Ah, but it seems like something that we aren't going to need. Why would someone duplicate the header? I don't see any reason someone would do so. Order I can understand if they are wrong, but maybe duplicating them is too much I believe. Maybe I am wrong, if people copy pasted it could happen, but we may need extra care not to slow down the usual path.