Enhancements to diagnostic formatters and testing utilities

[Matejkob]

Overview

This pull request introduces substantial enhancements to our diagnostic functionalities and testing utilities, focusing on greater customizability, readability, and effectiveness in diagnosing Swift code issues.

Details

Refactor DiagnosticsFormatter and Extend Functionality

1. Nested Diagnostic Support:

   • The formatter is now equipped to handle not just top-level diagnostics but also their associated notes.
   • This results in a more holistic and informative debugging experience.

2. Custom Decorators via DiagnosticDecorator:

   • Incorporated the newly introduced DiagnosticDecorator protocol.
   • Now, you can easily plug in custom decorators for specialized formatting and styling, offering a personalized touch to diagnostic output.

3. Context Size Control:

   • Introduced configurable options to control the ContextSize.
   • Depending on your preference, you can now expand or limit the amount of source code context displayed around each diagnostic.

Documentation:

• Comprehensive documentation has been updated to include:
  • Usage scenarios for the formatter.
  • Code snippets for quick integration.
  • Guidelines for future developments and feature extensions.

Testing:

• Added a set of robust unit tests specifically designed for the new features.

Future Work:

• Evaluate the possibility of adding more diagnostic decorators for additional formats (e.g. Fix-It).
• Explore options for further extending testing capabilities.

Fix #2166

[Matejkob]

How to render a scenario when a diagnostic has a note in a different line?
Should we add the information about parent diagnostic to each note?

[ahoppen]

I think this looks good if we use GroupedDiagnosticFormatter. The idea would be that each error basically gets its own printout of the source (we wouldn’t be merging multiple diagnostics into a single source printout anymore) and thus it’s clear which diagnostic the note belongs to.

[DougGregor]

Yes, this looks good to me, too. Right now, the Swift compiler is intentionally grouping things so that there is only a single top-level diagnostic, because that made sure all of the information about a single issue (with all of its notes) would occur in the one printout.

Notes in different files are still a little tricky---they look like top-level things right now, but @ahoppen has reasonably suggested that they should be in a "box" as if they were nested, because notes are never standalone---they're always associated with some other error/warning/remark.

[Matejkob]

@DougGregor in #2166 mentioned:

Note: a similar issue exists with Fix-Its, where we'd need a way to render what they actually do.

Do we already have a way to render this somewhere?

[DougGregor]

I don't think we should try to render Fix-Its into the diagnostic output. We tried with the diagnostic formatter that we've been using in the compiler for years now, and it's hard to provide that much information via a console interface without obscuring the rest of the error. Fix-Its are best handled by editors.

[Matejkob]

@ahoppen @DougGregor @bnbarham @kimdv
I've introduced a substantial number of changes and am now opening this PR for an initial round of reviews. While the code may have minor issues, I'd appreciate any general feedback before diving into the finer details.

[ahoppen]

Could you split this PR into multiple smaller PRs? 1800 is just quite hard to review. I think commits 1 and 2 would make a good PR by themselves.

[Matejkob]

Could you split this PR into multiple smaller PRs? 1800 is just quite hard to review. I think commits 1 and 2 would make a good PR by themselves.

I've split commit 1 and 2 into separate PR: #2238

[ahoppen]

Very nice with all the comments explaining what the code is doing. I mostly have nitpicky comments.

[ahoppen]

When do you think the full context range could be useful? I think recently, we have been aiming to only display a single error/warning + its inline buffer. If we display multiple errors inside the same context, it’s also not clear which notes belong to which errors.

CC @DougGregor

[Matejkob]

My thinking behind offering the full context range was to provide end-developers with extensive customization options if they would intent to use this entity in their projects. For example, the pointfree project uses contextSize set to max number of lines to achieve a full context view.

[ahoppen]

Ah, I see. I still have concerns about how this interacts with the notes but maybe it’s good enough even if the connection between notes and errors/warnings is not obvious.

[DougGregor]

I'm not opposed to having the full context range. From a user perspective, I do wonder if I want to have a different context range for the top-level diagnostic vs. diagnostics in nested buffers (e.g., for macro expansions), because the nested buffers generally have content that's been synthesized.

[ahoppen]

I think we should deprecate this and make contextRange public instead.

Suggested change

	public var contextSize: Int {
	public var contextSize: Int {

[ahoppen]

I don’t think you deprecated this one.

Also, I just remembered that we should add release notes for the deprecations.

[Matejkob]

So if we decide to discard the idea with full context we would need to deprecate this property. In this case, contextSize can still be an Int. So I guess we waiting for Doug's respond here: #2214 (comment)

[ahoppen]

Let’s deprecate this and make the ContextRange initializer public instead.

[ahoppen]

Same here, let’s deprecate it in favor of contextRange.

[ahoppen]

CC @DougGregor I’d like to hear your thoughts on the note printing as well.

[ahoppen]

Almost looks good to me. I’d like to

• Deprecate the contextSize methods mentioned in my last review
• Add release notes for the deprecations
• Get @DougGregor’s opinion on the change, in particular the way notes are printed and ContextRange.full

[ahoppen]

I chatted with @DougGregor about this the other day and he doesn’t have objections. If you could address the remaining open points from my previous comment, I would like to get this in.

[Matejkob]

I chatted with @DougGregor about this the other day and he doesn’t have objections. If you could address the remaining open points from my previous comment, I would like to get this in.

I'll try to carve out some time this weekend to tackle that. Thanks for your patience.

I do have one question, though:

Are we ready to finalize the design of DiagnosticDecorator and make the following initializer public?

  /// Initializes a new `DiagnosticsFormatter` instance.
  ///
  /// - Parameters:
  ///   - contextRange: Specifies the number of contextual lines around each diagnostic message. Default is `.limited(2)`.
  ///   - diagnosticDecorator: The decorator used for formatting diagnostic output. Default is `BasicDiagnosticDecorator`.
  init(
    contextRange: ContextRange = .limited(2),
    diagnosticDecorator: DiagnosticDecorator = BasicDiagnosticDecorator()
  ) {
    self.contextRange = contextRange
    self.diagnosticDecorator = diagnosticDecorator
  }