Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 6, 2023
+54 −92
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6113,10 +6113,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);
}
Expand Down
58 changes: 42 additions & 16 deletions lib/ASTGen/Sources/ASTGen/Macros.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -829,3 +814,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
}
8 changes: 7 additions & 1 deletion lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)) {
Expand Down
73 changes: 1 addition & 72 deletions lib/Refactoring/Refactoring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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:
// ```
Expand All @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion test/SourceKit/Macros/macro_basic.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DougGregor IMO it's better to match the compiler here (eg. the temporary output) rather than fake it in the refactoring. I'd like to change it when we expand the macro, but I didn't get time to look at the name lookup failure further.

// ATTACHED_EXPAND-EMPTY:
// ATTACHED_EXPAND-NEXT: private var _storage = _Storage()
// ATTACHED_EXPAND-NEXT: "
// ATTACHED_EXPAND-NEXT: source.edit.kind.active:
Expand Down