From 78d89ea6179a5b9023b4990f41d2461a5eb15693 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Wed, 2 Apr 2025 11:31:44 -0700 Subject: [PATCH] [Diagnostics] Make the node and position of Diagnostic and Note optional 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 `:0:0:` scheme the compiler users. --- .../Diagnostics.swift | 12 +++-- Sources/SwiftDiagnostics/Diagnostic.swift | 27 +++++++--- .../DiagnosticsFormatter.swift | 20 ++++++-- .../SwiftDiagnostics/GroupedDiagnostics.swift | 50 ++++++++++++++++--- Sources/SwiftDiagnostics/Note.swift | 25 ++++++---- .../ParseDiagnosticsGenerator.swift | 29 ++++++++--- .../Assertions.swift | 27 +++++++++- .../GroupDiagnosticsFormatterTests.swift | 38 ++++++++++++++ Tests/SwiftParserTest/Assertions.swift | 8 +-- 9 files changed, 188 insertions(+), 48 deletions(-) diff --git a/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift b/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift index 277212cd08c..2e68c1efd42 100644 --- a/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift +++ b/Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift @@ -68,10 +68,12 @@ extension PluginMessage.Diagnostic.Severity { extension PluginMessage.Diagnostic { init(from syntaxDiag: SwiftDiagnostics.Diagnostic, in sourceManager: SourceManager) { - if let position = sourceManager.position( - of: syntaxDiag.node, - at: .afterLeadingTrivia - ) { + if let node = syntaxDiag.node, + let position = sourceManager.position( + of: node, + at: .afterLeadingTrivia + ) + { self.position = .init(fileName: position.fileName, offset: position.utf8Offset) } else { self.position = .invalid @@ -92,7 +94,7 @@ extension PluginMessage.Diagnostic { } self.notes = syntaxDiag.notes.compactMap { - guard let pos = sourceManager.position(of: $0.node, at: .afterLeadingTrivia) else { + guard let node = $0.node, let pos = sourceManager.position(of: node, at: .afterLeadingTrivia) else { return nil } let position = PluginMessage.Diagnostic.Position( diff --git a/Sources/SwiftDiagnostics/Diagnostic.swift b/Sources/SwiftDiagnostics/Diagnostic.swift index c4655bb85c3..ab57eba39aa 100644 --- a/Sources/SwiftDiagnostics/Diagnostic.swift +++ b/Sources/SwiftDiagnostics/Diagnostic.swift @@ -21,11 +21,11 @@ public struct Diagnostic: CustomDebugStringConvertible, Sendable { public let diagMessage: DiagnosticMessage /// The node at whose start location the message should be displayed. - public let node: Syntax + 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 + public let position: AbsolutePosition? /// Nodes that should be highlighted in the source code. public let highlights: [Syntax] @@ -40,17 +40,17 @@ public struct Diagnostic: CustomDebugStringConvertible, Sendable { /// If `highlights` is `nil` then `node` will be highlighted. This is a /// reasonable default for almost all diagnostics. public init( - node: some SyntaxProtocol, + node: (some SyntaxProtocol)?, 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) } + self.position = position ?? node?.positionAfterSkippingLeadingTrivia self.diagMessage = message - self.highlights = highlights ?? [Syntax(node)] + self.highlights = highlights ?? node.map { [Syntax($0)] } ?? [] self.notes = notes self.fixIts = fixIts } @@ -67,13 +67,24 @@ public struct Diagnostic: CustomDebugStringConvertible, Sendable { } /// The location at which the diagnostic should be displayed. - public func location(converter: SourceLocationConverter) -> SourceLocation { + public func location(converter: SourceLocationConverter) -> SourceLocation? { + guard let position else { + return nil + } + return converter.location(for: position) } public var debugDescription: String { + guard let node else { + return "\(DiagnosticsFormatter.unknownFileName):0:0: \(message)" + } + let locationConverter = SourceLocationConverter(fileName: "", tree: node.root) - let location = location(converter: locationConverter) + guard let location = location(converter: locationConverter) else { + return "\(DiagnosticsFormatter.unknownFileName):0:0: \(message)" + } + return "\(location.line):\(location.column): \(message)" } } diff --git a/Sources/SwiftDiagnostics/DiagnosticsFormatter.swift b/Sources/SwiftDiagnostics/DiagnosticsFormatter.swift index 28f4d18bf4e..51f8619dc3b 100644 --- a/Sources/SwiftDiagnostics/DiagnosticsFormatter.swift +++ b/Sources/SwiftDiagnostics/DiagnosticsFormatter.swift @@ -54,6 +54,9 @@ extension Sequence where Element == Range { } public struct DiagnosticsFormatter { + /// The string used to identify an unknown file name. + @_spi(Testing) + public static let unknownFileName = "" /// A wrapper struct for a source line, its diagnostics, and any /// non-diagnostic text that follows the line. @@ -215,14 +218,19 @@ public struct DiagnosticsFormatter { suffixTexts: [AbsolutePosition: String], sourceLocationConverter: SourceLocationConverter? = nil ) -> String { - let slc = sourceLocationConverter ?? SourceLocationConverter(fileName: "", tree: tree) + let slc = + sourceLocationConverter ?? SourceLocationConverter(fileName: DiagnosticsFormatter.unknownFileName, tree: tree) // First, we need to put each line and its diagnostics together var annotatedSourceLines = [AnnotatedSourceLine]() for (sourceLineIndex, sourceLine) in slc.sourceLines.enumerated() { let diagsForLine = diags.filter { diag in - return diag.location(converter: slc).line == (sourceLineIndex + 1) + guard let location = diag.location(converter: slc) else { + return false + } + + return location.line == (sourceLineIndex + 1) } let suffixText = suffixTexts.compactMap { (position, text) in if slc.location(for: position).line == (sourceLineIndex + 1) { @@ -299,12 +307,14 @@ public struct DiagnosticsFormatter { } let columnsWithDiagnostics = Set( - annotatedLine.diagnostics.map { - annotatedLine.characterColumn(ofUtf8Column: $0.location(converter: slc).column) + annotatedLine.diagnostics.compactMap { + $0.location(converter: slc).map { + annotatedLine.characterColumn(ofUtf8Column: $0.column) + } } ) let diagsPerColumn = Dictionary(grouping: annotatedLine.diagnostics) { diag in - annotatedLine.characterColumn(ofUtf8Column: diag.location(converter: slc).column) + annotatedLine.characterColumn(ofUtf8Column: diag.location(converter: slc)!.column) }.sorted { lhs, rhs in lhs.key > rhs.key } diff --git a/Sources/SwiftDiagnostics/GroupedDiagnostics.swift b/Sources/SwiftDiagnostics/GroupedDiagnostics.swift index f1f3059bc19..f25108152c2 100644 --- a/Sources/SwiftDiagnostics/GroupedDiagnostics.swift +++ b/Sources/SwiftDiagnostics/GroupedDiagnostics.swift @@ -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] = [] + 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 sourceFileID = knownSourceFileID ?? findSourceFileContaining(diagnostic.node) else { - // Drop the diagnostic on the floor. + 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) + 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 { - let firstLine = sourceFile.diagnostics.first.map { $0.location(converter: slc).line } ?? 0 - prefixString = "\(sourceFile.displayName): \(firstLine):" + // 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 { - return group.rootSourceFiles.map { rootSourceFileID in - group.annotateSource(rootSourceFileID, formatter: self, indentString: "") - }.joined(separator: "\n") + // 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( + 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( diff --git a/Sources/SwiftDiagnostics/Note.swift b/Sources/SwiftDiagnostics/Note.swift index 8a85d79a969..06e9a5b5d31 100644 --- a/Sources/SwiftDiagnostics/Note.swift +++ b/Sources/SwiftDiagnostics/Note.swift @@ -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 + 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 + public let position: AbsolutePosition? /// A description of what this note is pointing at. public let noteMessage: NoteMessage public init( - node: Syntax, + node: Syntax?, position: AbsolutePosition? = nil, message: NoteMessage ) { self.node = node - self.position = position ?? node.positionAfterSkippingLeadingTrivia + 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 { + 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) { + if let root = node?.root.as(SourceFileSyntax.self) { let locationConverter = SourceLocationConverter(fileName: "", tree: root) - let location = location(converter: locationConverter) - return "\(location): \(message)" - } else { - return ": \(message)" + if let location = location(converter: locationConverter) { + return "\(location): \(message)" + } } + + return "\(DiagnosticsFormatter.unknownFileName):0:0: \(message)" } } diff --git a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift index 20b7c7702fd..c9e5d9198ea 100644 --- a/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift +++ b/Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift @@ -107,19 +107,27 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { let diagProducer = ParseDiagnosticsGenerator() diagProducer.walk(tree) diagProducer.diagnostics.sort { - if $0.position != $1.position { - return $0.position < $1.position + 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 $0.node.hasParent($1.node) { + if lhsNode.hasParent(rhsNode) { return true - } else if $1.node.hasParent($0.node) { + } 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 $0.node.id < $1.node.id + return lhsNode.id < rhsNode.id } } return diagProducer.diagnostics @@ -157,7 +165,16 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor { if suppressRemainingDiagnostics { return } - diagnostics.removeAll(where: { handledNodes.contains($0.node.id) }) + + diagnostics.removeAll( + where: { + if let nodeId = $0.node?.id { + return handledNodes.contains(nodeId) + } + + return false + } + ) diagnostics.append(diagnostic) self.handledNodes.append(contentsOf: handledNodes) } diff --git a/Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift b/Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift index 7d7e490cbc0..32363c35962 100644 --- a/Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift +++ b/Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift @@ -157,7 +157,18 @@ func assertNote( location: spec.failureLocation.underlying, failureHandler: { failureHandler(TestFailureSpec(underlying: $0)) } ) - let location = expansionContext.location(for: note.position, anchoredAt: note.node, fileName: "") + 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)) } ) - let location = expansionContext.location(for: diag.position, anchoredAt: diag.node, fileName: "") + 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( diff --git a/Tests/SwiftDiagnosticsTest/GroupDiagnosticsFormatterTests.swift b/Tests/SwiftDiagnosticsTest/GroupDiagnosticsFormatterTests.swift index d4892a26020..ff8244e20d1 100644 --- a/Tests/SwiftDiagnosticsTest/GroupDiagnosticsFormatterTests.swift +++ b/Tests/SwiftDiagnosticsTest/GroupDiagnosticsFormatterTests.swift @@ -85,6 +85,43 @@ final class GroupedDiagnosticsFormatterTests: XCTestCase { ) } + func testFloatingDiagnostics() { + var group = GroupedDiagnostics() + _ = group.addTestFile( + """ + func f() { } + """, + displayName: "main.swift", + diagnosticDescriptors: [] + ) + + group.addDiagnostic( + Diagnostic( + node: nil as Syntax?, + message: SimpleDiagnosticMessage( + message: "this is bad", + diagnosticID: MessageID( + domain: "swift-syntax", + id: "diagnostic test" + ), + severity: .error, + category: DiagnosticCategory( + name: "Badness", + documentationURL: nil + ) + ) + ) + ) + + let annotated = DiagnosticsFormatter.annotateSources(in: group) + assertStringsEqualWithDiff( + annotated, + """ + :0:0: error: this is bad [#Badness] + """ + ) + } + func testGroupingForMacroExpansion() { var group = GroupedDiagnostics() @@ -259,4 +296,5 @@ final class GroupedDiagnosticsFormatterTests: XCTestCase { """ ) } + } diff --git a/Tests/SwiftParserTest/Assertions.swift b/Tests/SwiftParserTest/Assertions.swift index cb537012a3c..42fdb4b1e46 100644 --- a/Tests/SwiftParserTest/Assertions.swift +++ b/Tests/SwiftParserTest/Assertions.swift @@ -10,7 +10,7 @@ // //===----------------------------------------------------------------------===// -import SwiftDiagnostics +@_spi(Testing) import SwiftDiagnostics @_spi(FixItApplier) import SwiftIDEUtils @_spi(Testing) @_spi(RawSyntax) @_spi(AlternateTokenIntrospection) @_spi(ExperimentalLanguageFeatures) import SwiftParser @_spi(RawSyntax) import SwiftParserDiagnostics @@ -324,7 +324,7 @@ func assertNote( XCTAssertEqual(note.message, spec.message, file: spec.file, line: spec.line) let locationConverter = SourceLocationConverter(fileName: "", tree: tree) assertLocation( - note.location(converter: locationConverter), + note.location(converter: locationConverter)!, in: tree, markerLocations: markerLocations, expectedLocationMarker: spec.locationMarker, @@ -343,7 +343,7 @@ func assertDiagnostic( ) { let locationConverter = SourceLocationConverter(fileName: "", tree: tree) assertLocation( - diag.location(converter: locationConverter), + diag.location(converter: locationConverter)!, in: tree, markerLocations: markerLocations, expectedLocationMarker: spec.locationMarker, @@ -368,7 +368,7 @@ func assertDiagnostic( ) } - let highlight = spec.highlight ?? diag.node.description + let highlight = spec.highlight ?? diag.node?.description ?? DiagnosticsFormatter.unknownFileName assertStringsEqualWithDiff( diag.highlights.map(\.description).joined().trimmingTrailingWhitespace(), highlight.trimmingTrailingWhitespace(),