[Macros] Add attached `extension` macros. #1859

hborla · 2023-06-28T00:04:53Z

The SwiftSyntax-side change to swiftlang/swift#66967

`expandAttachedMacro`.

hborla · 2023-06-28T07:43:39Z

swiftlang/swift#66967

@swift-ci please test

hborla · 2023-06-28T16:05:07Z

swiftlang/swift#66967

@swift-ci please test

Sources/SwiftCompilerPluginMessageHandling/PluginMessages.swift

DougGregor · 2023-06-28T17:34:24Z

Sources/SwiftSyntaxMacroExpansion/MacroExpansion.swift

+        )
+      }
+
+      let extensions = try _openExistential(


I suspect we don't need to do _openExistential tricks nowadays, but it's fine this way

DougGregor · 2023-06-28T17:51:39Z

Sources/SwiftSyntaxMacros/MacroProtocols/ExtensionMacro.swift

+
+/// Describes a macro that can add conformances to the declaration it's
+/// attached to.
+public protocol ExtensionMacro: AttachedMacro {


I had an idea! We could make ConformanceMacro refine ExtensionMacro, with a default implementation of the extension entry point based on the conformance entry point. That way, any macro implementations that implement ConformanceMacro today are automatically "upgraded" to ExtensionMacro implementations, and they provide the appropriate @attached(extension...) alongside the conformance one.

Oh! That is a really good idea

bnbarham · 2023-06-28T18:03:26Z

Tests/SwiftParserTest/AttributeTests.swift

+      macro m()
+      """
+    )
+  }


conformances: as well? Shouldn't really matter since it's just parsed as a custom attribute anyway, but good to have the test case IMO.

Matejkob

Hi there 👋,

As someone pretty fresh to this project, I've had a good look at this PR and wow - it's really cool! You've made some fantastic improvements here.

I've attempted to provide constructive feedback on some areas that I noticed while reviewing. Please bear in mind, my understanding of this project's conventions and code structure is still growing, so if any of my comments are off the mark or unrelated, I sincerely apologize. I deeply appreciate your understanding and patience.

Can't wait to get more involved with this project and learn from all of you. 💙

Matejkob · 2023-06-28T18:37:15Z

Sources/SwiftSyntaxMacroExpansion/MacroExpansion.swift

@@ -178,6 +184,7 @@ public func expandAttachedMacroWithoutCollapsing<Context: MacroExpansionContext>
  attributeNode: AttributeSyntax,
  declarationNode: DeclSyntax,
  parentDeclNode: DeclSyntax?,
+  extendedType: TypeSyntax?,


I noticed the new parameter introduced doesn't have documentation in the Parameters doc-comment section above. It might be a good idea to add some info there for clarity. We could use something similar to what we have for parentDeclNode, since it's also used exclusively for MacroRole.extension. If we agree on this, we might want to add the same info for expandAttachedMacro(...) function as well. Just a thought!

btw: I was wondering, instead of adding a new parameter for a specific case from MacroRole, what if we created an enum with associated values? Here's an idea I had:

enum MacroRoleContext { case expression case declaration case accessor case memberAttribute(parentDeclNode: DeclSyntax) case member case peer case conformance case codeItem case `extension`(extendedType: TypeSyntax) } public func expandAttachedMacroWithoutCollapsing<Context: MacroExpansionContext>( definition: Macro.Type, macroRole: MacroRoleContext, attributeNode: AttributeSyntax, declarationNode: DeclSyntax, in context: Context ) -> [String]? { (...)

I'd love to hear your thoughts on this! This is just something that popped into my mind, and I'm not sure if it makes any sense - just a thought. 🤔😊

I actually had the same thought while I was implementing this! But @rintaro pointed out to me that we might also need to make the extendedType an associated value of PluginMessage.MacroRole which would complicate the Codable conformance, so it seemed more straightforward to pass extendedType as an argument to expandAttachedMacro. I'm still interested in exploring this approach further separately though, because it seems plausible that we might want more role-specific arguments, and those conceptually make sense as enum associated values.

Oh yeah, I see.

Sources/SwiftSyntaxMacroExpansion/MacroExpansion.swift

Sources/SwiftSyntaxMacros/MacroProtocols/ExtensionMacro.swift

Matejkob · 2023-06-28T19:00:55Z

Sources/SwiftSyntaxMacros/MacroSystem.swift

+    let extensionMacroAttrs = getMacroAttributes(attachedTo: decl.as(DeclSyntax.self)!, ofType: ExtensionMacro.Type.self)
+    let extendedTypeSyntax = TypeSyntax("\(extendedType.trimmed)")
+    for (attribute, extensionMacro) in extensionMacroAttrs {
+      do {
+        let newExtensions = try extensionMacro.expansion(
+          of: attribute,
+          attachedTo: decl,
+          providingExtensionsOf: extendedTypeSyntax,
+          in: context
+        )
+
+        extensions.append(contentsOf: newExtensions.map(DeclSyntax.init))
+      } catch {
+        context.addDiagnostics(from: error, node: attribute)
+      }
+    }
+


I'm not entirely sure what the assumptions are about how we're structuring code in this project, but I wonder whether it might be better to extract this code into a separate method. The name and doc-comment of this method suggest that the responsibility of this method is solely to expand conformances - I guess expanding extensions could be seen as a bit different responsibility. Just a thought! 💜

Conformance macros are effectively a special case of extension macros. I'm going to take Doug's suggestion here in a follow-up PR and make ConformanceMacro refine ExtensionMacro so that they can be implemented in terms of extension macros. As part of that, I will clean up this code to only operate on ExtensionMacro, since ConformanceMacro will be a refinement of it!

That sounds really cool! Can't wait to see the result!

Matejkob · 2023-06-28T19:27:46Z

Sources/SwiftSyntaxMacros/MacroSystem.swift

@@ -438,6 +439,23 @@ extension MacroApplication {
      }
    }

+    let extensionMacroAttrs = getMacroAttributes(attachedTo: decl.as(DeclSyntax.self)!, ofType: ExtensionMacro.Type.self)
+    let extendedTypeSyntax = TypeSyntax("\(extendedType.trimmed)")


In the proposal it's mentioned:

Each ExtensionDeclSyntax in the resulting array must use the providingExtensionsOf parameter as the extended > type, which is a qualified type name. For example, for the following code:

struct Outer { @MyProtocol struct Inner {} }

The type syntax passed to ExtensionMacro.expansion for providingExtensionsOf is Outer.Inner.

I'm not sure if I've understood this part of the proposal correctly. Does it mean that if I create this macro:

public struct SomeExtensionMacro: ExtensionMacro { public static func expansion( of node: AttributeSyntax, attachedTo declaration: some DeclGroupSyntax, providingExtensionsOf type: some TypeSyntaxProtocol, in context: some MacroExpansionContext ) throws -> [ExtensionDeclSyntax] { [ExtensionDeclSyntax(extendedType: type, memberBlock: MemberDeclBlockSyntax {})] } }

and I attach it like so

struct Outer { @SomeExtensionMacro struct Inner {} }

are we expecting that extendedType.description here will look something like this: Outer.Inner? If I understand the assumptions correctly, I think this would happen because the code above:

let extendedType: Syntax if let identified = decl.asProtocol(IdentifiedDeclSyntax.self) { extendedType = Syntax(identified.identifier.trimmed) } else if let ext = decl.as(ExtensionDeclSyntax.self) { extendedType = Syntax(ext.extendedType.trimmed) } else { return [] }

can only extract the identifier from the root type.

Here is a test case covering this example using @AddSendableExtension macro:

func testExtensionExpansionOfNestedType() { assertMacroExpansion( """ struct Outer { @AddSendableExtension struct Inner { } } """, expandedSource: """ struct Outer { struct Inner { } } extension Outer.Inner: Sendable { } """, macros: testMacros, indentationWidth: indentationWidth ) }

right now it's failing with message:

failed - Actual output (+) differed from expected output (-): struct Outer { struct Inner { } } –extension Outer.Inner: Sendable { –} Actual expanded source: struct Outer { struct Inner { } } extension Outer.Inner: Sendable { }

Am I understanding it correctly that this shouldn't fail?

This is a great question, and there's a little bit of nuance here because the expansion performed by the MacroSystem in SwiftSyntax is only used for unit testing macros, but the real expansion is integrated into the compiler (and implemented in swiftlang/swift#66967). It's not possible for MacroSystem to form the same fully qualified type that the compiler can form in the general case, e.g. the compiler can look through type aliases, while the macro system here cannot. I believe there are also some known issues with where the expansion is inserted in MacroSystem for conformance macros, because it doesn't insert them at the top-level like the compiler does. I definitely think we can improve what MacroSystem does in many cases (e.g. we could form a qualified name from the lexical nesting structure), but it can't perfectly match what the compiler does during extension macro expansion.

I wasn't fully aware of that, I really appreciate the explanation.

… macros and add a macro plugin test for extension macros.

hborla · 2023-06-28T22:00:46Z

swiftlang/swift#66967

@swift-ci please test

hborla · 2023-06-29T00:52:47Z

swiftlang/swift#66967

@swift-ci please test

hborla · 2023-06-29T03:44:50Z

swiftlang/swift#66967

@swift-ci please test

…with PluginMessages.swift in the Swift repository.

hborla · 2023-06-29T06:16:39Z

swiftlang/swift#66967

@swift-ci please test

hborla · 2023-06-29T13:45:28Z

swiftlang/swift#66967

@swift-ci please test Windows

ahoppen · 2023-07-06T11:45:03Z

Sources/SwiftParser/Attributes.swift

+      let isAttached = self.peek().isAttachedKeyword
      return parseAttribute(argumentMode: .customAttribute) { parser in
-        let arguments = parser.parseArgumentListElements(pattern: .none)
+        let arguments: [RawTupleExprElementSyntax]
+        if isAttached {
+          arguments = parser.parseAttachedArguments()
+        } else {
+          arguments = parser.parseArgumentListElements(pattern: .none)
+        }
+


Instead of matching the string contents of the current token here, it’s much clearer to add attached as a contextual keyword. I’m fixing that in #1881.

hborla added 2 commits June 27, 2023 17:55

[Macros] Add attached extension macros.

aa6cc4a

[MacroSystem] Expand extension macros.

5d77ef4

hborla force-pushed the extension-macros branch from 602c84f to 5d77ef4 Compare June 28, 2023 00:56

hborla mentioned this pull request Jun 28, 2023

[Macros] Generalize conformance macros as extension macros swiftlang/swift#66967

Merged

[Macros] Pass the extended type syntax for extension macros to

50bacd8

`expandAttachedMacro`.

hborla marked this pull request as ready for review June 28, 2023 07:43

hborla requested review from DougGregor and ahoppen as code owners June 28, 2023 07:43

hborla mentioned this pull request Jun 28, 2023

[5.9][Macros] Add attached extension macros. #1861

Merged

Fix formatting

d11f4c7

rintaro reviewed Jun 28, 2023

View reviewed changes

Sources/SwiftCompilerPluginMessageHandling/PluginMessages.swift Show resolved Hide resolved

DougGregor approved these changes Jun 28, 2023

View reviewed changes

DougGregor reviewed Jun 28, 2023

View reviewed changes

bnbarham reviewed Jun 28, 2023

View reviewed changes

Matejkob reviewed Jun 28, 2023

View reviewed changes

hborla added 2 commits June 28, 2023 13:55

[Macros] Bump the PluginMessage protocol version number for extension…

1286637

… macros and add a macro plugin test for extension macros.

[Macros] Fix typos in comments and documentation for extension macros.

3e2c782

[NFC] Pass missing ExtensionMacros flag to compiler_plugin_basic.swift

9d1a869

hborla force-pushed the extension-macros branch from db68cd6 to 9d1a869 Compare June 29, 2023 03:44

hborla added 2 commits June 28, 2023 22:55

[Macros] Add missing MacroRole case to PluginMessages.swift to align …

0c1634b

…with PluginMessages.swift in the Swift repository.

[NFC] Remove extension macro test from compiler_plugin_basic.swift

0ac000d

hborla merged commit e85a14a into swiftlang:main Jun 29, 2023

hborla deleted the extension-macros branch June 29, 2023 15:57

ahoppen reviewed Jul 6, 2023

View reviewed changes

Matejkob mentioned this pull request Nov 8, 2023

[Macros] Introduce macro specification in assertMacroExpansion #2327

Merged

[Macros] Add attached extension macros. #1859

[Macros] Add attached extension macros. #1859

Uh oh!

Conversation

hborla commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hborla commented Jun 28, 2023

Uh oh!

hborla commented Jun 28, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matejkob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hborla commented Jun 28, 2023

Uh oh!

hborla commented Jun 29, 2023

Uh oh!

hborla commented Jun 29, 2023

Uh oh!

hborla commented Jun 29, 2023

Uh oh!

hborla commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[Macros] Add attached `extension` macros. #1859

[Macros] Add attached `extension` macros. #1859

hborla commented Jun 28, 2023 •

edited

Loading