swiftlang / swift-syntax

A set of Swift libraries for parsing, inspecting, generating, and transforming Swift source code.
Apache License 2.0
3.18k stars 403 forks source link

assertMacroExpansion always passes empty array as `conformingTo` parameter of extension macros #2031

Closed stephencelis closed 6 months ago

stephencelis commented 1 year ago

Description

Previously a bug with conformance macros:

This issue has regressed with the change to extension macros.

Steps to Reproduce

  1. Write a macro expansion test for an extension macro.
  2. Note that the expanded source does not include the extension.
ahoppen commented 1 year ago

Tracked in Apple’s issue tracker as rdar://113582092

ahoppen commented 1 year ago

@stephencelis Do you have an example where the extension is not included in the expanded source? Because the SendableExtensionMacro in swift-syntax’s test suite works just fine.

https://github.com/apple/swift-syntax/blob/5412040ebb79c529c16a8a8a6bbfe59baf06c5f6/Tests/SwiftSyntaxMacroExpansionTest/MacroSystemTests.swift#L1504-L1522

stephencelis commented 1 year ago

@ahoppen This might be a documentation problem, but the first example of an extension macro I found was this one in the Swift repo, which starts:

https://github.com/apple/swift/blob/7699834154417cb6eea9fc6438cf5a4c3e589dee/lib/Macros/Sources/SwiftMacros/OptionSetMacro.swift#L135-L137

These three lines of code make the macro expansion testing tool never add the conformance, presumably because macro expansion testing is totally decoupled from the library's macro declaration that specifies the allowed conformances.

ahoppen commented 1 year ago

Ah, yes. That’s a current limitation of assertMacroExpansion, which, at the moment, always passes an empty list for conformingTo. We are aware of the issue and are looking into it.

soumyamahunt commented 1 year ago

@ahoppen if there is limitation in inferring conformance for a type, I would expect all the protocols that macro declares conformance for all provided. That way existing tests will not break when migrating to extension macro from member macro.

ahoppen commented 1 year ago

The problem is that in assertMacroExpansion, we currently don’t know which conformances the macro declaration (ie the thing after @attached) contains because assertMacroExpansion only has a mapping from the macro name to the type. We would need to design some way of passing that information in to assertMacroExpansion here.

DougGregor commented 1 year ago

The simplest approach is to pass this information as an extra parameter through assertMacroExpansion, e.g.,

assertMacroExpansion( 
       """ 
       @AddSendableExtension 
       struct MyType { 
       } 
       """, 
       expandedSource: """ 

         struct MyType { 
         } 

         extension MyType: Sendable { 
         } 
         """, 
       macros: testMacros, 
       conformsTo: ["AddSendableExtension": ["Sendable"]],
       indentationWidth: indentationWidth 
     ) 

where the conformsTo parameter has a type like [String: [TypeSyntax]] so we can pass different sets of conformances to each macro.

There are more advanced approaches I could imagine that, for example, depend on passing the actual macro declarations into assertMacroExpansion, e.g., we could pass in this text

@attached(extension, conformances: Sendable)
macro AddSendableExtension() = ...

and then use the conformances in the attached macro role declaration to populate the conformances passed to the macro implementations. It's more automatic, but it's a lot more plumbing.

soumyamahunt commented 10 months ago

@ahoppen anything finalized on the approach yet, I am thinking of creating PR with @DougGregor's suggestion.