Error suggests additional `pub` when using `pub pub fn ...`

[poliorcetics]

I tried this code:

pub pub fn toto() {}

I expected to see this happen: Rust tells me to fix the issue by removing a pub.

Instead, this happened: Rust tells me to add a pub before the existing ones: pub pub pub:

3 | pub pub fn toto() {}
  |     ^^^
  |     |
  |     expected one of 9 possible tokens
  |     help: visibility `pub` must come before `pub pub`: `pub pub pub`

This will also be an issue with const async const (and similar errors with other keywords) after #87235 is merged.

Meta

rustc --version --verbose:

rustc 1.56.0-nightly (2827db2b1 2021-08-01)
binary: rustc
commit-hash: 2827db2b137e899242e81f1beea39ae26e245153
commit-date: 2021-08-01
host: x86_64-apple-darwin
release: 1.56.0-nightly
LLVM version: 12.0.1

Backtrace

   Compiling playground v0.1.0 (/Users/alexis/Projects/rust/playground)
error: expected one of `(`, `async`, `const`, `default`, `extern`, `fn`, `pub`, `unsafe`, or `use`, found keyword `pub`
 --> src/main.rs:3:5
  |
3 | pub pub fn toto() {}
  |     ^^^
  |     |
  |     expected one of 9 possible tokens
  |     help: visibility `pub` must come before `pub pub`: `pub pub pub`

error: could not compile `playground` due to previous error

@rustbot label A-diagnostics T-compiler

[Nicholas-Baron]

I have been looking at this problem and I would like to share my findings so far.

It seems that the second pub is being caught too late.
The function parse_fn_front_matter generates this error.
However, the visibility modifier (e.g. pub, crate) should have been taken by parse_visibility.

I am new to this area of the code. I have been trying to create a Err(DiagnosticBuilder) to return from parse_visibility, but I cannot figure out how to create or even get one. Any help is appreciated.

[poliorcetics]

It will probably be very hard to handle this from parse_visibility because a similar error occurs when using pub const pub ... and so parse_visibility will not detect this case either.

My first intuition would be to pass a parameter to parse_fn_front_matter that says whether a visibility was parsed earlier or not (something like has_vis: bool, or maybe an Option<Span>, depends on the error message you want to expose) and act accordingly. I'm waiting for my PR #87235 to be merged and I'll fix that behaviour for const, async and other keywords that can be found in front of a function.

Edit: actual error of pub const pub:

error: expected one of `async`, `extern`, `fn`, or `unsafe`, found keyword `pub`
 --> src/main.rs:3:11
  |
3 | pub const pub fn toto() {}
  |     ------^^^
  |     |     |
  |     |     expected one of `async`, `extern`, `fn`, or `unsafe`
  |     help: visibility `pub` must come before `const`: `pub const`

[Nicholas-Baron]

I started investigating the call tree between parse_visibility and parse_fn_front_matter.
The root is at parse_item_common_. First is parse_visibility, then is parse_item_kind.
parse_item_kind calls parse_fn, which calls parse_fn_front_matter.

&vis is already passed to parse_item_kind, so parse_fn will need to take it. However, parse_fn is called in parse_field_ident to help beginners, so Option<&Visibility> is the parameter type.

[Nicholas-Baron]

error: expected one of `(`, `async`, `const`, `default`, `extern`, `fn`, `pub`, `unsafe`, or `use`, found keyword `pub`
 --> src/main.rs:2:9
  |
2 |     pub pub fn toto(){}
  |     ----^^^
  |     |   |
  |     |   expected one of 9 possible tokens
  |     help: visibility `pub` was found before `pub`: `pub`

error: aborting due to previous error

I have got the error message changed! How should I word the message, and how should the spans be shown? The warning for pub macros seems like a decent place to start.

Link to my fork, where the work is being done.

[Nicholas-Baron]

@rustbot claim.

[Nicholas-Baron]

@rustbot release-assignment.

[poliorcetics]

@rustbot claim