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 @@ -233,12 +233,69 @@ std::vector MainFileTokens; }; +// Filter for clang diagnostics groups enabled by CTOptions.Checks. +// +// These are check names like clang-diagnostics-unused. +// Note that unlike -Wunused, clang-diagnostics-unused does not imply +// subcategories like clang-diagnostics-unused-function. +// +// This is used to determine which diagnostics can be enabled by ExtraArgs in +// the clang-tidy configuration. +class TidyDiagnosticGroups { + // Whether all diagnostic groups are enabled by default. + // True if we've seen clang-diagnostic-*. + bool Default = false; + // Set of diag::Group whose enablement != Default. + // If Default is false, this is foo where we've seen clang-diagnostic-foo. + llvm::DenseSet Exceptions; + +public: + TidyDiagnosticGroups(llvm::StringRef Checks) { + constexpr llvm::StringLiteral CDPrefix = "clang-diagnostic-"; + + llvm::StringRef Check; + while (!Checks.empty()) { + std::tie(Check, Checks) = Checks.split(','); + if (Check.empty()) + continue; + + bool Enable = !Check.consume_front("-"); + bool Glob = Check.consume_back("*"); + if (Glob) { + // Is this clang-diagnostic-*, or *, or so? + // (We ignore all other types of globs). + if (CDPrefix.startswith(Check)) { + Default = Enable; + Exceptions.clear(); + } + continue; + } + + // In "*,clang-diagnostic-foo", the latter is a no-op. + if (Default == Enable) + continue; + // The only non-glob entries we care about are clang-diagnostic-foo. + if (!Check.consume_front(CDPrefix)) + continue; + + if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check)) + Exceptions.insert(static_cast(*Group)); + } + } + + bool operator()(diag::Group GroupID) const { + return Exceptions.contains(static_cast(GroupID)) ? !Default + : Default; + } +}; + // Find -W and -Wno- options in ExtraArgs and apply them to Diags. // // This is used to handle ExtraArgs in clang-tidy configuration. // We don't use clang's standard handling of this as we want slightly different // behavior (e.g. we want to exclude these from -Wno-error). void applyWarningOptions(llvm::ArrayRef ExtraArgs, + llvm::function_ref EnabledGroups, DiagnosticsEngine &Diags) { for (llvm::StringRef Group : ExtraArgs) { // Only handle args that are of the form -W[no-]. @@ -259,6 +316,9 @@ if (Enable) { if (Diags.getDiagnosticLevel(ID, SourceLocation()) < DiagnosticsEngine::Warning) { + auto Group = DiagnosticIDs::getGroupForDiag(ID); + if (!Group || !EnabledGroups(*Group)) + continue; Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation()); if (Diags.getWarningsAsErrors()) NeedsWerrorExclusion = true; @@ -354,18 +414,25 @@ // - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings // - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out // - // We treat these as clang warnings, so the Checks part is not relevant. - // We must enable the warnings specified in ExtraArgs. + // In clang-tidy, diagnostics are emitted if they pass both checks. + // When groups contain subgroups, -Wparent includes the child, but + // clang-diagnostic-parent does not. // - // We *don't* want to change the compile command directly. this can have + // We *don't* want to change the compile command directly. This can have // too many unexpected effects: breaking the command, interactions with // -- and -Werror, etc. Besides, we've already parsed the command. // Instead we parse the -W flags and handle them directly. + // + // Similarly, we don't want to use Checks to filter clang diagnostics after + // they are generated, as this spreads clang-tidy emulation everywhere. + // Instead, we just use these to filter which extra diagnostics we enable. auto &Diags = Clang->getDiagnostics(); + TidyDiagnosticGroups TidyGroups(ClangTidyOpts.Checks ? *ClangTidyOpts.Checks + : llvm::StringRef()); if (ClangTidyOpts.ExtraArgsBefore) - applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags); + applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, TidyGroups, Diags); if (ClangTidyOpts.ExtraArgs) - applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags); + applyWarningOptions(*ClangTidyOpts.ExtraArgs, TidyGroups, Diags); } else { // Skips some analysis. Clang->getDiagnosticOpts().IgnoreWarnings = true; 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 @@ -517,13 +517,16 @@ diagSeverity(DiagnosticsEngine::Error))))); } -TidyProvider addClangArgs(std::vector ExtraArgs) { - return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts, - llvm::StringRef) { +TidyProvider addClangArgs(std::vector ExtraArgs, + llvm::StringRef Checks) { + return [ExtraArgs = std::move(ExtraArgs), Checks = Checks.str()]( + tidy::ClangTidyOptions &Opts, llvm::StringRef) { if (!Opts.ExtraArgs) Opts.ExtraArgs.emplace(); for (llvm::StringRef Arg : ExtraArgs) Opts.ExtraArgs->emplace_back(Arg); + if (!Checks.empty()) + Opts.Checks = Checks; }; } @@ -541,53 +544,78 @@ // Check the -Wunused warning isn't initially on. EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); - // We enable warnings based on clang-tidy extra args. - TU.ClangTidyProvider = addClangArgs({"-Wunused"}); + // We enable warnings based on clang-tidy extra args, if the matching + // clang-diagnostic- is there. + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function"); + EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning)); + + // clang-diagnostic-* is acceptable + TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-*"); EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning)); + // And plain * + TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*"); + EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning)); + // And we can explicitly exclude a category too. + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, "*,-clang-diagnostic-unused-function"); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); - // But we don't respect other args. - TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"}); + // Without the exact check specified, the warnings are not enabled. + TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-unused"); + EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); + + // We don't respect other args. + TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"}, + "clang-diagnostic-unused-function"); EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning)) << "Not unused function 'bar'!"; // -Werror doesn't apply to warnings enabled by clang-tidy extra args. TU.ExtraArgs = {"-Werror"}; - TU.ClangTidyProvider = addClangArgs({"-Wunused"}); + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function"); EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(diagSeverity(DiagnosticsEngine::Warning))); // But clang-tidy extra args won't *downgrade* errors to warnings either. TU.ExtraArgs = {"-Wunused", "-Werror"}; - TU.ClangTidyProvider = addClangArgs({"-Wunused"}); + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function"); EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(diagSeverity(DiagnosticsEngine::Error))); // FIXME: we're erroneously downgrading the whole group, this should be Error. TU.ExtraArgs = {"-Wunused-function", "-Werror"}; - TU.ClangTidyProvider = addClangArgs({"-Wunused"}); + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, "clang-diagnostic-unused-label"); EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(diagSeverity(DiagnosticsEngine::Warning))); // This looks silly, but it's the typical result if a warning is enabled by a // high-level .clang-tidy file and disabled by a low-level one. TU.ExtraArgs = {}; - TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"}); + TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"}, + "clang-diagnostic-unused-function"); EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); // Overriding only works in the proper order. - TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"}); + TU.ClangTidyProvider = + addClangArgs({"-Wunused"}, {"clang-diagnostic-unused-function"}); EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1)); // More specific vs less-specific: match clang behavior - TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"}); + TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"}, + {"clang-diagnostic-unused-function"}); EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); - TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"}); + TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"}, + {"clang-diagnostic-unused-function"}); EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); - // We do allow clang-tidy config to disable warnings from the compile command. - // It's unclear this is ideal, but it's hard to avoid. + // We do allow clang-tidy config to disable warnings from the compile + // command. It's unclear this is ideal, but it's hard to avoid. TU.ExtraArgs = {"-Wunused"}; - TU.ClangTidyProvider = addClangArgs({"-Wno-unused"}); + TU.ClangTidyProvider = addClangArgs({"-Wno-unused"}, {}); EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); } diff --git a/clang/include/clang/Basic/DiagnosticIDs.h b/clang/include/clang/Basic/DiagnosticIDs.h --- a/clang/include/clang/Basic/DiagnosticIDs.h +++ b/clang/include/clang/Basic/DiagnosticIDs.h @@ -231,6 +231,14 @@ /// "deprecated-declarations". static StringRef getWarningOptionForGroup(diag::Group); + /// Given a group ID, returns the flag that toggles the group. + /// For example, for "deprecated-declarations", returns + /// Group::DeprecatedDeclarations. + static llvm::Optional getGroupForWarningOption(StringRef); + + /// Return the lowest-level group that contains the specified diagnostic. + static llvm::Optional getGroupForDiag(unsigned DiagID); + /// Return the lowest-level warning option that enables the specified /// diagnostic. /// diff --git a/clang/lib/Basic/DiagnosticIDs.cpp b/clang/lib/Basic/DiagnosticIDs.cpp --- a/clang/lib/Basic/DiagnosticIDs.cpp +++ b/clang/lib/Basic/DiagnosticIDs.cpp @@ -628,13 +628,27 @@ return OptionTable[static_cast(Group)].getName(); } +llvm::Optional +DiagnosticIDs::getGroupForWarningOption(StringRef Name) { + const auto *Found = llvm::partition_point( + OptionTable, [=](const WarningOption &O) { return O.getName() < Name; }); + if (Found == std::end(OptionTable) || Found->getName() != Name) + return llvm::None; + return static_cast(Found - OptionTable); +} + +llvm::Optional DiagnosticIDs::getGroupForDiag(unsigned DiagID) { + if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID)) + return static_cast(Info->getOptionGroupIndex()); + return llvm::None; +} + /// getWarningOptionForDiag - Return the lowest-level warning option that /// enables the specified diagnostic. If there is no -Wfoo flag that controls /// the diagnostic, this returns null. StringRef DiagnosticIDs::getWarningOptionForDiag(unsigned DiagID) { - if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID)) - return getWarningOptionForGroup( - static_cast(Info->getOptionGroupIndex())); + if (auto G = getGroupForDiag(DiagID)) + return getWarningOptionForGroup(*G); return StringRef(); } @@ -683,12 +697,10 @@ bool DiagnosticIDs::getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group, SmallVectorImpl &Diags) const { - auto Found = llvm::partition_point( - OptionTable, [=](const WarningOption &O) { return O.getName() < Group; }); - if (Found == std::end(OptionTable) || Found->getName() != Group) - return true; // Option not found. - - return ::getDiagnosticsInGroup(Flavor, Found, Diags); + if (llvm::Optional G = getGroupForWarningOption(Group)) + return ::getDiagnosticsInGroup( + Flavor, &OptionTable[static_cast(*G)], Diags); + return true; } void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,