Merge pull request #2339 from bnbarham/cherry-macro-system-fixes

[5.10] Cherry-pick a couple of macro system fixes for attached accessor macros

Author: ahoppen

Sources/SwiftParser/Expressions.swift

@@ -792,7 +792,7 @@ extension Parser {
       }
 
       // Check for a trailing closure, if allowed.
-      if self.at(.leftBrace) && self.withLookahead({ $0.atValidTrailingClosure(flavor: flavor) }) {
+      if self.at(.leftBrace) && !leadingExpr.raw.kind.isLiteral && self.withLookahead({ $0.atValidTrailingClosure(flavor: flavor) }) {
         // FIXME: if Result has a trailing closure, break out.
         // Add dummy blank argument list to the call expression syntax.
         let list = RawLabeledExprListSyntax(elements: [], arena: self.arena)
@@ -1498,17 +1498,38 @@ extension Parser {
     var elements = [RawSyntax]()
     do {
       var collectionProgress = LoopProgressCondition()
-      COLLECTION_LOOP: while self.hasProgressed(&collectionProgress) {
+      var keepGoing: RawTokenSyntax?
+      COLLECTION_LOOP: repeat {
         elementKind = self.parseCollectionElement(elementKind)
 
+        /// Whether expression of an array element or the value of a dictionary
+        /// element is missing. If this is the case, we shouldn't recover from
+        /// a missing comma since most likely the closing `]` is missing.
+        var elementIsMissingExpression: Bool {
+          switch elementKind! {
+          case .dictionary(_, _, _, let value):
+            return value.is(RawMissingExprSyntax.self)
+          case .array(let rawExprSyntax):
+            return rawExprSyntax.is(RawMissingExprSyntax.self)
+          }
+        }
+
         // Parse the ',' if exists.
-        let comma = self.consume(if: .comma)
+        if let token = self.consume(if: .comma) {
+          keepGoing = token
+        } else if !self.at(.rightSquare, .endOfFile) && !self.atStartOfLine && !elementIsMissingExpression && !self.atStartOfDeclaration()
+          && !self.atStartOfStatement(preferExpr: false)
+        {
+          keepGoing = missingToken(.comma)
+        } else {
+          keepGoing = nil
+        }
 
         switch elementKind! {
         case .array(let el):
           let element = RawArrayElementSyntax(
             expression: el,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1522,7 +1543,7 @@ extension Parser {
             unexpectedBeforeColon,
             colon: colon,
             value: value,
-            trailingComma: comma,
+            trailingComma: keepGoing,
             arena: self.arena
           )
           if element.isEmpty {
@@ -1531,29 +1552,7 @@ extension Parser {
             elements.append(RawSyntax(element))
           }
         }
-
-        // If we saw a comma, that's a strong indicator we have more elements
-        // to process. If that's not the case, we have to do some legwork to
-        // determine if we should bail out.
-        guard comma == nil || self.at(.rightSquare, .endOfFile) else {
-          continue
-        }
-
-        // If we found EOF or the closing square bracket, bailout.
-        if self.at(.rightSquare, .endOfFile) {
-          break
-        }
-
-        // If the next token is at the beginning of a new line and can never start
-        // an element, break.
-        if self.atStartOfLine
-          && (self.at(.rightBrace, .poundEndif)
-            || self.atStartOfDeclaration()
-            || self.atStartOfStatement(preferExpr: false))
-        {
-          break
-        }
-      }
+      } while keepGoing != nil && self.hasProgressed(&collectionProgress)
     }
 
     let (unexpectedBeforeRSquare, rsquare) = self.expect(.rightSquare)
@@ -2519,3 +2518,14 @@ extension Parser.Lookahead {
     }
   }
 }
+
+extension SyntaxKind {
+  fileprivate var isLiteral: Bool {
+    switch self {
+    case .arrayExpr, .booleanLiteralExpr, .dictionaryExpr, .floatLiteralExpr, .integerLiteralExpr, .nilLiteralExpr, .regexLiteralExpr, .stringLiteralExpr:
+      return true
+    default:
+      return false
+    }
+  }
+}

Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift

@@ -267,7 +267,8 @@ private func expandAccessorMacroWithoutExistingAccessors(
       conformanceList: nil,
       in: context,
       indentationWidth: indentationWidth
-    )
+    ),
+    !expanded.isEmpty
   else {
     return nil
   }
@@ -306,7 +307,7 @@ private func expandAccessorMacroWithExistingAccessors(
     return nil
   }
 
-  // Separate the accessor from any existing accessors by two spaces
+  // Separate the accessor from any existing accessors by an empty line
   let indentedSource = "\n" + expanded.indented(by: attachedTo.indentationOfFirstLine + indentationWidth)
   return "\(raw: indentedSource)"
 }
@@ -635,13 +636,26 @@ private class MacroApplication<Context: MacroExpansionContext>: SyntaxRewriter {
       context.addDiagnostics(from: MacroApplicationError.accessorMacroOnVariableWithMultipleBindings, node: node)
       return DeclSyntax(node)
     }
-    node.bindings[node.bindings.startIndex].accessorBlock = expandAccessors(of: node, existingAccessors: binding.accessorBlock)
+
+    let expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock)
+    if expansion.accessors != binding.accessorBlock {
+      if binding.initializer != nil, expansion.expandsGetSet {
+        // The accessor block will have a leading space, but there will already be a
+        // space between the variable and the to-be-removed initializer. Remove the
+        // leading trivia on the accessor block so we don't double up.
+        node.bindings[node.bindings.startIndex].accessorBlock = expansion.accessors?.with(\.leadingTrivia, [])
+        node.bindings[node.bindings.startIndex].initializer = nil
+      } else {
+        node.bindings[node.bindings.startIndex].accessorBlock = expansion.accessors
+      }
+    }
+
     return DeclSyntax(node)
   }
 
   override func visit(_ node: SubscriptDeclSyntax) -> DeclSyntax {
     var node = super.visit(node).cast(SubscriptDeclSyntax.self)
-    node.accessorBlock = expandAccessors(of: node, existingAccessors: node.accessorBlock)
+    node.accessorBlock = expandAccessors(of: node, existingAccessors: node.accessorBlock).accessors
     return DeclSyntax(node)
   }
 }
@@ -802,14 +816,23 @@ extension MacroApplication {
     }
   }
 
-  /// Expand all 'accessor' macros attached to `storage` and return the `storage`
-  /// node.
+  /// Expand all 'accessor' macros attached to `storage`.
   ///
-  /// - Returns: The storage node with all macro-synthesized accessors applied.
-  private func expandAccessors(of storage: some DeclSyntaxProtocol, existingAccessors: AccessorBlockSyntax?) -> AccessorBlockSyntax? {
+  /// - Returns: The final accessors block that includes both the existing
+  ///   and expanded accessors, as well as whether any `get`/`set` were
+  ///   expanded (in which case any initializer on `storage` should be
+  ///   removed).
+  private func expandAccessors(of storage: some DeclSyntaxProtocol, existingAccessors: AccessorBlockSyntax?) -> (
+    accessors: AccessorBlockSyntax?, expandsGetSet: Bool
+  ) {
     let accessorMacros = macroAttributes(attachedTo: DeclSyntax(storage), ofType: AccessorMacro.Type.self)
 
     var newAccessorsBlock = existingAccessors
+    var expandsGetSet = false
+    func checkExpansions(_ accessors: AccessorDeclListSyntax?) {
+      guard let accessors else { return }
+      expandsGetSet = expandsGetSet || accessors.contains(where: \.isGetOrSet)
+    }
 
     for macro in accessorMacros {
       do {
@@ -827,6 +850,8 @@ extension MacroApplication {
             in: context,
             indentationWidth: indentationWidth
           ) {
+            checkExpansions(newAccessors)
+
             // If existingAccessors is not `nil`, then we also set
             // `newAccessorBlock` above to a a non-nil value, so
             // `newAccessorsBlock` also isn’t `nil`.
@@ -835,31 +860,33 @@ extension MacroApplication {
               indentationWidth: self.indentationWidth
             )
           }
-        } else {
-          let newAccessors = try expandAccessorMacroWithoutExistingAccessors(
-            definition: macro.definition,
-            attributeNode: macro.attributeNode,
-            attachedTo: DeclSyntax(storage),
-            in: context,
-            indentationWidth: indentationWidth
-          )
-          if newAccessorsBlock == nil {
-            newAccessorsBlock = newAccessors
-          } else if let newAccessors = newAccessors {
-            guard case .accessors(let accessorList) = newAccessors.accessors else {
-              throw MacroApplicationError.malformedAccessor
-            }
-            newAccessorsBlock = newAccessorsBlock!.addingAccessors(
+        } else if let newAccessors = try expandAccessorMacroWithoutExistingAccessors(
+          definition: macro.definition,
+          attributeNode: macro.attributeNode,
+          attachedTo: DeclSyntax(storage),
+          in: context,
+          indentationWidth: indentationWidth
+        ) {
+          guard case .accessors(let accessorList) = newAccessors.accessors else {
+            throw MacroApplicationError.malformedAccessor
+          }
+
+          checkExpansions(accessorList)
+
+          if let oldBlock = newAccessorsBlock {
+            newAccessorsBlock = oldBlock.addingAccessors(
               from: accessorList,
               indentationWidth: self.indentationWidth
             )
+          } else {
+            newAccessorsBlock = newAccessors
           }
         }
       } catch {
         context.addDiagnostics(from: error, node: macro.attributeNode)
       }
     }
-    return newAccessorsBlock
+    return (newAccessorsBlock, expandsGetSet)
   }
 }
 
@@ -1063,3 +1090,9 @@ private extension AttributeSyntax {
     return (detach(in: context, foldingWith: operatorTable) as Syntax).cast(Self.self)
   }
 }
+
+private extension AccessorDeclSyntax {
+  var isGetOrSet: Bool {
+    return accessorSpecifier.tokenKind == .keyword(.get) || accessorSpecifier.tokenKind == .keyword(.set)
+  }
+}

Tests/SwiftParserTest/DeclarationTests.swift

@@ -2513,4 +2513,11 @@ final class DeclarationTests: ParserTestCase {
       """
     )
   }
+
+  func testLiteralInitializerWithTrailingClosure() {
+    assertParse(
+      "let foo = 1 { return 1 }",
+      substructure: AccessorBlockSyntax(accessors: .getter([CodeBlockItemSyntax("return 1")]))
+    )
+  }
 }