Introduce Diagnostic Decorator and Testing Utilities

[Matejkob]

This PR is part of the efforts related to #2214.

Commit 1: Add DiagnosticDecorator Protocol, ANSIDiagnosticDecorator and BacisDiagnosticDecorator Implementations

• Introduce a new protocol, DiagnosticDecorator, to standardize the decoration of diagnostic output.
• Implement ANSIDiagnosticDecorator, which provides ANSI colorization and severity-based prefixes for enhanced visibility.

Commit 2: Introduce Diagnostic Testing Utilities

• Add DiagnosticTestingUtils.swift to facilitate more effective unit testing of diagnostics.

[ahoppen]

Thanks for splitting the PR. It makes the review a lot easier. And also thanks for the long comments, they also really help understand what’s going on.

I left a couple comments inline. On a high level, I’ve got a couple of questions:

• What do you imagine the use case of implementing a DiagnosticDecorator to be? AFAICT ANSIDiagnosticGenerator is the only one that we currently have and that you’ll introduce in Enhancements to diagnostic formatters and testing utilities #2214 and I would prefer to not have the DiagnosticDecorator protocol abstraction unless we actually use it.
• I would prefer to not make DiagnosticDecorator, ANSIDiagnosticDecorator public. If we do, it becomes part of the public API surface and it’s a lot harder to change in the future if we don’t want to break it.
• AFAICT nothing is using the DiagnosticDecorator yet. I understand that you will be using it in Enhancements to diagnostic formatters and testing utilities #2214 but I think it would be good to start using it in the current implementation already unless it’s a huge amount of work. I understand that it’s more work that might seem unnecessary but it would really split Enhancements to diagnostic formatters and testing utilities #2214 into two self-contained pieces that are easier to review.

[ahoppen]

Why do we need to return additionalHighlightedLine if it’s always nil?

[Matejkob]

It is nil in this implementation because the ANSI codes are able to underline lines directly. The assumption is that in future implementations it will contain characters like ~~~ or ---.

[ahoppen]

I would have a slight preference to only add additionalHighlightedLine once we actually add a diagnostic annotator that uses it, but I’m fine with keeping it as well.

My thought is just that having an API which we don’t actually test is just relatively error-prone.

Edit: And I just realized that it looks like the diagnostic formatter is even ignoring the additional line right now, which means that it’s quite confusing if we offer it on the protocol but don’t actually handle it.

[Matejkob]

In the Enhance diagnostic formatter to include trivia between highlight nodes thread, you mentioned the idea of changing assertSyntaxHighlightRanges to actually underline the highlighted ranges, either via DiagnosticDecorator or a custom function. I think that's a really neat idea. Given that we plan to use DiagnosticDecorator in the next PR, I'd opt for keeping it as is for now.

[Matejkob]

Thank you for taking the time to review my PR @ahoppen ; your feedback is invaluable, and I really appreciate the time you've invested.

Addressing your concerns:

• What do you imagine the use case of implementing a DiagnosticDecorator to be? AFAICT ANSIDiagnosticGenerator is the only one that we currently have and that you’ll introduce in Enhancements to diagnostic formatters and testing utilities #2214 and I would prefer to not have the DiagnosticDecorator protocol abstraction unless we actually use it.

The overarching vision for the DiagnosticDecorator, as well as other key components like DiagnosticsFormatter and GroupedDiagnostics in the SwiftDiagnostics package, which I have in mind, is to serve as highly customizable and modular tools. The long-term goal would be to empower end-users either by allowing them to build upon these components or by using them directly. To give you a concrete example, @pointfreeco swift-macro-testing library has already adopted the DiagnosticsFormatter. Unfortunately, they had to copy-paste the code into their project and manually adjust its implementation, even though their project depends on swift-syntax. This is hardly an ideal scenario and also illustrates the need for a more modular approach.

But it's not just about third-party implementations; we also have potential use-cases within our own ecosystem. For instance, consider the area of macro diagnostics testing. Currently, we have an assert method for macros, which, while functional, lacks advanced diagnostic capabilities. I believe there's a real opportunity here to provide the developer community with a simple yet effective tool for visualizing and debugging macro diagnostics. As we know, debugging macros can be challenging due to the compiler expansions involved, and this feature could significantly alleviate that pain.

Lastly, let's talk about the immediate benefits. We are already planning enhancements to DiagnosticsFormatter by adding missing diagnostic elements, which will require robust unit testing. Currently, those unit tests containing diagnostic's highlights are hard to read and maintain, especially when they include complicated diagnostic output expectations. Take this example:

let expectedOutput = """
  \u{001B}[0;36m1 │\u{001B}[0;0m for \u{001B}[4;39m(i\u{001B}[0;0m \u{001B}[4;39m= 🐮; i != 👩‍👩‍👦‍👦; i += 1)\u{001B}[0;0m { }
    \u{001B}[0;36m│\u{001B}[0;0m │      ╰─ \u{001B}[1;31merror: \u{001B}[1;39mexpected ')' to end tuple pattern\u{001B}[0;0m
    \u{001B}[0;36m│\u{001B}[0;0m ╰─ \u{001B}[1;31merror: \u{001B}[1;39mC-style for statement has been removed in Swift 3\u{001B}[0;0m
  """

With the decorator abstraction in place, we can create more readable and developer-friendly unit tests, as shown below:

let expectedOutput = """
  1 │ for (i = 🐮; i != 👩‍👩‍👦‍👦; i += 1) { }
    │     ~~ ~~~~~~~~~~~~~~~~~~~~~~~
    │ │      ╰─ error: expected ')' to end tuple pattern
    │ ╰─ error: C-style for statement has been removed in Swift 3

  """

• I would prefer to not make DiagnosticDecorator, ANSIDiagnosticDecorator public. If we do, it becomes part of the public API surface and it’s a lot harder to change in the future if we don’t want to break it.

You bring up a valid concern about making DiagnosticDecorator and ANSIDiagnosticDecorator public, given that it would lock us into maintaining those APIs and limit our ability to make future changes. I completely agree with you on this. In fact, with the understanding that these APIs are likely to evolve, I've already marked DiagnosticDecorator and ANSIDiagnosticDecorator with @_spi(Testing). This should help us maintain the flexibility to adapt and refine these components as we continue to build upon them.

• AFAICT nothing is using the DiagnosticDecorator yet. I understand that you will be using it in Enhancements to diagnostic formatters and testing utilities #2214 but I think it would be good to start using it in the current implementation already unless it’s a huge amount of work. I understand that it’s more work that might seem unnecessary but it would really split Enhancements to diagnostic formatters and testing utilities #2214 into two self-contained pieces that are easier to review.

Great point about immediate use. I've already integrated DiagnosticDecorator into DiagnosticsFormatter to make this PR more self-contained and easier to review.

Note

Disclaimer: I want to clarify that my proposed changes stem from research, requirement gathering, and learning about the existing landscape—rather than imposing my own singular vision. I recognize the challenges in fully capturing the current and future needs for diagnostic formatters. The PRs are meant as a collaborative stepping stone, combining what I've learned, observed in other projects, and a few of my own ideas. My aim is for us to build a unified vision together, using the code I've already written as a foundation.

[ahoppen]

Thanks for providing the detailed response. I think the overall direction is very good. I would just like to keep DiagnosticDecorator internal until we have gained some more experience with it. I think all the other comments are fairly minor.

[ahoppen]

Instead of silently failing and not generating a Diagnostic for a DiagnosticDescriptor if it fails, what do you think about throwing an error instead? I think that should make it a lot clearer what’s going wrong if you write eg. a DiagnosticDescriptor with a marker that doesn’t exist in the source code.

[Matejkob]

I believe that implementing the assert function in this way would indeed make the code clearer, but I'm not entirely convinced that the end result would be more transparent to the users of this high-level API.

My primary motivation behind this implementation approach was to avoid interrupting the assertion process unnecessarily. The assertAnnotated function takes a collection of diagnosticDescriptors as an argument. I chose to return nil while also invoking XCTFail so that we could iterate over all elements, inform the user only about the missing ones, and apply the existing ones to provide as much assertion validation as possible. For instance, if only one location marker out of four is missing, the user will end up with the following result:

I believe this approach benefits end clients. If I'm not mistaken, it wouldn't work in the same way if we chose to throw an error, would it?

[ahoppen]

Oh, I missed the fact that we always emitted an XCTFail if we return nil.

And I think me missing that is a reason why I would prefer to throw. My thinking is that if you are missing one of the diagnostic markers in your test input, there’s something wrong with the test case itself (not just a runtime failure) and thus early exiting is quite alright.

As it stands right now, every time I would read this code, I would wonder the same thing that I originally commented:

• Are we silently dropping one of our test case assertions here? That seems dangerous
• Looks at implementation/documentation: Oh, no, it’s fine we emit an XCTFail in that case.
• Now, what was I doing originally? Needs to restore mental context after side-stepping.

[Matejkob]

You make some solid points. You got me—I'm convinced.

I'll swap out the nil and XCTFail combo for a good ol' error throw. Thanks for the nudge!

[ahoppen]

Oh, I missed the fact that we always emitted an XCTFail if we return nil.

And I think me missing that is a reason why I would prefer to throw. My thinking is that if you are missing one of the diagnostic markers in your test input, there’s something wrong with the test case itself (not just a runtime failure) and thus early exiting is quite alright.

As it stands right now, every time I would read this code, I would wonder the same thing that I originally commented:

• Are we silently dropping one of our test case assertions here? That seems dangerous
• Looks at implementation/documentation: Oh, no, it’s fine we emit an XCTFail in that case.
• Now, what was I doing originally? Needs to restore mental context after side-stepping.

[ahoppen]

One really small nitpick, otherwise looks good to me 👍🏽

[ahoppen]

@swift-ci Please test

[Matejkob]

CI failed with the following message:

DiagnosticsFormatter.swift:80:28: error: cannot find type 'DiagnosticDecorator' in scope

From a Swift perspective, this doesn't seem to be failing, since it's in the same module and the type is internal. Should I modify CMakeLists.txt? Is there any documentation where I can learn more about what's going on here?

[bnbarham]

Should I modify CMakeLists.txt?

Yeah, you'll need to add all the new files to cmake as well.

Is there any documentation where I can learn more about what's going on here?

Not really. The compiler uses cmake rather than SPM to build swift-syntax (for the libraries included in the toolchain), so any new files have to be added to the relevant CMakeLists.txt.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[ahoppen]

Test Case '-[SwiftDiagnosticsTest.GroupedDiagnosticsFormatterTests testGroupingForMacroExpansion]' failed (0.487 seconds).

@Matejkob Could you look at the failure?

[Matejkob]

Oops! I mistakenly committed a modified test and disabled the GroupedDiagnosticsFormatterTests test case in Xcode. Consequently, when I ran all the tests, it didn't fail. My apologies for the oversight. 🙈

[ahoppen]

No problem. Let’s try again.

@swift-ci Please test

[Matejkob]

Is this the main problem /Users/ec2-user/jenkins/workspace/swift-syntax-PR-macOS/branch-main/swift/lib/LLVMPasses/ARCEntryPointBuilder.h:21:10: fatal error: 'llvm/TargetParser/Triple.h' file not found ?

[ahoppen]

Seems like an unrelated issue. Trying again.

@swift-ci Please test macOS

[ahoppen]

@swift-ci Please test Windows