[Diagnostics] Make the node and position of Diagnostic and Note optional #3038
Conversation
Some tools that wish to make use of the diagnostics machinery don't have source locations, or even a Swift source file to point into. For example, the compiler and driver both have diagnostics that don't involve anything in source code, e.g., because they are diagnosing problems at the module level such as conflicting or incorrect compiler options. At present, the driver uses a different scheme for emitting diagnostics, and the compiler falls back to using the LLVM diagnostics machinery when there is no source location information. Making the "node" and "position" of a Diagnostic optional make it possible to express diagnostics that aren't associated with sources. Do so, and update the diagnostic formatter to support displaying diagnostics with no source location using the same `<unknown>:0:0:` scheme the compiler users.
|
@swift-ci please test |
|
SwiftFormat breaks as well, which shouldn't be a surprise |
There was a problem hiding this comment.
I am a little worried about the source breakage this might cause and also about ambiguities about diagnostics that have node == nil && position != nil or node != nil && position == nil – I don’t know what that should signify and the diagnostic formatter also seems to be inconsistent about this.
Maybe we need to introduce another type like DiagnosticLocation to wrap node and position. That would also have the advantage that we could keep node and position as deprecated non-optional compatibility accessors.
Something like
enum DiagnosticLocation {
/// The diagnostic is anchored at the start of the given syntax node.
case startOfNode(Syntax)
/// The diagnostic is anchored at the given position within the syntax tree that contains the given node.
case position(AbsolutePosition, inTreeOf: Syntax)
/// The diagnostic does not have a source location associated with it.
case unknown
/// The node at whose start location the message should be displayed.
public var node: Syntax? { … }
/// The position at which the location should be anchored.
public var position: AbsolutePosition? { … }
}| position: AbsolutePosition? = nil, | ||
| message: DiagnosticMessage, | ||
| highlights: [Syntax]? = nil, | ||
| notes: [Note] = [], | ||
| fixIts: [FixIt] = [] | ||
| ) { | ||
| self.node = Syntax(node) | ||
| self.position = position ?? node.positionAfterSkippingLeadingTrivia | ||
| self.node = node.map { Syntax($0) } |
There was a problem hiding this comment.
I think we have a Syntax initializer that just proposes nil, so it should be possible to leave this as-is.
| $0.location(converter: slc).map { | ||
| annotatedLine.characterColumn(ofUtf8Column: $0.column) | ||
| } |
There was a problem hiding this comment.
Maybe it’s just me but I find Optional.map always hard to read and would prefer.
| $0.location(converter: slc).map { | |
| annotatedLine.characterColumn(ofUtf8Column: $0.column) | |
| } | |
| if guard let location = $0.location(converter: slc) else { | |
| return nil | |
| } | |
| return annotatedLine.characterColumn(ofUtf8Column: $0.column) |
| } | ||
|
|
||
| // Render any floating diagnostics, which have no anchor. | ||
| renderedDiags.append( |
There was a problem hiding this comment.
Opinionated question: Should we render the floating diagnostics before or after the diagnostics with source locations? I would have rendered them before because I visualize them on the first line.
Also, using += instead of append(contentsOf:) would remove a nesting level here.
| assertStringsEqualWithDiff( | ||
| annotated, | ||
| """ | ||
| <unknown>:0:0: error: this is bad [#Badness] |
There was a problem hiding this comment.
Can we suppress the rendering of 0:0 here? It doesn’t add any value.
| note.location(converter: locationConverter), | ||
| note.location(converter: locationConverter)!, |
There was a problem hiding this comment.
We should make sure that the test doesn’t crash if a note without a source location should be generated. Instead, we should raise a test failure.
| @@ -57,6 +57,10 @@ public struct GroupedDiagnostics { | |||
| /// source file IDs. | |||
| var rootIndexes: [Syntax: SourceFileID] = [:] | |||
|
|
|||
| /// Diagnostics that aren't associated with any source file because they have | |||
| /// no location information. | |||
| var floatingDiagnostics: [Diagnostic] = [] | |||
There was a problem hiding this comment.
Instead of introducing a new term floating here, how about calling this something like unknownPositionDiagnostics?
Some tools that wish to make use of the diagnostics machinery don't have source locations, or even a Swift source file to point into. For example, the compiler and driver both have diagnostics that don't involve anything in source code, e.g., because they are diagnosing problems at the module level such as conflicting or incorrect compiler options. At present, the driver uses a different scheme for emitting diagnostics, and the compiler falls back to using the LLVM diagnostics machinery when there is no source location information.
Making the "node" and "position" of a Diagnostic optional make it possible to express diagnostics that aren't associated with sources. Do so, and update the diagnostic formatter to support displaying diagnostics with no source location using the same
<unknown>:0:0:scheme the compiler users.