[clangd] Respect WarningsAsErrors configuration for clang-tidy

[nridge]

Summary

This completes the fix for https://bugs.llvm.org/show_bug.cgi?id=41218.

[ilya-biryukov]

clang-tools-extra/clangd/ClangdUnit.cpp

346 ↗

(On Diff #199200)

Why can't we call CTContext->treatAsError() here?

[nridge]

Address review comment

[nridge]

clang-tools-extra/clangd/ClangdUnit.cpp

346 ↗

(On Diff #199200)

Good point, that makes this much easier. Thanks!

[nridge]

Update as per changes to dependent patch D60953

[nridge]

Changing reviewer to @sammccall as the updates to the patch are closely related to the discussion in D60953.

Also added a couple of test cases.

[sammccall]

LG, thanks for the changes!

clang-tools-extra/trunk/clangd/ClangdUnit.cpp

llvm::Optional<tidy::ClangTidyContext> CTContext;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(llvm::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), Opts.ClangTidyOpts));
     CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
     CTContext->setASTContext(&Clang->getASTContext());
     CTContext->setCurrentFile(MainInput.getFile());
     CTFactories.createChecks(CTContext.getPointer(), CTChecks);
-    ASTDiags.suppressDiagnostics([&CTContext](
-                                     DiagnosticsEngine::Level DiagLevel,
+    ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
       if (CTContext) {
-        bool IsClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+        std::string CheckName = CTContext->getCheckName(Info.getID());
+        bool IsClangTidyDiag = !CheckName.empty();
         if (IsClangTidyDiag) {
-          // Skip the ShouldSuppressDiagnostic check for diagnostics not in
-          // the main file, because we don't want that function to query the
+          // Check for warning-as-error.
+          // We deliberately let this take precedence over suppression comments
+          // to match clang-tidy's behaviour.
+          if (DiagLevel == DiagnosticsEngine::Warning &&
+              CTContext->treatAsError(CheckName)) {
+            return DiagnosticsEngine::Error;
+          }
+
+          // Check for suppression comment. Skip the check for diagnostics not
+          // in the main file, because we don't want that function to query the
           // source buffer for preamble files. For the same reason, we ask
           // ShouldSuppressDiagnostic not to follow macro expansions, since
           // those might take us into a preamble file as well.
           bool IsInsideMainFile =
               Info.hasSourceManager() &&
               Info.getSourceManager().isWrittenInMainFile(
                   Info.getSourceManager().getFileLoc(Info.getLocation()));
           if (IsInsideMainFile && tidy::ShouldSuppressDiagnostic(
                                       DiagLevel, Info, *CTContext,
                                       /* CheckMacroExpansion = */ false)) {
-            return true;
+            return DiagnosticsEngine::Ignored;
           }
         }
       }
-      return false;
+      return DiagLevel;
     });
     Preprocessor *PP = &Clang->getPreprocessor();
     for (const auto &Check : CTChecks) {
       // FIXME: the PP callbacks skip the entire preamble.
       // Checks that want to see #includes in the main file do not see them.
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
       Check->registerMatchers(&CTFinder);
     }

clang-tools-extra/trunk/clangd/Diagnostics.h

public:
 
   void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
   void EndSourceFile() override;
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 
   using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
                                                    const clang::Diagnostic &)>;
-  using DiagFilter =
-      std::function<bool(DiagnosticsEngine::Level, const clang::Diagnostic &)>;
+  using LevelAdjuster = std::function<DiagnosticsEngine::Level(
+      DiagnosticsEngine::Level, const clang::Diagnostic &)>;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
-  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
-  void suppressDiagnostics(DiagFilter SuppressionFilter) {
-    this->SuppressionFilter = SuppressionFilter;
-  }
+  /// If set, this allows the client of this class to adjust the level of
+  /// diagnostics, such as promoting warnings to errors, or ignoring
+  /// diagnostics.
+  void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
-  DiagFilter SuppressionFilter = nullptr;
+  LevelAdjuster Adjuster = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
   llvm::DenseSet<int> IncludeLinesWithErrors;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/trunk/clangd/Diagnostics.cpp

auto AddFix = [&](bool SyntheticMessage) -> bool {
     LastDiag->Fixes.push_back(Fix{Message.str(), std::move(Edits)});
     return true;
   };
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
-    if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+    if (Adjuster) {
+      DiagLevel = Adjuster(DiagLevel, Info);
+      if (DiagLevel == DiagnosticsEngine::Ignored) {
         LastPrimaryDiagnosticWasSuppressed = true;
         return;
       }
+    }
     LastPrimaryDiagnosticWasSuppressed = false;
 
     LastDiag = Diag();
     LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
     adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
 
     if (!Info.getFixItHints().empty())

clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

*result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
                             .str();
     return false;
   }
   return true;
 }
 
 MATCHER_P(DiagSource, S, "") { return arg.Source == S; }
 MATCHER_P(DiagName, N, "") { return arg.Name == N; }
+MATCHER_P(DiagSeverity, S, "") { return arg.Severity == S; }
 
 MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   if (arg.Message != Fix.Message)
     return false;
   if (arg.Edits.size() != Fix.Edits.size())
     return false;
   for (std::size_t I = 0; I < arg.Edits.size(); ++I) {
     if (arg.Edits[I].range != Fix.Edits[I].range ||
TEST(DiagnosticTest, ClangTidySuppressionComment) {
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "result of integer division used in a floating "
                              "point context; possible loss of precision"),
           DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"))));
 }
 
+TEST(DiagnosticTest, ClangTidyWarningAsError) {
+  Annotations Main(R"cpp(
+    int main() {
+      int i = 3;
+      double f = [[8]] / i;
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(::testing::AllOf(
+          Diag(Main.range(), "result of integer division used in a floating "
+                             "point context; possible loss of precision"),
+          DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+          DiagSeverity(DiagnosticsEngine::Error))));
+}
+
+TEST(DiagnosticTest, ClangTidyWarningAsErrorTrumpsSuppressionComment) {
+  Annotations Main(R"cpp(
+    int main() {
+      int i = 3;
+      double f = [[8]] / i;  // NOLINT
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(::testing::AllOf(
+          Diag(Main.range(), "result of integer division used in a floating "
+                             "point context; possible loss of precision"),
+          DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+          DiagSeverity(DiagnosticsEngine::Error))));
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO

clang-tools-extra/trunk/clangd/unittests/TestTU.h

struct TestTU {
 
   // Name and contents of each file.
   llvm::StringMap<std::string> AdditionalFiles;
 
   // Extra arguments for the compiler invocation.
   std::vector<const char *> ExtraArgs;
 
   llvm::Optional<std::string> ClangTidyChecks;
+  llvm::Optional<std::string> ClangTidyWarningsAsErrors;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
   ParsedAST build() const;
   SymbolSlab headerSymbols() const;

clang-tools-extra/trunk/clangd/unittests/TestTU.cpp

ParsedAST TestTU::build() const {
   ParseInputs Inputs;
   Inputs.CompileCommand.Filename = FullFilename;
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
+  Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
     Inputs.Opts.SuggestMissingIncludes = true;
   auto CI = buildCompilerInvocation(Inputs);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
       buildPreamble(FullFilename, *CI,
                     /*OldPreamble=*/nullptr,