[clangd] Pull suppression logic into common path, apply for driver diagnostics

[kadircet]

Summary

Fixes https://github.com/clangd/clangd/issues/1142

[kadircet]

Just pass optional directly

[kadircet]

ping

clang-tools-extra/clangd/Diagnostics.h

private:
   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;
 };
 
-/// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
-bool isBuiltinDiagnosticSuppressed(unsigned ID,
-                                   const llvm::StringSet<> &Suppressed,
-                                   const LangOptions &);
 /// 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

 //===--- Diagnostics.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 "Diagnostics.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "Compiler.h"
+#include "Config.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "support/Logger.h"
 #include "clang/Basic/AllDiagnostics.h" // IWYU pragma: keep
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
void setTags(clangd::Diag &D) {
   };
   if (DeprecatedDiags->contains(D.ID)) {
     D.Tags.push_back(DiagnosticTag::Deprecated);
   } else if (UnusedDiags->contains(D.ID)) {
     D.Tags.push_back(DiagnosticTag::Unnecessary);
   }
   // FIXME: Set tags for tidy-based diagnostics too.
 }
+
+// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
+bool isBuiltinDiagnosticSuppressed(unsigned ID,
+                                   const llvm::StringSet<> &Suppress,
+                                   llvm::Optional<LangOptions> LO) {
+  // Don't complain about header-only stuff in mainfiles if it's a header.
+  // FIXME: would be cleaner to suppress in clang, once we decide whether the
+  //        behavior should be to silently-ignore or respect the pragma.
+  if (LO && ID == diag::pp_pragma_sysheader_in_main_file && LO->IsHeaderFile)
+    return true;
+
+  if (const char *CodePtr = getDiagnosticCode(ID)) {
+    if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
+      return true;
+  }
+  StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
+  if (!Warning.empty() && Suppress.contains(Warning))
+    return true;
+  return false;
+}
 } // namespace
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D) {
   OS << "[";
   if (!D.InsideMainFile)
     OS << D.File << ":";
   OS << D.Range.start << "-" << D.Range.end << "] ";
 
if (OrigSrcMgr && Info.hasSourceManager() &&
     IgnoreDiagnostics::log(DiagLevel, Info);
     return;
   }
 
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   bool OriginallyError =
       Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
           Info.getID());
+  if (!isNote(DiagLevel)) {
+    const Config &Cfg = Config::current();
+    // Check if diagnostics is suppressed (possibly by user), before doing any
+    // adjustments.
+    if (Cfg.Diagnostics.SuppressAll ||
+        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                      LangOpts)) {
+      DiagLevel = DiagnosticsEngine::Ignored;
+    } else if (Adjuster) {
+      // FIXME: Merge with feature modules.
+      DiagLevel = Adjuster(DiagLevel, Info);
+    }
+  }
 
   if (Info.getLocation().isInvalid()) {
     // Handle diagnostics coming from command-line arguments. The source manager
     // is *not* available at this point, so we cannot use it.
     if (!OriginallyError) {
       IgnoreDiagnostics::log(DiagLevel, Info);
       return; // non-errors add too much noise, do not show them.
     }
auto AddFix = [&](bool SyntheticMessage) -> bool {
     return true;
   };
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
     LastDiag = Diag();
-    // FIXME: Merge with feature modules.
-    if (Adjuster)
-      DiagLevel = Adjuster(DiagLevel, Info);
 
     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)
if (tryMoveToMainFile(*LastDiag, *LastDiagLoc)) {
         return;
     }
   }
   if (!mentionsMainFile(*LastDiag))
     return;
   Output.push_back(std::move(*LastDiag));
 }
 
-bool isBuiltinDiagnosticSuppressed(unsigned ID,
-                                   const llvm::StringSet<> &Suppress,
-                                   const LangOptions &LangOpts) {
-  // Don't complain about header-only stuff in mainfiles if it's a header.
-  // FIXME: would be cleaner to suppress in clang, once we decide whether the
-  //        behavior should be to silently-ignore or respect the pragma.
-  if (ID == diag::pp_pragma_sysheader_in_main_file && LangOpts.IsHeaderFile)
-    return true;
-
-  if (const char *CodePtr = getDiagnosticCode(ID)) {
-    if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
-      return true;
-  }
-  StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
-  if (!Warning.empty() && Suppress.contains(Warning))
-    return true;
-  return false;
-}
-
 llvm::StringRef normalizeSuppressedCode(llvm::StringRef Code) {
   Code.consume_front("err_");
   Code.consume_front("-W");
   return Code;
 }
 
 llvm::Optional<std::string> getDiagnosticDocURI(Diag::DiagSource Source,
                                                 unsigned ID,

clang-tools-extra/clangd/ParsedAST.cpp

for (auto ID : {diag::ext_implicit_function_decl_c99,
                     diag::warn_implicit_function_decl}) {
       OverriddenSeverity.try_emplace(
           ID, Clang->getDiagnostics().getDiagnosticLevel(ID, SourceLocation()));
       Clang->getDiagnostics().setSeverity(ID, diag::Severity::Error,
                                           SourceLocation());
     }
 
     const Config &Cfg = Config::current();
-    ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
-                                  const clang::Diagnostic &Info) {
-      if (Cfg.Diagnostics.SuppressAll ||
-          isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
-                                        Clang->getLangOpts()))
-        return DiagnosticsEngine::Ignored;
-
+    ASTDiags.setLevelAdjuster(
+        [&](DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) {
           auto It = OverriddenSeverity.find(Info.getID());
           if (It != OverriddenSeverity.end())
             DiagLevel = It->second;
 
           if (!CTChecks.empty()) {
             std::string CheckName = CTContext->getCheckName(Info.getID());
             bool IsClangTidyDiag = !CheckName.empty();
             if (IsClangTidyDiag) {
               if (Cfg.Diagnostics.Suppress.contains(CheckName))
                 return DiagnosticsEngine::Ignored;
-          // 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 to avoid I/O.
-          // We let suppression comments take precedence over warning-as-error
-          // to match clang-tidy's behaviour.
+              // 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 to avoid I/O. We let
+              // suppression comments take precedence over warning-as-error to
+              // match clang-tidy's behaviour.
               bool IsInsideMainFile =
                   Info.hasSourceManager() &&
                   isInsideMainFile(Info.getLocation(), Info.getSourceManager());
               SmallVector<tooling::Diagnostic, 1> TidySuppressedErrors;
               if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic(
                                           DiagLevel, Info, TidySuppressedErrors,
                                           /*AllowIO=*/false,
                                           /*EnableNolintBlocks=*/true)) {
                 // FIXME: should we expose the suppression error (invalid use of
                 // NOLINT comments)?
                 return DiagnosticsEngine::Ignored;
               }
 
               // Check for warning-as-error.
               if (DiagLevel == DiagnosticsEngine::Warning &&
                   CTContext->treatAsError(CheckName)) {
                 return DiagnosticsEngine::Error;
               }
             }
           }
           return DiagLevel;
         });
 
     // Add IncludeFixer which can recover diagnostics caused by missing includes
     // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
     auto BuildDir = VFS->getCurrentWorkingDirectory();
     if (Inputs.Index && !BuildDir.getError()) {
       auto Style =
           getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS);
       auto Inserter = std::make_shared<IncludeInserter>(

clang-tools-extra/clangd/Preamble.cpp

buildPreamble(PathRef FileName, CompilerInvocation CI,
   PreambleDiagnostics.setDiagCallback(
       [&ASTListeners](const clang::Diagnostic &D, clangd::Diag &Diag) {
         llvm::for_each(ASTListeners,
                        [&](const auto &L) { L->sawDiagnostic(D, Diag); });
       });
   llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnostics, false);
-  const Config &Cfg = Config::current();
-  PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
-                                           const clang::Diagnostic &Info) {
-    if (Cfg.Diagnostics.SuppressAll ||
-        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
-                                      *CI.getLangOpts()))
-      return DiagnosticsEngine::Ignored;
+  PreambleDiagnostics.setLevelAdjuster(
+      [&](DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) {
         switch (Info.getID()) {
         case diag::warn_no_newline_eof:
         case diag::warn_cxx98_compat_no_newline_eof:
         case diag::ext_no_newline_eof:
           // If the preamble doesn't span the whole file, drop the no newline at
           // eof warnings.
           return Bounds.Size != ContentsBuffer->getBufferSize()
                      ? DiagnosticsEngine::Level::Ignored
                      : DiagLevel;
         }
         return DiagLevel;
       });
 
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);
   CI.getFrontendOpts().SkipFunctionBodies = true;
   // We don't want to write comment locations into PCH. They are racy and slow
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;

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

 //===-- CompilerTests.cpp -------------------------------------------------===//
 //
 // 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 "Compiler.h"
+#include "Config.h"
+#include "Diagnostics.h"
 #include "TestTU.h"
 #include "clang/Frontend/DependencyOutputOptions.h"
 #include "clang/Frontend/FrontendOptions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
TEST(BuildCompilerInvocation, EmptyArgs) {
   IgnoreDiagnostics Diags;
   TestTU TU;
   auto Inputs = TU.inputs(FS);
   Inputs.CompileCommand.CommandLine.clear();
 
   // No crash.
   EXPECT_EQ(buildCompilerInvocation(Inputs, Diags), nullptr);
 }
+
+TEST(BuildCompilerInvocation, SuppressDiags) {
+  MockFS FS;
+  StoreDiags Diags;
+  TestTU TU;
+  TU.ExtraArgs = {"-funknown-arg"};
+  auto Inputs = TU.inputs(FS);
+
+  Config Cfg;
+  Cfg.Diagnostics.Suppress = {"drv_unknown_argument"};
+  WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+  EXPECT_NE(buildCompilerInvocation(Inputs, Diags), nullptr);
+  EXPECT_THAT(Diags.take(), IsEmpty());
+}
 } // namespace
 } // namespace clangd
 } // namespace clang

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

auto HeaderFilter = [this](llvm::StringRef Path) {
     }
     return false;
   };
   EXPECT_TRUE(HeaderFilter("foo.h"));
   EXPECT_FALSE(HeaderFilter("bar.h"));
 }
 
 TEST_F(ConfigCompileTests, DiagnosticSuppression) {
+  // Check that canonicalization and compilation works.
   Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
   Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
   Frag.Diagnostics.Suppress.emplace_back("-Wunused-variable");
   Frag.Diagnostics.Suppress.emplace_back("typecheck_bool_condition");
   Frag.Diagnostics.Suppress.emplace_back("err_unexpected_friend");
   Frag.Diagnostics.Suppress.emplace_back("warn_alloca");
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.Diagnostics.Suppress.keys(),
               UnorderedElementsAre("bugprone-use-after-move",
                                    "unreachable-code", "unused-variable",
                                    "typecheck_bool_condition",
                                    "unexpected_friend", "warn_alloca"));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
-      diag::warn_unreachable, Conf.Diagnostics.Suppress, LangOptions()));
-  // Subcategory not respected/suppressed.
-  EXPECT_FALSE(isBuiltinDiagnosticSuppressed(
-      diag::warn_unreachable_break, Conf.Diagnostics.Suppress, LangOptions()));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
-      diag::warn_unused_variable, Conf.Diagnostics.Suppress, LangOptions()));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_typecheck_bool_condition,
-                                            Conf.Diagnostics.Suppress,
-                                            LangOptions()));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
-      diag::err_unexpected_friend, Conf.Diagnostics.Suppress, LangOptions()));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
-      diag::warn_alloca, Conf.Diagnostics.Suppress, LangOptions()));
 
+  // Check that we treat * specially.
   Frag.Diagnostics.Suppress.emplace_back("*");
   EXPECT_TRUE(compileAndApply());
   EXPECT_TRUE(Conf.Diagnostics.SuppressAll);
   EXPECT_THAT(Conf.Diagnostics.Suppress, IsEmpty());
 }
 
 TEST_F(ConfigCompileTests, Tidy) {
   auto &Tidy = Frag.Diagnostics.ClangTidy;

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

EXPECT_THAT(
       *TU.build().getDiagnostics(),
       UnorderedElementsAre(AllOf(
           Diag(Source.range("diag"), "no matching function for call to 'foo'"),
           withFix(Fix(Source.range("fix"), "&",
                       "candidate function not viable: no known conversion from "
                       "'int' to 'int *' for 1st argument; take the address of "
                       "the argument with &")))));
 }
+
+TEST(DiagnosticsTest, DontSuppressSubcategories) {
+  Annotations Source(R"cpp(
+  /*error-ok*/
+    void bar(int x) {
+      switch(x) {
+      default:
+        break;
+        break;
+      }
+    })cpp");
+  TestTU TU;
+  TU.ExtraArgs.push_back("-Wunreachable-code-aggressive");
+  TU.Code = Source.code().str();
+  Config Cfg;
+  // This shouldn't suppress subcategory unreachable-break.
+  Cfg.Diagnostics.Suppress = {"unreachable-code"};
+  WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              ElementsAre(diagName("-Wunreachable-code-break")));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang