[Parser] Improve diagnostics for access-level modifiers

Improve the diagnostics for situations where multiple access-level modifiers are used on the same declaration.

Author: Visckmart

include/swift/AST/DiagnosticsParse.def

@@ -1485,6 +1485,10 @@ ERROR(duplicate_attribute,none,
       "duplicate %select{attribute|modifier}0", (bool))
 NOTE(previous_attribute,none,
      "%select{attribute|modifier}0 already specified here", (bool))
+ERROR(multiple_access_level_modifiers,none,
+      "multiple incompatible access-level modifiers specified", ())
+NOTE(previous_access_level_modifier,none,
+     "previous modifier specified here", ())
 ERROR(mutually_exclusive_attrs,none,
       "'%0' contradicts previous %select{attribute|modifier}2 '%1'", (StringRef, StringRef, bool))
 

lib/Parse/ParseDecl.cpp

@@ -3093,9 +3093,25 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
     if (!Tok.is(tok::l_paren)) {
       // Normal access control attribute.
       AttrRange = Loc;
-      DuplicateAttribute = Attributes.getAttribute<AccessControlAttr>();
-      if (!DuplicateAttribute)
+
+      if (!DiscardAttribute) {
         Attributes.add(new (Context) AccessControlAttr(AtLoc, Loc, access));
+        break;
+      }
+
+      // Diagnose if there's already an access control attribute on
+      // this declaration with a different access level.
+      if (access != cast<AccessControlAttr>(DuplicateAttribute)->getAccess()) {
+        diagnose(Loc, diag::multiple_access_level_modifiers)
+            .highlight(AttrRange);
+        diagnose(DuplicateAttribute->getLocation(),
+                 diag::previous_access_level_modifier)
+            .highlight(DuplicateAttribute->getRange());
+
+        // Remove the reference to the duplicate attribute
+        // to avoid the extra diagnostic.
+        DuplicateAttribute = nullptr;
+      }
       break;
     }
 
@@ -3125,11 +3141,28 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
       return makeParserSuccess();
     }
 
+    // Check for duplicate setter access control attributes.
     DuplicateAttribute = Attributes.getAttribute<SetterAccessAttr>();
-    if (!DuplicateAttribute) {
+    DiscardAttribute = DuplicateAttribute != nullptr;
+
+    if (!DiscardAttribute) {
       Attributes.add(new (Context) SetterAccessAttr(AtLoc, AttrRange, access));
+      break;
     }
 
+    // Diagnose if there's already a setter access control attribute on
+    // this declaration with a different access level.
+    if (access != cast<SetterAccessAttr>(DuplicateAttribute)->getAccess()) {
+      diagnose(Loc, diag::multiple_access_level_modifiers)
+        .highlight(AttrRange);
+      diagnose(DuplicateAttribute->getLocation(),
+               diag::previous_access_level_modifier)
+          .highlight(DuplicateAttribute->getRange());
+
+      // Remove the reference to the duplicate attribute
+      // to avoid the extra diagnostic.
+      DuplicateAttribute = nullptr;
+    }
     break;
   }
 

test/attr/accessibility.swift

@@ -5,26 +5,26 @@ private // expected-note {{modifier already specified here}}
 private // expected-error {{duplicate modifier}}
 func duplicateAttr() {}
 
-private // expected-note {{modifier already specified here}}
-public // expected-error {{duplicate modifier}}
+private // expected-note {{previous modifier specified here}}
+public // expected-error {{multiple incompatible access-level modifiers specified}}
 func duplicateAttrChanged() {}
 
-private // expected-note 2 {{modifier already specified here}}
-public // expected-error {{duplicate modifier}}
-internal // expected-error {{duplicate modifier}}
+private // expected-note 2 {{previous modifier specified here}}
+public // expected-error {{multiple incompatible access-level modifiers specified}}
+internal // expected-error {{multiple incompatible access-level modifiers specified}}
 func triplicateAttrChanged() {}
 
-private // expected-note 3 {{modifier already specified here}}
-public // expected-error {{duplicate modifier}}
-package // expected-error {{duplicate modifier}}
-internal // expected-error {{duplicate modifier}}
+private // expected-note 3 {{previous modifier specified here}}
+public // expected-error {{multiple incompatible access-level modifiers specified}}
+package // expected-error {{multiple incompatible access-level modifiers specified}}
+internal // expected-error {{multiple incompatible access-level modifiers specified}}
 func quadruplicateAttrChanged() {}
 
-private // expected-note 4 {{modifier already specified here}}
-public // expected-error {{duplicate modifier}}
-package // expected-error {{duplicate modifier}}
-internal // expected-error {{duplicate modifier}}
-fileprivate // expected-error {{duplicate modifier}}
+private // expected-note 4 {{previous modifier specified here}}
+public // expected-error {{multiple incompatible access-level modifiers specified}}
+package // expected-error {{multiple incompatible access-level modifiers specified}}
+internal // expected-error {{multiple incompatible access-level modifiers specified}}
+fileprivate // expected-error {{multiple incompatible access-level modifiers specified}}
 func quintuplicateAttrChanged() {}
 
 private(set)
@@ -51,22 +51,22 @@ internal(set)
 package
 var customSetter6 = 0
 
-private(set) // expected-note {{modifier already specified here}}
-public(set) // expected-error {{duplicate modifier}}
+private(set) // expected-note {{previous modifier specified here}}
+public(set) // expected-error {{multiple incompatible access-level modifiers specified}}
 var customSetterDuplicateAttr = 0
 
-private(set) // expected-note {{modifier already specified here}}
-public // expected-note {{modifier already specified here}}
-public(set) // expected-error {{duplicate modifier}}
-private // expected-error {{duplicate modifier}}
+private(set) // expected-note {{previous modifier specified here}}
+public // expected-note {{previous modifier specified here}}
+public(set) // expected-error {{multiple incompatible access-level modifiers specified}}
+private // expected-error {{multiple incompatible access-level modifiers specified}}
 var customSetterDuplicateAttrsAllAround = 0
 
-private(set) // expected-note {{modifier already specified here}}
-package(set) // expected-error {{duplicate modifier}}
+private(set) // expected-note {{previous modifier specified here}}
+package(set) // expected-error {{multiple incompatible access-level modifiers specified}}
 var customSetterDuplicateAttr2 = 0
 
-package(set) // expected-note {{modifier already specified here}}
-public(set) // expected-error {{duplicate modifier}}
+package(set) // expected-note {{previous modifier specified here}}
+public(set) // expected-error {{multiple incompatible access-level modifiers specified}}
 public var customSetterDuplicateAttr3 = 0
 
 private(get) // expected-error{{expected 'set' as subject of 'private' modifier}}

test/attr/open.swift

@@ -6,10 +6,10 @@ import OpenHelpers
 
 /**** General structural limitations on open. ****/
 
-open private class OpenIsNotCompatibleWithPrivate {} // expected-error {{duplicate modifier}} expected-note{{modifier already specified here}}
-open fileprivate class OpenIsNotCompatibleWithFilePrivate {} // expected-error {{duplicate modifier}} expected-note{{modifier already specified here}}
-open internal class OpenIsNotCompatibleWithInternal {} // expected-error {{duplicate modifier}} expected-note{{modifier already specified here}}
-open public class OpenIsNotCompatibleWithPublic {} // expected-error {{duplicate modifier}} expected-note{{modifier already specified here}}
+open private class OpenIsNotCompatibleWithPrivate {} // expected-error {{multiple incompatible access-level modifiers specified}} expected-note{{previous modifier specified here}}
+open fileprivate class OpenIsNotCompatibleWithFilePrivate {} // expected-error {{multiple incompatible access-level modifiers specified}} expected-note{{previous modifier specified here}}
+open internal class OpenIsNotCompatibleWithInternal {} // expected-error {{multiple incompatible access-level modifiers specified}} expected-note{{previous modifier specified here}}
+open public class OpenIsNotCompatibleWithPublic {} // expected-error {{multiple incompatible access-level modifiers specified}} expected-note{{previous modifier specified here}}
 open open class OpenIsNotCompatibleWithOpen {} // expected-error {{duplicate modifier}} expected-note{{modifier already specified here}}
 
 open typealias OpenIsNotAllowedOnTypeAliases = Int // expected-error {{only classes and overridable class members can be declared 'open'; use 'public'}}