SwiftSyntax support for experimental @abi attribute #2882

beccadax · 2024-10-16T00:48:01Z

Note that @abi introduces a situation we've never had before: the various DeclGroupSyntax decls can now have a nil memberBlock. This can only occur for children of an ABIAttributeArgumentsSyntax node, not any nodes that would occur in existing constructs. To preserve source compatibility with existing clients, I've made memberBlock an implicitly-unwrapped optional, but I'm open to opinions if that's not the design we'd like to have.

Another place I'd like feedback on is the way I'm parsing malformed @abi argument lists. Because the argument is literally an entire decl, I found that the default recovery behavior when the left paren was missing didn't work well, so I built something a little different. If you have better suggestions, I'd love to hear them.

Edit: This has been changed from the original implementation. The new version extracts a new "header" child node from nodes like DeclClassSyntax which contains everything but the member block. All of the DeclGroup nodes have this change applied. As proof that this works, I've updated the HasTrailingMemberDeclBlock initializer to take a header node, rather than a syntax string.

Generating the new decls I wanted—particularly their compatibility layers—took substantial refactoring of CodeGenerator, including the introduction of a new childHistory mechanism to replace deprecatedName and its ilk, extending it with the capability to model this kind of "extraction" of a child node from a parent. I've also made it so that base nodes can have children (these become protocol requirements), traits can optionally declare a memberwise failable initializer (this must be manually implemented at the moment), and deprecated properties are automatically generated for traits.

beccadax · 2024-10-16T00:48:30Z

With swiftlang/swift#76878

@swift-ci please test

ahoppen · 2024-10-16T19:26:47Z

I really dislike using implicitly unwrapped optionals here. It’s just waiting to haunt us later when we forget to check for nil. As you already noted, changing memberBlock to be optional has a pretty big API impact (because memberBlock is pretty widely used) and also feels a little weird if the only case where it can actually be nil is inside the relatively low-frequency @abi attribute. Thus, I’d prefer to not make memberBlock optional.

This leaves us with one more option: Add new syntax nodes for all the type declarations. Maybe it would be OK if they are completely separate from the existing type nodes but just shared the parsing logic. I’ll need to think about that some more.

allevato · 2024-10-16T19:40:01Z

Is there a way that we could use a non-optional MemberBlockSyntax with an empty item list, and some kind of dummy/missing-but-not-nil leftBrace and rightBrace tokens?

beccadax · 2024-10-16T20:14:11Z

Another option I considered was adding a new MemberBlockSyntax? property to replace memberBlock, and making the backwards-compatibility memberBlock create a dummy missing MemberBlockSyntax when the new property is nil. I'm not sure what we'd call the new property, though. members has already been expended.

beccadax · 2024-10-16T21:07:32Z

Add new syntax nodes for all the type declarations. Maybe it would be OK if they are completely separate from the existing type nodes but just shared the parsing logic. I’ll need to think about that some more.

I guess this would look something like extracting ClassDeclHeaderSyntax out of ClassDeclSyntax so that you end up with:

// `class SomeClass {}`
- SourceFileSyntax
├─statements: CodeBlockItemListSyntax
│ ├─[0]: CodeBlockItemSyntax
│ │ ╰─item: ClassDeclSyntax
│ │   ├─header: ClassDeclHeaderSyntax
│ │   │ ├─attributes: AttributeListSyntax
│ │   │ ├─modifiers: DeclModifierListSyntax
│ │   │ ├─classKeyword: keyword(_CompilerSwiftSyntax.Keyword.class)
│ │   │ ╰─name: identifier("SomeClass")
│ │   ╰─memberBlock: MemberBlockSyntax
│ │     ├─leftBrace: leftBrace
│ │     ├─members: MemberBlockItemListSyntax
│ │     ╰─rightBrace: rightBrace
...

Then you could put compatibility properties on ClassDeclSyntax that forward to header's corresponding properties. Repeat for each DeclGroupSyntax (or maybe each DeclSyntax period?), and then have ABIAttributeArguments use a child of type DeclHeaderSyntax instead of DeclSyntax.

This is a rather large refactor, but I think it could work. As a bonus, HasTrailingCodeBlock.init(header:bodyBuilder:) would have actual syntax nodes for its headers that it could work with.

beccadax · 2024-10-28T23:29:01Z

This has been changed from the original implementation. The new version extracts a new "header" child node from nodes like DeclClassSyntax which contains everything but the member block. All of the DeclGroup nodes have this change applied. As proof that this works, I've updated the HasTrailingMemberDeclBlock initializer to take a header node, rather than a syntax string.

Generating the new decls I wanted—particularly their compatibility layers—took substantial refactoring of CodeGenerator, including the introduction of a new childHistory mechanism to replace deprecatedName and its ilk, extending it with the capability to model this kind of "extraction" of a child node from a parent. I've also made it so that base nodes can have children (these become protocol requirements), traits can optionally declare a memberwise failable initializer (this must be manually implemented at the moment), and deprecated properties are automatically generated for traits.

beccadax · 2024-10-29T18:27:06Z

@ahoppen I'd love your opinion on this modification.

beccadax · 2024-10-29T18:48:24Z

With swiftlang/swift#76878

@swift-ci please test

ahoppen

Two preliminary comments from an initial look at the changes. I think we’re on the right track but I don’t think it makes sense to give this PR a full review until they are addressed.

ahoppen · 2024-10-30T17:40:37Z

CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift

@@ -180,13 +180,98 @@ public let COMMON_NODES: [Node] = [
    parserFunction: "parseDeclaration"
  ),

+  Node(
+    kind: .declGroupHeader,


None of the other base nodes (DeclSyntax, ExprSyntax etc.) have any children and I think we will get crashes if any of the nodes whose base is .declGroupHeader are layout-incompatible with declGroupHeader. DeclGroupHeaderSyntax will assume that the first child in the node is attributes but that might not be the case for a node inheriting from .declGroupHeader.

That said, I would make the base of all header nodes .syntax and create a trait for the decl group headers instead.

I'm a little reluctant to do this because I get a fair bit of mileage out of having DeclGroupHeaderSyntax be a concrete type instead of an existential. For instance, both ABIAttributeSyntax.Provider and the modified version of HasTrailingMemberBlock use the concrete DeclGroupHeaderSyntax; the former could not easily use an existential because there's no way to constrain a child node to have a trait, and the latter because a concrete type is needed to provide a contextual type for the string literal.

(If it helps, the DeclGroupHeaderSyntax accessors don't directly access the children—they indirect through the existential so they use the leaf type's layout.)

If you can suggest another design that would still give me a concrete node which could contain any of the headers, I'd be open to that change.

Let me think about it some more. It might also become clearer to me once #2888 is merged and this PR is rebased so it doesn’t contain those changes.

Hey @ahoppen , we now have the other PR merged. Do you have more specific suggestions in mind?

CodeGeneration/Sources/SyntaxSupport/Child.swift

This is accurate for all NamedDeclSyntax conformers so far, so I feel comfortable doing it. This also improves the accuracy of the “conforms if it can” validation by also checking the trait’s base kind against the node. That eliminates several false positives in the test.

This is the base node for the headers we are about to extract. Unlike other base nodes, DeclGroupHeaderSyntax has children, which are treated as protocol requirements for derived nodes.

It actually ought to be the union of three token kind subsets; turn it into a custom type so we can do that.

This also involves changing Child.Refactoring to support a new kind of change: “extracted”, in which a group of adjacent children are moved into a new child node. Future commits will introduce clients that use the new *DeclHeaderSyntax nodes directly.

Clears up a bunch of warnings.

`DeclGroup` now requires an initializer that can build a group from a header and member block. With that in place, we can reimplement `HasTrailingMemberDeclBlock` to avoid reparsing anything. Which is kind of neat.

Matches work in swiftlang/swift#76878.

ahoppen

Left a few comments inline.

Overall, I’m still skeptical of the new DeclGroupHeaderSyntax header base type because:

The reasons from SwiftSyntax support for experimental @abi attribute #2882 (comment)
Accessing any properties of it requires an existential conversion, which may be unexpected performance-wise.

I would prefer if ABIAttributeSyntax.Provider listed all the headers nodes it supports, just like it already lists types like typealiasDecl and funcDecl.

ahoppen · 2024-11-05T20:17:34Z

CodeGeneration/Sources/SyntaxSupport/AttributeNodes.swift

@@ -267,6 +272,31 @@ public let ATTRIBUTE_NODES: [Node] = [
    ]
  ),

+  Node(


Could you add an entry to the release notes for the new/updated nodes?

At this point, the node is hidden behind an experimental feature. Should I still document it now?

I probably didn’t put the comment at the right place. I meant adding a release note entry for the nodes that change and that are not SPI, eg. the introduction of ClassDeclSyntax.header.

ahoppen · 2024-11-05T20:33:22Z

Sources/SwiftSyntax/generated/RenamedChildrenCompatibility.swift

+  }
+
+  @available(*, deprecated, renamed: "actorHeader.name")
+  public var name: TokenSyntax {


I’m a little hesitant to deprecate these accessors and am debating whether we should keep them as non-deprecated convenience accessors. Some thoughts on why:

Using classDeclSyntax.classHeader.name feels more complicated than just classDeclSyntax.name if you just want to get a class’s name, which shouldn’t be a complicated operation.

But then, some other parts of the syntax tree are a little convoluted to access already (eg. GenericArgumentListSyntax contains GenericArgumentSyntax, which contains TypeSyntax and a trailing comma), so not sure if this is really a super strong argument.

I expect quite a few codebases to use these properties, so I expect the to incur deprecation warnings in quite a few codebases and they can’t fully switch over to classHeader.name if they need to support older swift-syntax versions as well.

The conformance of ClassDeclSyntax to NamedDeclHeader, WithAttributesSyntax, WithGenericParameters, and WithModifiers relies on these accessors. If we deprecate these accessors, we should also deprecate the conformance of ClassDeclSyntax to these traits. But there’s no way to deprecate protocol conformances and I assume that these are used somewhat wildly, so I’m not sure how to do this transition.

@rintaro I would appreciate your opinion here as well.

I do understand your concerns here; I separated out the deprecation changes into their own commit specifically so you could evaluate this impact.

I could add something in CodeGenerator to allow certain compatibility layer APIs to be non-deprecated. Just let me know what you'd like me to do here.

@rintaro Just chatted and agree that we shouldn’t deprecate ClassDeclSyntax.name.

ahoppen · 2024-11-05T20:33:52Z

CodeGeneration/Sources/SyntaxSupport/DeclNodes.swift

+    ],
+    children: [
+      Child(
+        name: "actorHeader",


I would just name this header. You’re on an actor declaration, so it’s clear that the header is for an actor.

If there is anything in DeclGroupSyntax that allows access to the header, it needs to have a different name from the header children in all of the concrete nodes since it will need to have a broader type.

I realized that as well as I continued reviewing and think this is an argument for not having a DeclHeaderSyntax node.

An alternative I've been considering is making the DeclGroupHeader node non-base and having it be something like:

Node( kind: .declGroupHeader, base: .syntax, nameForDiagnostics: "declaration group header", parserFunction: "parseDeclarationGroupHeader", traits: [ "WithAttributes", "WithModifiers", "DeclGroupHeaderProtocol", // slight naming issue I'll have to resolve ], children: [ Child( name: "header", kind: .nodeChoices(choices: [ Child(name: "actorHeader", kind: .node(kind: .actorDeclHeader)), Child(name: "classHeader", kind: .node(kind: .classDeclHeader)), Child(name: "enumHeader", kind: .node(kind: .enumDeclHeader)), Child(name: "extensionHeader", kind: .node(kind: .extensionDeclHeader)), Child(name: "protocolHeader", kind: .node(kind: .protocolDeclHeader)), Child(name: "structHeader", kind: .node(kind: .structDeclHeader)), ]) ) ] )

I would then add modifiers, attributes, etc. using a handwritten extension switching over the choices, so there'd be no existential overhead. Not quite as elegant, but it still gives us a concrete type that can represent any of the headers.

Do we really need a DeclGroupHeader node at all? Correct me if I’m wrong, but I think the primary use for it is to be an option to ABIAttributeArgumentsSyntax, which already lists all sorts of other declarations. If we listed all the nodes that are subtypes of DeclGroupHeaderSyntax in there as well, I don’t think there’s real need for DeclGroupHeaderSyntax anymore. Or am I missing something?

I also use it in the new implementation for HasTrailingMemberDeclBlock, and I believe I use it in ASTGen to generalize some of the lowering of DeclGroups, but those probably aren't mandatory—they're just nice little tricks I was able to do now that the new nodes existed.

I think none of those are sufficient reason to introduce DeclGroupHeaderSyntax.

ahoppen · 2024-11-05T20:37:08Z

CodeGeneration/Sources/SyntaxSupport/SyntaxNodeKind.swift

+    case .declGroupHeader:
+      return "DeclHeaderSyntax"


Should we rename declGroupHeader to declHeader so this isn’t needed?

I'm using "DeclGroupHeader" because only the DeclGroup nodes have separate headers. Do you think I'm being too picky with that?

I just find it add that we have this mismatch here. But depending on how we decide on the existence of DeclHeaderSyntax, this may be a moot point anyway.

ahoppen · 2024-11-05T20:38:46Z

CodeGeneration/Sources/generate-swift-syntax/InitSignature+Extensions.swift

@@ -63,19 +65,25 @@ extension InitSignature {
      )
    }

+    func transformParam(_ param: FunctionParameterSyntax) -> FunctionParameterSyntax {


Suggested change

func transformParam(_ param: FunctionParameterSyntax) -> FunctionParameterSyntax {

func transform(parameter: FunctionParameterSyntax) -> FunctionParameterSyntax {

That feels at odds with how we normally use argument labels because the word parameter is redundant information given the type. Should it just be transform(_:)?

transform(_:) seems too ambiguous to me. If you don’t like transform(parameter:), I don’t have super strong opinions on it and we can keep transformParam(_:)

ahoppen · 2024-11-05T21:32:05Z