[clang-tools-extra] 816399c - Reland [clangd] More precisely enable clang warnings through ClangTidy options
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 30 02:08:16 PDT 2022
Author: Sam McCall
Date: 2022-04-30T11:07:11+02:00
New Revision: 816399cac2475499437c5a62e21a165f2de6460a
URL: https://github.com/llvm/llvm-project/commit/816399cac2475499437c5a62e21a165f2de6460a
DIFF: https://github.com/llvm/llvm-project/commit/816399cac2475499437c5a62e21a165f2de6460a.diff
LOG: Reland [clangd] More precisely enable clang warnings through ClangTidy options
This reverts commit 26c82f3d1de11cdada57e499b63a05d24e18b656.
When tests enable 'Checks: *', we may get extra diagnostics.
Added:
Modified:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang/include/clang/Basic/DiagnosticIDs.h
clang/lib/Basic/DiagnosticIDs.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 235362b5d599c..54d8e391cd94a 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -233,12 +233,69 @@ class ReplayPreamble : private PPCallbacks {
std::vector<syntax::Token> 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<unsigned> 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<unsigned>(*Group));
+ }
+ }
+
+ bool operator()(diag::Group GroupID) const {
+ return Exceptions.contains(static_cast<unsigned>(GroupID)) ? !Default
+ : Default;
+ }
+};
+
// Find -W<group> and -Wno-<group> 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
diff erent
// behavior (e.g. we want to exclude these from -Wno-error).
void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
+ llvm::function_ref<bool(diag::Group)> EnabledGroups,
DiagnosticsEngine &Diags) {
for (llvm::StringRef Group : ExtraArgs) {
// Only handle args that are of the form -W[no-]<group>.
@@ -259,6 +316,9 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
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 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
// - 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<group> 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
index 18c16190aeba9..6a5e341ceeccb 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -517,13 +517,16 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
diagSeverity(DiagnosticsEngine::Error)))));
}
-TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
- return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
- llvm::StringRef) {
+TidyProvider addClangArgs(std::vector<llvm::StringRef> 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 @@ TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
// 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 * (may turn on other checks too).
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*");
+ EXPECT_THAT(*TU.build().getDiagnostics(), Contains(UnusedFooWarning));
+ // And we can explicitly exclude a category too.
+ TU.ClangTidyProvider = addClangArgs(
+ {"-Wunused"}, "clang-diagnostic-*,-clang-diagnostic-unused-function");
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+ // Without the exact check specified, the warnings are not enabled.
+ TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-unused");
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
- // But we don't respect other args.
- TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
+ // 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
index 8139ffd375a28..f27bf356888c4 100644
--- a/clang/include/clang/Basic/DiagnosticIDs.h
+++ b/clang/include/clang/Basic/DiagnosticIDs.h
@@ -231,6 +231,14 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
/// "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<diag::Group> getGroupForWarningOption(StringRef);
+
+ /// Return the lowest-level group that contains the specified diagnostic.
+ static llvm::Optional<diag::Group> 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
index 87db131992e47..a8a17994f7fc8 100644
--- a/clang/lib/Basic/DiagnosticIDs.cpp
+++ b/clang/lib/Basic/DiagnosticIDs.cpp
@@ -628,13 +628,27 @@ StringRef DiagnosticIDs::getWarningOptionForGroup(diag::Group Group) {
return OptionTable[static_cast<int>(Group)].getName();
}
+llvm::Optional<diag::Group>
+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<diag::Group>(Found - OptionTable);
+}
+
+llvm::Optional<diag::Group> DiagnosticIDs::getGroupForDiag(unsigned DiagID) {
+ if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
+ return static_cast<diag::Group>(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<diag::Group>(Info->getOptionGroupIndex()));
+ if (auto G = getGroupForDiag(DiagID))
+ return getWarningOptionForGroup(*G);
return StringRef();
}
@@ -683,12 +697,10 @@ static bool getDiagnosticsInGroup(diag::Flavor Flavor,
bool
DiagnosticIDs::getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group,
SmallVectorImpl<diag::kind> &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<diag::Group> G = getGroupForWarningOption(Group))
+ return ::getDiagnosticsInGroup(
+ Flavor, &OptionTable[static_cast<unsigned>(*G)], Diags);
+ return true;
}
void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,
More information about the cfe-commits
mailing list