-
Notifications
You must be signed in to change notification settings - Fork 379
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.
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
[WIP][Macro] Emit error if AccessorMacro is applied to node that is not a variable #2624
base: main
Are you sure you want to change the base?
Changes from 5 commits
fbc962f
09252c7
d870f06
7572558
d6b9cd2
ad9e5ae
83b3407
3fe3f77
e029a45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -16,6 +16,7 @@ internal import SwiftOperators | |||||
@_spi(MacroExpansion) internal import SwiftParser | ||||||
public import SwiftSyntax | ||||||
internal import SwiftSyntaxBuilder | ||||||
internal import SwiftParserDiagnostics | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these imports still necessary if you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it doesn't need any more. |
||||||
@_spi(MacroExpansion) @_spi(ExperimentalLanguageFeature) public import SwiftSyntaxMacros | ||||||
#else | ||||||
import SwiftDiagnostics | ||||||
|
@@ -24,6 +25,7 @@ import SwiftOperators | |||||
import SwiftSyntax | ||||||
import SwiftSyntaxBuilder | ||||||
@_spi(MacroExpansion) @_spi(ExperimentalLanguageFeature) import SwiftSyntaxMacros | ||||||
import SwiftParserDiagnostics | ||||||
#endif | ||||||
|
||||||
// MARK: - Public entry function | ||||||
|
@@ -633,6 +635,7 @@ private enum MacroApplicationError: DiagnosticMessage, Error { | |||||
case accessorMacroOnVariableWithMultipleBindings | ||||||
case peerMacroOnVariableWithMultipleBindings | ||||||
case malformedAccessor | ||||||
case macroAttachedToInvalidDecl(String, String) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add labels for these associated values so it’s easier to tell what the two strings do? |
||||||
|
||||||
var diagnosticID: MessageID { | ||||||
return MessageID(domain: diagnosticDomain, id: "\(self)") | ||||||
|
@@ -650,6 +653,8 @@ private enum MacroApplicationError: DiagnosticMessage, Error { | |||||
return """ | ||||||
macro returned a malformed accessor. Accessors should start with an introducer like 'get' or 'set'. | ||||||
""" | ||||||
case let .macroAttachedToInvalidDecl(macroType, declType): | ||||||
return "'\(macroType)' macro cannot be attached to \(declType)" | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -666,6 +671,7 @@ private class MacroApplication<Context: MacroExpansionContext>: SyntaxRewriter { | |||||
/// Store expanded extension while visiting member decls. This should be | ||||||
/// added to top-level 'CodeBlockItemList'. | ||||||
var extensions: [CodeBlockItemSyntax] = [] | ||||||
var expandedAttributes: [AttributeSyntax] = [] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be Also, I just tried to suggest a doc comment for it and couldn’t really come up with a great explanation, which is usually a sign that the design isn’t quite right (maybe you can find a good explanation). Thinking about it made me wonder if it would it be easier to
/// Attributes that look like macro attributes and should be expanded.
///
/// Attributes are added to this list from `visitAny`. Any attributes that weren’t actually expanded (eg. because they are accessor macros but not attached to a variable or subscript), will be diagnosed from `visitAny`.
var attributesToExpand: [AttributeSyntax] = []
Do you think that would work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I found that the id is still different. The It seems that in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also delved into the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I’ll need to play around with the code myself a little to figure out how we want to continue, but am too busy with other things at the moment. I’ll definitely come back to you but it might take a few weeks. |
||||||
|
||||||
init( | ||||||
macroSystem: MacroSystem, | ||||||
|
@@ -715,14 +721,56 @@ private class MacroApplication<Context: MacroExpansionContext>: SyntaxRewriter { | |||||
let visitedNode = self.visit(declSyntax) | ||||||
skipVisitAnyHandling.remove(Syntax(declSyntax)) | ||||||
|
||||||
let attributesToRemove = self.macroAttributes(attachedTo: visitedNode).map(\.attributeNode) | ||||||
|
||||||
return AttributeRemover(removingWhere: { attributesToRemove.contains($0) }).rewrite(visitedNode) | ||||||
let attributesToRemove = self.macroAttributes(attachedTo: visitedNode) | ||||||
attributesToRemove.forEach { (attribute, spec) in | ||||||
RayZhao1998 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if let index = self.expandedAttributes.firstIndex(where: { expandedAttribute in | ||||||
expandedAttribute.position == attribute.position | ||||||
}) { | ||||||
self.expandedAttributes.remove(at: index) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is removing necessary for correctness or do we just do it to keep the number of elements in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be incorrect if it's possible that two attributes with same position are actually different during the process. For example, the list still contains an attribute expanded in the last visit, and in the same position there is attribute not expanded in this visit, I think it'll go wrong. I think we should remove it from the list if it has been checked to make sure it's correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I thought we were fine because macros only ever introduce code after the the macro attribute itself, so I thought that the position would still uniquely identify a macro within the source file but thinking about it again, I realize that isn’t true because the macro itself gets removed, which shifts positions forward and thus allows another macro to have the same position. For example, you could have the following @NoOpMacroAB
@MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro
sturct Foo {
func foo() {}
} which expands to sturct Foo {
@MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro
func foo() {}
} And the two And yes, I think with the removal of attributes from I hope that I’m making sense here. I think it would be good to have a test case for what I described above and make sure that we do actually diagnose that |
||||||
} else { | ||||||
if let macroRole = try? inferAttachedMacroRole(definition: spec.type), | ||||||
isInvalidAttachedMacro(macroRole: macroRole, attachedTo: declSyntax) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even need to check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found out how it diagnose when AccessorMacro is applied to struct in the compiler and imitated it. Also the diagnostics I'v implemented below is just for attached macros(actually only AccessorMacro). If we should diagnose all macro attributes that didn't expand, what error messages should we emit, a common error message or the same as what compiler emit? Also I don't know all the cases that attribute won't expand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made some changes. I've abstracted a method called |
||||||
{ | ||||||
contextGenerator(node).addDiagnostics( | ||||||
from: MacroApplicationError.macroAttachedToInvalidDecl( | ||||||
macroRole.rawValue, | ||||||
declSyntax.nodeTypeNameForDiagnostics(allowBlockNames: true) ?? "" | ||||||
), | ||||||
node: declSyntax, | ||||||
fixIts: [ | ||||||
FixIt( | ||||||
message: SwiftSyntaxMacros.MacroExpansionFixItMessage( | ||||||
"Remove '\(attribute.trimmedDescription)'" | ||||||
), | ||||||
changes: [.replace(oldNode: Syntax(attribute), newNode: Syntax(AttributeListSyntax()))] | ||||||
) | ||||||
] | ||||||
) | ||||||
} | ||||||
} | ||||||
} | ||||||
return AttributeRemover(removingWhere: { attributesToRemove.map(\.attributeNode).contains($0) }).rewrite( | ||||||
visitedNode | ||||||
) | ||||||
} | ||||||
|
||||||
return nil | ||||||
} | ||||||
|
||||||
private func isInvalidAttachedMacro(macroRole: MacroRole, attachedTo: DeclSyntax) -> Bool { | ||||||
switch macroRole { | ||||||
case .accessor: | ||||||
// Only var decls and subscripts have accessors | ||||||
if (!attachedTo.is(VariableDeclSyntax.self) && !attachedTo.is(SubscriptDeclSyntax.self)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return true | ||||||
} | ||||||
break | ||||||
default: | ||||||
break | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think your error message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case, I believe there's no need to diagnose based on the type of |
||||||
} | ||||||
return false | ||||||
} | ||||||
|
||||||
/// Visit for both the body and preamble macros. | ||||||
func visitBodyAndPreambleMacros<Node: DeclSyntaxProtocol & WithOptionalCodeBlockSyntax>( | ||||||
_ node: Node | ||||||
|
@@ -1023,6 +1071,7 @@ extension MacroApplication { | |||||
macroAttribute.conformanceList | ||||||
) { | ||||||
result += expanded | ||||||
self.expandedAttributes.append(macroAttribute.attributeNode) | ||||||
} | ||||||
} catch { | ||||||
contextGenerator(Syntax(decl)).addDiagnostics(from: error, node: macroAttribute.attributeNode) | ||||||
|
@@ -1170,6 +1219,7 @@ extension MacroApplication { | |||||
from: newAccessors, | ||||||
indentationWidth: self.indentationWidth | ||||||
) | ||||||
self.expandedAttributes.append(macro.attributeNode) | ||||||
} | ||||||
} else if let newAccessors = try expandAccessorMacroWithoutExistingAccessors( | ||||||
definition: macro.definition, | ||||||
|
@@ -1192,11 +1242,13 @@ extension MacroApplication { | |||||
} else { | ||||||
newAccessorsBlock = newAccessors | ||||||
} | ||||||
self.expandedAttributes.append(macro.attributeNode) | ||||||
} | ||||||
} catch { | ||||||
contextGenerator(Syntax(storage)).addDiagnostics(from: error, node: macro.attributeNode) | ||||||
} | ||||||
} | ||||||
|
||||||
return (newAccessorsBlock, expandsGetSet) | ||||||
} | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we should make this method public. The entire logic with
allowBlockNames
is very specific toSwiftParserDiagnsotic
. UsingSyntaxKind.nameForDiagnostics
would probably be the better choice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to make
nameForDiagnostics
public? I found the file is generated.By the way, if the files are generated, should we add generated files to
.gitignore
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files are being generated as described here:
https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#generating-source-code
We need to commit the generated files to resolve a circular dependency: The sources are being generated using swift-syntax itself, which means that you need a version of swift-syntax to generate the sources of swift-syntax. If the generated sources weren’t committed anywhere, you couldn’t build swift-syntax.