[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

400

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

400

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

400

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

400

Whoops, just changed it to assert

[aaron.ballman]

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

400

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

400

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

400

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

400

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

400

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 {

 /// Parses LineFilter from JSON and stores it to the \p Options.
 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::StringRef 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;
llvm::ErrorOr<ClangTidyOptions> parseConfiguration(StringRef 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