-
-
Notifications
You must be signed in to change notification settings - Fork 15
Add Wrappers for Class, Actor, Extension #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Wrappers for Class, Actor, Extension #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love these changes! Really cleans things up nicely and resolves a few issues 🎉
Mostly great, just a few consistency/style things but nothing too major.
The main feedback regarding the approach was the use of rawValue
vs _syntax
for DeclGroupProtocol
(in comparison to the other existing protocols). I'd love to hear your thoughts on what you think would be the most ergonomic solution (I'm completely open to breaking changes to improve ergonomics, the library's still in its early days).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the RepresentableBySyntax
protocol ❤️ will definitely clean things up!
I've left a few comments on formatting/docs, but the main comments are about the splitting of DeclGroupProtocol
into AnyDeclGroupProtocol
and DeclProtocol
; definitely an interesting idea and I'd love to hear your thoughts on whether the split approach is worth the added complexity (i.e. more types to maintain and understand for contributors/users) for the benefits (whichever ones you think are most important).
/// An enum that encapsulates various types of declaration groups (`struct`, `class`, `enum`, `actor`, `extension`) | ||
/// and provides a unified interface for interacting with them. This enum conforms to `AnyDeclGroupProtocol`, | ||
/// allowing access to common properties of declaration groups. | ||
public enum AnyDeclGroup: AnyDeclGroupProtocol { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for renaming this to AnyDeclGroup
? I can see a few upsides and would be happy to discuss, but best to apply this convention to the other root-level enums as well if we decide it's a good idea
Edit: I've now gone through the rest of the changes and have seen that it was so that DeclGroupProtocol
can have a hard requirement on the type of its wrapped syntax, I've left a more open question/remark at the where clause. Not sure what the best design would be, and would love to hear your thoughts since you've been trying out a few different approaches and likely have a bit more insight into the problem than I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of renaming to AnyDeclGroup
was to align with Swift's conventions for type-erased types, like AnyView
, AnyHashable
, and AnyCancellable
, making the distinction between concrete types and their type-erased counterparts clear and intuitive. This consistency with the standard library and clarity in differentiation helps in understanding the purpose of AnyDeclGroup
. The aim was to allow DeclGroupProtocol
to have a hard requirement on the type of its wrapped syntax while maintaining flexibility, and type erasure with AnyDeclGroup
achieves this by holding any type conforming to DeclGroupProtocol
. If we adopt AnyDeclGroup
, similar updates should be applied to other root-level enums for consistency, but this is beyond the scope of the current PR and should be handled separately. For now, I've reverted to DeclGroup
to match the existing naming convention, and if we decide to use the Any
prefix, we should plan a broader refactor for consistency. I appreciate your feedback and am open to further discussion to find the best design approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can see why the Any
prefix made sense cause of DeclGroupSyntax
being a protocol for whatever weird reason. The reason that I don't have the Any
prefix on any of the others (e.g. Decl
) is because they're still concrete and not erasing any info (since they're enums), so I guess in this case it depends what's more important to devs; the fact that it's erasing the _syntax
escape hatch, or the fact that it's not erasing the underlying decl group information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In-fact, DeclGroup
can still conform to RepresentableBySyntax
, but just with the underlying syntax type being Syntax
instead (the main use would just be for using the syntax node in diagnostics, since all other useful decl group syntax info would unfortunately be erased)
/// - Conform to `AnyDeclGroupProtocol`, providing properties for `identifier`, `members`, `properties`, and `inheritedTypes`. | ||
/// - Conform to `RepresentableBySyntax`, providing an underlying syntax node of type `DeclGroupSyntax`. | ||
public protocol DeclGroupProtocol: AnyDeclGroupProtocol, RepresentableBySyntax | ||
where UnderlyingSyntax: DeclGroupSyntax {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the requirement in this where clause gets removed then DeclGroupProtocol
and AnyDeclGroupProtocol
would be able to be merged back together (basically what you had before), definitely a tricky trade off though, I'm unsure what would be the best mix of type safety, ergonomics, and simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue lies with the RepresentableBySyntax
protocol, which has an associated type UnderlyingSyntax
. This requires any type conforming to it to have a concrete type for UnderlyingSyntax
. Since DeclGroupSyntax
is a protocol, there isn't a generic concrete struct that can represent any type conforming to DeclGroupSyntax
. I updated to make types with an underlying syntax type explicitly conform to RepresentableBySyntax
.
Another approach could be to use a type-erased syntax, similar to the following:
public struct AnySyntax: SyntaxProtocol {
public static var structure: SyntaxNodeStructure {
// Returning a default or dummy structure, indicating this shouldn't be used directly
return SyntaxNodeStructure.choices([.node(Syntax.self)])
}
public var _syntaxNode: Syntax { _syntaxNodeClosure() }
private let _syntaxNodeClosure: () -> Syntax
public init<S: SyntaxProtocol>(_ syntax: S) {
self._syntaxNodeClosure = { syntax._syntaxNode }
}
}
But perhaps it wouldn’t make sense for something like DeclGroup
to have RepresentableBySyntax
conformance and the DeclGroupProtcol
shouldn't require it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, strange that DeclSyntax
is concrete and DeclGroupSyntax
is a protocol, wonder why that is. Yeah something like that would work, or we could just set its associated syntax to be of type Syntax
for now even though that's too broad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now, I definitely prefer this over the AnyDeclGroupProtocol
split now that I've seen it. I think it's unlikely for people to actually use RepresentableBySyntax
as a generic requirement very often as it's kinda just an escape hatch to get back to swift syntax land, and therefore I think it's sensible to keep it split from DeclGroupProtocol
to keep things simple 👍 (for other protocols without the issue of missing a concrete root syntax type, it's probably a different story)
The only comments I've left this time are requested formatting changes, basically ready to merge 👌
@@ -60,7 +60,8 @@ extension Sequence where Element == FunctionParameter { | |||
let parameters = Array(self) | |||
for (index, parameter) in parameters.enumerated() { | |||
let isLast = index == parameters.count - 1 | |||
let syntax = parameter._syntax.with(\.trailingComma, isLast ? nil : TokenSyntax.commaToken()) | |||
let syntax = parameter._syntax.with( | |||
\.trailingComma, isLast ? nil : TokenSyntax.commaToken()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swift format makes some odd formatting decisions sometimes 😅
parameter._syntax
.with(\.trailingComma, isLast ? nil : TokenSyntax.commaToken())
@@ -5,7 +5,8 @@ public struct ClassRestrictionType: TypeProtocol { | |||
public var _baseSyntax: ClassRestrictionTypeSyntax | |||
public var _attributedSyntax: AttributedTypeSyntax? | |||
|
|||
public init(_ syntax: ClassRestrictionTypeSyntax, attributedSyntax: AttributedTypeSyntax? = nil) { | |||
public init(_ syntax: ClassRestrictionTypeSyntax, attributedSyntax: AttributedTypeSyntax? = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap these arguments onto newlines instead of wrapping the brace (which I assume that swift format did 😅),
public init(
_ syntax: ClassRestrictionTypeSyntax,
attributedSyntax: AttributedTypeSyntax? = nil
) {
import SwiftSyntaxMacros | ||
|
||
// Modified from: https://github.com/DougGregor/swift-macro-examples/blob/f61ac7cdca8dc3557e53f86e7e03df1353908d3e/MacroExamplesPlugin/AddAsyncMacro.swift | ||
|
||
enum AddAsyncMacroCore { | ||
static func expansion(of node: AttributeSyntax?, providingFunctionOf declaration: some DeclSyntaxProtocol) throws -> DeclSyntax { | ||
static func expansion( | ||
of node: AttributeSyntax?, providingFunctionOf declaration: some DeclSyntaxProtocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put these wrapped arguments on separate lines (just makes it a bit more skim readable in my opinion, i should configure swift format to do that automatically when wrapping arguments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the configuration for swift-format and didn't find anything. Honestly though, I've always used Nick Lockwood's version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks! Sorry for taking a bit long to review, your commits slipped through my radar 😅
This PR introduces wrappers for the remaining declaration groups:
Class
,Actor
,Extension
, andProtocol
. This enhancement allows the toolkit to handle a broader range of Swift declarations, improving its utility and versatility.Key Changes:
Class
,Actor
,Extension
, andProtocol
declarations.DeclGroupProtocol
to enable macros to generically operate over the members of a declaration group.New Wrappers
The new wrappers are implemented in a manner consistent with existing
Enum
andStruct
wrappers. Each new wrapper is defined in its respective file and conforms to theDeclGroupProtocol
.DeclGroupProtocol
The
DeclGroupProtocol
enables macros to generically operate over the members of a declaration group without needing to know the specific type of the declaration group. This protocol standardizes access to common properties likemembers
,properties
,inheritedTypes
,accessLevel
, anddeclarationContext
.Tests
Comprehensive tests have been added to cover the new wrappers and the
DeclGroupProtocol
. The tests validate the initialization and properties of each declaration group wrapper, ensuring correct functionality.Benefits:
Closes:
Resolves #9