[Parser] Improve diagnostics for access-level modifiers

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

Implement the new diagnostics for setter access control attributes

Update the tests with the new diagnostics' messages

Keep the original duplicate message if the access levels match

Removing a DuplicateAttribute reassignment and improving the other

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", (StringRef))
 ERROR(mutually_exclusive_attrs,none,
       "'%0' contradicts previous %select{attribute|modifier}2 '%1'", (StringRef, StringRef, bool))
 

lib/Parse/ParseDecl.cpp

@@ -3063,9 +3063,26 @@ 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,
+                 DuplicateAttribute->getAttrName())
+          .highlight(DuplicateAttribute->getRange());
+        
+        // Remove the reference to the duplicate attribute
+        // to avoid the extra diagnostic.
+        DuplicateAttribute = nullptr;
+      }
       break;
     }
 
@@ -3095,11 +3112,29 @@ 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,
+               DuplicateAttribute->getAttrName())
+        .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'}}