From 8a0dc2b4a86df1e35dd2d97c9a4f637897c1744f Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 5 Mar 2025 17:55:25 -0800 Subject: [PATCH 1/4] [DiagnosticBridge] Make sure that diagnostic queues up its child notes `DiagnosticEngine` has an API that allows to attach notes to a "primary" diagnostic (an error or a warning). This works well with old formatting (`llvm`) but `swift` formatter doesn't display attached notes which makes some diagnostics very hard to work with i.e. `invalid_redecl`. --- lib/AST/DiagnosticBridge.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/AST/DiagnosticBridge.cpp b/lib/AST/DiagnosticBridge.cpp index f1f650aaf190a..35f8b095e50ae 100644 --- a/lib/AST/DiagnosticBridge.cpp +++ b/lib/AST/DiagnosticBridge.cpp @@ -77,6 +77,13 @@ static void addQueueDiagnostic(void *queuedDiagnostics, documentationPath.size(), highlightRanges.data(), highlightRanges.size() / 2); + + // TODO: A better way to do this would be to pass the notes as an + // argument to `swift_ASTGen_addQueuedDiagnostic` but that requires + // bridging of `Note` structure and new serialization. + for (auto *childNote : info.ChildDiagnosticInfo) { + addQueueDiagnostic(queuedDiagnostics, *childNote, SM); + } } void DiagnosticBridge::enqueueDiagnostic(SourceManager &SM, From b7eecc32dc703d14642fd28b396f155d14fa9ea5 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 6 Mar 2025 13:28:32 -0800 Subject: [PATCH 2/4] [Frontend] Treat empty diagnostic category as "not present" Serialized diagnostic stores an `std::string` which means that the category gets marked as present even if its empty, which is incorrect. --- lib/Frontend/CachedDiagnostics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Frontend/CachedDiagnostics.cpp b/lib/Frontend/CachedDiagnostics.cpp index a7e0aa3d47d39..fbd74f6ca4bca 100644 --- a/lib/Frontend/CachedDiagnostics.cpp +++ b/lib/Frontend/CachedDiagnostics.cpp @@ -617,7 +617,7 @@ llvm::Error DiagnosticSerializer::deserializeDiagnosticInfo( Kind, Info.FormatString, {}, - Info.Category, + Info.Category.empty() ? StringRef() : Info.Category, *BICD, ChildDiagPtrs, Ranges, From b26c8aa930ad032e523bdd28e8f02e4229646006 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 6 Mar 2025 15:15:08 -0800 Subject: [PATCH 3/4] [Tests] NFC: Add a test-case for printing child notes associated with errors/warnings --- test/diagnostics/pretty-printed-diagnostics.swift | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/diagnostics/pretty-printed-diagnostics.swift b/test/diagnostics/pretty-printed-diagnostics.swift index 01a93656a1278..222cd441329a8 100644 --- a/test/diagnostics/pretty-printed-diagnostics.swift +++ b/test/diagnostics/pretty-printed-diagnostics.swift @@ -66,6 +66,9 @@ foo(b: 1, a: 2) +// Test for child notes attached directly to a "primary" error/warning diagnostic +func test(a: Int) {} +func test(a: Int) {} // Test fallback for non-ASCII characters. // CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-diagnostics.swift:[[#LINE:]]:11 @@ -85,3 +88,9 @@ foo(b: // CHECK: [[#LINE]] | foo(b: 1, a: 2) // CHECK: | `- error: argument 'a' must precede argument 'b' // CHECK: [[#LINE+1]] | + +// CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-diagnostics.swift:[[#LINE:]]:6 +// CHECK: [[#LINE-1]] | func test(a: Int) {} +// CHECK: | `- note: 'test(a:)' previously declared here +// CHECK: [[#LINE]] | func test(a: Int) {} +// CHECL: [[#LINE+1]] | `- error: invalid redeclaration of 'test(a:)' From a7a9a4d11a08d47a80cbf4b3641b0beb4a09dae7 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Thu, 6 Mar 2025 17:44:58 -0800 Subject: [PATCH 4/4] [DiagnosticsBridge] Treat empty category name as `nil` Default value for `Category` for serialization purposes is an empty string but it should be handled as `nil` while bridging because category name cannot be empty when present. --- lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift | 9 ++++++++- lib/Frontend/CachedDiagnostics.cpp | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift b/lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift index 0a911139e0064..a689a3c24b0ed 100644 --- a/lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift +++ b/lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift @@ -339,13 +339,20 @@ public func addQueuedDiagnostic( } } - let category: DiagnosticCategory? = categoryName.map { categoryNamePtr in + let category: DiagnosticCategory? = categoryName.flatMap { categoryNamePtr in let categoryNameBuffer = UnsafeBufferPointer( start: categoryNamePtr, count: categoryLength ) let categoryName = String(decoding: categoryNameBuffer, as: UTF8.self) + // If the data comes from serialized diagnostics, it's possible that + // the category name is empty because StringRef() is serialized into + // an empty string. + guard !categoryName.isEmpty else { + return nil + } + let documentationURL = documentationPath.map { documentationPathPtr in let documentationPathBuffer = UnsafeBufferPointer( start: documentationPathPtr, diff --git a/lib/Frontend/CachedDiagnostics.cpp b/lib/Frontend/CachedDiagnostics.cpp index fbd74f6ca4bca..a7e0aa3d47d39 100644 --- a/lib/Frontend/CachedDiagnostics.cpp +++ b/lib/Frontend/CachedDiagnostics.cpp @@ -617,7 +617,7 @@ llvm::Error DiagnosticSerializer::deserializeDiagnosticInfo( Kind, Info.FormatString, {}, - Info.Category.empty() ? StringRef() : Info.Category, + Info.Category, *BICD, ChildDiagPtrs, Ranges,