Merge pull request #84128 from rintaro/parse-attr-lparen-space-rdar147785544

[Parse] Change whitespace rule between attribute name and '(' in Swift 6

Author: rintaro

include/swift/Parse/Parser.h

@@ -574,7 +574,11 @@ class Parser {
   /// the '(' by a space.
   ///
   /// If the next token is not '(' or it's on a new line, return false.
-  bool consumeIfAttributeLParen();
+  bool consumeIfAttributeLParen(bool isCustomAttr = false);
+
+  /// Check if the current token is '(' and it looks like a start of an
+  /// attribute argument list.
+  bool isAtAttributeLParen(bool isCustomAttr = false);
 
   bool consumeIfNotAtStartOfLine(tok K) {
     if (Tok.isAtStartOfLine()) return false;
@@ -1147,7 +1151,6 @@ class Parser {
                                   SourceLoc AtEndLoc,
                                   bool isFromClangAttribute = false);
 
-  bool isCustomAttributeArgument();
   bool canParseCustomAttribute();
 
   /// Parse a custom attribute after the initial '@'.

lib/Parse/ParseDecl.cpp

@@ -877,7 +877,7 @@ bool Parser::parseSpecializeAttribute(
   assert(ClosingBrace == tok::r_paren || ClosingBrace == tok::r_square);
 
   SourceLoc lParenLoc;
-  if (Tok.is(tok::l_paren)) {
+  if (isAtAttributeLParen()) {
     lParenLoc = consumeAttributeLParen();
   } else {
     // SIL parsing is positioned at _specialize when entering this and parses
@@ -2261,7 +2261,7 @@ Parser::parseMacroRoleAttribute(
     break;
   }
 
-  if (!Tok.isFollowingLParen()) {
+  if (!isAtAttributeLParen()) {
     diagnose(Tok, diag::attr_expected_lparen, attrName, false);
     return makeParserError();
   }
@@ -2503,7 +2503,7 @@ static std::optional<Identifier> parseSingleAttrOptionImpl(
   };
   bool isDeclModifier = DeclAttribute::isDeclModifier(DK);
 
-  if (!P.Tok.isFollowingLParen()) {
+  if (!P.isAtAttributeLParen()) {
     if (allowOmitted)
       return Identifier();
 
@@ -2883,7 +2883,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
       .Case("public", AccessLevel::Public)
       .Case("open", AccessLevel::Open);
 
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       // Normal access control attribute.
       AttrRange = Loc;
 
@@ -3467,7 +3467,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
   }
   case DeclAttrKind::PrivateImport: {
     // Parse the leading '('.
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return makeParserSuccess();
@@ -3516,7 +3516,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
   }
   case DeclAttrKind::ObjC: {
     // Unnamed @objc attribute.
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       auto attr = ObjCAttr::createUnnamed(Context, AtLoc, Loc);
       Attributes.add(attr);
       break;
@@ -3584,7 +3584,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
 
   case DeclAttrKind::DynamicReplacement: {
     // Parse the leading '('.
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return makeParserSuccess();
@@ -3635,7 +3635,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
 
   case DeclAttrKind::TypeEraser: {
     // Parse leading '('
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return makeParserSuccess();
@@ -3665,7 +3665,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
 
   case DeclAttrKind::Specialize:
   case DeclAttrKind::Specialized: {
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return makeParserSuccess();
@@ -3894,7 +3894,7 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
     break;
   }
   case DeclAttrKind::RawLayout: {
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
                DeclAttribute::isDeclModifier(DK));
       return makeParserSuccess();
@@ -4164,27 +4164,11 @@ bool Parser::parseVersionTuple(llvm::VersionTuple &Version,
   return false;
 }
 
-bool Parser::isCustomAttributeArgument() {
-  BacktrackingScope backtrack(*this);
-  if (skipSingle().hasCodeCompletion())
-    return true;
-
-  // If we have any keyword, identifier, or token that follows a function
-  // type's parameter list, this is a parameter list and not an attribute.
-  // Alternatively, we might have a token that illustrates we're not going to
-  // get anything following the attribute, which means the parentheses describe
-  // what follows the attribute.
-  return !Tok.isAny(
-      tok::arrow, tok::kw_throw, tok::kw_throws, tok::kw_rethrows,
-      tok::r_paren, tok::r_brace, tok::r_square, tok::r_angle) &&
-    !Tok.isContextualKeyword("async") && !Tok.isContextualKeyword("reasync") ;
-}
-
 bool Parser::canParseCustomAttribute() {
   if (!canParseType())
     return false;
 
-  if (Tok.isFollowingLParen() && isCustomAttributeArgument())
+  if (isAtAttributeLParen(/*isCustomAttribute=*/true))
     skipSingle();
 
   return true;
@@ -4196,7 +4180,7 @@ ParserResult<CustomAttr> Parser::parseCustomAttribute(SourceLoc atLoc) {
   // Parse a custom attribute.
   auto type = parseType(diag::expected_type, ParseTypeReason::CustomAttribute);
   if (type.hasCodeCompletion() || type.isNull()) {
-    if (Tok.isFollowingLParen() && isCustomAttributeArgument())
+    if (isAtAttributeLParen(/*isCustomAttribute=*/true))
       skipSingle();
 
     return ParserResult<CustomAttr>(ParserStatus(type));
@@ -4208,7 +4192,11 @@ ParserResult<CustomAttr> Parser::parseCustomAttribute(SourceLoc atLoc) {
   ParserStatus status;
   ArgumentList *argList = nullptr;
   CustomAttributeInitializer *initContext = nullptr;
-  if (Tok.isFollowingLParen() && isCustomAttributeArgument()) {
+  if (isAtAttributeLParen(/*isCustomAttribute=*/true)) {
+    if (getEndOfPreviousLoc() != Tok.getLoc()) {
+      diagnose(getEndOfPreviousLoc(), diag::attr_extra_whitespace_before_lparen)
+          .warnUntilSwiftVersion(6);
+    }
     // If we have no local context to parse the initial value into, create
     // one for the attribute.
     std::optional<ParseFunctionBody> initParser;
@@ -4217,10 +4205,6 @@ ParserResult<CustomAttr> Parser::parseCustomAttribute(SourceLoc atLoc) {
       initContext = CustomAttributeInitializer::create(CurDeclContext);
       initParser.emplace(*this, initContext);
     }
-    if (getEndOfPreviousLoc() != Tok.getLoc()) {
-      diagnose(getEndOfPreviousLoc(), diag::attr_extra_whitespace_before_lparen)
-          .warnUntilSwiftVersion(6);
-    }
     auto result = parseArgumentList(tok::l_paren, tok::r_paren,
                                     /*isExprBasic*/ true,
                                     /*allowTrailingClosure*/ false);
@@ -4378,7 +4362,7 @@ ParserStatus Parser::parseDeclAttribute(DeclAttributes &Attributes,
     SourceLoc attrLoc = consumeToken();
 
     // @warn_unused_result with no arguments.
-    if (!Tok.isFollowingLParen()) {
+    if (!isAtAttributeLParen()) {
       diagnose(AtLoc, diag::attr_warn_unused_result_removed)
         .fixItRemove(SourceRange(AtLoc, attrLoc));
 
@@ -4462,7 +4446,7 @@ ParserStatus Parser::parseDeclAttribute(DeclAttributes &Attributes,
 
   // Recover by eating @foo(...) when foo is not known.
   consumeToken();
-  if (Tok.isFollowingLParen())
+  if (isAtAttributeLParen())
     skipSingle();
 
   return makeParserError();
@@ -4745,15 +4729,8 @@ ParserStatus Parser::parseTypeAttribute(TypeOrCustomAttr &result,
       // Recover by eating @foo(...) when foo is not known.
       consumeToken();
 
-      if (Tok.is(tok::l_paren) && getEndOfPreviousLoc() == Tok.getLoc()) {
-        CancellableBacktrackingScope backtrack(*this);
+      if (isAtAttributeLParen(/*isCustomAttr=*/true))
         skipSingle();
-        // If we found '->', or 'throws' after paren, it's likely a parameter
-        // of function type.
-        if (Tok.isNot(tok::arrow, tok::kw_throws, tok::kw_rethrows,
-                      tok::kw_throw))
-          backtrack.cancelBacktrack();
-      }
 
       return makeParserError();
     }
@@ -4814,7 +4791,7 @@ ParserStatus Parser::parseTypeAttribute(TypeOrCustomAttr &result,
 
   case TypeAttrKind::Isolated: {
     SourceLoc lpLoc = Tok.getLoc(), kindLoc, rpLoc;
-    if (!consumeIfNotAtStartOfLine(tok::l_paren)) {
+    if (!consumeIfAttributeLParen()) {
       if (!justChecking) {
         diagnose(Tok, diag::attr_isolated_expected_lparen);
         // TODO: should we suggest removing the `@`?
@@ -4862,7 +4839,7 @@ ParserStatus Parser::parseTypeAttribute(TypeOrCustomAttr &result,
   case TypeAttrKind::Opened: {
     // Parse the opened existential ID string in parens
     SourceLoc beginLoc = Tok.getLoc(), idLoc, endLoc;
-    if (!consumeAttributeLParen()) {
+    if (!consumeIfAttributeLParen()) {
       if (!justChecking)
         diagnose(Tok, diag::opened_attribute_expected_lparen);
       return makeParserError();
@@ -5069,7 +5046,7 @@ ParserResult<LifetimeEntry> Parser::parseLifetimeEntry(SourceLoc loc) {
     return std::nullopt;
   };
 
-  if (!Tok.isFollowingLParen()) {
+  if (!isAtAttributeLParen()) {
     diagnose(loc, diag::expected_lparen_after_lifetime_dependence);
     status.setIsParseError();
     return status;
@@ -5777,6 +5754,7 @@ static bool consumeIfParenthesizedNonisolated(Parser &P) {
 }
 
 static void skipAttribute(Parser &P) {
+  P.consumeToken(tok::at_sign);
   // Consider unexpected tokens to be incomplete attributes.
 
   // Parse the attribute name, which can be qualified, have
@@ -5793,12 +5771,8 @@ static void skipAttribute(Parser &P) {
   } while (P.consumeIf(tok::period));
 
   // Skip an argument clause after the attribute name.
-  if (P.consumeIf(tok::l_paren)) {
-    while (P.Tok.isNot(tok::r_brace, tok::eof, tok::pound_endif)) {
-      if (P.consumeIf(tok::r_paren)) break;
-      P.skipSingle();
-    }
-  }
+  if (P.isAtAttributeLParen())
+    P.skipSingle();
 }
 
 bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes,
@@ -5842,7 +5816,7 @@ bool Parser::isStartOfSwiftDecl(bool allowPoundIfAttributes,
   // in positions like generic argument lists.
   if (Tok.is(tok::at_sign)) {
     BacktrackingScope backtrack(*this);
-    while (consumeIf(tok::at_sign))
+    while (Tok.is(tok::at_sign))
       skipAttribute(*this);
 
     // If this attribute is the last element in the block,

lib/Parse/ParseStmt.cpp

@@ -230,7 +230,7 @@ static bool isAtStartOfSwitchCase(Parser &parser,
 
     parser.consumeToken(tok::at_sign);
     parser.consumeToken(tok::identifier);
-    if (parser.Tok.is(tok::l_paren))
+    if (parser.isAtAttributeLParen())
       parser.skipSingle();
   }
 
@@ -404,7 +404,10 @@ ParserStatus Parser::parseBraceItems(SmallVectorImpl<ASTNode> &Entries,
       ParserStatus Status = parseLineDirective(false);
       BraceItemsStatus |= Status;
       NeedParseErrorRecovery = Status.isErrorOrHasCompletion();
-    } else if (isStartOfSwiftDecl()) {
+    } else if (isStartOfSwiftDecl() ||
+               // Force parsing '@<identifier>' as a declaration, as there's no
+               // valid expression or statement starting with an attribute.
+               (Tok.is(tok::at_sign) && peekToken().is(tok::identifier))) {
       SmallVector<Decl*, 8> TmpDecls;
       ParserStatus DeclResult =
           parseDecl(IsAtStartOfLineOrPreviousHadSemi,
@@ -2711,7 +2714,7 @@ ParserResult<CaseStmt> Parser::parseStmtCase(bool IsActive) {
       }
       consumeToken(tok::identifier);
 
-      if (Tok.is(tok::l_paren)) {
+      if (isAtAttributeLParen()) {
         diagnose(Tok, diag::unexpected_lparen_in_attribute, "unknown");
         skipSingle();
       }
@@ -2722,7 +2725,7 @@ ParserResult<CaseStmt> Parser::parseStmtCase(bool IsActive) {
       diagnose(Tok, diag::unknown_attr_name, Tok.getText());
       consumeToken(tok::identifier);
 
-      if (Tok.is(tok::l_paren))
+      if (isAtAttributeLParen())
         skipSingle();
     }
   }

lib/Parse/Parser.cpp

@@ -612,14 +612,49 @@ SourceLoc Parser::consumeAttributeLParen() {
   return consumeToken(tok::l_paren);
 }
 
-bool Parser::consumeIfAttributeLParen() {
-  if (!Tok.isFollowingLParen()) {
+bool Parser::consumeIfAttributeLParen(bool isCustomAttr) {
+  if (!isAtAttributeLParen(isCustomAttr))
     return false;
-  }
-  consumeAttributeLParen();
+  (void)consumeAttributeLParen();
   return true;
 }
 
+bool Parser::isAtAttributeLParen(bool isCustomAttr) {
+  if (!Tok.isFollowingLParen())
+    return false;
+
+  if (Context.isSwiftVersionAtLeast(6)) {
+    // No-space '(' are always arguments.
+    if (getEndOfPreviousLoc() == Tok.getLoc())
+      return true;
+
+    // Otherwise it's an error, but for recovery, parse it as an argument list
+    // if it's obvious.
+    BacktrackingScope backtrack(*this);
+    skipSingle();
+    return Tok.is(tok::at_sign) || isStartOfSwiftDecl();
+  } else {
+    // In <=5, builtin attributes only checks 'isFollowingLParen()'.
+    if (!isCustomAttr)
+      return true;
+
+    BacktrackingScope backtrack(*this);
+    if (skipSingle().hasCodeCompletion())
+      return true;
+
+    // If we have any keyword, identifier, or token that follows a function
+    // type's parameter list, this is a parameter list and not an attribute.
+    // Alternatively, we might have a token that illustrates we're not going to
+    // get anything following the attribute, which means the parentheses
+    // describe what follows the attribute.
+    return (!Tok.isAny(tok::arrow, tok::kw_throw, tok::kw_throws,
+                       tok::kw_rethrows, tok::r_paren, tok::r_brace,
+                       tok::r_square, tok::r_angle) &&
+            !Tok.isContextualKeyword("async") &&
+            !Tok.isContextualKeyword("reasync"));
+  }
+}
+
 SourceLoc Parser::consumeStartingCharacterOfCurrentToken(tok Kind, size_t Len) {
   // Consumes prefix of token and returns its location.
   // (like '?', '<', '>' or '!' immediately followed by '<') 

test/Macros/macro_self.swift

@@ -1,12 +1,14 @@
 // RUN: %target-swift-frontend -parse %s -verify
 
-@freestanding(expression) // expected-error {{expected expression}}
+@freestanding(expression)
 macro self() = #externalMacro(module: "MacroDefinition", type: "InvalidMacro")
+// expected-error@-1 {{expected declaration}}
 
 func sync() {}
 
-@freestanding(expression) // expected-error {{expected expression}}
+@freestanding(expression)
 macro Self() = #externalMacro(module: "MacroDefinition", type: "InvalidMacro")
+// expected-error@-1 {{expected declaration}}
 
 func testSelfAsFreestandingMacro() {
   _ = #self