[Diagnostics] Make the node and position of Diagnostic and Note optional #3038
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 |
||
|
|
||
| public init() {} | ||
|
|
||
| /// Add a new source file to the set of grouped diagnostics. | ||
|
|
@@ -122,8 +126,16 @@ public struct GroupedDiagnostics { | |
| _ diagnostic: Diagnostic, | ||
| in knownSourceFileID: SourceFileID? = nil | ||
| ) { | ||
| guard let node = diagnostic.node else { | ||
| // There is no node anchoring the diagnostic in a source file, so treat | ||
| // it as floating. | ||
| floatingDiagnostics.append(diagnostic) | ||
| return | ||
| } | ||
|
|
||
| guard let sourceFileID = knownSourceFileID ?? findSourceFileContaining(node) else { | ||
| // This diagnostic is in source file, but we aren't producing diagnostics | ||
| // for that file. Drop the diagnostic on the floor. | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -205,7 +217,9 @@ extension GroupedDiagnostics { | |
| // If there's a primary diagnostic, print it first. | ||
| if let (primaryDiagSourceFile, primaryDiag) = findPrimaryDiagnostic(in: sourceFile) { | ||
| let primaryDiagSLC = primaryDiagSourceFile.sourceLocationConverter | ||
| let location = | ||
| primaryDiag.location(converter: primaryDiagSLC) | ||
| ?? SourceLocation(line: 0, column: 0, offset: 0, file: DiagnosticsFormatter.unknownFileName) | ||
|
|
||
| // Display file/line/column and diagnostic text for the primary diagnostic. | ||
| prefixString = | ||
|
|
@@ -233,9 +247,13 @@ extension GroupedDiagnostics { | |
| prefixString += "`- \(bufferLoc.file):\(bufferLoc.line):\(bufferLoc.column): \(decoratedMessage)\n" | ||
| } | ||
| } | ||
| } else if let firstDiag = sourceFile.diagnostics.first, | ||
| let firstDiagLoc = firstDiag.location(converter: slc) | ||
| { | ||
| prefixString = "\(sourceFile.displayName): \(firstDiagLoc.line):" | ||
| } else { | ||
| // There are no diagnostics to print from this file. | ||
| return "" | ||
| } | ||
|
|
||
| suffixString = "" | ||
|
|
@@ -275,9 +293,25 @@ extension GroupedDiagnostics { | |
| extension DiagnosticsFormatter { | ||
| /// Annotate all of the source files in the given set of grouped diagnostics. | ||
| public func annotateSources(in group: GroupedDiagnostics) -> String { | ||
| // Render all grouped diagnostics. | ||
| var renderedDiags = group.rootSourceFiles.compactMap { rootSourceFileID in | ||
| let annotated = group.annotateSource( | ||
| rootSourceFileID, | ||
| formatter: self, | ||
| indentString: "" | ||
| ) | ||
| return annotated.isEmpty ? nil : annotated | ||
| } | ||
|
|
||
| // 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 |
||
| contentsOf: group.floatingDiagnostics.map { diag in | ||
| let renderedMessage = diagnosticDecorator.decorateDiagnosticMessage(diag.diagMessage) | ||
| return "\(DiagnosticsFormatter.unknownFileName):0:0: \(renderedMessage)" | ||
| } | ||
| ) | ||
|
|
||
| return renderedDiags.joined(separator: "\n") | ||
| } | ||
|
|
||
| public static func annotateSources( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,22 +37,22 @@ extension NoteMessage { | |
| /// A note that points to another node that's relevant for a Diagnostic. | ||
| public struct Note: CustomDebugStringConvertible, Sendable { | ||
| /// The node whose location the node is pointing. | ||
| public let node: Syntax? | ||
|
|
||
| /// The position at which the location should be anchored. | ||
| /// By default, this is the start location of `node`. | ||
| public let position: AbsolutePosition? | ||
|
|
||
| /// A description of what this note is pointing at. | ||
| public let noteMessage: NoteMessage | ||
|
|
||
| public init( | ||
| node: Syntax?, | ||
| position: AbsolutePosition? = nil, | ||
| message: NoteMessage | ||
| ) { | ||
| self.node = node | ||
| self.position = position ?? node?.positionAfterSkippingLeadingTrivia | ||
| self.noteMessage = message | ||
| } | ||
|
|
||
|
|
@@ -62,17 +62,22 @@ public struct Note: CustomDebugStringConvertible, Sendable { | |
| } | ||
|
|
||
| /// The location at which the note should be displayed. | ||
| public func location(converter: SourceLocationConverter) -> SourceLocation? { | ||
| guard let position else { | ||
| return nil | ||
| } | ||
|
|
||
| return converter.location(for: position) | ||
| } | ||
|
|
||
| public var debugDescription: String { | ||
| if let root = node?.root.as(SourceFileSyntax.self) { | ||
| let locationConverter = SourceLocationConverter(fileName: "", tree: root) | ||
| if let location = location(converter: locationConverter) { | ||
| return "\(location): \(message)" | ||
| } | ||
| } | ||
|
|
||
| return "\(DiagnosticsFormatter.unknownFileName):0:0: \(message)" | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,19 +107,27 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { | |
| let diagProducer = ParseDiagnosticsGenerator() | ||
| diagProducer.walk(tree) | ||
| diagProducer.diagnostics.sort { | ||
| guard let lhsPosition = $0.position, let rhsPosition = $1.position else { | ||
| return $0.position == nil | ||
| } | ||
|
|
||
| if lhsPosition != rhsPosition { | ||
| return lhsPosition < rhsPosition | ||
| } | ||
|
|
||
| guard let lhsNode = $0.node, let rhsNode = $1.node else { | ||
| return $0.node == nil | ||
| } | ||
|
|
||
| // Emit children diagnostics before parent diagnostics. | ||
| // This makes sure that for missing declarations with attributes, we emit diagnostics on the attribute before we complain about the missing declaration. | ||
| if lhsNode.hasParent(rhsNode) { | ||
| return true | ||
| } else if rhsNode.hasParent(lhsNode) { | ||
| return false | ||
| } else { | ||
| // If multiple tokens are missing at the same location, emit diagnostics about nodes that occur earlier in the tree first. | ||
| return lhsNode.id < rhsNode.id | ||
| } | ||
| } | ||
| return diagProducer.diagnostics | ||
|
|
@@ -157,7 +165,16 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { | |
| if suppressRemainingDiagnostics { | ||
| return | ||
| } | ||
|
|
||
| diagnostics.removeAll( | ||
| where: { | ||
| if let nodeId = $0.node?.id { | ||
| return handledNodes.contains(nodeId) | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| ) | ||
| diagnostics.append(diagnostic) | ||
| self.handledNodes.append(contentsOf: handledNodes) | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,7 +157,18 @@ func assertNote( | |
| location: spec.failureLocation.underlying, | ||
| failureHandler: { failureHandler(TestFailureSpec(underlying: $0)) } | ||
| ) | ||
| guard let position = note.position, let node = note.node else { | ||
| failureHandler( | ||
| TestFailureSpec( | ||
| message: "note has no position", | ||
| location: spec.failureLocation | ||
| ) | ||
| ) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| let location = expansionContext.location(for: position, anchoredAt: node, fileName: "") | ||
| if location.line != spec.line { | ||
| failureHandler( | ||
| TestFailureSpec( | ||
|
|
@@ -387,7 +398,19 @@ public func assertDiagnostic( | |
| location: spec.failureLocation.underlying, | ||
| failureHandler: { failureHandler(TestFailureSpec(underlying: $0)) } | ||
| ) | ||
| guard let position = diag.position, | ||
| let node = diag.node | ||
| else { | ||
| failureHandler( | ||
| TestFailureSpec( | ||
| message: "diagnostic missing location info", | ||
| location: spec.failureLocation | ||
| ) | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| let location = expansionContext.location(for: position, anchoredAt: node, fileName: "") | ||
| if location.line != spec.line { | ||
| failureHandler( | ||
| TestFailureSpec( | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we have a
Syntaxinitializer that just proposesnil, so it should be possible to leave this as-is.