Skip to content

Commit 5227be8

Browse files
sam-mccallsam-mccall
committed
[clangd] More precisely enable clang warnings through ClangTidy options
clang-tidy's behavior is to add the -W flags, and then map all clang diagnostics to "clang-diagnostic-foo" pseudo-check-names, then use Checks to filter those. Previous to this patch, we were handling -W flags but not filtering the diagnostics, assuming both sets of information encoded the same thing. However this intersection is nontrivial when diagnostic group hierarchy is involved. e.g. -Wunused + clang-diagnostic-unused-function should not enable unused label warnings. This patch more closely emulates clang-tidy's behavior, while not going to the extreme of generating tidy check names for all clang diagnostics and filtering them with regexes. Differential Revision: https://reviews.llvm.org/D124679
1 parent c428a3d commit 5227be8

File tree

4 files changed

+146
-31
lines changed

4 files changed

+146
-31
lines changed

‎clang-tools-extra/clangd/ParsedAST.cpp

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -233,12 +233,69 @@ class ReplayPreamble : private PPCallbacks {
233233
std::vector<syntax::Token> MainFileTokens;
234234
};
235235

236+
// Filter for clang diagnostics groups enabled by CTOptions.Checks.
237+
//
238+
// These are check names like clang-diagnostics-unused.
239+
// Note that unlike -Wunused, clang-diagnostics-unused does not imply
240+
// subcategories like clang-diagnostics-unused-function.
241+
//
242+
// This is used to determine which diagnostics can be enabled by ExtraArgs in
243+
// the clang-tidy configuration.
244+
class TidyDiagnosticGroups {
245+
// Whether all diagnostic groups are enabled by default.
246+
// True if we've seen clang-diagnostic-*.
247+
bool Default = false;
248+
// Set of diag::Group whose enablement != Default.
249+
// If Default is false, this is foo where we've seen clang-diagnostic-foo.
250+
llvm::DenseSet<unsigned> Exceptions;
251+
252+
public:
253+
TidyDiagnosticGroups(llvm::StringRef Checks) {
254+
constexpr llvm::StringLiteral CDPrefix = "clang-diagnostic-";
255+
256+
llvm::StringRef Check;
257+
while (!Checks.empty()) {
258+
std::tie(Check, Checks) = Checks.split(',');
259+
if (Check.empty())
260+
continue;
261+
262+
bool Enable = !Check.consume_front("-");
263+
bool Glob = Check.consume_back("*");
264+
if (Glob) {
265+
// Is this clang-diagnostic-*, or *, or so?
266+
// (We ignore all other types of globs).
267+
if (CDPrefix.startswith(Check)) {
268+
Default = Enable;
269+
Exceptions.clear();
270+
}
271+
continue;
272+
}
273+
274+
// In "*,clang-diagnostic-foo", the latter is a no-op.
275+
if (Default == Enable)
276+
continue;
277+
// The only non-glob entries we care about are clang-diagnostic-foo.
278+
if (!Check.consume_front(CDPrefix))
279+
continue;
280+
281+
if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check))
282+
Exceptions.insert(static_cast<unsigned>(*Group));
283+
}
284+
}
285+
286+
bool operator()(diag::Group GroupID) const {
287+
return Exceptions.contains(static_cast<unsigned>(GroupID)) ? !Default
288+
: Default;
289+
}
290+
};
291+
236292
// Find -W<group> and -Wno-<group> options in ExtraArgs and apply them to Diags.
237293
//
238294
// This is used to handle ExtraArgs in clang-tidy configuration.
239295
// We don't use clang's standard handling of this as we want slightly different
240296
// behavior (e.g. we want to exclude these from -Wno-error).
241297
void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
298+
llvm::function_ref<bool(diag::Group)> EnabledGroups,
242299
DiagnosticsEngine &Diags) {
243300
for (llvm::StringRef Group : ExtraArgs) {
244301
// Only handle args that are of the form -W[no-]<group>.
@@ -259,6 +316,9 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
259316
if (Enable) {
260317
if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
261318
DiagnosticsEngine::Warning) {
319+
auto Group = DiagnosticIDs::getGroupForDiag(ID);
320+
if (!Group || !EnabledGroups(*Group))
321+
continue;
262322
Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
263323
if (Diags.getWarningsAsErrors())
264324
NeedsWerrorExclusion = true;
@@ -354,18 +414,25 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
354414
// - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings
355415
// - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out
356416
//
357-
// We treat these as clang warnings, so the Checks part is not relevant.
358-
// We must enable the warnings specified in ExtraArgs.
417+
// In clang-tidy, diagnostics are emitted if they pass both checks.
418+
// When groups contain subgroups, -Wparent includes the child, but
419+
// clang-diagnostic-parent does not.
359420
//
360-
// We *don't* want to change the compile command directly. this can have
421+
// We *don't* want to change the compile command directly. This can have
361422
// too many unexpected effects: breaking the command, interactions with
362423
// -- and -Werror, etc. Besides, we've already parsed the command.
363424
// Instead we parse the -W<group> flags and handle them directly.
425+
//
426+
// Similarly, we don't want to use Checks to filter clang diagnostics after
427+
// they are generated, as this spreads clang-tidy emulation everywhere.
428+
// Instead, we just use these to filter which extra diagnostics we enable.
364429
auto &Diags = Clang->getDiagnostics();
430+
TidyDiagnosticGroups TidyGroups(ClangTidyOpts.Checks ? *ClangTidyOpts.Checks
431+
: llvm::StringRef());
365432
if (ClangTidyOpts.ExtraArgsBefore)
366-
applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags);
433+
applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, TidyGroups, Diags);
367434
if (ClangTidyOpts.ExtraArgs)
368-
applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags);
435+
applyWarningOptions(*ClangTidyOpts.ExtraArgs, TidyGroups, Diags);
369436
} else {
370437
// Skips some analysis.
371438
Clang->getDiagnosticOpts().IgnoreWarnings = true;

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

Lines changed: 45 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -517,13 +517,16 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
517517
diagSeverity(DiagnosticsEngine::Error)))));
518518
}
519519

520-
TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
521-
return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
522-
llvm::StringRef) {
520+
TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs,
521+
llvm::StringRef Checks) {
522+
return [ExtraArgs = std::move(ExtraArgs), Checks = Checks.str()](
523+
tidy::ClangTidyOptions &Opts, llvm::StringRef) {
523524
if (!Opts.ExtraArgs)
524525
Opts.ExtraArgs.emplace();
525526
for (llvm::StringRef Arg : ExtraArgs)
526527
Opts.ExtraArgs->emplace_back(Arg);
528+
if (!Checks.empty())
529+
Opts.Checks = Checks;
527530
};
528531
}
529532

@@ -541,53 +544,78 @@ TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
541544
// Check the -Wunused warning isn't initially on.
542545
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
543546

544-
// We enable warnings based on clang-tidy extra args.
545-
TU.ClangTidyProvider = addClangArgs({"-Wunused"});
547+
// We enable warnings based on clang-tidy extra args, if the matching
548+
// clang-diagnostic- is there.
549+
TU.ClangTidyProvider =
550+
addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
551+
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
552+
553+
// clang-diagnostic-* is acceptable
554+
TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-*");
546555
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
556+
// And plain *
557+
TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*");
558+
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
559+
// And we can explicitly exclude a category too.
560+
TU.ClangTidyProvider =
561+
addClangArgs({"-Wunused"}, "*,-clang-diagnostic-unused-function");
562+
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
547563

548-
// But we don't respect other args.
549-
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
564+
// Without the exact check specified, the warnings are not enabled.
565+
TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-unused");
566+
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
567+
568+
// We don't respect other args.
569+
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"},
570+
"clang-diagnostic-unused-function");
550571
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning))
551572
<< "Not unused function 'bar'!";
552573

553574
// -Werror doesn't apply to warnings enabled by clang-tidy extra args.
554575
TU.ExtraArgs = {"-Werror"};
555-
TU.ClangTidyProvider = addClangArgs({"-Wunused"});
576+
TU.ClangTidyProvider =
577+
addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
556578
EXPECT_THAT(*TU.build().getDiagnostics(),
557579
ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
558580

559581
// But clang-tidy extra args won't *downgrade* errors to warnings either.
560582
TU.ExtraArgs = {"-Wunused", "-Werror"};
561-
TU.ClangTidyProvider = addClangArgs({"-Wunused"});
583+
TU.ClangTidyProvider =
584+
addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
562585
EXPECT_THAT(*TU.build().getDiagnostics(),
563586
ElementsAre(diagSeverity(DiagnosticsEngine::Error)));
564587

565588
// FIXME: we're erroneously downgrading the whole group, this should be Error.
566589
TU.ExtraArgs = {"-Wunused-function", "-Werror"};
567-
TU.ClangTidyProvider = addClangArgs({"-Wunused"});
590+
TU.ClangTidyProvider =
591+
addClangArgs({"-Wunused"}, "clang-diagnostic-unused-label");
568592
EXPECT_THAT(*TU.build().getDiagnostics(),
569593
ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
570594

571595
// This looks silly, but it's the typical result if a warning is enabled by a
572596
// high-level .clang-tidy file and disabled by a low-level one.
573597
TU.ExtraArgs = {};
574-
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"});
598+
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"},
599+
"clang-diagnostic-unused-function");
575600
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
576601

577602
// Overriding only works in the proper order.
578-
TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"});
603+
TU.ClangTidyProvider =
604+
addClangArgs({"-Wunused"}, {"clang-diagnostic-unused-function"});
579605
EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1));
580606

581607
// More specific vs less-specific: match clang behavior
582-
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"});
608+
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"},
609+
{"clang-diagnostic-unused-function"});
583610
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
584-
TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"});
611+
TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"},
612+
{"clang-diagnostic-unused-function"});
585613
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
586614

587-
// We do allow clang-tidy config to disable warnings from the compile command.
588-
// It's unclear this is ideal, but it's hard to avoid.
615+
// We do allow clang-tidy config to disable warnings from the compile
616+
// command. It's unclear this is ideal, but it's hard to avoid.
589617
TU.ExtraArgs = {"-Wunused"};
590-
TU.ClangTidyProvider = addClangArgs({"-Wno-unused"});
618+
TU.ClangTidyProvider = addClangArgs({"-Wno-unused"}, {});
591619
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
592620
}
593621

‎clang/include/clang/Basic/DiagnosticIDs.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,14 @@ class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
231231
/// "deprecated-declarations".
232232
static StringRef getWarningOptionForGroup(diag::Group);
233233

234+
/// Given a group ID, returns the flag that toggles the group.
235+
/// For example, for "deprecated-declarations", returns
236+
/// Group::DeprecatedDeclarations.
237+
static llvm::Optional<diag::Group> getGroupForWarningOption(StringRef);
238+
239+
/// Return the lowest-level group that contains the specified diagnostic.
240+
static llvm::Optional<diag::Group> getGroupForDiag(unsigned DiagID);
241+
234242
/// Return the lowest-level warning option that enables the specified
235243
/// diagnostic.
236244
///

‎clang/lib/Basic/DiagnosticIDs.cpp

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -628,13 +628,27 @@ StringRef DiagnosticIDs::getWarningOptionForGroup(diag::Group Group) {
628628
return OptionTable[static_cast<int>(Group)].getName();
629629
}
630630

631+
llvm::Optional<diag::Group>
632+
DiagnosticIDs::getGroupForWarningOption(StringRef Name) {
633+
const auto *Found = llvm::partition_point(
634+
OptionTable, [=](const WarningOption &O) { return O.getName() < Name; });
635+
if (Found == std::end(OptionTable) || Found->getName() != Name)
636+
return llvm::None;
637+
return static_cast<diag::Group>(Found - OptionTable);
638+
}
639+
640+
llvm::Optional<diag::Group> DiagnosticIDs::getGroupForDiag(unsigned DiagID) {
641+
if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
642+
return static_cast<diag::Group>(Info->getOptionGroupIndex());
643+
return llvm::None;
644+
}
645+
631646
/// getWarningOptionForDiag - Return the lowest-level warning option that
632647
/// enables the specified diagnostic. If there is no -Wfoo flag that controls
633648
/// the diagnostic, this returns null.
634649
StringRef DiagnosticIDs::getWarningOptionForDiag(unsigned DiagID) {
635-
if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
636-
return getWarningOptionForGroup(
637-
static_cast<diag::Group>(Info->getOptionGroupIndex()));
650+
if (auto G = getGroupForDiag(DiagID))
651+
return getWarningOptionForGroup(*G);
638652
return StringRef();
639653
}
640654

@@ -683,12 +697,10 @@ static bool getDiagnosticsInGroup(diag::Flavor Flavor,
683697
bool
684698
DiagnosticIDs::getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group,
685699
SmallVectorImpl<diag::kind> &Diags) const {
686-
auto Found = llvm::partition_point(
687-
OptionTable, [=](const WarningOption &O) { return O.getName() < Group; });
688-
if (Found == std::end(OptionTable) || Found->getName() != Group)
689-
return true; // Option not found.
690-
691-
return ::getDiagnosticsInGroup(Flavor, Found, Diags);
700+
if (llvm::Optional<diag::Group> G = getGroupForWarningOption(Group))
701+
return ::getDiagnosticsInGroup(
702+
Flavor, &OptionTable[static_cast<unsigned>(*G)], Diags);
703+
return true;
692704
}
693705

694706
void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,

0 commit comments

Comments
 (0)