diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -217,6 +217,23 @@ bool AllowEnablingAnalyzerAlphaCheckers; }; +/// Check whether a given diagnostic should be suppressed due to the presence +/// of a "NOLINT" suppression comment. +/// This is exposed so that other tools that present clang-tidy diagnostics +/// (such as clangd) can respect the same suppression rules as clang-tidy. +/// This does not handle suppression of notes following a suppressed diagnostic; +/// that is left to the caller is it requires maintaining state in between calls +/// to this function. +/// The `CheckMacroExpansion` parameter determines whether the function should +/// handle the case where the diagnostic is inside a macro expansion. A degree +/// of control over this is needed because handling this case can require +/// examining source files other than the one in which the diagnostic is +/// located, and in some use cases we cannot rely on such other files being +/// mapped in the SourceMapper. +bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel, + const Diagnostic &Info, ClangTidyContext &Context, + bool CheckMacroExpansion = true); + /// \brief A diagnostic consumer that turns each \c Diagnostic into a /// \c SourceManager-independent \c ClangTidyError. // @@ -246,7 +263,7 @@ /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter /// according to the diagnostic \p Location. - void checkFilters(SourceLocation Location, const SourceManager& Sources); + void checkFilters(SourceLocation Location, const SourceManager &Sources); bool passesLineFilter(StringRef FileName, unsigned LineNumber) const; ClangTidyContext &Context; diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -166,11 +166,13 @@ bool contains(StringRef S) { switch (auto &Result = Cache[S]) { - case Yes: return true; - case No: return false; - case None: - Result = Globs.contains(S) ? Yes : No; - return Result == Yes; + case Yes: + return true; + case No: + return false; + case None: + Result = Globs.contains(S) ? Yes : No; + return Result == Yes; } llvm_unreachable("invalid enum"); } @@ -387,16 +389,30 @@ return false; } +namespace clang { +namespace tidy { + +bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel, + const Diagnostic &Info, ClangTidyContext &Context, + bool CheckMacroExpansion) { + return Info.getLocation().isValid() && + DiagLevel != DiagnosticsEngine::Error && + DiagLevel != DiagnosticsEngine::Fatal && + (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro + : LineIsMarkedWithNOLINT)(Info.getSourceManager(), + Info.getLocation(), + Info.getID(), Context); +} + +} // namespace tidy +} // namespace clang + void ClangTidyDiagnosticConsumer::HandleDiagnostic( DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) { if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note) return; - if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error && - DiagLevel != DiagnosticsEngine::Fatal && - LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(), - Info.getLocation(), Info.getID(), - Context)) { + if (ShouldSuppressDiagnostic(DiagLevel, Info, Context)) { ++Context.Stats.ErrorsIgnoredNOLINT; // Ignored a warning, should ignore related notes as well LastErrorWasIgnored = true; diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp --- a/clang-tools-extra/clangd/ClangdUnit.cpp +++ b/clang-tools-extra/clangd/ClangdUnit.cpp @@ -113,12 +113,12 @@ } void EndOfMainFile() { - for (const auto& Entry : MainFileMacros) + for (const auto &Entry : MainFileMacros) Out->push_back(Entry.getKey()); llvm::sort(*Out); } - private: +private: const SourceManager &SM; bool InMainFile = true; llvm::StringSet<> MainFileMacros; @@ -275,6 +275,39 @@ const LangOptions &LangOpts; }; +// A wrapper around StoreDiags to handle suppression comments for +// clang-tidy diagnostics (and possibly other clang-tidy customizations in the +// future). +class ClangdDiagnosticConsumer : public StoreDiags { +public: + ClangdDiagnosticConsumer() { + suppressDiagnostics([this](DiagnosticsEngine::Level DiagLevel, + const clang::Diagnostic &Info) { + if (CTContext) { + bool IsClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty(); + // Skip the ShouldSuppressDiagnostic 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. + if (IsClangTidyDiag && isInsideMainFile(Info) && + tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext, + /* CheckMacroExpansion = */ false)) { + return true; + } + } + return false; + }); + } + + void setClangTidyContext(tidy::ClangTidyContext *CTContext) { + this->CTContext = CTContext; + } + +private: + tidy::ClangTidyContext *CTContext = nullptr; +}; + } // namespace void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS) { @@ -294,7 +327,7 @@ const PrecompiledPreamble *PreamblePCH = Preamble ? &Preamble->Preamble : nullptr; - StoreDiags ASTDiags; + ClangdDiagnosticConsumer ASTDiags; std::string Content = Buffer->getBuffer(); auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH, @@ -332,6 +365,7 @@ CTContext->setASTContext(&Clang->getASTContext()); CTContext->setCurrentFile(MainInput.getFile()); CTFactories.createChecks(CTContext.getPointer(), CTChecks); + ASTDiags.setClangTidyContext(CTContext.getPointer()); Preprocessor *PP = &Clang->getPreprocessor(); for (const auto &Check : CTChecks) { // FIXME: the PP callbacks skip the entire preamble. 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 @@ -112,6 +112,9 @@ /// Convert from clang diagnostic level to LSP severity. int getSeverity(DiagnosticsEngine::Level L); +/// Check if a diagnostic is inside the main file. +bool isInsideMainFile(const clang::Diagnostic &D); + /// StoreDiags collects the diagnostics that can later be reported by /// clangd. It groups all notes for a diagnostic into a single Diag /// and filters out diagnostics that don't mention the main file (i.e. neither @@ -128,17 +131,25 @@ using DiagFixer = std::function(DiagnosticsEngine::Level, const clang::Diagnostic &)>; + using DiagFilter = + std::function; /// 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; + } private: void flushLastDiag(); DiagFixer Fixer = nullptr; + DiagFilter SuppressionFilter = nullptr; std::vector Output; llvm::Optional LangOpts; llvm::Optional LastDiag; llvm::DenseSet IncludeLinesWithErrors; + bool LastPrimaryDiagnosticWasSuppressed = false; }; } // namespace clangd 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 @@ -146,6 +146,8 @@ return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc)); } +} // namespace + bool isInsideMainFile(const clang::Diagnostic &D) { if (!D.hasSourceManager()) return false; @@ -153,6 +155,8 @@ return isInsideMainFile(D.getLocation(), D.getSourceManager()); } +namespace { + bool isNote(DiagnosticsEngine::Level L) { return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark; } @@ -514,6 +518,12 @@ // Handle the new main diagnostic. flushLastDiag(); + if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) { + LastPrimaryDiagnosticWasSuppressed = true; + return; + } + LastPrimaryDiagnosticWasSuppressed = false; + LastDiag = Diag(); LastDiag->ID = Info.getID(); FillDiagBase(*LastDiag); @@ -528,6 +538,13 @@ } } 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); 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 @@ -767,6 +767,24 @@ "a type specifier for all declarations"), WithNote(Diag(Header.range(), "error occurred here"))))); } + +TEST(ClangTidy, SuppressionComment) { + Annotations Main(R"cpp( + int main() { + int i = 3; + double d = 8 / i; // NOLINT + // NOLINTNEXTLINE + double e = 8 / i; + double f = [[8]] / i; + } + )cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.ClangTidyChecks = "bugprone-integer-division"; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(Diag( + Main.range(), "result of integer division used in a floating " + "point context; possible loss of precision"))); +} } // namespace } // namespace clangd