Merge pull request github#13133 from github/redsun82/swift-diagnostics-locations

Swift: add location and visibility support to TSP diagnostics

Author: redsun82

swift/integration-tests/diagnostics_test_utils.py

@@ -63,5 +63,5 @@ def check_diagnostics(test_dir=".", test_db="db"):
             actual_out.write(actual)
         actual = actual.splitlines(keepends=True)
         expected = expected.splitlines(keepends=True)
-        print("".join(difflib.unified_diff(actual, expected, fromfile="diagnostics.actual", tofile="diagnostics.expected")), file=sys.stderr)
+        print("".join(difflib.unified_diff(expected, actual, fromfile="diagnostics.expected", tofile="diagnostics.actual")), file=sys.stderr)
         sys.exit(1)

swift/logging/BUILD.bazel

@@ -6,6 +6,7 @@ cc_library(
     deps = [
         "@absl//absl/strings",
         "@binlog",
+        "@fmt",
         "@json",
     ],
 )

swift/logging/SwiftDiagnostics.cpp

@@ -1,68 +1,60 @@
 #include "swift/logging/SwiftDiagnostics.h"
 
 #include <binlog/Entries.hpp>
-#include <nlohmann/json.hpp>
 #include "absl/strings/str_join.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_split.h"
 #include "swift/logging/SwiftAssert.h"
 
 namespace codeql {
 
-namespace {
-Logger& logger() {
-  static Logger ret{"diagnostics"};
-  return ret;
-}
-}  // namespace
-
-void SwiftDiagnosticsSource::emit(std::ostream& out,
-                                  std::string_view timestamp,
-                                  std::string_view message) const {
-  nlohmann::json entry = {
+nlohmann::json SwiftDiagnostic::json(const std::chrono::system_clock::time_point& timestamp,
+                                     std::string_view message) const {
+  nlohmann::json ret{
       {"source",
        {
-           {"id", sourceId()},
+           {"id", absl::StrJoin({extractorName, programName, id}, "/")},
            {"name", name},
            {"extractorName", extractorName},
        }},
       {"visibility",
        {
-           {"statusPage", true},
-           {"cliSummaryTable", true},
-           {"telemetry", true},
+           {"statusPage", has(Visibility::statusPage)},
+           {"cliSummaryTable", has(Visibility::cliSummaryTable)},
+           {"telemetry", has(Visibility::telemetry)},
        }},
       {"severity", "error"},
       {"helpLinks", std::vector<std::string_view>(absl::StrSplit(helpLinks, ' '))},
       {"plaintextMessage", absl::StrCat(message, ".\n\n", action, ".")},
-      {"timestamp", timestamp},
+      {"timestamp", fmt::format("{:%FT%T%z}", timestamp)},
   };
-  out << entry << '\n';
+  if (location) {
+    ret["location"] = location->json();
+  }
+  return ret;
 }
 
-std::string SwiftDiagnosticsSource::sourceId() const {
-  auto ret = absl::StrJoin({extractorName, programName, id}, "/");
-  std::replace(ret.begin(), ret.end(), '_', '-');
-  return ret;
+std::string SwiftDiagnostic::abbreviation() const {
+  if (location) {
+    return absl::StrCat(id, "@", location->str());
+  }
+  return std::string{id};
 }
-void SwiftDiagnosticsSource::registerImpl(const SwiftDiagnosticsSource* source) {
-  auto [it, inserted] = map().emplace(source->id, source);
-  CODEQL_ASSERT(inserted, "duplicate diagnostics source detected with id {}", source->id);
+
+bool SwiftDiagnostic::has(SwiftDiagnostic::Visibility v) const {
+  return (visibility & v) != Visibility::none;
 }
 
-void SwiftDiagnosticsDumper::write(const char* buffer, std::size_t bufferSize) {
-  binlog::Range range{buffer, bufferSize};
-  binlog::RangeEntryStream input{range};
-  while (auto event = events.nextEvent(input)) {
-    const auto& source = SwiftDiagnosticsSource::get(event->source->category);
-    std::ostringstream oss;
-    timestampedMessagePrinter.printEvent(oss, *event, events.writerProp(), events.clockSync());
-    // TODO(C++20) use oss.view() directly
-    auto data = oss.str();
-    std::string_view view = data;
-    using ViewPair = std::pair<std::string_view, std::string_view>;
-    auto [timestamp, message] = ViewPair(absl::StrSplit(view, absl::MaxSplits(' ', 1)));
-    source.emit(output, timestamp, message);
-  }
+nlohmann::json SwiftDiagnosticsLocation::json() const {
+  nlohmann::json ret{{"file", file}};
+  if (startLine) ret["startLine"] = startLine;
+  if (startColumn) ret["startColumn"] = startColumn;
+  if (endLine) ret["endLine"] = endLine;
+  if (endColumn) ret["endColumn"] = endColumn;
+  return ret;
+}
+
+std::string SwiftDiagnosticsLocation::str() const {
+  return absl::StrJoin(std::tuple{file, startLine, startColumn, endLine, endColumn}, ":");
 }
 }  // namespace codeql

swift/logging/SwiftDiagnostics.h

@@ -1,90 +1,108 @@
 #pragma once
 
-#include <binlog/EventStream.hpp>
-#include <binlog/PrettyPrinter.hpp>
+#include <binlog/binlog.hpp>
 #include <string>
 #include <vector>
 #include <unordered_map>
+#include <optional>
 #include <cassert>
 #include <fstream>
 #include <filesystem>
 #include <sstream>
 #include <mutex>
+#include <fmt/format.h>
+#include <fmt/chrono.h>
+#include <nlohmann/json.hpp>
 
 namespace codeql {
 
 extern const std::string_view programName;
 
+struct SwiftDiagnosticsLocation {
+  std::string_view file;
+  unsigned startLine;
+  unsigned startColumn;
+  unsigned endLine;
+  unsigned endColumn;
+
+  nlohmann::json json() const;
+  std::string str() const;
+};
+
 // Models a diagnostic source for Swift, holding static information that goes out into a diagnostic
 // These are internally stored into a map on id's. A specific error log can use binlog's category
 // as id, which will then be used to recover the diagnostic source while dumping.
-struct SwiftDiagnosticsSource {
+struct SwiftDiagnostic {
+  enum class Visibility : unsigned char {
+    none = 0b000,
+    statusPage = 0b001,
+    cliSummaryTable = 0b010,
+    telemetry = 0b100,
+    all = 0b111,
+  };
+
   std::string_view id;
   std::string_view name;
   static constexpr std::string_view extractorName = "swift";
   std::string_view action;
   // space separated if more than 1. Not a vector to allow constexpr
   // TODO(C++20) with vector going constexpr this can be turned to `std::vector<std::string_view>`
   std::string_view helpLinks;
-
   // for the moment, we only output errors, so no need to store the severity
 
-  // registers a diagnostics source for later retrieval with get, if not done yet
-  template <const SwiftDiagnosticsSource* Source>
-  static void ensureRegistered() {
-    static std::once_flag once;
-    std::call_once(once, registerImpl, Source);
-  }
+  Visibility visibility{Visibility::all};
 
-  // gets a previously inscribed SwiftDiagnosticsSource for the given id. Will abort if none exists
-  static const SwiftDiagnosticsSource& get(const std::string& id) { return *map().at(id); }
+  std::optional<SwiftDiagnosticsLocation> location{};
 
-  // emit a JSON diagnostics for this source with the given timestamp and message to out
-  // A plaintextMessage is used that includes both the message and the action to take. Dots are
-  // appended to both. The id is used to construct the source id in the form
-  // `swift/<prog name>/<id with '-' replacing '_'>`
-  void emit(std::ostream& out, std::string_view timestamp, std::string_view message) const;
-
- private:
-  static void registerImpl(const SwiftDiagnosticsSource* source);
+  constexpr SwiftDiagnostic(std::string_view id,
+                            std::string_view name,
+                            std::string_view action = "",
+                            std::string_view helpLinks = "",
+                            Visibility visibility = Visibility::all)
+      : id{id}, name{name}, action{action}, helpLinks{helpLinks}, visibility{visibility} {}
 
-  std::string sourceId() const;
-  using Map = std::unordered_map<std::string, const SwiftDiagnosticsSource*>;
+  constexpr SwiftDiagnostic(std::string_view id, std::string_view name, Visibility visibility)
+      : SwiftDiagnostic(id, name, "", "", visibility) {}
 
-  static Map& map() {
-    static Map ret;
+  // create a JSON diagnostics for this source with the given timestamp and message to out
+  // A plaintextMessage is used that includes both the message and the action to take. Dots are
+  // appended to both. The id is used to construct the source id in the form
+  // `swift/<prog name>/<id>`
+  nlohmann::json json(const std::chrono::system_clock::time_point& timestamp,
+                      std::string_view message) const;
+
+  // returns <id> or <id>@<location> if a location is present
+  std::string abbreviation() const;
+
+  SwiftDiagnostic withLocation(std::string_view file,
+                               unsigned startLine = 0,
+                               unsigned startColumn = 0,
+                               unsigned endLine = 0,
+                               unsigned endColumn = 0) const {
+    auto ret = *this;
+    ret.location = SwiftDiagnosticsLocation{file, startLine, startColumn, endLine, endColumn};
     return ret;
   }
-};
-
-// An output modeling binlog's output stream concept that intercepts binlog entries and translates
-// them to appropriate diagnostics JSON entries
-class SwiftDiagnosticsDumper {
- public:
-  // opens path for writing out JSON entries. Returns whether the operation was successful.
-  bool open(const std::filesystem::path& path) {
-    output.open(path);
-    return output.good();
-  }
-
-  void flush() { output.flush(); }
-
-  // write out binlog entries as corresponding JSON diagnostics entries. Expects all entries to have
-  // a category equal to an id of a previously created SwiftDiagnosticSource.
-  void write(const char* buffer, std::size_t bufferSize);
 
  private:
-  binlog::EventStream events;
-  std::ofstream output;
-  binlog::PrettyPrinter timestampedMessagePrinter{"%u %m", "%Y-%m-%dT%H:%M:%S.%NZ"};
+  bool has(Visibility v) const;
 };
 
-}  // namespace codeql
+inline constexpr SwiftDiagnostic::Visibility operator|(SwiftDiagnostic::Visibility lhs,
+                                                       SwiftDiagnostic::Visibility rhs) {
+  return static_cast<SwiftDiagnostic::Visibility>(static_cast<unsigned char>(lhs) |
+                                                  static_cast<unsigned char>(rhs));
+}
 
-namespace codeql_diagnostics {
-constexpr codeql::SwiftDiagnosticsSource internal_error{
-    "internal_error",
+inline constexpr SwiftDiagnostic::Visibility operator&(SwiftDiagnostic::Visibility lhs,
+                                                       SwiftDiagnostic::Visibility rhs) {
+  return static_cast<SwiftDiagnostic::Visibility>(static_cast<unsigned char>(lhs) &
+                                                  static_cast<unsigned char>(rhs));
+}
+
+constexpr SwiftDiagnostic internalError{
+    "internal-error",
     "Internal error",
-    "Contact us about this issue",
+    SwiftDiagnostic::Visibility::telemetry,
 };
-}  // namespace codeql_diagnostics
+}  // namespace codeql

swift/logging/SwiftLogging.cpp

@@ -60,7 +60,7 @@ std::vector<std::string> Log::collectLevelRulesAndReturnProblems(const char* env
     // expect comma-separated <glob pattern>:<log severity>
     std::regex comma{","};
     std::regex levelAssignment{
-        R"((?:([*./\w]+)|(?:out:(binary|text|console|diagnostics))):()" LEVEL_REGEX_PATTERN ")"};
+        R"((?:([*./\w]+)|(?:out:(binary|text|console))):()" LEVEL_REGEX_PATTERN ")"};
     std::cregex_token_iterator begin{levels, levels + strlen(levels), comma, -1};
     std::cregex_token_iterator end{};
     for (auto it = begin; it != end; ++it) {
@@ -84,8 +84,6 @@ std::vector<std::string> Log::collectLevelRulesAndReturnProblems(const char* env
             text.level = level;
           } else if (out == "console") {
             console.level = level;
-          } else if (out == "diagnostics") {
-            diagnostics.level = level;
           }
         }
       } else {
@@ -132,30 +130,28 @@ void Log::configure() {
       text.level = Level::no_logs;
     }
   }
-  if (diagnostics) {
-    std::filesystem::path diagFile =
-        getEnvOr("CODEQL_EXTRACTOR_SWIFT_DIAGNOSTIC_DIR", "extractor-out/diagnostics");
+  if (auto diagDir = getEnvOr("CODEQL_EXTRACTOR_SWIFT_DIAGNOSTIC_DIR", nullptr)) {
+    std::filesystem::path diagFile = diagDir;
     diagFile /= programName;
     diagFile /= logBaseName;
     diagFile.replace_extension(".jsonl");
     std::error_code ec;
     std::filesystem::create_directories(diagFile.parent_path(), ec);
     if (!ec) {
-      if (!diagnostics.output.open(diagFile)) {
+      diagnostics.open(diagFile);
+      if (!diagnostics) {
         problems.emplace_back("Unable to open diagnostics json file " + diagFile.string());
-        diagnostics.level = Level::no_logs;
       }
     } else {
       problems.emplace_back("Unable to create diagnostics directory " +
                             diagFile.parent_path().string() + ": " + ec.message());
-      diagnostics.level = Level::no_logs;
     }
   }
   for (const auto& problem : problems) {
     LOG_ERROR("{}", problem);
   }
   LOG_INFO("Logging configured (binary: {}, text: {}, console: {}, diagnostics: {})", binary.level,
-           text.level, console.level, diagnostics.level);
+           text.level, console.level, diagnostics.good());
   initialized = true;
   flushImpl();
 }
@@ -169,7 +165,17 @@ void Log::flushImpl() {
     binary.output.flush();
   }
   if (diagnostics) {
-    diagnostics.output.flush();
+    diagnostics.flush();
+  }
+}
+
+void Log::diagnoseImpl(const SwiftDiagnostic& source,
+                       const std::chrono::nanoseconds& elapsed,
+                       std::string_view message) {
+  using Clock = std::chrono::system_clock;
+  Clock::time_point time{std::chrono::duration_cast<Clock::duration>(elapsed)};
+  if (diagnostics) {
+    diagnostics << source.json(time, message) << '\n';
   }
 }
 
@@ -190,13 +196,11 @@ Log& Log::write(const char* buffer, std::streamsize size) {
   if (text) text.write(buffer, size);
   if (binary) binary.write(buffer, size);
   if (console) console.write(buffer, size);
-  if (diagnostics) diagnostics.write(buffer, size);
   return *this;
 }
 
 Logger& Log::logger() {
   static Logger ret{getLoggerConfigurationImpl("logging")};
   return ret;
 }
-
 }  // namespace codeql