[clang-tidy] Fix crash when handling invalid config values

[PiotrZSL]

Summary

Currently due to missing DiagnosticsEngine when invalid
enum config value is passed to check, and --dump-config
is used, clang-tidy would crash. This change fixes this
and extends --verify-config command to actually verify
those invalid enum values instead of ignoring them and
printing that 'No config errors detected.'.

Fixes:

• https://github.com/llvm/llvm-project/issues/50876

[PiotrZSL]

Ping + Rebase

[njames93]

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

61–69

I don't think this is the correct approach here
getAllChecksAndOptions should instead return an llvm::Expected<NamesAndOptions>.
You can create an error class that wraps a std::vector<ClangTidyError> and then still get the same behaviour on the error path.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

528

Given how options are read, we only care about the last option source that defined the option
Therefore this should be changed to return a std::optional<StringRef> and the for loop below can just search the RawOptions backwards for the first match, then return that.

529

542

CheckOptions is a StringMap, so key lookup should be used instead of iterating through all the entries

[PiotrZSL]

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

61–69

No, I need both Errors and Names. Not one of them.
And I think that some usages ignore Errors even if they exist.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

528

--verify-config checks all config files, and prints diagnostic for all of them with a path of file.
In this way if I will have same invalid value in 2 different files, even that one override second, it will still provide warning for both.
No need to limit only to last entry...

529

Ack.

542

Ack.

[njames93]

Ok, taking a step back here, this patch is trying to address 2 unrelated things at once:

• using --dump-config with bad check options results in a crash
• using --verify-config won't actually verify that the checks are able to parse values from their respective CheckOptions

I think this should be split up into 2 separate patches, one for each of the proposed changes and each can be reviewed on their own.

[PiotrZSL]

In D146842#4271089, @njames93 wrote:

I think this should be split up into 2 separate patches, one for each of the proposed changes and each can be reviewed on their own.

I don't plan to split this.

[njames93]

In D146842#4271144, @PiotrZSL wrote:

In D146842#4271089, @njames93 wrote:

I think this should be split up into 2 separate patches, one for each of the proposed changes and each can be reviewed on their own.

I don't plan to split this.

The LLVM Guidelines state that patches should only contain one isolated change, independent changes should be sent as multiple patches.
Given the only overlap between these 2 changes is the fact they are using the same test file. These qualify as being isolated changes and should be handled as such.

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

private:
   std::unique_ptr<ClangTidyCheckFactories> CheckFactories;
 };
 
 /// Fills the list of check names that are enabled when the provided
 /// filters are applied.
 std::vector<std::string> getCheckNames(const ClangTidyOptions &Options,
                                        bool AllowEnablingAnalyzerAlphaCheckers);
 
 struct NamesAndOptions {
   llvm::StringSet<> Names;
   llvm::StringSet<> Options;
+  std::vector<ClangTidyError> Errors;
 };
 
 NamesAndOptions
-getAllChecksAndOptions(bool AllowEnablingAnalyzerAlphaCheckers = true);
+getAllChecksAndOptions(const ClangTidyOptions &Options,
+                       bool AllowEnablingAnalyzerAlphaCheckers = true);
 
 /// Returns the effective check-specific options.
 ///
 /// The method configures ClangTidy with the specified \p Options and collects
 /// effective options from all created checks. The returned set of options
 /// includes default check-specific options for all keys not overridden by \p
 /// Options.
 ClangTidyOptions::OptionMap

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

 
 ClangTidyOptions::OptionMap
 getCheckOptions(const ClangTidyOptions &Options,
                 bool AllowEnablingAnalyzerAlphaCheckers) {
   clang::tidy::ClangTidyContext Context(
       std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options),
       AllowEnablingAnalyzerAlphaCheckers);
+  ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, false, false);
+  DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
+                       &DiagConsumer, /*ShouldOwnClient=*/false);
+  Context.setDiagnosticsEngine(&DE);
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
              const CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
for (const auto &Error : Errors) {
     TUD.Diagnostics.insert(TUD.Diagnostics.end(), Diag);
   }
 
   yaml::Output YAML(OS);
   YAML << TUD;
 }
 
 NamesAndOptions
-getAllChecksAndOptions(bool AllowEnablingAnalyzerAlphaCheckers) {
+getAllChecksAndOptions(const ClangTidyOptions &Options,
+                       bool AllowEnablingAnalyzerAlphaCheckers) {
   NamesAndOptions Result;
-  ClangTidyOptions Opts;
-  Opts.Checks = "*";
+  ClangTidyOptions Opt(Options);
+  Opt.Checks = "*";
   clang::tidy::ClangTidyContext Context(
-      std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(), Opts),
+      std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(), Opt),
       AllowEnablingAnalyzerAlphaCheckers);
+  ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, false, false);
+  DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
+                       &DiagConsumer, /*ShouldOwnClient=*/false);
+  Context.setDiagnosticsEngine(&DE);
   ClangTidyCheckFactories Factories;
   for (const ClangTidyModuleRegistry::entry &Module :
        ClangTidyModuleRegistry::entries()) {
     Module.instantiate()->addCheckFactories(Factories);
   }
 
   for (const auto &Factory : Factories)
     Result.Names.insert(Factory.getKey());
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
   }
 #endif // CLANG_TIDY_ENABLE_STATIC_ANALYZER
 
   Context.setOptionsCollector(&Result.Options);
   for (const auto &Factory : Factories) {
     Factory.getValue()(Factory.getKey(), &Context);
   }
 
+  Result.Errors = DiagConsumer.take();
   return Result;
 }
 } // namespace clang::tidy

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

for (const auto &ImplementationExtension : ImplementationFileExtensions) {
                        << ImplementationExtension << '\'';
         Output << VerifyConfigWarningEnd;
       }
     }
   }
   return AnyInvalid;
 }
 
+static raw_ostream &getDiagnosticForLevel(tooling::Diagnostic::Level DiagLevel,
+                                          llvm::StringRef Prefix = "") {
+  switch (DiagLevel) {
+  case tooling::Diagnostic::Error:
+    return llvm::WithColor::error(llvm::errs(), Prefix);
+  case tooling::Diagnostic::Warning:
+    return llvm::WithColor::warning(llvm::errs(), Prefix);
+  case tooling::Diagnostic::Remark:
+    return llvm::WithColor::remark(llvm::errs(), Prefix);
+  }
+}
+
+static llvm::StringSet<> findSourcesForDiagnostic(
+    const std::vector<ClangTidyOptionsProvider::OptionsSource> &RawOptions,
+    llvm::StringRef Diagnostic) {
+  llvm::StringSet<> Result;
+  if (!Diagnostic.consume_front("invalid configuration value '") ||
+      !Diagnostic.contains("' for option '")) {
+    Result.insert("");
+    return Result;
+  }
+
+  auto [Value, Rest] = Diagnostic.split("' for option '");
+  auto [Option, _] = Rest.split('\'');
+
+  for (const auto &[Opts, Source] : RawOptions) {
+    for (const auto &[ItOption, ItValue] : Opts.CheckOptions) {
+      if (ItOption == Option && ItValue.Value == Value) {
+        Result.insert(Source);
+        break;
+      }
+    }
+  }
+
+  if (Result.empty())
+    Result.insert("");
+
+  return Result;
+}
+
+static bool verifyDiagnosticErrors(
+    const std::vector<ClangTidyError> &Errors,
+    const std::vector<ClangTidyOptionsProvider::OptionsSource> &RawOptions) {
+  for (const ClangTidyError &Error : Errors) {
+    for (const auto &Source :
+         findSourcesForDiagnostic(RawOptions, Error.Message.Message)) {
+      getDiagnosticForLevel(Error.DiagLevel, Source.getKey())
+          << Error.Message.Message << " [" << Error.DiagnosticName << "]\n";
+      for (const DiagnosticMessage &Note : Error.Notes)
+        llvm::WithColor::note(llvm::errs(), Source.getKey())
+            << Note.Message << '\n';
+    }
+  }
+
+  return !Errors.empty();
+}
+
 int clangTidyMain(int argc, const char **argv) {
   llvm::InitLLVM X(argc, argv);
 
   // Enable help for -load option, if plugins are enabled.
   if (cl::Option *LoadOpt = cl::getRegisteredOptions().lookup("load"))
     LoadOpt->addCategory(ClangTidyCategory);
 
   llvm::Expected<CommonOptionsParser> OptionsParser =
llvm::outs() << configurationAsText(ClangTidyOptions::getDefaults().merge(
                         EffectiveOptions, 0))
                  << "\n";
     return 0;
   }
 
   if (VerifyConfig) {
     std::vector<ClangTidyOptionsProvider::OptionsSource> RawOptions =
         OptionsProvider->getRawOptions(FileName);
-    NamesAndOptions Valid =
-        getAllChecksAndOptions(AllowEnablingAnalyzerAlphaCheckers);
+    NamesAndOptions Valid = getAllChecksAndOptions(
+        EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
     bool AnyInvalid = false;
+    AnyInvalid |= verifyDiagnosticErrors(Valid.Errors, RawOptions);
     for (const auto &[Opts, Source] : RawOptions) {
       if (Opts.Checks)
         AnyInvalid |= verifyChecks(Valid.Names, *Opts.Checks, Source);
 
       if (Opts.HeaderFileExtensions && Opts.ImplementationFileExtensions)
         AnyInvalid |=
             verifyFileExtensions(*Opts.HeaderFileExtensions,
                                  *Opts.ImplementationFileExtensions, Source);

clang-tools-extra/test/clang-tidy/infrastructure/invalid-enum-config-value.cpp

+// RUN: clang-tidy --checks="-*,modernize-make-shared" \
+// RUN:   --config="CheckOptions: [{ \
+// RUN:      key: modernize-make-shared.IncludeStyle, value: '0' }]" \
+// RUN:   --dump-config -- | FileCheck %s -check-prefix=CHECK-DUMP
+// RUN: not clang-tidy --checks="-*,modernize-make-shared" \
+// RUN:   --config="CheckOptions: [{ \
+// RUN:      key: modernize-make-shared.IncludeStyle, value: '0' }]" \
+// RUN:   --verify-config -- 2>&1 | FileCheck %s -check-prefix=CHECK-VERIFY
+
+// CHECK-DUMP: modernize-make-shared.IncludeStyle: llvm
+// CHECK-VERIFY: command-line option '-config': warning: invalid configuration value '0' for option 'modernize-make-shared.IncludeStyle' [clang-tidy-config]