swift-syntax icon indicating copy to clipboard operation
swift-syntax copied to clipboard

SwiftSyntax support for experimental @abi attribute

Open beccadax opened this issue 1 year ago • 8 comments

Matches swiftlang/swift#76878.

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 avatar Oct 16 '24 00:10 beccadax

With https://github.com/swiftlang/swift/pull/76878

@swift-ci please test

beccadax avatar Oct 16 '24 00:10 beccadax

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.

ahoppen avatar Oct 16 '24 19:10 ahoppen

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?

allevato avatar Oct 16 '24 19:10 allevato

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 avatar Oct 16 '24 20:10 beccadax

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 avatar Oct 16 '24 21:10 beccadax

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 avatar Oct 28 '24 23:10 beccadax

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

beccadax avatar Oct 29 '24 18:10 beccadax

With https://github.com/swiftlang/swift/pull/76878

@swift-ci please test

beccadax avatar Oct 29 '24 18:10 beccadax