From 80d27128d85044774e5a8b4d681c8ebb37246348 Mon Sep 17 00:00:00 2001 From: Ben Barham Date: Fri, 3 Mar 2023 10:58:21 -0800 Subject: [PATCH] [Macros] Do not edit macro buffer or position when refactoring Rather than editing the macro buffer in refactoring, add appropriate padding and braces when creating the macro. Don't edit the insertion location - we should update this in a later PR as well. --- lib/AST/Decl.cpp | 4 +- lib/ASTGen/Sources/ASTGen/Macros.swift | 58 ++++++++++++++------ lib/Parse/ParseDecl.cpp | 8 ++- lib/Refactoring/Refactoring.cpp | 73 +------------------------ test/SourceKit/Macros/macro_basic.swift | 3 +- 5 files changed, 54 insertions(+), 92 deletions(-) diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 6634a7a01abd4..1dc488443e316 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -6113,10 +6113,10 @@ void AbstractStorageDecl::setAccessors(SourceLoc lbraceLoc, ArrayRef 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); } diff --git a/lib/ASTGen/Sources/ASTGen/Macros.swift b/lib/ASTGen/Sources/ASTGen/Macros.swift index cff7de3b3ebaf..2cb907f5e066c 100644 --- a/lib/ASTGen/Sources/ASTGen/Macros.swift +++ b/lib/ASTGen/Sources/ASTGen/Macros.swift @@ -515,22 +515,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 @@ -829,3 +814,44 @@ func expandAttachedMacroInProcess( return expandedSources } + +fileprivate func collapse( + 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 +} diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 30d51747bd28b..f758e4b29e38b 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -7082,11 +7082,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; @@ -7100,6 +7102,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)) { diff --git a/lib/Refactoring/Refactoring.cpp b/lib/Refactoring/Refactoring.cpp index d3bf050e1fe1d..d716fe7006977 100644 --- a/lib/Refactoring/Refactoring.cpp +++ b/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(); - if (!decl) - continue; - - SourceLoc leftBraceLoc; - if (auto nominal = dyn_cast(decl)) { - leftBraceLoc = nominal->getBraces().Start; - } else if (auto ext = dyn_cast(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; diff --git a/test/SourceKit/Macros/macro_basic.swift b/test/SourceKit/Macros/macro_basic.swift index b9adba3d0fa44..808e0a4583ea0 100644 --- a/test/SourceKit/Macros/macro_basic.swift +++ b/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: