Swift: go back to explicit DIAGNOSE_ERROR macros

swift/integration-tests/diagnostics_test_utils.py

@@ -60,5 +60,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/SwiftDiagnostics.cpp

@@ -8,12 +8,12 @@
 
 namespace codeql {
 
-nlohmann::json SwiftDiagnosticsSource::json(const std::chrono::system_clock::time_point& timestamp,
-                                            std::string_view message) const {
-  return {
+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},
        }},
@@ -28,26 +28,29 @@ nlohmann::json SwiftDiagnosticsSource::json(const std::chrono::system_clock::tim
       {"plaintextMessage", absl::StrCat(message, ".\n\n", action, ".")},
       {"timestamp", fmt::format("{:%FT%T%z}", timestamp)},
   };
-}
-
-std::string SwiftDiagnosticsSource::sourceId() const {
-  return absl::StrJoin({extractorName, programName, id}, "/");
-}
-
-nlohmann::json SwiftDiagnosticsSourceWithLocation::json(
-    const std::chrono::system_clock::time_point& timestamp,
-    std::string_view message) const {
-  auto ret = source.json(timestamp, message);
-  auto& location = ret["location"] = {{"file", file}};
-  if (startLine) location["startLine"] = startLine;
-  if (startColumn) location["startColumn"] = startColumn;
-  if (endLine) location["endLine"] = endLine;
-  if (endColumn) location["endColumn"] = endColumn;
+  if (location) {
+    ret["location"] = location->json();
+  }
   return ret;
 }
 
-std::string SwiftDiagnosticsSourceWithLocation::str() const {
-  return absl::StrCat(source.id, "@", file, ":", startLine, ":", startColumn, ":", endLine, ":",
-                      endColumn);
+std::string SwiftDiagnostic::abbreviation() const {
+  if (location) {
+    return absl::StrCat(id, "@", location->str());
+  }
+  return std::string{id};
+}
+
+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

@@ -4,6 +4,7 @@
 #include <string>
 #include <vector>
 #include <unordered_map>
+#include <optional>
 #include <cassert>
 #include <fstream>
 #include <filesystem>
@@ -17,19 +18,14 @@ namespace codeql {
 
 extern const std::string_view programName;
 
-struct SwiftDiagnosticsSource;
-
-struct SwiftDiagnosticsSourceWithLocation {
-  const SwiftDiagnosticsSource& source;
+struct SwiftDiagnosticsLocation {
   std::string_view file;
   unsigned startLine;
   unsigned startColumn;
   unsigned endLine;
   unsigned endColumn;
 
-  // see SwiftDiagnosticsSource::json
-  nlohmann::json json(const std::chrono::system_clock::time_point& timestamp,
-                      std::string_view message) const;
+  nlohmann::json json() const;
 
   std::string str() const;
 };
@@ -37,7 +33,7 @@ struct SwiftDiagnosticsSourceWithLocation {
 // 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 {
   std::string_view id;
   std::string_view name;
   static constexpr std::string_view extractorName = "swift";
@@ -46,6 +42,7 @@ struct SwiftDiagnosticsSource {
   // TODO(C++20) with vector going constexpr this can be turned to `std::vector<std::string_view>`
   std::string_view helpLinks;
 
+  std::optional<SwiftDiagnosticsLocation> location;
   // for the moment, we only output errors, so no need to store the severity
 
   // create a JSON diagnostics for this source with the given timestamp and message to out
@@ -55,16 +52,18 @@ struct SwiftDiagnosticsSource {
   nlohmann::json json(const std::chrono::system_clock::time_point& timestamp,
                       std::string_view message) const;
 
-  SwiftDiagnosticsSourceWithLocation withLocation(std::string_view file,
-                                                  unsigned startLine = 0,
-                                                  unsigned startColumn = 0,
-                                                  unsigned endLine = 0,
-                                                  unsigned endColumn = 0) const {
-    return {*this, file, startLine, startColumn, endLine, endColumn};
-  }
+  // returns <id> or <id>@<location> if a location is present
+  std::string abbreviation() const;
 
- private:
-  std::string sourceId() 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;
+  }
 };
 
 class SwiftDiagnosticsDumper {
@@ -77,8 +76,7 @@ class SwiftDiagnosticsDumper {
 
   void flush() { output.flush(); }
 
-  template <typename Source>
-  void write(const Source& source,
+  void write(const SwiftDiagnostic& source,
              const std::chrono::system_clock::time_point& timestamp,
              std::string_view message) {
     if (output) {
@@ -93,49 +91,9 @@ class SwiftDiagnosticsDumper {
   std::ofstream output;
 };
 
-constexpr codeql::SwiftDiagnosticsSource internalError{
+constexpr SwiftDiagnostic internalError{
     "internal-error",
     "Internal error",
     "Contact us about this issue",
 };
 }  // namespace codeql
-
-namespace mserialize {
-// log diagnostic sources using just their id, using binlog/mserialize internal plumbing
-template <>
-struct CustomTag<codeql::SwiftDiagnosticsSource, void> : detail::BuiltinTag<std::string> {
-  using T = codeql::SwiftDiagnosticsSource;
-};
-
-template <>
-struct CustomSerializer<codeql::SwiftDiagnosticsSource, void> {
-  template <typename OutputStream>
-  static void serialize(const codeql::SwiftDiagnosticsSource& source, OutputStream& out) {
-    mserialize::serialize(source.id, out);
-  }
-
-  static size_t serialized_size(const codeql::SwiftDiagnosticsSource& source) {
-    return mserialize::serialized_size(source.id);
-  }
-};
-
-template <>
-struct CustomTag<codeql::SwiftDiagnosticsSourceWithLocation, void>
-    : detail::BuiltinTag<std::string> {
-  using T = codeql::SwiftDiagnosticsSourceWithLocation;
-};
-
-template <>
-struct CustomSerializer<codeql::SwiftDiagnosticsSourceWithLocation, void> {
-  template <typename OutputStream>
-  static void serialize(const codeql::SwiftDiagnosticsSourceWithLocation& source,
-                        OutputStream& out) {
-    mserialize::serialize(source.str(), out);
-  }
-
-  static size_t serialized_size(const codeql::SwiftDiagnosticsSourceWithLocation& source) {
-    return mserialize::serialized_size(source.str());
-  }
-};
-
-}  // namespace mserialize

swift/logging/SwiftLogging.h

@@ -35,29 +35,31 @@
 #define LOG_DEBUG(...) LOG_WITH_LEVEL(debug, __VA_ARGS__)
 #define LOG_TRACE(...) LOG_WITH_LEVEL(trace, __VA_ARGS__)
 
-#define CODEQL_GET_SECOND(...) CODEQL_GET_SECOND_I(__VA_ARGS__, 0, 0)
-#define CODEQL_GET_SECOND_I(X, Y, ...) Y
-
 // only do the actual logging if the picked up `Logger` instance is configured to handle the
 // provided log level. `LEVEL` must be a compile-time constant. `logger()` is evaluated once
-// TODO(C++20) replace non-standard ##__VA_ARGS__ with __VA_OPT__(,) __VA_ARGS__
-#define LOG_WITH_LEVEL_AND_CATEGORY(LEVEL, CATEGORY, ...)                                      \
-  do {                                                                                         \
-    static_assert(::codeql::detail::checkLogArgs<decltype(CODEQL_GET_SECOND(__VA_ARGS__))>(    \
-                      MSERIALIZE_FIRST(__VA_ARGS__)),                                          \
-                  "diagnostics logs must have format starting with \"[{}]\"");                 \
-    constexpr auto _level = ::codeql::Log::Level::LEVEL;                                       \
-    ::codeql::Logger& _logger = logger();                                                      \
-    if (_level >= _logger.level()) {                                                           \
-      BINLOG_CREATE_SOURCE_AND_EVENT(_logger.writer(), _level, CATEGORY, ::binlog::clockNow(), \
-                                     __VA_ARGS__);                                             \
-    }                                                                                          \
-    if (_level >= ::codeql::Log::Level::error) {                                               \
-      ::codeql::Log::flush();                                                                  \
-    }                                                                                          \
+#define LOG_WITH_LEVEL(LEVEL, ...)                                                                 \
+  do {                                                                                             \
+    constexpr auto _level = ::codeql::Log::Level::LEVEL;                                           \
+    ::codeql::Logger& _logger = logger();                                                          \
+    if (_level >= _logger.level()) {                                                               \
+      BINLOG_CREATE_SOURCE_AND_EVENT(_logger.writer(), _level, /*category*/, ::binlog::clockNow(), \
+                                     __VA_ARGS__);                                                 \
+    }                                                                                              \
+    if (_level >= ::codeql::Log::Level::error) {                                                   \
+      ::codeql::Log::flush();                                                                      \
+    }                                                                                              \
   } while (false)
 
-#define LOG_WITH_LEVEL(LEVEL, ...) LOG_WITH_LEVEL_AND_CATEGORY(LEVEL, , __VA_ARGS__)
+// Emit errors with a specified SwiftDiagnostic object. These will be both logged and outputted as
+// JSON DB diagnostics
+#define DIAGNOSE_CRITICAL(ID, ...) DIAGNOSE_WITH_LEVEL(critical, ID, __VA_ARGS__)
+#define DIAGNOSE_ERROR(ID, ...) DIAGNOSE_WITH_LEVEL(error, ID, __VA_ARGS__)
+
+#define CODEQL_DIAGNOSTIC_LOG_FORMAT_PREFIX "[{}] "
+// TODO(C++20) replace non-standard , ##__VA_ARGS__ with __VA_OPT__(,) __VA_ARGS__
+#define DIAGNOSE_WITH_LEVEL(LEVEL, ID, FORMAT, ...)                 \
+  LOG_WITH_LEVEL(LEVEL, CODEQL_DIAGNOSTIC_LOG_FORMAT_PREFIX FORMAT, \
+                 ::codeql::detail::SwiftDiagnosticLogWrapper{ID}, ##__VA_ARGS__);
 
 // avoid calling into binlog's original macros
 #undef BINLOG_CRITICAL
@@ -126,8 +128,7 @@ class Log {
     return instance().getLoggerConfigurationImpl(name);
   }
 
-  template <typename Source>
-  static void diagnose(const Source& source,
+  static void diagnose(const SwiftDiagnostic& source,
                        const std::chrono::system_clock::time_point& time,
                        std::string_view message) {
     instance().diagnostics.write(source, time, message);
@@ -224,31 +225,9 @@ class Logger {
 };
 
 namespace detail {
-constexpr std::string_view diagnosticsFormatPrefix = "[{}] ";
-
-template <typename T>
-constexpr bool checkLogArgs(std::string_view format) {
-  using Type = std::remove_cv_t<std::remove_reference_t<T>>;
-  constexpr bool isDiagnostic = std::is_same_v<Type, SwiftDiagnosticsSource> ||
-                                std::is_same_v<Type, SwiftDiagnosticsSourceWithLocation>;
-  return !isDiagnostic ||
-         format.substr(0, diagnosticsFormatPrefix.size()) == diagnosticsFormatPrefix;
-}
-
-template <typename Writer, typename Source, typename... T>
-void binlogAddEventIgnoreFirstOverload(Writer& writer,
-                                       std::uint64_t eventSourceId,
-                                       std::uint64_t clock,
-                                       const char* format,
-                                       const Source& source,
-                                       T&&... t) {
-  std::chrono::system_clock::time_point point{
-      std::chrono::duration_cast<std::chrono::system_clock::duration>(
-          std::chrono::nanoseconds{clock})};
-  constexpr auto offset = ::codeql::detail::diagnosticsFormatPrefix.size();
-  ::codeql::Log::diagnose(source, point, fmt::format(format + offset, t...));
-  writer.addEvent(eventSourceId, clock, source, std::forward<T>(t)...);
-}
+struct SwiftDiagnosticLogWrapper {
+  const SwiftDiagnostic& value;
+};
 
 }  // namespace detail
 }  // namespace codeql
@@ -262,32 +241,37 @@ void addEventIgnoreFirst(Writer& writer,
                          std::uint64_t eventSourceId,
                          std::uint64_t clock,
                          const char (&format)[N],
-                         const codeql::SwiftDiagnosticsSource& source,
+                         codeql::detail::SwiftDiagnosticLogWrapper&& source,
                          T&&... t) {
-  codeql::detail::binlogAddEventIgnoreFirstOverload(writer, eventSourceId, clock, format, source,
-                                                    std::forward<T>(t)...);
+  std::chrono::system_clock::time_point point{
+      std::chrono::duration_cast<std::chrono::system_clock::duration>(
+          std::chrono::nanoseconds{clock})};
+  constexpr std::string_view prefix = CODEQL_DIAGNOSTIC_LOG_FORMAT_PREFIX;
+  ::codeql::Log::diagnose(source.value, point, fmt::format(format + prefix.size(), t...));
+  writer.addEvent(eventSourceId, clock, source, std::forward<T>(t)...);
 }
-
-template <typename Writer, size_t N, typename... T>
-void addEventIgnoreFirst(Writer& writer,
-                         std::uint64_t eventSourceId,
-                         std::uint64_t clock,
-                         const char (&format)[N],
-                         codeql::SwiftDiagnosticsSourceWithLocation&& source,
-                         T&&... t) {
-  codeql::detail::binlogAddEventIgnoreFirstOverload(writer, eventSourceId, clock, format, source,
-                                                    std::forward<T>(t)...);
-}
-
-template <typename Writer, size_t N, typename... T>
-void addEventIgnoreFirst(Writer& writer,
-                         std::uint64_t eventSourceId,
-                         std::uint64_t clock,
-                         const char (&format)[N],
-                         const codeql::SwiftDiagnosticsSourceWithLocation& source,
-                         T&&... t) {
-  codeql::detail::binlogAddEventIgnoreFirstOverload(writer, eventSourceId, clock, format, source,
-                                                    std::forward<T>(t)...);
-}
-
 }  // namespace binlog::detail
+
+namespace mserialize {
+// log diagnostics wrapper using the abbreviation of the underlying diagnostic, using
+// binlog/mserialize internal plumbing
+template <>
+struct CustomTag<codeql::detail::SwiftDiagnosticLogWrapper, void>
+    : detail::BuiltinTag<std::string> {
+  using T = codeql::detail::SwiftDiagnosticLogWrapper;
+};
+
+template <>
+struct CustomSerializer<codeql::detail::SwiftDiagnosticLogWrapper, void> {
+  template <typename OutputStream>
+  static void serialize(const codeql::detail::SwiftDiagnosticLogWrapper& source,
+                        OutputStream& out) {
+    mserialize::serialize(source.value.abbreviation(), out);
+  }
+
+  static size_t serialized_size(const codeql::detail::SwiftDiagnosticLogWrapper& source) {
+    return mserialize::serialized_size(source.value.abbreviation());
+  }
+};
+
+}  // namespace mserialize

swift/xcode-autobuilder/XcodeBuildRunner.cpp

@@ -8,11 +8,9 @@
 #include "swift/logging/SwiftLogging.h"
 #include "swift/xcode-autobuilder/CustomizingBuildDiagnostics.h"
 
-namespace codeql {
-constexpr SwiftDiagnosticsSource buildCommandFailed{
-    "build-command-failed", "Detected build command failed", customizingBuildAction,
-    customizingBuildHelpLinks};
-}
+constexpr codeql::SwiftDiagnostic buildCommandFailed{
+    "build-command-failed", "Detected build command failed", codeql::customizingBuildAction,
+    codeql::customizingBuildHelpLinks};
 
 static codeql::Logger& logger() {
   static codeql::Logger ret{"build"};
@@ -70,8 +68,8 @@ void buildTarget(Target& target, bool dryRun) {
     std::cout << absl::StrJoin(argv, " ") << "\n";
   } else {
     if (!exec(argv)) {
-      LOG_ERROR("[{}] The detected build command failed (tried {})", codeql::buildCommandFailed,
-                absl::StrJoin(argv, " "));
+      DIAGNOSE_ERROR(buildCommandFailed, "The detected build command failed (tried {})",
+                     absl::StrJoin(argv, " "));
       codeql::Log::flush();
       exit(1);
     }

swift/xcode-autobuilder/xcode-autobuilder.cpp

@@ -12,19 +12,17 @@ static const char* unitTest = "com.apple.product-type.bundle.unit-test";
 
 const std::string_view codeql::programName = "autobuilder";
 
-namespace codeql {
-constexpr SwiftDiagnosticsSource noProjectFound{"no-project-found",
-                                                "No Xcode project or workspace detected",
-                                                customizingBuildAction, customizingBuildHelpLinks};
+constexpr codeql::SwiftDiagnostic noProjectFound{
+    "no-project-found", "No Xcode project or workspace detected", codeql::customizingBuildAction,
+    codeql::customizingBuildHelpLinks};
 
-constexpr SwiftDiagnosticsSource noSwiftTarget{"no-swift-target",
-                                               "No Swift compilation target found",
-                                               customizingBuildAction, customizingBuildHelpLinks};
+constexpr codeql::SwiftDiagnostic noSwiftTarget{
+    "no-swift-target", "No Swift compilation target found", codeql::customizingBuildAction,
+    codeql::customizingBuildHelpLinks};
 
-constexpr SwiftDiagnosticsSource spmNotSupported{
+constexpr codeql::SwiftDiagnostic spmNotSupported{
     "spm-not-supported", "Swift Package Manager build unsupported by autobuild",
-    customizingBuildAction, customizingBuildHelpLinks};
-}  // namespace codeql
+    codeql::customizingBuildAction, codeql::customizingBuildHelpLinks};
 
 static codeql::Logger& logger() {
   static codeql::Logger ret{"main"};
@@ -53,15 +51,14 @@ static void autobuild(const CLIArgs& args) {
   std::sort(std::begin(targets), std::end(targets),
             [](Target& lhs, Target& rhs) { return lhs.fileCount > rhs.fileCount; });
   if ((!collected.xcodeEncountered || targets.empty()) && collected.swiftPackageEncountered) {
-    LOG_ERROR("[{}] No viable Swift Xcode target was found but a Swift package was detected. Swift "
-              "Package Manager builds are not yet supported by the autobuilder",
-              codeql::spmNotSupported);
+    DIAGNOSE_ERROR(spmNotSupported,
+                   "No viable Swift Xcode target was found but a Swift package was detected. Swift "
+                   "Package Manager builds are not yet supported by the autobuilder");
   } else if (!collected.xcodeEncountered) {
-    LOG_ERROR("[{}] No Xcode project or workspace was found", codeql::noProjectFound);
+    DIAGNOSE_ERROR(noProjectFound, "No Xcode project or workspace was found");
   } else if (targets.empty()) {
-    LOG_ERROR("[{}] All targets found within Xcode projects or workspaces either "
-              "have no Swift sources or are tests",
-              codeql::noSwiftTarget);
+    DIAGNOSE_ERROR(noSwiftTarget, "All targets found within Xcode projects or workspaces either "
+                                  "have no Swift sources or are tests");
   } else {
     LOG_INFO("Selected {}", targets.front());
     buildTarget(targets.front(), args.dryRun);