Improve default diagnostic formatter

[arthurcro]

Bug/issue #, if applicable: #496

Summary

Hey there! 👋

After some great discussions on #496, I'm opening this pull request to improve the default diagnostic output formatting.

At present, the default diagnostic output shares the same format as the one designed for tool parsing. Consequently, it can be challenging for people to read and comprehend diagnostic output on the command line.

/Users/arthurcrocquevieille/Developer/Forks/swift-docc/Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestOverview.tutorial:17:7-17:57: fixit: 
/Users/arthurcrocquevieille/Developer/Forks/swift-docc/Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestTutorial.tutorial:44:87: warning: 'TestTutoria' doesn't exist at '/Test-Bundle' Replace 'TestTutoria' with 'TestTutorial' Replace 'TestTutoria' with 'TestTutorial2' Replace 'TestTutoria' with 'TestTutorialArticle'
/Users/arthurcrocquevieille/Developer/Forks/swift-docc/Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestTutorial.tutorial:46:74: warning: Can't resolve 'Test-Bundl' Replace 'Test-Bundl' with 'Test-Bundle'
/Users/arthurcrocquevieille/Developer/Forks/swift-docc/Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestTutorial.tutorial:46:85-46:95: fixit: Test-Bundle

We believe there is significant potential to improve the readability of diagnostic output that is not intended for tool parsing. This pull requests makes the following changes:

• Displaying file paths relative to the DocC catalog instead of using absolute file paths
• Including the relevant line of content for the diagnostic range (and possibly some surrounding context)
• Listing related solutions, notes, and explanations alongside the main diagnostic
• Utilizing style and colors in environments where they are supported

Notes

• Diagnostics are displayed one at a time, order by source path and source range.
• Displaying solutions inline with the content relies a lot on the information contained in a solution summary.
• For possible solutions that have a single replacement, they are displayed at the location of the replacement. For possible solutions with zero or more than one replacement, they are displayed at diagnostic source location.

Testing

To test this, you can run the following:

swift run docc convert Tests/SwiftDocCTests/Test\ Bundles/TestBundle.docc/

To get a wider variety of diagnostics, you can modify files in TestBundle.docc.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary

[stackotter]

I love it! I've always found the primarily tool parsable diagnostics of apple's proprietary tools (docc having been open sourced but originally being an internal tool), such as docc and xcodebuild quite terrible. I think these changes are super valuable for the user experience.

Feedback

Many people use terminals with limited widths, so putting all of the suggestions for an issue after the first column of the issue (the part highlighted green) would probably mean that they get wrapped pretty often and end up being quite confusing to read. I've made a tool for reformatting the output of the Swift compiler (which is kind of similar) and here's an example of the output that it produces. It does have a lot of repetition of the issue line (so that the fixits can be displayed) but the docc suggestions don't have fixits of that style so you could possibly just have the suggestions listed like they are now in the screenshot you attached, but not indented all the way to line up with the issue. Hopefully that makes sense, I'm open to changing my opinion!

(I'm not completely happy with the output my tool produces either, especially the big long arrows for the fixits, but I haven't had the time to work on it for a while)

I have a feeling we both took inspiration from the Rust compiler diagnostics 😅

[franklinsch]

This looks super nice @arthurcro. I also like that this gives us more space to provide more detailed information about diagnostics if we'd like to in the future. What does the experience look like if the terminal doesn't support colors/formatting?

[arthurcro]

Hi @stackotter! Thank you for the feedback, it's greatly appreciated!
The work done on apple/swift-syntax#874 and this post has indeed served an inspiration for this draft.

I agree that lines wrapping due to limited widths could be an issue when displaying multiple suggestions under each other. I was inspired by apple/swift-syntax#874 which does a great job at displaying multiple issues on a same line of content. Although it does not address the line wrapping issue.

I believe that duplicating the same line of content for each suggestion might be redundant and could potentially make it more challenging to discern the surrounding context. What are your thoughts on this?

[arthurcro]

Hi @franklinsch! Thank you for you feedback, I'm super excited for this improvement! 😊

Ansi annotations, responsible for colors and formatting, will be applied only when the terminal supports them.
If the terminal doesn't support colors or formatting, the output would look like this:

I find it more challenging to find the suggestions within the lines of contents although the absence of line number and the "suggestion" prefix help a bit.
What are your thoughts on this? Do you have any other ideas for how it could be improved?

[d-ronnqvist]

One small change to improve the situation where the terminal doesn't support colors and styled text would be to use a different character than | after the line number for the highlighted line like in the screenshot that @stackotter posted (where the diagnosed line starts with +>).

[arthurcro]

Thanks for the feedback! You're right, using a different character for the highlighted line significantly improves readability. I've incorporated this change into the draft PR. 🙂

I also just updated the implementation to make sure we highlight the output only when supported. I was inspired by https://github.com/apple/swift-syntax/blob/560df352d048295cc451e806eb30f7d79021d617/Sources/swift-parser-cli/TerminalUtils.swift#L27.
Unless I receive further feedback, I'll proceed with writing tests to prepare this PR for review! 🙂

[d-ronnqvist]

This generally looks good to me. All the feedback I have is very minor suggestions and aren't blocking.

The only change that I'd request is to not make the changes to the DiagnosticsConsoleWriter initialized public because I think we'd like to change that again in the future and it'd be nice to not need to make that a public breaking change.

[d-ronnqvist]

This to me highlights an issue with the existing abstraction for how formatters and formatting options are defined (this issue existed before your PR, it's just more noticeable now).

I don't think it's necessary (or even preferable) to fix the abstraction in this PR but I would like to make it easier to fix the abstraction later. To make that easier I think it would be good to keep the previous public initializer unchanged and add a new internal initializer that accept all 4 arguments. That way, we can change how the base URL and highlighting are configured in the future without breaking public API.

[d-ronnqvist]

This isn't feedback on this PR but rather some additional thoughts about how this abstraction could be improved in the future.

Here are some of the issues that this is making me notice:

• the base URL and highlighting are only applicable to one initializer formatter
• the only existing DiagnosticFormattingOptions value is used to configure which formatter is used, not how that formatter is configured.
• DiagnosticFormattingOptions can't really hold configuration with values (like the URL)

I don't know what would be a better abstraction for this. I think that each formatter should have their own configuration so that it's not necessary or possible to specify configuration which will be ignored because it doesn't apply). Having separate configuration for each formatter would also make it easier to disallow conflicting configuration.

One way to accomplish this could be to take the formatter as an argument but I'm hesitant to make the formatter a public type since I don't feel that it's API is refined enough to solidify yet. One way to workaround that could be to use different (public) option types for each formatter. It's also possible that work done by the DiagnosticConsoleWriter is simple enough that the formatter abstraction isn't useful and that each formatter should be a DiagnosticFormattingConsumer instead.

[arthurcro]

That's a great point, thank you for highlighting this! I update the pull request with the changes mentioned to make the abstraction improvements easier later on.

[arthurcro]

I don't think it's necessary (or even preferable) to fix the abstraction in this PR but I would like to make it easier to fix the abstraction later. To make that easier I think it would be good to keep the previous public initializer unchanged and add a new internal initializer that accept all 4 arguments. That way, we can change how the base URL and highlighting are configured in the future without breaking public API.

@d-ronnqvist I noticed that a public initializer is required if we want to set the baseUrl in both ConvertAction and IndexAction since they are defined in SwiftDocCUtilities. The baseUrl is used to show the relative path of the source files.
This is currently failing the build as it needs to be updated. I just wanted to confirm that this is what we want or we want to make it available to those actions? After I get confirmaiton I'll push a commit to fix this and the build should pass! 🙂

[d-ronnqvist]

I don't think this would be able to read the relevant lines from a symbol's in-source documentation comment if the original source file wasn't there. When DocC is used as part of a build workflow the original source files would be present at their original locations but it's also possible to build documentation with a symbol graph file without the original source being there (and the symbol graph file contains a copy of what each symbol's documentation comment was).

I don't think this needs to be fixed in this PR but it would be good with a todo comment that mentions adding support for getting the source lines from the symbol graph files.

[arthurcro]

Great point, I did not consider this scenario. I added a todo comment to add support for this at a later time!

[stackotter]

One small change to improve the situation where the terminal doesn't support colors and styled text would be to use a different character than | after the line number for the highlighted line like in the screenshot that @stackotter posted (where the diagnosed line starts with +>).

Sorry for the late reply, now that I think about it, this simple change would basically address my main readability concerns 👍 looks good to me

[d-ronnqvist]

I think this probably looks good to me to merge to main. It'd be good for someone else on the team to also look at this.

[d-ronnqvist]

Apologies for the delay. People have been busy with other responsibilities around WWDC and are catching up on things.

I'll try and see if someone else can also review this—I'm mostly looking to see if other people have thoughts on the output formatting—otherwise I'll do a second pass of the code and people can iterate on the output formatting after it's merged.

[arthurcro]

No worries @d-ronnqvist! I figured you all were probably busy with other things! I appreciate the heads up 😊
I look forward to getting more feedback on this!

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

Sorry for the long delay. This looks good to me. Thank you.

I'm marking this as approved even though there are two remaining actions:

• A Linux specific build failure (see this comment)
• Merge the latest changes from main (the "Update branch" button)

This won't need another review after those changes.

[Kyle-Ye]

LGTM. Only some small suggestion

[Kyle-Ye]

@swift-ci please test

[Kyle-Ye]

Can't build it successfully both on my local machine and the CI env.

See https://ci.swift.org/job/pr-swift-docc-macos/879/console cc @arthurcro

[arthurcro]

Can't build it successfully both on my local machine and the CI env.

See https://ci.swift.org/job/pr-swift-docc-macos/879/console cc @arthurcro

@Kyle-Ye Yes, thanks for the heads up. It's because of #535 (comment).

[d-ronnqvist]

Can't build it successfully both on my local machine and the CI env.
See https://ci.swift.org/job/pr-swift-docc-macos/879/console cc @arthurcro

@Kyle-Ye Yes, thanks for the heads up. It's because of #535 (comment).

Sorry, I missed when I made that comment that the initializer was called from another module.

Given that, I think it'd make the most sense to make the initializer public in this PR. In my opinion, adding and later removing public initializer like this one isn't "bad" enough to be worth adding other workarounds for.

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

@arthurcro I rebased and merged this for you. Thanks for all your work on this!

[arthurcro]

@d-ronnqvist Thank you for all the support! 😊 I look forward to contributing further to DocC!