maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics 

This mechanism is used almost exclusively to enable extra warnings in clang-tidy
using ExtraArgs=-Wfoo, Checks="clang-diagnostic-foo".
Its presence is a strong signal that these flags are useful.

We choose not to actually emit them as clang-tidy diagnostics, but under their
"main" name - this ensures we show the same diagnostic in a consistent way.

We don't add the ExtraArgs to the compile command in general, but rather just
handle the -W<group> flags, which is the common case and avoids unexpected
side-effects.
And we only do this for the main file parse, when producing diagnostics.

Differential Revision: https://reviews.llvm.org/D116147
This commit is contained in:
Sam McCall 2021-12-22 11:07:50 +01:00
parent 7505aeefc4
commit 9e6f88b31a
2 changed files with 143 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
clang-tools-extra/clangd/ParsedAST.cpp
View File

@ -246,6 +246,49 @@ private:
std::vector<syntax::Token> MainFileTokens;
};
// 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 different
// behavior (e.g. we want to exclude these from -Wno-error).
void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
DiagnosticsEngine &Diags) {
for (llvm::StringRef Group : ExtraArgs) {
// Only handle args that are of the form -W[no-]<group>.
// Other flags are possible but rare and deliberately out of scope.
llvm::SmallVector<diag::kind> Members;
if (!Group.consume_front("-W") || Group.empty())
continue;
bool Enable = !Group.consume_front("no-");
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
diag::Flavor::WarningOrError, Group, Members))
continue;
// Upgrade (or downgrade) the severity of each diagnostic in the group.
// If -Werror is on, newly added warnings will be treated as errors.
// We don't want this, so keep track of them to fix afterwards.
bool NeedsWerrorExclusion = false;
for (diag::kind ID : Members) {
if (Enable) {
if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
DiagnosticsEngine::Warning) {
Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
if (Diags.getWarningsAsErrors())
NeedsWerrorExclusion = true;
}
} else {
Diags.setSeverity(ID, diag::Severity::Ignored, SourceLocation());
}
}
if (NeedsWerrorExclusion) {
// FIXME: there's no API to suppress -Werror for single diagnostics.
// In some cases with sub-groups, we may end up erroneously
// downgrading diagnostics that were -Werror in the compile command.
Diags.setDiagnosticGroupWarningAsError(Group, false);
}
}
}
} // namespace
llvm::Optional<ParsedAST>
@ -311,7 +354,32 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
: "unknown error");
return None;
}
if (!PreserveDiags) {
tidy::ClangTidyOptions ClangTidyOpts;
if (PreserveDiags) {
trace::Span Tracer("ClangTidyOpts");
ClangTidyOpts = getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename);
dlog("ClangTidy configuration for file {0}: {1}", Filename,
tidy::configurationAsText(ClangTidyOpts));
// If clang-tidy is configured to emit clang warnings, we should too.
//
// Such clang-tidy configuration consists of two parts:
// - 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.
//
// 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.
auto &Diags = Clang->getDiagnostics();
if (ClangTidyOpts.ExtraArgsBefore)
applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags);
if (ClangTidyOpts.ExtraArgs)
applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags);
} else {
// Skips some analysis.
Clang->getDiagnosticOpts().IgnoreWarnings = true;
}
@ -348,10 +416,6 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
// diagnostics.
if (PreserveDiags) {
trace::Span Tracer("ClangTidyInit");
tidy::ClangTidyOptions ClangTidyOpts =
getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename);
dlog("ClangTidy configuration for file {0}: {1}", Filename,
tidy::configurationAsText(ClangTidyOpts));
tidy::ClangTidyCheckFactories CTFactories;
for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
E.instantiate()->addCheckFactories(CTFactories);

74
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
View File

@ -517,6 +517,80 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
DiagSeverity(DiagnosticsEngine::Error)))));
}
TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
llvm::StringRef) {
if (!Opts.ExtraArgs)
Opts.ExtraArgs.emplace();
for (llvm::StringRef Arg : ExtraArgs)
Opts.ExtraArgs->emplace_back(Arg);
};
}
TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
Annotations Main(R"cpp( // error-ok
static void [[foo]]() {}
)cpp");
TestTU TU = TestTU::withCode(Main.code());
// This is always emitted as a clang warning, not a clang-tidy diagnostic.
auto UnusedFooWarning =
AllOf(Diag(Main.range(), "unused function 'foo'"),
DiagName("-Wunused-function"), DiagSource(Diag::Clang),
DiagSeverity(DiagnosticsEngine::Warning));
// 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"});
EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
// But we don't respect other args.
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
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"});
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"});
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"});
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"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
// Overriding only works in the proper order.
TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"});
EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1));
// More specific vs less-specific: match clang behavior
TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"});
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.
TU.ExtraArgs = {"-Wunused"};
TU.ClangTidyProvider = addClangArgs({"-Wno-unused"});
EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
}
TEST(DiagnosticTest, LongFixMessages) {
// We limit the size of printed code.
Annotations Source(R"cpp(