Merge pull request #64095 from bnbarham/do-not-change-refactoring

[Macros] Do not edit macro buffer or position when refactoring

Author: bnbarham

lib/AST/Decl.cpp

@@ -6127,10 +6127,10 @@ void AbstractStorageDecl::setAccessors(SourceLoc lbraceLoc,
                                        ArrayRef<AccessorDecl *> accessors,
                                        SourceLoc rbraceLoc) {
   // This method is called after we've already recorded an accessors clause
-  // only on recovery paths and only when that clause was empty.
+  // only on recovery paths and only when that clause was empty, or when a
+  // macro expands to accessors (in which case, we may already have accessors).
   auto record = Accessors.getPointer();
   if (record) {
-    assert(record->getAllAccessors().empty());
     for (auto accessor : accessors) {
       (void) record->addOpaqueAccessor(accessor);
     }

lib/ASTGen/Sources/ASTGen/Macros.swift

@@ -523,22 +523,7 @@ func expandAttachedMacro(
   }
 
   // Fixup the source.
-  var expandedSource: String
-  switch MacroRole(rawValue: rawMacroRole)! {
-  case .Accessor:
-    expandedSource = expandedSources.joined(separator: "\n\n")
-  case .Member:
-    expandedSource = expandedSources.joined(separator: "\n\n")
-  case .MemberAttribute:
-    expandedSource = expandedSources.joined(separator: " ")
-  case .Peer:
-    expandedSource = expandedSources.joined(separator: "\n\n")
-  case .Conformance:
-    expandedSource = expandedSources.joined(separator: "\n\n")
-  case .Expression,
-      .FreestandingDeclaration:
-    fatalError("unreachable")
-  }
+  var expandedSource: String = collapse(expansions: expandedSources, for: MacroRole(rawValue: rawMacroRole)!, attachedTo: declarationNode)
 
   // Form the result buffer for our caller.
   expandedSource.withUTF8 { utf8 in
@@ -837,3 +822,44 @@ func expandAttachedMacroInProcess(
 
   return expandedSources
 }
+
+fileprivate func collapse<Node: SyntaxProtocol>(
+  expansions: [String],
+  for role: MacroRole,
+  attachedTo declarationNode: Node
+) -> String {
+  var separator: String = "\n\n"
+  var prefix: String = ""
+  var suffix: String = ""
+
+  switch role {
+  case .Accessor:
+    if let varDecl = declarationNode.as(VariableDeclSyntax.self),
+       let binding = varDecl.bindings.first,
+       binding.accessor == nil {
+      prefix = "{\n"
+      suffix = "\n}"
+    }
+  case .Member:
+    prefix = "\n\n"
+    suffix = "\n"
+  case .MemberAttribute:
+    separator = " "
+    suffix = " "
+  case .Peer:
+    prefix = "\n\n"
+    suffix = "\n"
+  case .Conformance:
+    prefix = "\n\n"
+    suffix = "\n"
+  case .Expression,
+      .FreestandingDeclaration:
+    fatalError("unreachable")
+  }
+
+  let separated = expansions.joined(separator: separator)
+  if separated.isEmpty {
+    return separated
+  }
+  return prefix + separated + suffix
+}

lib/Parse/ParseDecl.cpp

@@ -7098,11 +7098,13 @@ void Parser::parseTopLevelAccessors(
     staticLoc = binding->getStaticLoc();
   }
 
+  bool hadLBrace = consumeIf(tok::l_brace);
+
   ParserStatus status;
   ParsedAccessors accessors;
   bool hasEffectfulGet = false;
   bool parsingLimitedSyntax = false;
-  while (!Tok.is(tok::eof)) {
+  while (!Tok.isAny(tok::r_brace, tok::eof)) {
     DeclAttributes attributes;
     AccessorKind kind = AccessorKind::Get;
     SourceLoc loc;
@@ -7116,6 +7118,10 @@ void Parser::parseTopLevelAccessors(
     );
   }
 
+  if (hadLBrace && Tok.is(tok::r_brace)) {
+    consumeToken(tok::r_brace);
+  }
+
   // Consume remaining tokens.
   // FIXME: Emit a diagnostic here?
   while (!Tok.is(tok::eof)) {

lib/Refactoring/Refactoring.cpp

@@ -8617,51 +8617,6 @@ bool RefactoringActionExpandMacro::isApplicable(ResolvedCursorInfoPtr Info,
   return !getMacroExpansionBuffers(Diag.SourceMgr, Info).empty();
 }
 
-/// Given the expanded code for a particular macro, perform whitespace
-/// adjustments to make the refactoring more.
-static StringRef adjustMacroExpansionWhitespace(
-    GeneratedSourceInfo::Kind kind, StringRef expandedCode,
-    llvm::SmallString<64> &scratch
-) {
-  scratch.clear();
-
-  switch (kind) {
-  case GeneratedSourceInfo::ExpressionMacroExpansion:
-  case GeneratedSourceInfo::FreestandingDeclMacroExpansion:
-    return expandedCode;
-
-  case GeneratedSourceInfo::AccessorMacroExpansion:
-      // For accessor macros, wrap curly braces around the buffer contents.
-    scratch += "{\n";
-    scratch += expandedCode;
-    scratch += "\n}";
-    return scratch;
-
-  case GeneratedSourceInfo::MemberAttributeMacroExpansion:
-      // For member-attribute macros, add a space at the end.
-    scratch += expandedCode;
-    scratch += " ";
-    return scratch;
-
-  case GeneratedSourceInfo::PeerMacroExpansion:
-  case GeneratedSourceInfo::ConformanceMacroExpansion:
-      // For peers and conformances, add a newline to create some separation.
-    scratch += "\n";
-    LLVM_FALLTHROUGH;
-
-  case GeneratedSourceInfo::MemberMacroExpansion:
-      // For members, add a newline.
-    scratch += "\n";
-    scratch += expandedCode;
-    scratch += "\n";
-    return scratch;
-
-  case GeneratedSourceInfo::ReplacedFunctionBody:
-  case GeneratedSourceInfo::PrettyPrinted:
-    return expandedCode;
-  }
-}
-
 bool RefactoringActionExpandMacro::performChange() {
   auto bufferIDs = getMacroExpansionBuffers(SM, CursorInfo);
   if (bufferIDs.empty())
@@ -8682,33 +8637,6 @@ bool RefactoringActionExpandMacro::performChange() {
         rewrittenBuffer.empty())
       continue;
 
-    auto originalSourceRange = generatedInfo->originalSourceRange;
-
-    SmallString<64> scratchBuffer;
-    if (generatedInfo->kind == GeneratedSourceInfo::MemberMacroExpansion) {
-      // For member macros, adjust the source range from before-the-close-brace
-      // to after-the-open-brace.
-      ASTNode node = ASTNode::getFromOpaqueValue(generatedInfo->astNode);
-      auto decl = node.dyn_cast<Decl *>();
-      if (!decl)
-        continue;
-
-      SourceLoc leftBraceLoc;
-      if (auto nominal = dyn_cast<NominalTypeDecl>(decl)) {
-        leftBraceLoc = nominal->getBraces().Start;
-      } else if (auto ext = dyn_cast<ExtensionDecl>(decl)) {
-        leftBraceLoc = ext->getBraces().Start;
-      }
-      if (leftBraceLoc.isInvalid())
-        continue;
-
-      auto afterLeftBraceLoc = Lexer::getLocForEndOfToken(SM, leftBraceLoc);
-      originalSourceRange = CharSourceRange(afterLeftBraceLoc, 0);
-    }
-
-    rewrittenBuffer = adjustMacroExpansionWhitespace(
-        generatedInfo->kind, rewrittenBuffer, scratchBuffer);
-
     // `TheFile` is the file of the actual expansion site, where as
     // `OriginalFile` is the possibly enclosing buffer. Concretely:
     // ```
@@ -8727,6 +8655,7 @@ bool RefactoringActionExpandMacro::performChange() {
     // site (`@_someBufferName`). Thus, we need to include the path to the
     // original source as well. Note that this path could itself be another
     // expansion.
+    auto originalSourceRange = generatedInfo->originalSourceRange;
     SourceFile *originalFile =
         MD->getSourceFileContainingLocation(originalSourceRange.getStart());
     StringRef originalPath;

test/SourceKit/Macros/macro_basic.swift

@@ -139,7 +139,8 @@ struct S4 { }
 // ATTACHED_EXPAND-NEXT: source.edit.kind.active:
 // ATTACHED_EXPAND-NEXT: 24:3-24:3 (@__swiftmacro_9MacroUser1SV13myTypeWrapperfMA0_.swift) "@accessViaStorage "
 // ATTACHED_EXPAND-NEXT: source.edit.kind.active:
-// ATTACHED_EXPAND-NEXT: 22:11-22:11 (@__swiftmacro_9MacroUser1SV13myTypeWrapperfMm_.swift) "
+// ATTACHED_EXPAND-NEXT: 25:1-25:1 (@__swiftmacro_9MacroUser1SV13myTypeWrapperfMm_.swift) "
+// ATTACHED_EXPAND-EMPTY:
 // ATTACHED_EXPAND-NEXT: private var _storage = _Storage()
 // ATTACHED_EXPAND-NEXT: "
 // ATTACHED_EXPAND-NEXT: source.edit.kind.active: