Enhance assertMacroExpansion to Validate Post-Fix-It Macro Expansions

[Matejkob]

This PR introduces an enhancement to the assertMacroExpansion function in SwiftSyntax's testing suite. The key addition is the capability to validate macro expansions on source code after the application of Fix-Its. This change ensures a more robust and comprehensive testing process, particularly for macros that propose fix-its for diagnostics.

Modifications include:

• Addition of a new parameter expandedFixedSource in test assertions. This parameter allows users to assert against the expanded source code after applying Fix-Its.
• Update of assertMacroExpansion to handle the new testing workflow. It now performs macro expansion tests on both the original and the fixed source code, ensuring that both the pre-fix and post-fix states are correctly validated.
• Refinement of the internal logic for applying Fix-Its and conducting macro expansion tests, ensuring accuracy and efficiency in the testing process.
• Update of test cases in AddBlockerTests.swift and AddCompletionHandlerMacroTests.swift to demonstrate and validate the new functionality.

Resolves: #2333

[Matejkob]

Blocked by #2329

[ahoppen]

I’m not sure how useful this is since the compiler would never automatically apply the Fix-Its and then expand the macro. IMO you should write a test case that checks the error case (and fixedSource) and then test the actual expansion in a different test case.

[Matejkob]

I’m not sure how useful this is since the compiler would never automatically apply the Fix-Its and then expand the macro. IMO you should write a test case that checks the error case (and fixedSource) and then test the actual expansion in a different test case.

Thank you for your input. I understand your point about the compiler's typical behavior. However, the enhancement to assertMacroExpansion is intended to streamline testing by simulating a developer's real-world workflow. In practice, developers often apply Fix-Its and then expect macros to function correctly. This update allows for testing this entire process within a single test case, reflecting how users actually interact with macros.

By incorporating expandedFixedSource, we're not just following the compiler's process, but providing a more efficient and comprehensive testing approach. This helps in validating the macro's behavior in scenarios that developers are likely to encounter, making our testing suite more practical and robust.

[bnbarham]

I understand the motivation here, but assertMacroExpansion is already relatively complex and adding yet another parameter to it only adds to that complexity. IMO it's generally more understandable for each test to have a focus - in this case, tests for the fix-its and other tests for the expansion of valid code. Mixing them together ends up creating giant tests that are difficult to process and likely duplicates already tested functionality.

[Matejkob]

In that case I'm closing the PR and associated issue

[mbrandonw]

IMO it's generally more understandable for each test to have a focus - in this case, tests for the fix-its and other tests for the expansion of valid code. Mixing them together ends up creating giant tests that are difficult to process and likely duplicates already tested functionality.

There are also downsides to not automatically expanding fix-its that are worth considering. It becomes very easy to assert that some fix-it is emitted but forget to test how the fix-it applies to the source code. In fact, Apple's recent swift-mmio makes this mistake a number of times where it is never asserted how the fix-it applies.

So, while the test size can increase with the automatic application of fix-its, it does make the test more correct and less chance of forgetting something. And in our experience, it's still possible to keep the test size down to a reasonable size, even for complex macros (here's an example).

[Matejkob]

There are also downsides to not automatically expanding fix-its that are worth considering. It becomes very easy to assert that some fix-it is emitted but forget how the fix-it applies to the source code. In fact, Apple's recent swift-mmio makes this mistake a number of times where it is never asserted how the fix-it applies.

It would be beneficial if the application of fix-its were covered in those test cases. However, if I'm not mistaken, the current official version of swift-syntax does not include the feature to assert fix-it applications in the assert macro function. 😟

[ahoppen]

It would be beneficial if the application of fix-its were covered in those test cases. However, if I'm not mistaken, the current official version of swift-syntax does not include the feature to assert fix-it applications in the assert macro function. 😟

FWIW @kimdv added the capability to test the fixed source in #2021 but it’s not part of a swift-syntax release yet.