Fix trivia handling in AttributeRemover of MacroSystem

[gohanlon]

Initially opened for discussion of #2203, and now a fix for it.

Also, adds AttributeRemoverTests covering a variety of attribute removal cases involving surrounding trivia.

Fixes #2203
rdar://115585919

[gohanlon]

@ahoppen I think I see a simple fix that will get all these tests passing. But first, do these test cases and expected outputs look good to you?

[gohanlon]

The tests I added exercise MacroSystem.AttributeRemover, but I've added the tests to PeerMacroTests. Should I move the tests to a new test file, e.g. AttributeRemoverTests?

I did a cursory search and turned up these tests that seem to test MacroSystem.AttributeRemover:

• AccessorMacroTests.testEmpty
• MemberAttributeMacroTests.testEmpty
• MemberMacroTests
  • testCommentAroundeAttachedMacro [sic]
  • testEmpty
• PeerMacroTests (includes this PR)
  • testEmptyOnSameLineAsVariable
  • testEmptyTwiceOnSameLineAsVariable
  • testEmptyOnOwnLineBeforeVariable
  • testEmptyTwiceOnOwnLineBeforeVariable
  • testEmptyAndAttributeOnOwnLineBeforeVariable
  • testAttributeAndEmptyOnOwnLineBeforeVariable
  • testAttributeAndEmptyAndCommentOnOwnLineBeforeVariable
  • testAttributeAndEmptyAndCommentOnOwnLineBeforeVariable2
  • testCommentAroundAttachedMacro
  • testAttributeAndEmptyOnOwnLinesBeforeVariable
  • testEmptyAndAttributeOnOwnLinesBeforeVariable
  • testAttributeOnOwnLineThenEmptyBeforeVariable
  • testEmptyOnOwnLineThenEmptyBeforeVariable
  • testEmptyOnMemberVariable
  • testEmptyBeforeAttributeOnSameLineAsMemberVariable
  • testEmptyAfterAttributeOnSameLineAsMemberVariable
  • testEmptyAfterAttributeOnSameLineAsMemberVariable_AwkwardWhitespace
  • testEmptyOnOwnLineThenAttributedMemberVariable
  • testAttributeOnOwnLineThenEmptyOnMemberVariable

[gohanlon]

Getting a little ahead, I sketched out the fix I have in mind. With the following change, the test suite passes:

diff --git a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift
index f97f02b5..1198c292 100644
--- a/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift
+++ b/Sources/SwiftSyntaxMacroExpansion/MacroSystem.swift
@@ -397,11 +397,12 @@ private class AttributeRemover: SyntaxRewriter {
     var filteredAttributes: [AttributeListSyntax.Element] = []
     for case .attribute(let attribute) in node {
       if attributesToRemove.contains(attribute) {
-        var leadingTrivia = node.leadingTrivia
+        var leadingTrivia = attribute.leadingTrivia
         if let lastNewline = leadingTrivia.pieces.lastIndex(where: { $0.isNewline }),
           leadingTrivia.pieces[lastNewline...].allSatisfy(\.isWhitespace),
-          node.trailingTrivia.isEmpty,
-          node.nextToken(viewMode: .sourceAccurate)?.leadingTrivia.first?.isNewline ?? false
+          attribute.trailingTrivia.isEmpty//,
+        // Both before this PR and in this PR, removing  `node.nextToken`/`attribute.nextToken` condition has no effect on the test suite
+//          attribute.nextToken(viewMode: .sourceAccurate)?.leadingTrivia.first?.isNewline ?? false
         {
           // If the attribute is on its own line based on the following conditions,
           // remove the newline from it so we don’t end up with an empty line
@@ -414,22 +415,25 @@ private class AttributeRemover: SyntaxRewriter {
         let trailingTrivia = Trivia(pieces: attribute.trailingTrivia.drop(while: { $0.isSpaceOrTab }))
         triviaToAttachToNextToken += leadingTrivia + trailingTrivia
       } else {
-        filteredAttributes.append(.attribute(attribute))
+        filteredAttributes.append(.attribute(prependAccumulatedTrivia(attribute)))
       }
     }
     return AttributeListSyntax(filteredAttributes)
   }
 
   override func visit(_ token: TokenSyntax) -> TokenSyntax {
-    if !triviaToAttachToNextToken.isEmpty {
-      defer { triviaToAttachToNextToken = Trivia() }
-      return token.with(\.leadingTrivia, triviaToAttachToNextToken + token.leadingTrivia)
-    } else {
-      return token
-    }
+    return prependAccumulatedTrivia(token)
+  }
+
+  private func prependAccumulatedTrivia<T: SyntaxProtocol>(_ node: T) -> T {
+    guard !triviaToAttachToNextToken.isEmpty else { return node }
+
+    defer { triviaToAttachToNextToken = Trivia() }
+    return node.with(\.leadingTrivia, triviaToAttachToNextToken + node.leadingTrivia)
   }
 }
 
 let diagnosticDomain: String = "SwiftSyntaxMacroExpansion"
 
 private enum MacroApplicationError: DiagnosticMessage, Error {

[ahoppen]

The tests look good to me and the source changes also make sense on first sight. Can you push them and then we can work from there.

[gohanlon]

The tests look good to me and the source changes also make sense on first sight. Can you push them and then we can work from there.

Pushed!

All the tests are passing, but note the caveat — I'll address that caveat Thursday evening.

[ahoppen]

Thanks. Your removal of the newline check made me realize that attributes don’t need to be separated by spaces to be valid and that we have a bug surrounding that as well. I added a suggestion of how to fix that one as well, since you’re at it.

[gohanlon]

@ahoppen Pending your feedback, I think this PR is in a pretty good spot.

There are a few improvements I'd like to make/discuss:

• The test coverage is encouraging. You can comment out specific blocks or conditions and get a failing test or tests that reveal the purpose of that code. For me, this pretty much negates the need for the multiline comments—they could be replaced with single-line summarizing comments, with any remaining necessary details moved to the outer doc comment. (And removing the multiline comments would mean we wouldn't have to worry about keeping them updated).
• Change if attributesToRemove.contains(attribute) { … } else { … } to if !attributesToRemove.contains(attribute) { … continue } to remove the distant else and decrease nesting. I've held off on making this change because it'd make the diffs much harder to read.
• Now, almost all the tests in PeerMacroTests actually test MacroSystem.AttributeRemover. Should we create a new test file, e.g. AttributeRemoverTests?
• MacroSystem.AttributeRemover now has a lot of conditional logic. It would be worthwhile to attempt to refactor it to be more succint/readable and ensure there isn't any unnecessary logic.

[Matejkob]

Fantastic job on the unit tests you've added! 🎉 It's really refreshing to see such comprehensive testing, especially in a codebase as complex as ours.

That said, I noticed there's a hefty chunk of tests focused specifically on attribute removal. Have you considered breaking these out into a separate test file? It might make it easier to navigate and maintain down the line.

On another note, free thought here: what do you think about running those attribute removal test cases directly against AttributeRemover instead of going through the PeerMacro? It could make the tests a bit more targeted, and might even expose some edge cases we haven't thought of.

Looking forward to your thoughts! Keep up the awesome work. 👏👏

[Matejkob]

Ah, I see you've already been thinking along the same lines:

• Most tests in PeerMacroTests are actually focused on MacroSystem.AttributeRemover. Should we move these to a new test file, perhaps called AttributeRemoverTests?
• MacroSystem.AttributeRemover has become quite complex with lots of conditional logic. It might be a good idea to refactor it for better readability and to eliminate any unnecessary logic.

I'm in full agreement with moving these tests. What is more, I don't see a compelling reason to keep MacroSystem.AttributeRemover as private. Making it visible for testing could allow us to cover it more directly, don't you think?

As for the refactoring, it's definitely a good idea but probably in follow-up PR?

[bnbarham]

I'm personally fine with either direction here - targeted testing or testing just on MacroSystem itself. But if we go for the former, AttributeRemover should be SPI as it shouldn't be part of the public API. We should also still keep one or two tests on MacroSystem to ensure it's using AttributeRemover correctly.

[ahoppen]

The test coverage is encouraging. You can comment out specific blocks or conditions and get a failing test or tests that reveal the purpose of that code. For me, this pretty much negates the need for the multiline comments—they could be replaced with single-line summarizing comments, with any remaining necessary details moved to the outer doc comment. (And removing the multiline comments would mean we wouldn't have to worry about keeping them updated).

I disagree here. For example when reviewing code on GitHub, I don’t have the option of uncommenting the line and re-running the tests. Also, while reading the code, IMO it’s good practice to capture the intended purpose of these lines since it explains the Why in addition to the What. The purpose of the test cases is, IMO, to verify that the code works on specific examples, which is yet another level of specifying what the code does and why it does it.

• Change if attributesToRemove.contains(attribute) { … } else { … } to if !attributesToRemove.contains(attribute) { … continue } to remove the distant else and decrease nesting. I've held off on making this change because it'd make the diffs much harder to read.

Sounds good to me. Do you want to create a follow-up PR once this one is merged?

• Now, almost all the tests in PeerMacroTests actually test MacroSystem.AttributeRemover. Should we create a new test file, e.g. AttributeRemoverTests?

Splitting seems like a good idea to me. Like @bnbarham I don’t have a strong opinion of whether we should test AttributeRemover directly or do it as part of MacrosSystem testing.

• MacroSystem.AttributeRemover now has a lot of conditional logic. It would be worthwhile to attempt to refactor it to be more succint/readable and ensure there isn't any unnecessary logic.

I’m always in favor of refactorings that make code easier to read. I personally don’t think there’s to be much/any unnecessary code that can be deleted in AttributeRemove right now and I don’t have any ideas off the top of my head of how to improve it. But if you have ideas, I’m happy to see them.

[ahoppen]

Who would have guessed that removing an attribute could be this difficult? Thanks for your work on fixing it. I have one more functional comment. Otherwise it looks good to me.

[gohanlon]

@ahoppen I believe I've addressed your feedback. Care to take a (perhaps!) final look?

Now that we're close, would you like me to squash this PR into a single commit, and annotate it with references to the GitHub and rdar issues?

[ahoppen]

Yes, this looks great. If you could squash the commits, that would be great and I’ll trigger CI.

[gohanlon]

Yes, this looks great. If you could squash the commits, that would be great and I’ll trigger CI.

@ahoppen Squashed. Let me know if there's anything else!

[ahoppen]

@swift-ci Please test

[ahoppen]

Let’s get this in 🚢

[ahoppen]

@swift-ci Please test Windowss

[gohanlon]

@ahoppen Is there something in this PR that needs to be done to satisfy the Windows CI, or perhaps Windows CI is/was generally broken?

[bnbarham]

Nothing on your side. I'll run again, though could be there's a failure that needs fixing.

[bnbarham]

@swift-ci please test Windows platform

[gohanlon]

@ahoppen Quick question: Do you know the release plan for this PR's changes?

[ahoppen]

This was merged into main after we cut the release/5.10 branch in early September, so this will be part of swift-syntax 600, aligned with Swift 6.