Fix Accessor Macros Attached to Properties With Trailing Comments #2722
Conversation
| func testAccessorOnVariableDeclWithTrailingLineCommentAndAccessorBlock() { | ||
| assertMacroExpansion( | ||
| """ | ||
| @constantOne | ||
| var x: Int { // hello | ||
| 1 | ||
| } | ||
| """, | ||
| expandedSource: """ | ||
| var x: Int { // hello | ||
| get { | ||
| 1 | ||
| } | ||
| get { | ||
| return 1 | ||
| } | ||
| } | ||
| """, |
There was a problem hiding this comment.
I discovered that adding a trailing comment to the left brace of the accessor block will mess up the indentation levels of the generated getter.
AppAppWorks
force-pushed
the
fix-accessor-macros-properties-with-trailing-comments
branch
from
5c6edb1 to
8a5f2d1
Compare
AppAppWorks
force-pushed
the
fix-accessor-macros-properties-with-trailing-comments
branch
from
8a5f2d1 to
552e539
Compare
|
|
||
| let expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock) | ||
|
|
||
| let bindingKeyPath = \VariableDeclSyntax.bindings[node.bindings.startIndex] |
There was a problem hiding this comment.
I mildly prefer just keeping node.bindings[node.bindings.startIndex] TBH. Maybe we should have a mutate method or something similar to make this a little nicer, or just wait until we can use variadic generics and then update with...
There was a problem hiding this comment.
I agree that the bindings key path is a little confusing, it just seems a little over-complicated
What do you think about having var binding = node.bindings.first in the guard, then using binding.accessorBlock = … etc below and finalizing the new node by setting node.bindings = [binding]. That seems like the cleanest solution to me.
|
@swift-ci please test |
AppAppWorks
force-pushed
the
fix-accessor-macros-properties-with-trailing-comments
branch
from
552e539 to
6ee8632
Compare
|
I've run swift-format, please test again. |
|
@swift-ci please test |
|
|
||
| let expansion = expandAccessors(of: node, existingAccessors: binding.accessorBlock) | ||
|
|
||
| let bindingKeyPath = \VariableDeclSyntax.bindings[node.bindings.startIndex] |
There was a problem hiding this comment.
I agree that the bindings key path is a little confusing, it just seems a little over-complicated
What do you think about having var binding = node.bindings.first in the guard, then using binding.accessorBlock = … etc below and finalizing the new node by setting node.bindings = [binding]. That seems like the cleanest solution to me.
| if binding.accessorBlock == nil { | ||
| // remove the trailing trivia of the variable declaration and move it | ||
| // to the trailing trivia of the left brace of the newly created accessor block | ||
| node[keyPath: bindingKeyPath].trailingTrivia = [] | ||
| expansion.accessors?.leftBrace.trailingTrivia = binding.trailingTrivia | ||
| } |
There was a problem hiding this comment.
This would still be an issue if the macro expands to an accessor that doesn’t have a newline after the {, right? Ie. you have a macro that expands to { 1 } and has formatting disabled, so no newline gets added by BasicFormat.
If that’s the case, I would just move the trailing trivia after the closing brace. That way you can conceptually think of the accessor as being inserted before the binding’s trailing trivia.
There was a problem hiding this comment.
How could I disable formatting for a test case?
There was a problem hiding this comment.
You'll need to add a new macro and then set static let formatMode: FormatMode = .disabled in its definition. Also, nice catch @ahoppen! I forgot about that case :)
There was a problem hiding this comment.
I couldn't figure out how to make an AccessorMacro expand into something like { 1 }, how could I do this?
There was a problem hiding this comment.
The following should test it
func testNew() {
struct ConstantOneSingleLineGetter: AccessorMacro {
static var formatMode: FormatMode { .disabled }
static func expansion(
of node: AttributeSyntax,
providingAccessorsOf declaration: some DeclSyntaxProtocol,
in context: some MacroExpansionContext
) throws -> [AccessorDeclSyntax] {
return [
"""
get { 1 }
"""
]
}
}
assertMacroExpansion(
"""
@constantOne
var x: Int // hello
""",
expandedSource: """
var x: Int { // hello
get { 1 }
}
""",
macros: ["constantOne": ConstantOneSingleLineGetter.self],
indentationWidth: indentationWidth
)
}
|
@swift-ci Please test Windows |
AppAppWorks
force-pushed
the
fix-accessor-macros-properties-with-trailing-comments
branch
from
6ee8632 to
b3649bb
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
@AppAppWorks Could you add the test case I suggested in #2722 (comment)? |
fixed the bug that trailing comments of a property would stay in place after macro expansion, commenting out the left brace of the newly created accessor block - the bug affected only properties without an accessor block while having trailing line comments - fixed by moving the trailing trivia of the variable declaration to the trailing trivia of the left brace of the accessor block added tests on properties with trailing comments and with/without an accessor block
AppAppWorks
force-pushed
the
fix-accessor-macros-properties-with-trailing-comments
branch
from
b3649bb to
8bb0a0a
Compare
Done, please have a look. |
|
@swift-ci Please test |
|
Could you test Windows too? :) |
|
@swift-ci please test Windows platform |
Fixed #2614