Skip to content

[SwiftDiagnostics] Make GroupedDiagnostics Emit Notes#3279

Open
filip-sakel wants to merge 7 commits intoswiftlang:mainfrom
filip-sakel:notes-diagnostic-group
Open

[SwiftDiagnostics] Make GroupedDiagnostics Emit Notes#3279
filip-sakel wants to merge 7 commits intoswiftlang:mainfrom
filip-sakel:notes-diagnostic-group

Conversation

@filip-sakel
Copy link

The PR addresses this issue. Originally, #2214 tried tackling this, but it got split into several PRs (including #2238 which I heavily lean on in this PR). Since #2214 was based on a 2024 version of the project, I opted to just fork main instead of that PR's branch.

I'm not sure if there's a cleaner approach here, but I just modified DiagnosticFormatter to allow including notes in the output. DiagnosticFormatter already handles multiple diagnostics per line, so I just modified it to print “annotations” (an array of diagnostics and notes) instead of just diagnostics.

Also, I added a simple test with notes at different nesting levels, and one multi-line diagnostic. Of course, I can add more tests if we want to be more thorough.

Finally, you’ll notice some auxiliary changes in DiagnosticFormatter.annotatedSources that aren't technically necessary:

  1. Constructing annotationsPerLine with dictionaries: The previous code iterated over the source lines and filtered the diagnostics every iteration, resulting in quadratic time complexity. I just rewrote this code using dictionaries. This approach simplifies adding “annotations” (which are either diagnostics or notes), and is slightly more efficient. As a result, the I was able to convert the for-loop that creates annotatedSourceLines to an Array.map.
  2. Additional comments: I noticed that now that we handle both diagnostics and notes, the function became too large and a bit hard to read. I added some comments to section the code, and to briefly explain what some big code blocks are doing. Hopefully, the resulting function is more legible.

Please let me know if I should try a different approach altogether, revert some of the auxiliary changes, or add more tests.

@filip-sakel
Copy link
Author

Hi @ahoppen, I hope you're well! I'm following up as I'll have some free time in the next few days to work on this code. Please let me know if you’d like me to start reviewing it so we can get the PR merged.

@kathygray-pl
Copy link

Could you add a test where there are multiple error diagnostics anchored at the same location? As well as multiple notes?

@filip-sakel
Copy link
Author

Sorry for the delay; here are the changes I made. For one, I generalized the test name to "testGroupingForSameLineDiagnostics" to better match the rest of the tests for group diagnostics. Now, we test a variety of things:

  1. Multiple errors in the same column (mixed with note diagnostics), e.g.
    |  +--- #invertedEqualityCheck ---------------------------------------
    |  |1 | !(__a == __b)
    |  |  | | |   |  `- note: Inferred type 'Int' here
    |  |  | | |   |- error: no matching operator '==' for types 'String' and 'Int'
    |  |  | | |   |- error: operator `==` is unavailable: unavailable in embedded Swift
    |  |  | | |   `- error: #invertedEqualityCheck expects operands to be integer literals 
    
  2. Multiple notes in different columns, e.g.
    2 | #myAssert(pi == 3)
      | |               `- note: Inferred type 'Int' here
      | `- note: in expansion of macro 'myAssert' here
    
  3. Multiple notes in the same diagnostic and column, e.g.
    |3 | if #invertedEqualityCheck(__a, __b) {
    |  |    |- note: in expansion of macro 'invertedEqualityCheck' here
    |  |    |- note: #invertedEqualityCheck expects operands of the same type
    |  |    `- note: #invertedEqualityCheck expects '__a' and '__b' to have the same type
    
    

Please let me know if you'd like additional tests or if you think I should break up this test function into smaller, separate functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants