-
Notifications
You must be signed in to change notification settings - Fork 448
Add "grouped diagnostics" that allows rendering across source files #1350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@swift-ci please test |
Extend the diagnostics renderer to handle a set of "grouped" diagnostics, where the diagnostics occur across different source files that are related through source code generation, for example because one is a macro expansion buffer that was triggered within the other. These inner source files are shown as nested source files in the output, underneath the location of where the outer source file where they logically occur.
e490e49
to
344158c
Compare
@swift-ci please test |
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice! I’ve got a few minor comments inline.
|
||
/// Mapping from the root source file syntax nodes to the corresponding | ||
/// source file IDs. | ||
var rootIndexes: [SourceFileSyntax: SourceFileID] = [:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: Could we use the root SourceFileSynax
as the SourceFileID
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? It felt more fragile than if we made this explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are maintaining a map from SourceFileSyntax
to SourceFileID
, don’t we already have any fragility from SourceFileSyntax
now?
Also, note that equality that the identity of Syntax
is defined by its SyntaxIdentifier
, so we don’t do any tree comparisons or anything of the sort when comparing two syntax nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mapping is only used in cases where the client isn't supplying a buffer ID, so you never need to depend on it. We could either drop the mapping internally (and have clients always supply the buffer ID when adding a diagnostic) or drop the buffer ID (and rely on the mapping everywhere), but this does feel like a weird middle ground.
Tests/SwiftDiagnosticsTest/GroupDiagnosticsFormatterTests.swift
Outdated
Show resolved
Hide resolved
Help frame the formatter buffer for color terminals by making the buffer outline and line numbers cyan.
When colorizing output, underline highlighted source ranges from diagnostic messages.
@swift-ci please test |
tree: SyntaxType, | ||
diags: [Diagnostic], | ||
indentString: String, | ||
suffixTexts: [(AbsolutePosition, String)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn’t it be conceptually simpler if this was [AbsolutePosition: String]
or [AbsolutePosition: [String]]
(I’d prefer the former if it’s possible). That would suggest better to me there’s a key value relationship between AbsolutePosition
and String
here.
That being said, I understand that we’re never accessing suffixTexts
by its AbsolutePosition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little nicer, so I went ahead and did it in #1365
|
||
/// Mapping from the root source file syntax nodes to the corresponding | ||
/// source file IDs. | ||
var rootIndexes: [SourceFileSyntax: SourceFileID] = [:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are maintaining a map from SourceFileSyntax
to SourceFileID
, don’t we already have any fragility from SourceFileSyntax
now?
Also, note that equality that the identity of Syntax
is defined by its SyntaxIdentifier
, so we don’t do any tree comparisons or anything of the sort when comparing two syntax nodes.
Extend the diagnostics renderer to handle a set of "grouped"
diagnostics, where the diagnostics occur across different source files
that are related through source code generation, for example because
one is a macro expansion buffer that was triggered within the other.
These inner source files are shown as nested source files in the
output, underneath the location of where the outer source file where
they logically occur.
Introduce a few other small improvements to the colorization in the
diagnostics formatter that became more apparent when we do this:
Here's an example from the tests: