Diagnostics: Put notes under associated diag for clients and json output

stack-info: PR: #1925, branch: AndrewNolte/stack/25

Author: AndrewNolte

include/slang/diagnostics/DiagnosticEngine.h

@@ -29,16 +29,30 @@ class DiagnosticClient;
 class SourceManager;
 class WaiverManager;
 
-struct SLANG_EXPORT ReportedDiagnostic {
+/// Data shared by primary diagnostics and their associated notes after preparation for reporting.
+struct SLANG_EXPORT ReportedDiagnosticInfo {
     const Diagnostic& originalDiagnostic;
-    std::span<const SourceLocation> expansionLocs;
+    SmallVector<SourceLocation> expansionLocs;
     std::span<const SourceRange> ranges;
     SourceLocation location;
-    std::string_view formattedMessage;
-    DiagnosticSeverity severity = DiagnosticSeverity::Ignored;
+    std::string formattedMessage;
     bool shouldShowIncludeStack = false;
 
-    ReportedDiagnostic(const Diagnostic& original) : originalDiagnostic(original) {}
+    ReportedDiagnosticInfo(const Diagnostic& original) : originalDiagnostic(original) {}
+};
+
+/// A note associated with a ReportedDiagnostic.
+struct SLANG_EXPORT ReportedNote : ReportedDiagnosticInfo {
+    ReportedNote(ReportedDiagnosticInfo&& info) : ReportedDiagnosticInfo(std::move(info)) {}
+};
+
+/// A primary diagnostic prepared for reporting to a DiagnosticClient.
+struct SLANG_EXPORT ReportedDiagnostic : ReportedDiagnosticInfo {
+    std::vector<ReportedNote> notes;
+    DiagnosticSeverity severity;
+
+    ReportedDiagnostic(ReportedDiagnosticInfo&& info, DiagnosticSeverity severity) :
+        ReportedDiagnosticInfo(std::move(info)), severity(severity) {}
 };
 
 /// The DiagnosticEngine is the central point for controlling how diagnostics are
@@ -300,7 +314,9 @@ class SLANG_EXPORT DiagnosticEngine {
     std::optional<DiagnosticSeverity> findPerBufferSeverity(DiagCode code, SourceLocation location,
                                                             bool& overrideWarnAsError) const;
 
-    bool issueImpl(const Diagnostic& diagnostic, DiagnosticSeverity severity);
+    // Resolves and formats a diagnostic for reporting. Returns nullopt if the diagnostic is
+    // suppressed by a path filter or waiver. Note diagnostics are never suppressed here.
+    std::optional<ReportedDiagnosticInfo> getReportedDiag(const Diagnostic& diagnostic);
 
     template<typename TDirective>
     void setMappingsFromPragmasImpl(BufferID buffer, std::span<const TDirective> directives,

include/slang/diagnostics/JsonDiagnosticClient.h

@@ -15,7 +15,8 @@ namespace slang {
 
 class JsonWriter;
 
-/// A diagnostic client that serializes each diagnostic to JSON format.
+/// A diagnostic client that serializes diagnostics to JSON format, with associated
+/// notes nested under their parent diagnostic.
 class SLANG_EXPORT JsonDiagnosticClient : public DiagnosticClient {
 public:
     /// Constructs a new JsonDiagnosticClient that outputs to the given JsonWriter.
@@ -27,9 +28,7 @@ class SLANG_EXPORT JsonDiagnosticClient : public DiagnosticClient {
 private:
     JsonWriter& writer;
 
-    void formatDiag(SourceLocation loc, std::span<const SourceRange> ranges,
-                    DiagnosticSeverity severity, std::string_view message,
-                    std::string_view optionName);
+    void writeDiagnostic(const ReportedDiagnosticInfo& diagnostic);
 };
 
 } // namespace slang

include/slang/diagnostics/TextDiagnosticClient.h

@@ -97,6 +97,7 @@ class SLANG_EXPORT TextDiagnosticClient : public DiagnosticClient {
     bool includeExpansion = true;
     ShowHierarchyPathOption includeHierarchy = ShowHierarchyPathOption::Auto;
 
+    void writeDiagnostic(const ReportedDiagnosticInfo& diagnostic, DiagnosticSeverity severity);
     void formatDiag(SourceLocation loc, std::span<const SourceRange> ranges,
                     DiagnosticSeverity severity, std::string_view message,
                     std::string_view optionName);

source/diagnostics/DiagnosticEngine.cpp

@@ -213,13 +213,43 @@ void DiagnosticEngine::issue(const Diagnostic& diagnostic) {
                          severity == DiagnosticSeverity::Fatal;
     if (isError && errorLimit && numErrors >= errorLimit) {
         Diagnostic diag(diag::TooManyErrors, SourceLocation::NoLocation);
-        issueImpl(diag, DiagnosticSeverity::Fatal);
+        auto info = getReportedDiag(diag);
+        SLANG_ASSERT(info);
+        ReportedDiagnostic report(std::move(*info), DiagnosticSeverity::Fatal);
+        for (auto& client : clients)
+            client->report(report);
         issuedOverLimitErr = true;
         return;
     }
 
-    if (!issueImpl(diagnostic, severity))
+    auto info = getReportedDiag(diagnostic);
+    if (!info)
         return;
+    ReportedDiagnostic report(std::move(*info), severity);
+
+    for (const Diagnostic& note : diagnostic.notes) {
+        // Notes shouldn't have subnotes. We should change this shape too eventually, unless we
+        // want to support that.
+        SLANG_ASSERT(note.notes.empty());
+
+        // Notes are ignored if location is "NoLocation" since they frequently make no
+        // sense without location information.
+        if (note.location == SourceLocation::NoLocation && !note.code.showNoteWithNoLocation())
+            continue;
+
+        auto noteSeverity = getSeverity(note.code, note.location);
+        if (noteSeverity == DiagnosticSeverity::Ignored)
+            continue;
+
+        SLANG_ASSERT(noteSeverity == DiagnosticSeverity::Note);
+
+        auto noteInfo = getReportedDiag(note);
+        SLANG_ASSERT(noteInfo);
+        report.notes.emplace_back(std::move(*noteInfo));
+    }
+
+    for (auto& client : clients)
+        client->report(report);
 
     if (severity == DiagnosticSeverity::Warning)
         numWarnings++;
@@ -232,7 +262,8 @@ void DiagnosticEngine::issue(const Diagnostics& diagnostics) {
         issue(diag);
 }
 
-bool DiagnosticEngine::issueImpl(const Diagnostic& diagnostic, DiagnosticSeverity severity) {
+std::optional<ReportedDiagnosticInfo> DiagnosticEngine::getReportedDiag(
+    const Diagnostic& diagnostic) {
     // Walk out until we find a location for this diagnostic that isn't inside a macro.
     SmallVector<SourceLocation, 8> expansionLocs;
     SourceLocation loc = diagnostic.location;
@@ -256,12 +287,22 @@ bool DiagnosticEngine::issueImpl(const Diagnostic& diagnostic, DiagnosticSeverit
         }
 
         showIncludeStack = reportedIncludeStack.emplace(loc.buffer()).second;
+    }
+
+    ReportedDiagnosticInfo report(diagnostic);
+    report.expansionLocs.assign(expansionLocs.begin() + ptrdiff_t(ignoreExpansionsUntil),
+                                expansionLocs.end());
+    report.ranges = diagnostic.ranges;
+    report.location = loc;
+    report.shouldShowIncludeStack = showIncludeStack;
 
-        auto checkSuppressed = [&](const std::vector<fs::path>& patterns, SourceLocation loc) {
+    if (loc != SourceLocation::NoLocation &&
+        getDefaultSeverity(diagnostic.code) != DiagnosticSeverity::Note) {
+        auto checkPath = [&](const std::vector<fs::path>& patterns, SourceLocation location) {
             if (patterns.empty())
                 return false;
 
-            auto& path = sourceManager.getFullPath(loc.buffer());
+            auto& path = sourceManager.getFullPath(location.buffer());
             for (auto& pattern : patterns) {
                 if (svGlobMatches(path, pattern))
                     return true;
@@ -270,43 +311,22 @@ bool DiagnosticEngine::issueImpl(const Diagnostic& diagnostic, DiagnosticSeverit
         };
 
         if (getDefaultSeverity(diagnostic.code) == DiagnosticSeverity::Warning) {
-            if (checkSuppressed(ignoreWarnPatterns, loc))
-                return false;
+            if (checkPath(ignoreWarnPatterns, loc))
+                return std::nullopt;
 
-            if (ignoreExpansionsUntil < expansionLocs.size() && !ignoreMacroWarnPatterns.empty()) {
-                auto originalLoc = sourceManager.getFullyOriginalLoc(
-                    expansionLocs[ignoreExpansionsUntil]);
-
-                if (checkSuppressed(ignoreMacroWarnPatterns, originalLoc))
-                    return false;
+            if (!report.expansionLocs.empty() && !ignoreMacroWarnPatterns.empty()) {
+                auto originalLoc = sourceManager.getFullyOriginalLoc(report.expansionLocs.front());
+                if (checkPath(ignoreMacroWarnPatterns, originalLoc))
+                    return std::nullopt;
             }
         }
 
         if (waiverManager && waiverManager->shouldWaive(diagnostic, loc, sourceManager, *this))
-            return false;
-    }
-
-    std::string message = formatMessage(diagnostic);
-
-    ReportedDiagnostic report(diagnostic);
-    report.expansionLocs = std::span<SourceLocation>(expansionLocs).subspan(ignoreExpansionsUntil);
-    report.ranges = diagnostic.ranges;
-    report.location = loc;
-    report.severity = severity;
-    report.formattedMessage = message;
-    report.shouldShowIncludeStack = showIncludeStack;
-
-    for (auto& client : clients)
-        client->report(report);
-
-    // Notes are ignored if location is "NoLocation" since they frequently make no
-    // sense without location information.
-    for (const Diagnostic& note : diagnostic.notes) {
-        if (note.location != SourceLocation::NoLocation || note.code.showNoteWithNoLocation())
-            issue(note);
+            return std::nullopt;
     }
 
-    return true;
+    report.formattedMessage = formatMessage(diagnostic);
+    return report;
 }
 
 const DiagnosticEngine::FormatterMap& DiagnosticEngine::getFormatters() const {

source/diagnostics/JsonDiagnosticClient.cpp

@@ -18,8 +18,28 @@ void JsonDiagnosticClient::report(const ReportedDiagnostic& diag) {
     writer.startObject();
     writer.writeProperty("severity");
     writer.writeValue(getSeverityString(diag.severity));
+
+    writeDiagnostic(diag);
+
+    if (!diag.notes.empty()) {
+        writer.writeProperty("notes");
+        writer.startArray();
+        for (auto& note : diag.notes) {
+            writer.startObject();
+            writeDiagnostic(note);
+            writer.endObject();
+        }
+        writer.endArray();
+    }
+
+    writer.endObject();
+}
+
+void JsonDiagnosticClient::writeDiagnostic(const ReportedDiagnosticInfo& diag) {
     writer.writeProperty("message");
     writer.writeValue(diag.formattedMessage);
+    writer.writeProperty("code");
+    writer.writeValue(toString(diag.originalDiagnostic.code));
 
     auto optionName = engine->getOptionName(diag.originalDiagnostic.code);
     if (!optionName.empty()) {
@@ -34,6 +54,33 @@ void JsonDiagnosticClient::report(const ReportedDiagnostic& diag) {
                                       sourceManager->getLineNumber(loc), getColumnNumber(loc)));
     }
 
+    SmallVector<SourceRange> mappedRanges;
+    engine->mapSourceRanges(diag.location, diag.ranges, mappedRanges);
+    if (!mappedRanges.empty()) {
+        auto writeLocation = [&](SourceLocation loc) {
+            writer.startObject();
+            writer.writeProperty("file");
+            writer.writeValue(getFileName(loc));
+            writer.writeProperty("line");
+            writer.writeValue(sourceManager->getLineNumber(loc));
+            writer.writeProperty("column");
+            writer.writeValue(getColumnNumber(loc));
+            writer.endObject();
+        };
+
+        writer.writeProperty("ranges");
+        writer.startArray();
+        for (auto range : mappedRanges) {
+            writer.startObject();
+            writer.writeProperty("start");
+            writeLocation(range.start());
+            writer.writeProperty("end");
+            writeLocation(range.end());
+            writer.endObject();
+        }
+        writer.endArray();
+    }
+
     if (diag.shouldShowIncludeStack) {
         SmallVector<SourceLocation> includeStack;
         getIncludeStack(diag.location.buffer(), includeStack);
@@ -81,7 +128,6 @@ void JsonDiagnosticClient::report(const ReportedDiagnostic& diag) {
         }
         writer.endArray();
     }
-    writer.endObject();
 }
 
 } // namespace slang

source/diagnostics/TextDiagnosticClient.cpp

@@ -47,6 +47,13 @@ TerminalColor TextDiagnosticClient::getSeverityColor(DiagnosticSeverity severity
 }
 
 void TextDiagnosticClient::report(const ReportedDiagnostic& diag) {
+    writeDiagnostic(diag, diag.severity);
+    for (auto& note : diag.notes)
+        writeDiagnostic(note, DiagnosticSeverity::Note);
+}
+
+void TextDiagnosticClient::writeDiagnostic(const ReportedDiagnosticInfo& diag,
+                                           DiagnosticSeverity severity) {
     if (diag.shouldShowIncludeStack && includeFileStack) {
         SmallVector<SourceLocation> includeStack;
         getIncludeStack(diag.location.buffer(), includeStack);
@@ -79,7 +86,7 @@ void TextDiagnosticClient::report(const ReportedDiagnostic& diag) {
     engine->mapSourceRanges(diag.location, diag.ranges, mappedRanges);
 
     // Write the diagnostic.
-    formatDiag(diag.location, mappedRanges, diag.severity, diag.formattedMessage,
+    formatDiag(diag.location, mappedRanges, severity, diag.formattedMessage,
                engine->getOptionName(diag.originalDiagnostic.code));
 
     // Write out macro expansions, if we have any, in reverse order.