This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG9e6f88b31a7f: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo
only) when producing diagnostics (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D116147?vs=395858&id=397076#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116147/new/
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,80 @@
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(
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -246,6 +246,49 @@
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 @@
: "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 @@
// 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
