[clangd] Fix feature modules to drop diagnostics

Ignored diagnostics were only checked after level adjusters and assumed it would stay the same for the rest. But it can also be modified by FeatureModules. Differential Revision: https://reviews.llvm.org/D103387
matthiaskrgr · Jun 17, 2021 · 204014e · 204014e
1 parent b662651
commit 204014e
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 24 deletions.
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -76,10 +76,10 @@ bool mentionsMainFile(const Diag &D) {
   return false;
 }
 
-bool isExcluded(const Diag &D) {
+bool isExcluded(unsigned DiagID) {
   // clang will always fail parsing MS ASM, we don't link in desc + asm parser.
-  if (D.ID == clang::diag::err_msasm_unable_to_create_target ||
-      D.ID == clang::diag::err_msasm_unsupported_arch)
+  if (DiagID == clang::diag::err_msasm_unable_to_create_target ||
+      DiagID == clang::diag::err_msasm_unsupported_arch)
     return true;
   return false;
 }
@@ -726,44 +726,42 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
     // Handle the new main diagnostic.
     flushLastDiag();
 
-    if (Adjuster) {
+    LastDiag = Diag();
+    // FIXME: Merge with feature modules.
+    if (Adjuster)
       DiagLevel = Adjuster(DiagLevel, Info);
-      if (DiagLevel == DiagnosticsEngine::Ignored) {
-        LastPrimaryDiagnosticWasSuppressed = true;
-        return;
-      }
-    }
-    LastPrimaryDiagnosticWasSuppressed = false;
 
-    LastDiag = Diag();
     FillDiagBase(*LastDiag);
+    if (isExcluded(LastDiag->ID))
+      LastDiag->Severity = DiagnosticsEngine::Ignored;
+    if (DiagCB)
+      DiagCB(Info, *LastDiag);
+    // Don't bother filling in the rest if diag is going to be dropped.
+    if (LastDiag->Severity == DiagnosticsEngine::Ignored)
+      return;
+
     LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager());
     LastDiagOriginallyError = OriginallyError;
-
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
     if (Fixer) {
-      auto ExtraFixes = Fixer(DiagLevel, Info);
+      auto ExtraFixes = Fixer(LastDiag->Severity, Info);
       LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
                              ExtraFixes.end());
     }
-    if (DiagCB)
-      DiagCB(Info, *LastDiag);
   } else {
     // Handle a note to an existing diagnostic.
-
-    // If a diagnostic was suppressed due to the suppression filter,
-    // also suppress notes associated with it.
-    if (LastPrimaryDiagnosticWasSuppressed) {
-      return;
-    }
-
     if (!LastDiag) {
       assert(false && "Adding a note without main diagnostic");
       IgnoreDiagnostics::log(DiagLevel, Info);
       return;
     }
 
+    // If a diagnostic was suppressed due to the suppression filter,
+    // also suppress notes associated with it.
+    if (LastDiag->Severity == DiagnosticsEngine::Ignored)
+      return;
+
     if (!Info.getFixItHints().empty()) {
       // A clang note with fix-it is not a separate diagnostic in clangd. We
       // attach it as a Fix to the main diagnostic instead.
@@ -788,7 +786,7 @@ void StoreDiags::flushLastDiag() {
     LastDiag.reset();
   });
 
-  if (isExcluded(*LastDiag))
+  if (LastDiag->Severity == DiagnosticsEngine::Ignored)
     return;
   // Move errors that occur from headers into main file.
   if (!LastDiag->InsideMainFile && LastDiagLoc && LastDiagOriginallyError) {

diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
@@ -171,7 +171,6 @@ class StoreDiags : public DiagnosticConsumer {
   SourceManager *OrigSrcMgr = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
-  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.

diff --git a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "Annotations.h"
 #include "FeatureModule.h"
 #include "Selection.h"
 #include "TestTU.h"
@@ -53,6 +54,37 @@ TEST(FeatureModulesTest, ContributesTweak) {
   EXPECT_EQ(Actual->get()->id(), TweakID);
 }
 
+TEST(FeatureModulesTest, SuppressDiags) {
+  struct DiagModifierModule final : public FeatureModule {
+    struct Listener : public FeatureModule::ASTListener {
+      void sawDiagnostic(const clang::Diagnostic &Info,
+                         clangd::Diag &Diag) override {
+        Diag.Severity = DiagnosticsEngine::Ignored;
+      }
+    };
+    std::unique_ptr<ASTListener> astListeners() override {
+      return std::make_unique<Listener>();
+    };
+  };
+  FeatureModuleSet FMS;
+  FMS.add(std::make_unique<DiagModifierModule>());
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+
+  {
+    auto AST = TU.build();
+    EXPECT_THAT(*AST.getDiagnostics(), testing::Not(testing::IsEmpty()));
+  }
+
+  TU.FeatureModules = &FMS;
+  {
+    auto AST = TU.build();
+    EXPECT_THAT(*AST.getDiagnostics(), testing::IsEmpty());
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang