[clangd] Fix feature modules to drop diagnostics

[kadircet]

Summary

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.

[kadircet]

Exit after introducing cleanup function

[kadircet]

Get rid of LastDiagWasSuppressed state in StoreDiags

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

732–733

If I'm reading this right:

• we previously discarded the diagnostic "quickly" without creating a Diag
• now, we construct the Diag as normal but never emit it
• ... but we still use the old early-exit codepath for notes, if the primary diagnostic will be discarded

The new scheme seems functionally correct, but it potentially constructs more Diags. Question is, is it ever enough to care about performance?

I believe *warnings in headers* are ultimately filtered out by the adjuster and are now extra work/allocations after this patch. Mostly this only affects preamble (exceptions: things like llvm tablegen includes) which is already slow, maybe it doesn't matter.

If you want to avoid regressing it, I'd suggest keeping the early bail out here, but instead of setting LastPrimaryDiagnosticWasSuppressed, just construct an empty Diag and set its level to Ignored. Rest of the logic can stay the same I think.

Doesn't seem like much extra code, but if you don't think it's worthwhile that's fine too of course.

789

(Hmm, the isExcluded case seems like it belongs next to the level adjuster logic rather than at the end.)

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

732–733

I also had the same hesitation, but it looked like this is only suppressing diags based on config and tidy suppression comments (ones from header are still handled at flush).

So didn't feel like a big deal, But still it can be regression in cases when these diags have some complicated fixes attached to them (we'll compute them just for dropping on the floor).

The main reasoning for dropping the early exit was because I wanted the logic to look more uniform between diagcb and adjusters (to not think about it again when the day comes and we merge the two).

So how do you feel about filling in the diagbase in any case (which has some string copies, especially around diag message), then running Adjuster, DiagCB and isExcluded, and after that we perform early return before adding fixes or running include fixer?

789

i agree, but didn't want to raise eyebrows :P moving it next to adjuster logic.

[kadircet]

• Bail out early before filling in diag info
• Move isExcluded check into handleDiagnostics, rather than handling it during

flushing

[sammccall]

LG, I'm a bit sad to see we're not able to skip much work in the diagnostics-outside-main-file case but that's a separate patch.

clang-tools-extra/clangd/Diagnostics.h

private:
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
   llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
   bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
   SourceManager *OrigSrcMgr = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
-  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
 bool isBuiltinDiagnosticSuppressed(unsigned ID,
                                    const llvm::StringSet<> &Suppressed);
 /// Take a user-specified diagnostic code, and convert it to a normalized form
 /// stored in the config and consumed by isBuiltinDiagnosticsSuppressed.
 ///
 /// (This strips err_ and -W prefix so we can match with or without them.)
 llvm::StringRef normalizeSuppressedCode(llvm::StringRef);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/clangd/Diagnostics.cpp

if (!D.Fixes.empty())
     return true;
   for (auto &N : D.Notes) {
     if (N.InsideMainFile)
       return true;
   }
   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;
 }
 
 // Checks whether a location is within a half-open range.
 // Note that clang also uses closed source ranges, which this can't handle!
 bool locationInRange(SourceLocation L, CharSourceRange R,
                      const SourceManager &M) {
LastDiag->Fixes.push_back(
         Fix{std::string(Message.str()), std::move(Edits)});
     return true;
   };
 
   if (!isNote(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.
       if (!AddFix(false /* use the note as the message */))
         IgnoreDiagnostics::log(DiagLevel, Info);
     } else {
       // A clang note without fix-its corresponds to clangd::Note.
       Note N;
       FillDiagBase(N);
 
       LastDiag->Notes.push_back(std::move(N));
     }
   }
 }
 
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
     return;
   auto Finish = llvm::make_scope_exit([&, NDiags(Output.size())] {
     if (Output.size() == NDiags) // No new diag emitted.
       vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
     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) {
     if (tryMoveToMainFile(*LastDiag, *LastDiagLoc)) {
       // Suppress multiple errors from the same inclusion.
       if (!IncludedErrorLocations
                .insert({LastDiag->Range.start.line,
                         LastDiag->Range.start.character})

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

 //===--- FeatureModulesTests.cpp  -------------------------------*- 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 "Annotations.h"
 #include "FeatureModule.h"
 #include "Selection.h"
 #include "TestTU.h"
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
auto Tree =
       SelectionTree::createRight(AST.getASTContext(), AST.getTokens(), 0, 0);
   auto Actual = prepareTweak(
       TweakID, Tweak::Selection(nullptr, AST, 0, 0, std::move(Tree), nullptr),
       &Set);
   ASSERT_TRUE(bool(Actual));
   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