[PATCH] D116147: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics
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=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 ExtraArgs) {
+ return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions ,
+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 MainFileTokens;
};
+// Find -W and -Wno- 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 ExtraArgs,
+ DiagnosticsEngine ) {
+ for (llvm::StringRef
[PATCH] D116147: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:270
+// We don't want this, so keep track of them to fix afterwards.
+llvm::DenseSet NeedsWerrorExclusion;
+for (diag::kind ID : Members) {
why do we need a set here, rather than a flag?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116147/new/
https://reviews.llvm.org/D116147
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116147: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics
sammccall updated this revision to Diff 395858.
sammccall added a comment.
Also handle -Wno- flags
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 ExtraArgs) {
+ return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions ,
+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 MainFileTokens;
};
+// Find -W and -Wno- 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 ExtraArgs,
+ DiagnosticsEngine ) {
+ for (llvm::StringRef Group : ExtraArgs) {
+// Only handle args that are of the form -W[no-].
+// Other flags are possible but rare and deliberately out of scope.
+llvm::SmallVector Members;
+if (!Group.consume_front("-W") || Group.empty())
+
[PATCH] D116147: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics
sammccall planned changes to this revision. sammccall added a comment. Actually we should probably handle `-Wno-` too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116147/new/ https://reviews.llvm.org/D116147 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116147: [clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics
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 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 ExtraArgs) {
+ return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions ,
+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.
+
