Merge pull request #82058 from a7medev/feat/access-control-set-fix-it

[Diagnostics] Add fix-its for missing `set` and `)` after access modifier

Author: hamishknight

include/swift/Parse/Parser.h

@@ -690,6 +690,14 @@ class Parser {
     return Context.LangOpts.EnableExperimentalConcurrency;
   }
 
+  /// Returns true if a Swift declaration starts after the current token,
+  /// otherwise returns false.
+  bool isNextStartOfSwiftDecl() {
+    BacktrackingScope backtrack(*this);
+    consumeToken();
+    return isStartOfSwiftDecl();
+  }
+
 public:
   InFlightDiagnostic diagnose(SourceLoc Loc, DiagRef Diag) {
     if (Diags.isDiagnosticPointsToFirstBadToken(Diag.getID()) &&

lib/Parse/ParseDecl.cpp

@@ -2908,7 +2908,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
       break;
     }
 
-    consumeAttributeLParen();
+    auto LParenLoc = consumeAttributeLParen();
 
     if (Tok.is(tok::code_complete)) {
       if (CodeCompletionCallbacks) {
@@ -2920,27 +2920,42 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
     }
 
     // Parse the subject.
-    if (Tok.isContextualKeyword("set")) {
-      consumeToken();
-    } else {
-      diagnose(Loc, diag::attr_access_expected_set, AttrName);
+    if (!Tok.isContextualKeyword("set")) {
+      auto diag = diagnose(Loc, diag::attr_access_expected_set, AttrName);
 
-      // Minimal recovery: if there's a single token and then an r_paren,
-      // consume them both. If there's just an r_paren, consume that.
-      if (!consumeIf(tok::r_paren)) {
-        if (Tok.isNot(tok::l_paren) && peekToken().is(tok::r_paren)) {
-          consumeToken();
-          consumeToken(tok::r_paren);
-        }
+      if (Tok.is(tok::r_paren)) {
+        // Suggest `set` between empty parens e.g. `private()` -> `private(set)`
+        auto SetLoc = consumeToken(tok::r_paren);
+        diag.fixItInsert(SetLoc, "set");
+      } else if (!Tok.is(tok::l_paren) && peekToken().is(tok::r_paren)) {
+        // Suggest `set` in place of an invalid token between parens
+        // e.g. `private(<invalid>)` -> `private(set)`
+        auto SetLoc = consumeToken();
+        diag.fixItReplace(SetLoc, "set");
+        consumeToken(tok::r_paren);
+      } else if (isNextStartOfSwiftDecl()) {
+        // Suggest `set)` in place of an invalid token after l_paren followed by
+        // a valid declaration start.
+        // e.g. `private(<invalid> var x: Int` -> `private(set) var x: Int`
+        auto SetLoc = consumeToken();
+        diag.fixItReplace(SetLoc, "set)");
+      } else {
+        // Suggest `set)` after l_paren if not followed by a valid declaration
+        // e.g. `private( <invalid>` -> `private(set) <invalid>`
+        diag.fixItInsertAfter(LParenLoc, "set)");
       }
+
       return makeParserSuccess();
     }
 
+    auto SubjectLoc = consumeToken();
+
     AttrRange = SourceRange(Loc, Tok.getLoc());
 
     if (!consumeIf(tok::r_paren)) {
       diagnose(Loc, diag::attr_expected_rparen, AttrName,
-               DeclAttribute::isDeclModifier(DK));
+               DeclAttribute::isDeclModifier(DK))
+          .fixItInsertAfter(SubjectLoc, ")");
       return makeParserSuccess();
     }
 

test/attr/accessibility.swift

@@ -69,32 +69,38 @@ 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}}
+private(get) // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-12=set}}
 var invalidSubject = 0
 
-private(42) // expected-error{{expected 'set' as subject of 'private' modifier}}
+private(42) // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-11=set}}
 var invalidSubject2 = 0
 
 private(a bunch of random tokens) // expected-error{{expected 'set' as subject of 'private' modifier}} expected-error{{expected declaration}}
 var invalidSubject3 = 0
 
 
-package(get) // expected-error{{expected 'set' as subject of 'package' modifier}}
+package(get) // expected-error{{expected 'set' as subject of 'package' modifier}} {{9-12=set}}
 var invalidSubject4 = 0
 
-package(42) // expected-error{{expected 'set' as subject of 'package' modifier}}
+package(42) // expected-error{{expected 'set' as subject of 'package' modifier}} {{9-11=set}}
 var invalidSubject5 = 0
 
-private(set // expected-error{{expected ')' in 'private' modifier}}
+private((())) // expected-error{{expected 'set' as subject of 'private' modifier}} expected-error{{expected declaration}}
+var invalidSubject6 = 0
+
+private( missingFunc(_ x: Int) -> Bool // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-9=set)}} expected-error{{expected declaration}}
+let independentVar1 = 0
+
+private(set // expected-error{{expected ')' in 'private' modifier}} {{12-12=)}}
 var unterminatedSubject = 0
 
-private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} expected-error{{expected declaration}}
+private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-11=set)}}
 var unterminatedInvalidSubject = 0
 
-private() // expected-error{{expected 'set' as subject of 'private' modifier}}
+private() // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-9=set}}
 var emptySubject = 0
 
-private( // expected-error{{expected 'set' as subject of 'private' modifier}}
+private( // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-9=set)}}
 var unterminatedEmptySubject = 0
 
 // Check that the parser made it here.
@@ -341,3 +347,10 @@ package extension PkgGenericStruct where Param: InternalProto {} // expected-err
 extension PkgGenericStruct where Param: InternalProto {
   package func foo() {} // expected-error {{cannot declare a package instance method in an extension with internal requirements}} {{3-10=internal}}
 }
+
+func f() {}
+private( // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-9=set)}}
+if true { // expected-error{{expected declaration}} {{none}}
+  f()
+}
+var unrelatedVar = "Swift"