Do we even need to check isInvalidAttachedMacro here? I would think that we should diagnose all macro attributes that didn’t actually expand. Or am I missing something?

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.

I made some changes. I've abstracted a method called diagnosticForUnexpandedMacro. Currently, it only implements diagnostics for AccessorMacro. The rest can be left for others to implement.

I don’t think we should make this method public. The entire logic with allowBlockNames is very specific to SwiftParserDiagnsotic. Using SyntaxKind.nameForDiagnostics would probably be the better choice.

Using SyntaxKind.nameForDiagnostics would probably be the better choice.

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?

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.

Is removing necessary for correctness or do we just do it to keep the number of elements in self.expandedAttributes low (which is a good reason on its own). Would be worth a comment though, I think.

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

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 @MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro have the same location despite being two different macro expansions.

And yes, I think with the removal of attributes from self.expandedAttributes, this should be fine because we always clear that list whenever removing attributes.

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 MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro can’t be attached to func foo(). Also, feel free to pick a shorter name for MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro. I only picked it to convey the meaning of the attribute without writing an implementation.

Could you add labels for these associated values so it’s easier to tell what the two strings do?

Are these imports still necessary if you use nameKindForDiagnostics? Same for the dependency declaration in the package manifest.

Yes, it doesn't need any more.

I think your error message '\(macroType)' macro cannot be attached to \(declType) is sufficiently generic that we could apply it for all cases, not just the accessor macro attached to a non-variable and non-subscript case.

If that's the case, I believe there's no need to diagnose based on the type of macroType or the type of DeclSyntax. A unified diagnosis with message '\(macroType)' macro cannot be attached to \(declType) will suffice.

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 @MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro have the same location despite being two different macro expansions.

And yes, I think with the removal of attributes from self.expandedAttributes, this should be fine because we always clear that list whenever removing attributes.

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 MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro can’t be attached to func foo(). Also, feel free to pick a shorter name for MacroThatExpandsToMemberAttributesMacroAndWhichIsAlsoAnAccessorMacro. I only picked it to convey the meaning of the attribute without writing an implementation.

I think this can be private.

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

• Change this variable 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] = []

• In visitAny set this variable to self.macroAttributes(attachedTo: declSyntax)
• Whenever we expand a macro, remove it from the list (I think we might even do this based on the ID now and don’t need to resort to the position).
• If there are macros left over in attributesToExpand, diagnose them.
• After visitAny is done, restore attributesToExpand to the value it had before we assigned it to self.macroAttributes(attachedTo: declSyntax).

Do you think that would work?

I think we might even do this based on the ID now and don’t need to resort to the position

I found that the id is still different. The macroAttributes method depends on the attachedTo node. In visitAny the declSyntax's root is SourceFileSyntax, while in visit(for example visit(_ node: VariableDeclSyntax)) the node's root is VariableDeclSyntax(itself).

It seems that in visitChildren, the node will have a new layout and generate a new syntax without origin root info. Is this by design?

I also delved into the implementation of AttributeSyntax.position. It's the position of the start of this node's leading trivia(not the root). For Attribute, it always represents its index in the AttributeList. So I don't think it's a proper way to compare two attributes by position

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.