sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116147
Files:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -517,6 +517,58 @@
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)));
+}
+
TEST(DiagnosticTest, LongFixMessages) {
// We limit the size of printed code.
Annotations Source(R"cpp(
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -311,7 +311,63 @@
: "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 ProcessWarningOptions = [&](llvm::ArrayRef<std::string> ExtraArgs) {
+ auto &Diags = Clang->getDiagnostics();
+ for (llvm::StringRef Group : ExtraArgs) {
+ // Only handle args that are of the form -W<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;
+ if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
+ diag::Flavor::WarningOrError, Group, Members))
+ continue;
+
+ // Upgrade the severity of each diagnostic in the group to warning.
+ // If -Werror is on, these will be treated as errors, which we must fix.
+ llvm::DenseSet<diag::kind> NeedsWerrorExclusion;
+ for (diag::kind ID : Members) {
+ if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
+ DiagnosticsEngine::Warning) {
+ Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
+ if (Diags.getWarningsAsErrors())
+ NeedsWerrorExclusion.insert(ID);
+ }
+ }
+ if (!NeedsWerrorExclusion.empty()) {
+ // 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);
+ }
+ }
+ };
+
+ if (ClangTidyOpts.ExtraArgs)
+ ProcessWarningOptions(*ClangTidyOpts.ExtraArgs);
+ if (ClangTidyOpts.ExtraArgsBefore)
+ ProcessWarningOptions(*ClangTidyOpts.ExtraArgsBefore);
+ } else {
// Skips some analysis.
Clang->getDiagnosticOpts().IgnoreWarnings = true;
}
@@ -348,10 +404,6 @@
// 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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
