Improve presentation of diagnostics on the command line

[d-ronnqvist]

Feature Name

Improve presentation of diagnostics on the command line

Description

The current format for diagnostics (warnings and errors) on the command line is in some ways limited by needed to be parsed by tools.

With #494 there's a flag for tools to opt-in to an output format intended to be parsed by tools. This frees up the default command line output format to present diagnostics in a way that's more readable for people.

For example, a readability improvements we could make with the default format

• Use file paths relative to the DocC catalog instead of absolute file paths
• Include the relevant line of content for the diagnostic range (and possibly some surrounding context)
• List related solutions, notes, and explanations together with the main diagnostic
• Use style and colors in environments where they're supported

Motivation

Improving the readability of diagnostics on the command line helps developers who are using DocC outside of an IDE be more productive by making it easier to act on warnings and errors.

Importance

This enhancement is fairly limited to developers using DocC on the command line directly. That is already possible today but improvements to the experience could make developers more productive in their command line use cases.

Alternatives Considered

No response

[d-ronnqvist]

I labeled this "good first issue" because it's an isolated area for someone who is new to the code base to get started with but the downside of that is that it doesn't let a new developer interact with the more central parts of the code base.

[arthurcro]

Hey! 👋

I'm not familiar with how diagnostics works in DocC but I'm interested.
This issues sounds like a good opportunity to learn more about that part of the codebase. I'd love to take a look at this if that's okay with you!

I had a brief look at the ideas for readability improvements mentioned and I have a couple of follow up questions:

• Use file paths relative to the DocC catalog instead of absolute file paths

Would displaying the diagnostic.source URL relatively to the bundle.baseURL achieve the desired result here?

• Include the relevant line of content for the diagnostic range (and possibly some surrounding context)

Did you have any ideas to achieve this properly? I thought about using the diagnostic.range and diagnostic.source to retrieve the relevant line but it doesn't seem right. I also thought about passing the relevant line (if it makes sense to have one at all) to the Diagnostic and display it if it exists but I'm not sure how we could get some surrounding context.

Thanks!

[d-ronnqvist]

This issues sounds like a good opportunity to learn more about that part of the codebase. I'd love to take a look at this if that's okay with you!

Great. I assigned this issue to you.

There's a lot of creative freedom in how to present these diagnostics where there's no definitive right or wrongs approaches. Don't hesitate to ask questions if you wonder how something works or if you just want a feedback on the format/presentation of the diagnostic.

I had a brief look at the ideas for readability improvements mentioned and I have a couple of follow up questions:

• Use file paths relative to the DocC catalog instead of absolute file paths

Would displaying the diagnostic.source URL relatively to the bundle.baseURL achieve the desired result here?

No, the bundle.baseURL is used for adding a prefix to the documentation pages' web URL, for example if the developer host their documentation at a specific sub path on their website.

The bundle itself doesn't have a property for the url to its catalog but that could be worth adding.

It's also possible to have a "bundle" that only consist of symbols graph files and no .docc catalog. The directory of symbol graph files is most likely going to be build directory somewhere so it likely doesn't make a meaningful base URL to the developer. In this case it may be better to use the currentDirectoryPath as the base instead.

• Include the relevant line of content for the diagnostic range (and possibly some surrounding context)

Did you have any ideas to achieve this properly? I thought about using the diagnostic.range and diagnostic.source to retrieve the relevant line but it doesn't seem right. I also thought about passing the relevant line (if it makes sense to have one at all) to the Diagnostic and display it if it exists but I'm not sure how we could get some surrounding context.

Using the diagnostic.range and diagnostic.source is probably the easiest way to retrieve the relevant line. Various places hold parsed versions of the source but I don't think there's currently a convenient or general way to retrieve the original.

The one recommendation I would make with reading the lines from the source is that you check if there are multiple diagnostics in the same file and only read that file content once and extract all the lines. Reading from disk can be comparatively very slow.

By "surrounding context" I meant some number of lines above and below the line with the diagnostic. It's probably best to start with just the line with the diagnostic but I could see cases where the extra context helps; for example it's possible that a diagnostic about a directive argument isn't on the same line as the name of the directive.

(It's also possible that the diagnostic.range is more than one line)

The diagnostic.range also contains information about where in that line the diagnostic relates to. This can be useful to present somehow to visually distinguish 2 separate diagnostics for the same source line.

[d-ronnqvist]

The specific diagnostic formatting that's intended to be read by people happens in DefaultDiagnosticConsoleFormatter but some presentation improvements likely need to involve DiagnosticConsoleWriter as well. For example, it could be nice to present all diagnostic in a specific file after one another or alternatively to present all diagnosis of a specific type together.

[arthurcro]

That's super helpful thank you so much! I appreciate it. I'll keep you updated on my progress here. 😊

Don't hesitate to ask questions if you wonder how something works or if you just want a feedback on the format/presentation of the diagnostic.

I'll probably have more questions as I start playing more with those suggestions, I'll post them here as well. Thank you again for the support!

The one recommendation I would make with reading the lines from the source is that you check if there are multiple diagnostics in the same file and only read that file content once and extract all the lines. Reading from disk can be comparatively very slow.

That's a great point. Could you point me to some existing logic for reading lines from a source? I'm wondering if there is some existing work we could reuse but I couldn't find what I was looking for.

[d-ronnqvist]

Could you point me to some existing logic for reading lines from a source? I'm wondering if there is some existing work we could reuse but I couldn't find what I was looking for.

The documentation processing flow mainly operates on already parsed markdown or on line lists from symbol graph files so it doesn't read lines from the source directly. A handful of tests use .components(separatedBy: .newlines) to prepare tests data. I'd probably start with that.

[arthurcro]

Hi @d-ronnqvist! I got a chance to try out a few things regarding this issue. I opened a pull request on my fork to help with the discussions. I will open the pull request on this repository once it's cleaned up and all the features are added, I hope that's okay!

Could you take a look at this pull request when you have some time and let me know if it's going in the right direction? I would appreciate some feedback.
I'll shortly be adding colors and styling when supported. I wanted to make sure I'm tackling this issue the right way beforehand.

[d-ronnqvist]

Could you take a look at this pull request when you have some time and let me know if it's going in the right direction? I would appreciate some feedback.

Yes, this looks like the right direction to me.

My main feedback would be to run this on some content and tweak the output according to your preferences for what feels useful and most understandable. For example, the TestBundle test content has some intentional diagnostics:

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

You may also want to add some more issues to that content to have a few more diagnostics with solutions to make adjustments for. For example; remove the level-1 heading in an article, introduce a typo in a link, change a SeeAlso section to a level-3 heading, or skip from a level-2 heading to a level-4 heading.

As you are tweaking the output; if that's easy to iterate on, then that's a good sign. When you open the PR on the DocC repo I suspect that more people will have feedback on the output based on their preferences. The goal is to find something that most people can agree upon. Those final refinements will be easier if it's easy to change the output format in the code.

[d-ronnqvist]

Also, you'll possible find that some diagnostics have incorrect ranges.

For example, I think this is a bug that the diagnostic range spans the entire @Step directive and not the @Image directive.

Found in TestTutorial.tutorial:
  warning: Resource 'xcode.png' couldn't be found
  Source:
    179       @Step { 
    180         Lorem ipsum dolor sit amet, consectetur.
    181 
    182         @Image(source: xcode.png, alt: xcode )
    183       }

I would consider those changes out-of-scope for this issue.

[arthurcro]

Thank you so much for the quick feedback! I'll continue with this approach and keep you updated!

My main feedback would be to run this on some content and tweak the output according to your preferences for what feels useful and most understandable.

Right, that's the approach I took so far. I'll continue to tweak the output and introduce some more problems to get a wider range of diagnostics.

When you open the PR on the DocC repo I suspect that more people will have feedback on the output based on their preferences. The goal is to find something that most people can agree upon.

That definitely makes sense, I look forward to hear ideas from more people once the PR is up!
Also, did you have something in mind regarding displaying solutions and fixits? I thought of some diff like text for each replacements in a possible solution but I'm not sure how accurate it would be and wether or not it would be helpful.

[d-ronnqvist]

Also, did you have something in mind regarding displaying solutions and fixits? I thought of some diff like text for each replacements in a possible solution but I'm not sure how accurate it would be and wether or not it would be helpful.

One idea would be look at the output from the Swift compiler. Here I added some build errors to the DocC code

/my/local/path/to/swift-docc/Sources/SwiftDocC/Semantics/Tutorial/Tasks/TutorialSection.swift:35:17: error: 'case' label in a 'switch' must have at least one executable statement
                case .stack(let stack): 
                ^~~~~~~~~~~~~~~~~~~~~~~
                                        break
/my/local/path/to/swift-docc/Sources/SwiftDocC/Semantics/Tutorial/Tasks/TutorialSection.swift:35:33: warning: immutable value 'stack' was never used; consider replacing with '_' or removing it
                case .stack(let stack): 
                            ~~~~^~~~~
                            _

This style of presentation highlights the range of the diagnostic with ~ and displays the replacement below the highlight.

However, it doesn't include the description of the solution, so there's room for improvement there.

---

I introduced some link warnings in TestBundle.docc to get a few examples to work with. Using the approach of highlighting and displaying the replacement would looks something like this for those 3 (I manually edited this output):

Found in documentation/mykit.md:
  warning: 'MyClas' doesn't exist at '/MyKit'
  Source:
    14 - ``MyClas``
           ~~~~~~
           MyClass
    
  warning: 'abc123' isn't a disambiguation for 'globalFunction(_:considering:)' at '/MyKit'
  Source:
    25  - ``globalFunction(_:considering:)-abc123``
                                          ~~~~~~~

Found in documentation/myclass.md:
  warning: 'init()' is ambiguous at '/MyKit/MyClass'
  Source:
    16  - ``init()``
            ~~~~~
                 -33vaw
                 -3743d

For the first 2 issues I think the range and replacement is enough to be understandable but the 2 suggestions for the 3rd issue are not understandable without the individual fixit descriptions. One approach there could be to display each description inline:

Found in documentation/myclass.md:
  warning: 'init()' is ambiguous at '/MyKit/MyClass'
  Source:
    16  - ``init()``
            ~~~~~~
                  -33vaw      Insert '33vaw' for 'init()'
                  -3743d      Insert '3743d' for 'init()'

(these are actually identical copies in the test data so the collision provides less detail than a real example would)

One little complication here is that the fixit description sometimes spans multiple lines. One approach to this could be to always display the fixit descriptions on one line.

Another alternative could be to only display the summary description. That could also help avoid formatting issues if the line is long, the diagnostic happens late in the line, and the fixit summary is long. For example

Found in documentation/myclass.md:
  warning: 'init()' is ambiguous at '/MyKit/MyClass'
  Source:
    16  This is my long line that eventually ends with this ambiguous link: ``init()``
                                                                              ~~~~~~
                                                                                    -33vaw      These are hypothetical long fixit descriptions
                                                                                    -3743d      These are hypothetical long fixit descriptions

[d-ronnqvist]

These are just some ideas.

Another area to look at for inspiration are the diagnostics from Rust. This post contains some examples. Like the blog post describes, these diagnostics present more information inline with the code.

Another place to look at for inspiration is the new diagnostic formatter in Swift-Syntax.

---

In both these cases I think that source code has more reasons to display multiple source ranges together to point out where a variable was defined or mutated. So it's possible that diagnostics for documentation content wouldn't benefit as much from this type of formatting.

[arthurcro]

Thank you for those ideas, it's super helpful, I particularly found this post very interesting.

In both these cases I think that source code has more reasons to display multiple source ranges together to point out where a variable was defined or mutated. So it's possible that diagnostics for documentation content wouldn't benefit as much from this type of formatting.

Right, I tried an approach without highlighting possible solutions and replacements in the code. I think that by adding enough context around the diagnostic source range and displaying solutions/replacements close to the source code, it's relatively easy to figure out what to change to fix the problem.

warning: Can't resolve 'Test-Bunle'
  --> TestTutorial.tutorial:44:64
41 |          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
42 |          ut labore et dolore magna aliqua. Phasellus faucibus scelerisque eleifend donec pretium.
43 |          Ultrices dui sapien eget mi proin sed libero enim. Quis auctor elit sed vulputate mi sit amet.
44 |          This section link refers to this section itself: <doc:/tutorials/Test-Bunle/TestTutorial#Create-a-New-AR-Project-%F0%9F%92%BB>.
45 |          This is an external link to Swift documentation: [Swift Documentation](https://swift.org/documentation/).
46 |          This section link refers to the next section in this file: <doc:/tutorials/Test-Bundle/TestTutorial#Initiate-ARKit-Plane-Detection>.
47 |          This link will never resolve: <doc:ThisWillNeverResolve>.
help: Replace 'Test-Bunle' with 'Test-Bundle'
      fixit:44:75-44:85 Test-Bundle

warning: Duplicate title in 'Section' directive
'Section' directives are identified and linked using their titles and so must be unique within a 'Tutorial' directive; this directive will be dropped
First 'Section' directive with the title 'Duplicate' written here
   --> TestTutorial.tutorial:187:9
184 |       }
185 |    }
186 |
187 |    @Section(title: "Duplicate") {
188 |       @ContentAndMedia {
189 |          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
190 |          ut labore et dolore magna aliqua. Phasellus faucibus scelerisque eleifend donec pretium.

warning: An article is expected to start with a top-level heading title
 --> article.md:1:1
1 | My Cool Article
2 |
3 | This is an article.
4 |
help: Add a title
      fixit:1:1-1:16 # My Cool Article

I also found that highlighting, either with color or some style, the diagnostic summary and the diagnostic source range in the source code display really makes a difference to pinpoint the issue in the source code.
I'm not satisfied with the state of the code in arthurcro#1, it's very much still a draft but I think it's already a good step towards improving the default diagnostic formatting. I would appreciate if you had time to give it another spin and let me know if you have some feedback.

[d-ronnqvist]

This is a big improvement over the current formatting.

I would appreciate if you had time to give it another spin and let me know if you have some feedback.

In some of these diagnostics I find that 3 lines before and 3 lines after is too much context and makes it hard to spot where the issue is. The paragraph of text in the TestTutorial.tutorial:44:64 above illustrate this well.

Other than that; I would maybe consider "suggestion" as an alternate phrase for "help" only because I find it more inviting to read.

I'm not sure what would be a good solution for displaying the fixit replacements. One way could be to "apply" the replacements and display them as a line of code. For example:

warning: An article is expected to start with a top-level heading title
 --> article.md:1:1
1 | My Cool Article
2 |
3 | This is an article.
4 |
suggestion: Add a title
1 | # My Cool Article

However, this may end up mostly repeating the same line, making it hard for the reader to spot what changed. For example:

warning: Can't resolve 'Test-Bunle'
  --> TestTutorial.tutorial:44:64
41 |          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
42 |          ut labore et dolore magna aliqua. Phasellus faucibus scelerisque eleifend donec pretium.
43 |          Ultrices dui sapien eget mi proin sed libero enim. Quis auctor elit sed vulputate mi sit amet.
44 |          This section link refers to this section itself: <doc:/tutorials/Test-Bunle/TestTutorial#Create-a-New-AR-Project-%F0%9F%92%BB>.
45 |          This is an external link to Swift documentation: [Swift Documentation](https://swift.org/documentation/).
46 |          This section link refers to the next section in this file: <doc:/tutorials/Test-Bundle/TestTutorial#Initiate-ARKit-Plane-Detection>.
47 |          This link will never resolve: <doc:ThisWillNeverResolve>.
suggestion: Replace 'Test-Bunle' with 'Test-Bundle'
44 |          This section link refers to this section itself: <doc:/tutorials/Test-Bundle/TestTutorial#Create-a-New-AR-Project-%F0%9F%92%BB>.

Another option would be to display the replacement inline and reference the suggestion description below

warning: Can't resolve 'Test-Bunle'
  --> TestTutorial.tutorial:44:64
41 |          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
42 |          ut labore et dolore magna aliqua. Phasellus faucibus scelerisque eleifend donec pretium.
43 |          Ultrices dui sapien eget mi proin sed libero enim. Quis auctor elit sed vulputate mi sit amet.
44 |          This section link refers to this section itself: <doc:/tutorials/Test-Bunle/TestTutorial#Create-a-New-AR-Project-%F0%9F%92%BB>.
   |                                                                           Test-Bundle       [see suggestion 1]
45 |          This is an external link to Swift documentation: [Swift Documentation](https://swift.org/documentation/).
46 |          This section link refers to the next section in this file: <doc:/tutorials/Test-Bundle/TestTutorial#Initiate-ARKit-Plane-Detection>.
47 |          This link will never resolve: <doc:ThisWillNeverResolve>.
suggestion 1: Replace 'Test-Bunle' with 'Test-Bundle'

[arthurcro]

In some of these diagnostics I find that 3 lines before and 3 lines after is too much context and makes it hard to spot where the issue is. The paragraph of text in the TestTutorial.tutorial:44:64 above illustrate this well.

That's a great point. I think 2 lines before and after should be enough then.

I'm not sure what would be a good solution for displaying the fixit replacements.

Right, that's the topic I'm struggling with now. I took a look at showing the fixits inline but I faced a few roadblocks.

Now, for possible solutions with a single replacement, I'm leaning only showing summary inline at the replacement source location:

warning: 'init()' is ambiguous at '/MyKit/MyClass'
  --> documentation/myclass.md:15:6
13 | Relatively linked symbols
14 |
15 |  - ``init()``
   |            ^suggestion: Insert '33vaw' for 'init()'
   |            ^suggestion: Insert '33vaw' for 'init()'
16 |  - ``init()-3743d``
17 |  - ``myFunction()``

warning: Can't resolve 'Test-Bunle'
  --> TestTutorial.tutorial:44:64
41 |          Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt
42 |          ut labore et dolore magna aliqua. Phasellus faucibus scelerisque eleifend donec pretium.
43 |          Ultrices dui sapien eget mi proin sed libero enim. Quis auctor elit sed vulputate mi sit amet.
44 |          This section link refers to this section itself: <doc:/tutorials/Test-Bunle/TestTutorial#Create-a-New-AR-Project-%F0%9F%92%BB>.
   |                                                                           ^suggestion: Replace 'Test-Bunle' with 'Test-Bundle'
45 |          This is an external link to Swift documentation: [Swift Documentation](https://swift.org/documentation/).
46 |          This section link refers to the next section in this file: <doc:/tutorials/Test-Bundle/TestTutorial#Initiate-ARKit-Plane-Detection>.
47 |          This link will never resolve: <doc:ThisWillNeverResolve>.

I noticed that displaying both replacement and summary for a possible solution could be redundant.

warning: 'init()' is ambiguous at '/MyKit/MyClass'
  --> documentation/myclass.md:15:6
13 | Relatively linked symbols
14 |
15 |  - ``init()``
   |            ^-33vaw: Insert '33vaw' for 'init()'
   |            ^-3743d: Insert '3743d' for 'init()'
16 |  - ``init()-3743d``
17 |  - ``myFunction()``

Also, it might happen that some replacements for a solution are far from the diagnostic source line so showing those source lines to the user might be confusing. In those cases where the replacement is too far from the diagnostic source line or if there are more than one replacement, I'm not sure what is the best way to approach this. I'm thinking dispalying the solution summary at the diagnostic source line might be enough in that case?

[d-ronnqvist]

I'm leaning towards only displaying the solution summary and not the solution replacement.

To some extent this depend on the way solutions summaries are phrased and since DocC didn't use to display solutions we haven't spent much time on guidelines for phrasing solution summaries. I think once the solutions are more visible people will be more aware of them and more motivated to improve the solution summaries.

[d-ronnqvist]

I feel that this is ready for more input from more people on the diagnostic output format. Do you want to bring over the PR to the DocC repo and we can continue the conversation there with more people?

[arthurcro]

I feel that this is ready for more input from more people on the diagnostic output format. Do you want to bring over the PR to the DocC repo and we can continue the conversation there with more people?

Right, it makes sense to me! I'll clean up some of the code a bit and I will bring the PR over to the DocC repo shortly! Thank you for your amazing support on this again! 🙇🏻 I look forward to hear more feedback from more people!

[d-ronnqvist]

With #535 merged I'd say that this issue is complete.

If there are future enhancements we want to do to the presentation of diagnostics we should open new issues for those.