diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -179,10 +179,6 @@ llvm::DenseSet> 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. /// diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -9,6 +9,7 @@ #include "Diagnostics.h" #include "../clang-tidy/ClangTidyDiagnosticConsumer.h" #include "Compiler.h" +#include "Config.h" #include "Protocol.h" #include "SourceCode.h" #include "support/Logger.h" @@ -377,6 +378,26 @@ } // 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 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) { @@ -679,6 +700,19 @@ 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 @@ -793,9 +827,6 @@ flushLastDiag(); LastDiag = Diag(); - // FIXME: Merge with feature modules. - if (Adjuster) - DiagLevel = Adjuster(DiagLevel, Info); FillDiagBase(*LastDiag); if (isExcluded(LastDiag->ID)) @@ -882,25 +913,6 @@ 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"); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -502,51 +502,46 @@ } 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; - - 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. - bool IsInsideMainFile = - Info.hasSourceManager() && - isInsideMainFile(Info.getLocation(), Info.getSourceManager()); - SmallVector 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; + 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. + bool IsInsideMainFile = + Info.hasSourceManager() && + isInsideMainFile(Info.getLocation(), Info.getSourceManager()); + SmallVector 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; + } + } } - - // Check for warning-as-error. - if (DiagLevel == DiagnosticsEngine::Warning && - CTContext->treatAsError(CheckName)) { - return DiagnosticsEngine::Error; - } - } - } - return DiagLevel; - }); + return DiagLevel; + }); // Add IncludeFixer which can recover diagnostics caused by missing includes // (e.g. incomplete type) and attach include insertion fixes to diagnostics. diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -491,25 +491,20 @@ llvm::IntrusiveRefCntPtr 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; - 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; - }); + 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. diff --git a/clang-tools-extra/clangd/unittests/CompilerTests.cpp b/clang-tools-extra/clangd/unittests/CompilerTests.cpp --- a/clang-tools-extra/clangd/unittests/CompilerTests.cpp +++ b/clang-tools-extra/clangd/unittests/CompilerTests.cpp @@ -7,6 +7,8 @@ //===----------------------------------------------------------------------===// #include "Compiler.h" +#include "Config.h" +#include "Diagnostics.h" #include "TestTU.h" #include "clang/Frontend/DependencyOutputOptions.h" #include "clang/Frontend/FrontendOptions.h" @@ -113,6 +115,20 @@ // 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 diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -284,6 +284,7 @@ } 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"); @@ -296,21 +297,8 @@ "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); diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1870,6 +1870,27 @@ "'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