[clang-tidy] Add a diagnostic callback to parseConfiguration

[njames93]

Summary

Currently errors detected when parsing the YAML for .clang-tidy files are always printed to errs.
For clang-tidy binary workflows this usually isn't an issue, but using clang-tidy as a library for integrations may want to handle displaying those errors in their own specific way.

[njames93]

Use MemoryBufferRef instead of StringRef so the Diagnostic can have a meaningful filename.

[aaron.ballman]

Nothing is calling the new parseConfigurationWithDiags(), is that intentional? Also, is there a way to add test coverage for the change?

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Would it make sense to require Handler to be nonnull with an assertion?

[njames93]

In D92920#2443578, @aaron.ballman wrote:

Nothing is calling the new parseConfigurationWithDiags(), is that intentional? Also, is there a way to add test coverage for the change?

Yes it is intentional, the idea is for programs embedding clang-tidy to use this interface if they wish to handle diagnostics from parsing themselves.

Tests could be added

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Wasn't sure which way to go with that one, happy to use an assert if you think it's a good idea

[aaron.ballman]

In D92920#2443748, @njames93 wrote:

In D92920#2443578, @aaron.ballman wrote:

Nothing is calling the new parseConfigurationWithDiags(), is that intentional? Also, is there a way to add test coverage for the change?

Yes it is intentional, the idea is for programs embedding clang-tidy to use this interface if they wish to handle diagnostics from parsing themselves.

Ahh, I see, thank you for the explanation.

Tests could be added

I think they should be added.

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Now that I understand the use for this change better, I think the code is good as-is.

[njames93]

Added test cases.
Added assert to ensure Handler is valid.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Whoops, just changed it to assert

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Heh, sorry about that! Before changing it again, let's make sure we agree. My thinking is: this is a general API (rather than a specific one only to be used internally) and not every caller may care about reporting diagnostics; the error return code is sufficient to tell any caller whether the parsing was successful or not.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

That's pretty much it, For external users. If they don't care about capturing the diagnostics then the parseConfiguration function should be used. If they do care and call this new function with an empty callable, they have probably made a mistake.

[njames93]

Fix up test case now bug in yaml error locations has been fixed

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

Okay, then I apologize for the churn, but can you go back to accepting a null input (and a test case for it)? With that, I think the patch LG.

[njames93]

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

With a null handler, we can't capture any test output to validate anything.

[njames93]

Added back null check

[aaron.ballman]

LG!

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

401

I meant more just to demonstrate that it's a supported part of the interface and won't crash, assert, or do bad things. If you don't think it will be that useful, I don't insist.

clang-tools-extra/clang-tidy/ClangTidyOptions.h

 //===--- ClangTidyOptions.h - clang-tidy ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
 
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <functional>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 namespace clang {

 std::error_code parseLineFilter(llvm::StringRef LineFilter,
                                 ClangTidyGlobalOptions &Options);
 
 /// Parses configuration from JSON and returns \c ClangTidyOptions or an
 /// error.
 llvm::ErrorOr<ClangTidyOptions>
 parseConfiguration(llvm::MemoryBufferRef Config);
 
+using DiagCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
+
+llvm::ErrorOr<ClangTidyOptions>
+parseConfigurationWithDiags(llvm::MemoryBufferRef Config, DiagCallback Handler);
+
 /// Serializes configuration to a YAML-encoded string.
 std::string configurationAsText(const ClangTidyOptions &Options);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H

clang-tools-extra/clang-tidy/ClangTidyOptions.cpp

 //===--- ClangTidyOptions.cpp - clang-tidy ----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyOptions.h"
 #include "ClangTidyModuleRegistry.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 #include <utility>
 
 #define DEBUG_TYPE "clang-tidy-options"
 
 using clang::tidy::ClangTidyOptions;
parseConfiguration(llvm::MemoryBufferRef Config) {
   llvm::yaml::Input Input(Config);
   ClangTidyOptions Options;
   Input >> Options;
   if (Input.error())
     return Input.error();
   return Options;
 }
 
+static void diagHandlerImpl(const llvm::SMDiagnostic &Diag, void *Ctx) {
+  (*reinterpret_cast<DiagCallback *>(Ctx))(Diag);
+};
+
+llvm::ErrorOr<ClangTidyOptions>
+parseConfigurationWithDiags(llvm::MemoryBufferRef Config,
+                            DiagCallback Handler) {
+  llvm::yaml::Input Input(Config, nullptr, Handler ? diagHandlerImpl : nullptr,
+                          &Handler);
+  ClangTidyOptions Options;
+  Input >> Options;
+  if (Input.error())
+    return Input.error();
+  return Options;
+}
+
 std::string configurationAsText(const ClangTidyOptions &Options) {
   std::string Text;
   llvm::raw_string_ostream Stream(Text);
   llvm::yaml::Output Output(Stream);
   // We use the same mapping method for input and output, so we need a non-const
   // reference here.
   ClangTidyOptions NonConstValue = Options;
   Output << NonConstValue;
   return Stream.str();
 }
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
+  TestingSupport
   )
 
 get_filename_component(CLANG_LINT_SOURCE_DIR
   ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-tidy REALPATH)
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
   AddConstTest.cpp

clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

 #include "ClangTidyOptions.h"
 #include "ClangTidyCheck.h"
 #include "ClangTidyDiagnosticConsumer.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Testing/Support/Annotations.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tidy {
 
 enum class Colours { Red, Orange, Yellow, Green, Blue, Indigo, Violet };
 
 template <> struct OptionEnumMapping<Colours> {
TEST(ParseConfiguration, MergeConfigurations) {
   EXPECT_EQ("arg-before1,arg-before2,arg-before3,arg-before4",
             llvm::join(Options.ExtraArgsBefore->begin(),
                        Options.ExtraArgsBefore->end(), ","));
   ASSERT_TRUE(Options.UseColor.hasValue());
   EXPECT_TRUE(*Options.UseColor);
 }
 
 namespace {
+class DiagCollecter {
+public:
+  struct Diag {
+  private:
+    static size_t posToOffset(const llvm::SMLoc Loc,
+                              const llvm::SourceMgr *Src) {
+      return Loc.getPointer() -
+             Src->getMemoryBuffer(Src->FindBufferContainingLoc(Loc))
+                 ->getBufferStart();
+    }
+
+  public:
+    Diag(const llvm::SMDiagnostic &D)
+        : Message(D.getMessage()), Kind(D.getKind()),
+          Pos(posToOffset(D.getLoc(), D.getSourceMgr())) {
+      if (!D.getRanges().empty()) {
+        // Ranges are stored as column numbers on the line that has the error.
+        unsigned Offset = Pos - D.getColumnNo();
+        Range.emplace();
+        Range->Begin = Offset + D.getRanges().front().first,
+        Range->End = Offset + D.getRanges().front().second;
+      }
+    }
+    std::string Message;
+    llvm::SourceMgr::DiagKind Kind;
+    size_t Pos;
+    Optional<llvm::Annotations::Range> Range;
+
+    friend void PrintTo(const Diag &D, std::ostream *OS) {
+      *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
+          << D.Message << "@" << llvm::to_string(D.Pos);
+      if (D.Range)
+        *OS << ":[" << D.Range->Begin << ", " << D.Range->End << ")";
+    }
+  };
+
+  DiagCollecter() = default;
+  DiagCollecter(const DiagCollecter &) = delete;
+
+  std::function<void(const llvm::SMDiagnostic &)>
+  getCallback(bool Clear = true) & {
+    if (Clear)
+      Diags.clear();
+    return [&](const llvm::SMDiagnostic &Diag) { Diags.emplace_back(Diag); };
+  }
+
+  std::function<void(const llvm::SMDiagnostic &)>
+  getCallback(bool Clear = true) && = delete;
+
+  llvm::ArrayRef<Diag> getDiags() const { return Diags; }
+
+private:
+  std::vector<Diag> Diags;
+};
+
+MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
+MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
+MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
+MATCHER_P(DiagRange, P, "") { return arg.Range && *arg.Range == P; }
+} // namespace
+
+using ::testing::AllOf;
+using ::testing::ElementsAre;
+
+TEST(ParseConfiguration, CollectDiags) {
+  DiagCollecter Collector;
+  auto ParseWithDiags = [&](llvm::StringRef Buffer) {
+    return parseConfigurationWithDiags(llvm::MemoryBufferRef(Buffer, "Options"),
+                                       Collector.getCallback());
+  };
+  llvm::Annotations Options(R"(
+    [[Check]]: llvm-include-order
+  )");
+  llvm::ErrorOr<ClangTidyOptions> ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+              testing::ElementsAre(AllOf(DiagMessage("unknown key 'Check'"),
+                                         DiagKind(llvm::SourceMgr::DK_Error),
+                                         DiagPos(Options.range().Begin),
+                                         DiagRange(Options.range()))));
+
+  Options = llvm::Annotations(R"(
+    UseColor: [[NotABool]]
+  )");
+  ParsedOpt = ParseWithDiags(Options.code());
+  EXPECT_TRUE(!ParsedOpt);
+  EXPECT_THAT(Collector.getDiags(),
+              testing::ElementsAre(AllOf(DiagMessage("invalid boolean"),
+                                         DiagKind(llvm::SourceMgr::DK_Error),
+                                         DiagPos(Options.range().Begin),
+                                         DiagRange(Options.range()))));
+}
+
+namespace {
 class TestCheck : public ClangTidyCheck {
 public:
   TestCheck(ClangTidyContext *Context) : ClangTidyCheck("test", Context) {}
 
   template <typename... Args> auto getLocal(Args &&... Arguments) {
     return Options.get(std::forward<Args>(Arguments)...);
   }