[Parser] Improving diagnostics for access-level modifiers

[Visckmart]

Problem

When declaring a method with multiple access-level modifiers (a programming mistake), the current diagnostic is not clear nor accurate.

As an example, when we compile the following block of code:

class Foo {
  public private func exampleMethod() { }
}

We get the following diagnostics:

error: duplicate modifier
  public private func exampleMethod() { }
         ^~~~~~~
note: modifier already specified here
  public private func exampleMethod() { }
  ^~~~~~

It can be argued that both modifiers are from the same "type" (access-level modifiers), but the example shows a situation where they aren't a duplicate of each other as the error message says.

Solution

The solution implemented on this pull request is a new diagnostic: an error message that describes the problem more clearly and a note that refers to the previous modifier by name.

error: multiple access-level modifiers specified
  public private func exampleMethod() { }
         ^~~~
note: 'public' previously specified here
  public private func exampleMethod() { }
  ^~~~~~~

Resolves #60914.

[AnthonyLatsis]

@ahoppen Are you OK with accepting this sort of improvement into the legacy parser? I would guess swift-syntax is not really expected to diagnose this stuff anyway.

[ahoppen]

Are you OK with accepting this sort of improvement into the legacy parser? I would guess swift-syntax is not really expected to diagnose this stuff anyway.

Yes, swift-syntax allows duplicate access level modifiers because it just models them as a list. ASTGen will need to diagnose these sorts of problems.

And I think this change is a good improvement. It would be nice to add a test case for it, though.

[Visckmart]

Cool! I updated the diagnostics' messages to reflect the latest from the discussion with @AnthonyLatsis and updated the existing tests to reflect the new diagnostics as @ahoppen requested.

[AnthonyLatsis]

Awesome! Just one optional stylistic suggestion.

[AnthonyLatsis]

@swift-ci please smoke test macOS

[AnthonyLatsis]

@rintaro ping

[rintaro]

@swift-ci Please smoke test

[rintaro]

The change looks good to me. But I feel these all attribute diagnostics including others (e.g. "concurrency only") should be in Sema (TypeCheckAttr.cpp) not in Parse or ASTGen. For example, @differentiable only diagnose duplication if the parameter is semantically the same. WDYT @ahoppen ?

It's not a blocker for merging this PR though.

[ahoppen]

I agree with @rintaro. Implementing these checks in the type checker will make them applicable to both the old parser and the new parser.

[ahoppen]

@swift-ci Please test Windows

[Visckmart]

I just noticed I had mistakenly kept an extra argument on the note diagnostic. It's fixed now. I also ran the code formatter and cleaned up the commit message to make it more succinct.

Sorry for making these changes after the review comments and the tests have been run. @AnthonyLatsis would you mind triggering the tests once more?

[AnthonyLatsis]

@rintaro Do we want anyone else to take a look at this?

[AnthonyLatsis]

@swift-ci please smoke test

[hborla]

Thank you for your contribution! Exploring moving some of these attribute diagnostics out of the parser and into TypeCheckAttr.cpp would be a good next task if you're interested in continuing contributions in this area 🙂

[Visckmart]

I’m definitely interested! I’ll take a look at that once I have some time.

I’d like to quickly thank @AnthonyLatsis for helping me a lot with this the process of making a contribution and @rintaro, @ahoppen and @hborla for the review and approval of my first pull request. ☺️