[clangd] Respect WarningsAsErrors configuration for clang-tidy

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

Reviewed By: sammccall

Patch by Nathan Ridge!

Differential Revision: https://reviews.llvm.org/D61841

llvm-svn: 361113

clang-tools-extra/clangd/ClangdUnit.cpp

@@ -332,14 +332,22 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
     CTContext->setASTContext(&Clang->getASTContext());
     CTContext->setCurrentFile(MainInput.getFile());
     CTFactories.createChecks(CTContext.getPointer(), CTChecks);
-    ASTDiags.suppressDiagnostics([&CTContext](
-                                     DiagnosticsEngine::Level DiagLevel,
-                                     const clang::Diagnostic &Info) {
+    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.
@@ -350,11 +358,11 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
           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) {

clang-tools-extra/clangd/Diagnostics.cpp

@@ -514,9 +514,12 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     // Handle the new main diagnostic.
     flushLastDiag();
 
-    if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
-      LastPrimaryDiagnosticWasSuppressed = true;
-      return;
+    if (Adjuster) {
+      DiagLevel = Adjuster(DiagLevel, Info);
+      if (DiagLevel == DiagnosticsEngine::Ignored) {
+        LastPrimaryDiagnosticWasSuppressed = true;
+        return;
+      }
     }
     LastPrimaryDiagnosticWasSuppressed = false;
 

clang-tools-extra/clangd/Diagnostics.h

@@ -128,20 +128,20 @@ public:
 
   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;

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

@@ -73,6 +73,7 @@ MATCHER_P(EqualToLSPDiag, LSPDiag,
 
 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)
@@ -227,6 +228,44 @@ TEST(DiagnosticTest, ClangTidySuppressionComment) {
           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:

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

@@ -48,6 +48,7 @@ ParsedAST TestTU::build() const {
   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;

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

@@ -57,6 +57,7 @@ struct TestTU {
   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;