[clangd] Add code action to suppress clang-tidy diagnostics#188796
Open
[clangd] Add code action to suppress clang-tidy diagnostics#188796
Conversation
This patch adds code actions to suppress diagnostics generated by clang-tidy. There are two variants of the code action. The first one suppresses such diagnostics by appending `// NOLINT(check-name)` to the end of the source line. The second one suppresses such diagnostics by inserting a source line containing `// NOLINTNEXTLINE(check-name)` before the source line of the diagnostics. A previous PR llvm#114661 attempted to land similar feature, but it ends up being closed by its author. Assisted-by: Github Copilot / Claude Opus 4.6
Member
|
@llvm/pr-subscribers-clangd Author: Sirui Mu (Lancern) ChangesThis patch adds code actions to suppress diagnostics generated by clang-tidy. There are two variants of the code action. The first one suppresses such diagnostics by appending A previous PR #114661 attempted to land similar feature, but it ends up being closed by its author. Assisted-by: Github Copilot / Claude Opus 4.6 Patch is 20.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/188796.diff 10 Files Affected:
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index f68092b9a0886..bf638c52d00aa 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -571,7 +571,73 @@ int getSeverity(DiagnosticsEngine::Level L) {
llvm_unreachable("Unknown diagnostic level!");
}
-std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
+// For clang-tidy diagnostics, generates a fix that suppresses the diagnostic
+// on the current line by appending a NOLINT comment.
+static std::optional<Fix> makeNolintFix(llvm::StringRef Code, const Diag &D) {
+ if (D.Source != Diag::ClangTidy || D.Name.empty() || !D.InsideMainFile)
+ return std::nullopt;
+
+ llvm::Expected<size_t> StartOffset = positionToOffset(Code, D.Range.start);
+ if (!StartOffset) {
+ llvm::consumeError(StartOffset.takeError());
+ return std::nullopt;
+ }
+ size_t LineEnd = Code.find('\n', *StartOffset);
+ if (LineEnd == llvm::StringRef::npos)
+ LineEnd = Code.size();
+ Position InsertPos = offsetToPosition(Code, LineEnd);
+
+ TextEdit Edit;
+ Edit.range = {InsertPos, InsertPos};
+ Edit.newText = llvm::formatv(" // NOLINT({0})", D.Name);
+
+ Fix F;
+ F.Message = llvm::formatv("suppress this warning with NOLINT");
+ F.Edits.push_back(std::move(Edit));
+
+ return F;
+}
+
+// For clang-tidy diagnostics, generates a fix that suppresses the diagnostic on
+// the current line by inserting a NOLINTNEXTLINE comment before the current
+// line.
+static std::optional<Fix> makeNolintNextLineFix(llvm::StringRef Code,
+ const Diag &D) {
+ if (D.Source != Diag::ClangTidy || D.Name.empty() || !D.InsideMainFile)
+ return std::nullopt;
+
+ llvm::Expected<size_t> StartOffset = positionToOffset(Code, D.Range.start);
+ if (!StartOffset) {
+ llvm::consumeError(StartOffset.takeError());
+ return std::nullopt;
+ }
+ size_t LineStart = Code.rfind('\n', *StartOffset);
+ if (LineStart == llvm::StringRef::npos)
+ LineStart = 0;
+ else
+ ++LineStart;
+
+ size_t LineTextStart = Code.find_first_not_of(" \t", LineStart);
+ if (LineTextStart == llvm::StringRef::npos || LineTextStart > *StartOffset)
+ LineTextStart = *StartOffset;
+
+ size_t Indentation = LineTextStart - LineStart;
+ Position InsertPos = offsetToPosition(Code, LineStart);
+
+ TextEdit Edit;
+ Edit.range = {InsertPos, InsertPos};
+ Edit.newText = std::string(Indentation, ' ');
+ Edit.newText.append(llvm::formatv("// NOLINTNEXTLINE({0})\n", D.Name));
+
+ Fix F;
+ F.Message = llvm::formatv("suppress this warning with NOLINTNEXTLINE");
+ F.Edits.push_back(std::move(Edit));
+
+ return F;
+}
+
+std::vector<Diag> StoreDiags::take(llvm::StringRef Code,
+ const clang::tidy::ClangTidyContext *Tidy) {
// Do not forget to emit a pending diagnostic if there is one.
flushLastDiag();
@@ -605,6 +671,10 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
if (!TidyDiag.empty()) {
Diag.Name = std::move(TidyDiag);
Diag.Source = Diag::ClangTidy;
+ if (auto NolintFix = makeNolintFix(Code, Diag))
+ Diag.Fixes.push_back(std::move(*NolintFix));
+ if (auto NolintNextLineFix = makeNolintNextLineFix(Code, Diag))
+ Diag.Fixes.push_back(std::move(*NolintNextLineFix));
// clang-tidy bakes the name into diagnostic messages. Strip it out.
// It would be much nicer to make clang-tidy not do this.
auto CleanMessage = [&](std::string &Msg) {
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index d433abb530151..641468c2a8974 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -138,7 +138,8 @@ std::optional<std::string> getDiagnosticDocURI(Diag::DiagSource, unsigned ID,
class StoreDiags : public DiagnosticConsumer {
public:
// The ClangTidyContext populates Source and Name for clang-tidy diagnostics.
- std::vector<Diag> take(const clang::tidy::ClangTidyContext *Tidy = nullptr);
+ std::vector<Diag> take(llvm::StringRef Code,
+ const clang::tidy::ClangTidyContext *Tidy = nullptr);
void BeginSourceFile(const LangOptions &Opts,
const Preprocessor *PP) override;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 4e873f1257a17..41b6977b71930 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -471,7 +471,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
if (!Clang) {
// The last diagnostic contains information about the reason of this
// failure.
- std::vector<Diag> Diags(ASTDiags.take());
+ std::vector<Diag> Diags(ASTDiags.take(Inputs.Contents));
elog("Failed to prepare a compiler instance: {0}",
!Diags.empty() ? static_cast<DiagBase &>(Diags.back()).Message
: "unknown error");
@@ -748,7 +748,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
llvm::append_range(Diags, Patch->patchedDiags());
// Finally, add diagnostics coming from the AST.
{
- std::vector<Diag> D = ASTDiags.take(&*CTContext);
+ std::vector<Diag> D = ASTDiags.take(Inputs.Contents, &*CTContext);
Diags.insert(Diags.end(), D.begin(), D.end());
}
ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index f5e512793e98e..984c9369fe27b 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -661,7 +661,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
log("Built preamble of size {0} for file {1} version {2} in {3} seconds",
BuiltPreamble->getSize(), FileName, Inputs.Version,
PreambleTimer.getTime());
- std::vector<Diag> Diags = PreambleDiagnostics.take();
+ std::vector<Diag> Diags = PreambleDiagnostics.take(Inputs.Contents);
auto Result = std::make_shared<PreambleData>(std::move(*BuiltPreamble));
Result->Version = Inputs.Version;
Result->CompileCommand = Inputs.CompileCommand;
@@ -708,7 +708,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
Inputs.Version, BuiltPreamble.getError().message());
- for (const Diag &D : PreambleDiagnostics.take()) {
+ for (const Diag &D : PreambleDiagnostics.take(Inputs.Contents)) {
if (D.Severity < DiagnosticsEngine::Error)
continue;
// Not an ideal way to show errors, but better than nothing!
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 0661ecb58008e..ee4f786503960 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -918,7 +918,7 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
if (!CC1Args.empty())
vlog("Driver produced command: cc1 {0}", printArgv(CC1Args));
std::vector<Diag> CompilerInvocationDiags =
- CompilerInvocationDiagConsumer.take();
+ CompilerInvocationDiagConsumer.take(Inputs.Contents);
if (!Invocation) {
elog("Could not build CompilerInvocation for file {0}", FileName);
// Remove the old AST if it's still in cache.
@@ -995,9 +995,10 @@ void ASTWorker::runWithAST(
// return a compatible preamble as ASTWorker::update blocks.
std::optional<ParsedAST> NewAST;
if (Invocation) {
- NewAST = ParsedAST::build(FileName, FileInputs, std::move(Invocation),
- CompilerInvocationDiagConsumer.take(),
- getPossiblyStalePreamble());
+ NewAST = ParsedAST::build(
+ FileName, FileInputs, std::move(Invocation),
+ CompilerInvocationDiagConsumer.take(FileInputs.Contents),
+ getPossiblyStalePreamble());
++ASTBuildCount;
}
AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
diff --git a/clang-tools-extra/clangd/test/diagnostics-tidy.test b/clang-tools-extra/clangd/test/diagnostics-tidy.test
index e592c9a0be7c3..864d028d8ad4d 100644
--- a/clang-tools-extra/clangd/test/diagnostics-tidy.test
+++ b/clang-tools-extra/clangd/test/diagnostics-tidy.test
@@ -11,7 +11,7 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "https://clang.llvm.org/extra/clang-tidy/checks/bugprone/sizeof-expression.html"
# CHECK-NEXT: },
-# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'?",
+# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'? (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 16,
@@ -30,6 +30,82 @@
# CHECK-NEXT: "version": 0
# CHECK-NEXT: }
---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":1,"character":6},"end":{"line":1,"character":16}},"context":{"diagnostics":[{"range":{"start":{"line":1,"character":6},"end":{"line":1,"character":16}},"severity":2,"message":"Suspicious usage of 'sizeof(K)'; did you mean 'K'? (fixes available)","code":"bugprone-sizeof-expression","source":"clang-tidy"}]}}}
+# CHECK: {
+# CHECK: "result": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "changes": {
+# CHECK-NEXT: "file:///{{.*}}/foo.c": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "newText": " // NOLINT(bugprone-sizeof-expression)",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 17,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 17,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyFix",
+# CHECK-NEXT: "title": "Apply fix: suppress this warning with NOLINT"
+# CHECK-NEXT: },
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "changes": {
+# CHECK-NEXT: "file:///{{.*}}/foo.c": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(bugprone-sizeof-expression)\n",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyFix",
+# CHECK-NEXT: "title": "Apply fix: suppress this warning with NOLINTNEXTLINE"
+# CHECK-NEXT: },
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "file": "file:///{{.*}}/foo.c",
+# CHECK-NEXT: "selection": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 16,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 6,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: },
+# CHECK-NEXT: "tweakID": "ExtractVariable"
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyTweak",
+# CHECK-NEXT: "title": "Extract subexpression to variable"
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+---
{"jsonrpc":"2.0","id":2,"method":"sync","params":null}
---
{"jsonrpc":"2.0","method":"textDocument/didClose","params":{"textDocument":{"uri":"test:///foo.c"}}}
diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
index 03c4f58a49c9c..35695e9b4e995 100644
--- a/clang-tools-extra/clangd/tool/Check.cpp
+++ b/clang-tools-extra/clangd/tool/Check.cpp
@@ -227,7 +227,7 @@ class Checker {
log("Parsing command...");
Invocation =
buildCompilerInvocation(Inputs, CaptureInvocationDiags, &CC1Args);
- auto InvocationDiags = CaptureInvocationDiags.take();
+ auto InvocationDiags = CaptureInvocationDiags.take(Inputs.Contents);
ErrCount += showErrors(InvocationDiags);
log("internal (cc1) args are: {0}", printArgv(CC1Args));
if (!Invocation) {
diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index 95bf5e54fc792..b743f3d312496 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -223,23 +223,37 @@ TEST_F(LSPTest, ClangTidyRename) {
ASSERT_TRUE(Diags && !Diags->empty());
auto RenameDiag = Diags->front();
- auto RenameCommand =
- (*Client
- .call("textDocument/codeAction",
- llvm::json::Object{
- {"textDocument", Client.documentID("foo.cpp")},
- {"context",
- llvm::json::Object{
- {"diagnostics", llvm::json::Array{RenameDiag}}}},
- {"range", Source.range()}})
- .takeValue()
- .getAsArray())[0];
-
- ASSERT_EQ((*RenameCommand.getAsObject())["title"],
+ auto CodeActions =
+ *Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{RenameDiag}}}},
+ {"range", Source.range()}})
+ .takeValue()
+ .getAsArray();
+
+ // Find the rename code action by title.
+ const llvm::json::Value *RenameCommand = nullptr;
+ for (const auto &CA : CodeActions) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->starts_with("Apply fix: change")) {
+ RenameCommand = &CA;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(RenameCommand, nullptr);
+
+ ASSERT_EQ(*RenameCommand->getAsObject()->getString("title"),
"Apply fix: change 'foo' to 'Foo'");
Client.expectServerCall("workspace/applyEdit");
- Client.call("workspace/executeCommand", RenameCommand);
+ Client.call("workspace/executeCommand", *RenameCommand);
Client.sync();
auto Params = Client.takeCallParams("workspace/applyEdit");
@@ -281,6 +295,125 @@ TEST_F(LSPTest, ClangTidyCrash_Issue109367) {
Client.sync();
}
+TEST_F(LSPTest, ClangTidyNolintCodeAction) {
+ // This test requires clang-tidy checks to be linked in.
+ if (!CLANGD_TIDY_CHECKS)
+ return;
+ Annotations Source(R"cpp(
+ int *$diag[[p]] = 0;$comment[[]]
+ )cpp");
+ constexpr auto ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ ClangTidyOpts.Checks = {"-*,modernize-use-nullptr"};
+ };
+ Opts.ClangTidyProvider = ClangTidyProvider;
+ auto &Client = start();
+ Client.didOpen("foo.cpp", Source.code());
+
+ auto Diags = Client.diagnostics("foo.cpp");
+ ASSERT_TRUE(Diags && !Diags->empty());
+ auto UnusedDiag = Diags->front();
+
+ auto CodeActions =
+ Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{UnusedDiag}}}},
+ {"range", Source.range("diag")}})
+ .takeValue();
+
+ // Find the NOLINT code action.
+ const llvm::json::Object *NolintAction = nullptr;
+ for (const auto &CA : *CodeActions.getAsArray()) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->contains("NOLINT") && !Title->contains("NOLINTNEXTLINE")) {
+ NolintAction = Obj;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(NolintAction, nullptr) << "Expected a NOLINT code action";
+ EXPECT_EQ(NolintAction->getString("title"),
+ "Apply fix: suppress this warning with NOLINT");
+
+ auto Uri = [&](llvm::StringRef Path) {
+ return Client.uri(Path).getAsString().value().str();
+ };
+ llvm::json::Array ExpectedArguments = llvm::json::Array{llvm::json::Object{
+ {"changes",
+ llvm::json::Object{{Uri("foo.cpp"),
+ llvm::json::Array{llvm::json::Object{
+ {"range", Source.range("comment")},
+ {"newText", " // NOLINT(modernize-use-nullptr)"},
+ }}}}}}};
+ EXPECT_EQ(*NolintAction->getArray("arguments"), ExpectedArguments);
+}
+
+TEST_F(LSPTest, ClangTidyNolintNextLineCodeAction) {
+ // This test requires clang-tidy checks to be linked in.
+ if (!CLANGD_TIDY_CHECKS)
+ return;
+ Annotations Source(R"cpp(
+$comment[[]] int *$diag[[p]] = 0;
+ )cpp");
+ constexpr auto ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ ClangTidyOpts.Checks = {"-*,modernize-use-nullptr"};
+ };
+ Opts.ClangTidyProvider = ClangTidyProvider;
+ auto &Client = start();
+ Client.didOpen("foo.cpp", Source.code());
+
+ auto Diags = Client.diagnostics("foo.cpp");
+ ASSERT_TRUE(Diags && !Diags->empty());
+ auto UnusedDiag = Diags->front();
+
+ auto CodeActions =
+ Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{UnusedDiag}}}},
+ {"range", Source.range("diag")}})
+ .takeValue();
+
+ // Find the NOLINT code action.
+ const llvm::json::Object *NolintAction = nullptr;
+ for (const auto &CA : *CodeActions.getAsArray()) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->contains("NOLINTNEXTLINE")) {
+ NolintAction = Obj;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(NolintAction, nullptr) << "Expected a NOLINTNEXTLINE code action";
+ EXPECT_EQ(NolintAction->getString("title"),
+ "Apply fix: suppress this warning with NOLINTNEXTLINE");
+
+ auto Uri = [&](llvm::StringRef Path) {
+ return Client.uri(Path).getAsString().value().str();
+ };
+ llvm::json::Array ExpectedArguments = llvm::json::Array{llvm::json::Object{
+ {"changes",
+ llvm::json::Object{
+ {Uri("foo.cpp"),
+ llvm::json::Array{llvm::json::Object{
+ {"range", Source.range("comment")},
+ {"newText", " // NOLINTNEXTLINE(modernize-use-nullptr)\n"},
+ }}}}}}};
+ EXPECT_EQ(*NolintAction->getArray("arguments"), ExpectedArguments);
+}
+
TEST_F(LSPTest, IncomingCalls) {
Annotations Code(R"cpp(
void calle^e(int);
diff --git a/clang-tools-extra/clangd/unittests/CompilerTests.cpp b/clang-tools-extra/clangd/unittests/CompilerTests.cpp
index 9c8ad8d70b47b..0534a9f711c80 100644
--- a/clang-tools-extra/clangd/unittests/CompilerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompilerTests.cpp
@@ -126,7 +126,7 @@ TEST(BuildCompilerInvocation, SuppressDiags) {
Cfg.Diagnostics.Suppress = {"drv_unknown_argument"};
Wit...
[truncated]
|
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Sirui Mu (Lancern) ChangesThis patch adds code actions to suppress diagnostics generated by clang-tidy. There are two variants of the code action. The first one suppresses such diagnostics by appending A previous PR #114661 attempted to land similar feature, but it ends up being closed by its author. Assisted-by: Github Copilot / Claude Opus 4.6 Patch is 20.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/188796.diff 10 Files Affected:
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index f68092b9a0886..bf638c52d00aa 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -571,7 +571,73 @@ int getSeverity(DiagnosticsEngine::Level L) {
llvm_unreachable("Unknown diagnostic level!");
}
-std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
+// For clang-tidy diagnostics, generates a fix that suppresses the diagnostic
+// on the current line by appending a NOLINT comment.
+static std::optional<Fix> makeNolintFix(llvm::StringRef Code, const Diag &D) {
+ if (D.Source != Diag::ClangTidy || D.Name.empty() || !D.InsideMainFile)
+ return std::nullopt;
+
+ llvm::Expected<size_t> StartOffset = positionToOffset(Code, D.Range.start);
+ if (!StartOffset) {
+ llvm::consumeError(StartOffset.takeError());
+ return std::nullopt;
+ }
+ size_t LineEnd = Code.find('\n', *StartOffset);
+ if (LineEnd == llvm::StringRef::npos)
+ LineEnd = Code.size();
+ Position InsertPos = offsetToPosition(Code, LineEnd);
+
+ TextEdit Edit;
+ Edit.range = {InsertPos, InsertPos};
+ Edit.newText = llvm::formatv(" // NOLINT({0})", D.Name);
+
+ Fix F;
+ F.Message = llvm::formatv("suppress this warning with NOLINT");
+ F.Edits.push_back(std::move(Edit));
+
+ return F;
+}
+
+// For clang-tidy diagnostics, generates a fix that suppresses the diagnostic on
+// the current line by inserting a NOLINTNEXTLINE comment before the current
+// line.
+static std::optional<Fix> makeNolintNextLineFix(llvm::StringRef Code,
+ const Diag &D) {
+ if (D.Source != Diag::ClangTidy || D.Name.empty() || !D.InsideMainFile)
+ return std::nullopt;
+
+ llvm::Expected<size_t> StartOffset = positionToOffset(Code, D.Range.start);
+ if (!StartOffset) {
+ llvm::consumeError(StartOffset.takeError());
+ return std::nullopt;
+ }
+ size_t LineStart = Code.rfind('\n', *StartOffset);
+ if (LineStart == llvm::StringRef::npos)
+ LineStart = 0;
+ else
+ ++LineStart;
+
+ size_t LineTextStart = Code.find_first_not_of(" \t", LineStart);
+ if (LineTextStart == llvm::StringRef::npos || LineTextStart > *StartOffset)
+ LineTextStart = *StartOffset;
+
+ size_t Indentation = LineTextStart - LineStart;
+ Position InsertPos = offsetToPosition(Code, LineStart);
+
+ TextEdit Edit;
+ Edit.range = {InsertPos, InsertPos};
+ Edit.newText = std::string(Indentation, ' ');
+ Edit.newText.append(llvm::formatv("// NOLINTNEXTLINE({0})\n", D.Name));
+
+ Fix F;
+ F.Message = llvm::formatv("suppress this warning with NOLINTNEXTLINE");
+ F.Edits.push_back(std::move(Edit));
+
+ return F;
+}
+
+std::vector<Diag> StoreDiags::take(llvm::StringRef Code,
+ const clang::tidy::ClangTidyContext *Tidy) {
// Do not forget to emit a pending diagnostic if there is one.
flushLastDiag();
@@ -605,6 +671,10 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) {
if (!TidyDiag.empty()) {
Diag.Name = std::move(TidyDiag);
Diag.Source = Diag::ClangTidy;
+ if (auto NolintFix = makeNolintFix(Code, Diag))
+ Diag.Fixes.push_back(std::move(*NolintFix));
+ if (auto NolintNextLineFix = makeNolintNextLineFix(Code, Diag))
+ Diag.Fixes.push_back(std::move(*NolintNextLineFix));
// clang-tidy bakes the name into diagnostic messages. Strip it out.
// It would be much nicer to make clang-tidy not do this.
auto CleanMessage = [&](std::string &Msg) {
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index d433abb530151..641468c2a8974 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -138,7 +138,8 @@ std::optional<std::string> getDiagnosticDocURI(Diag::DiagSource, unsigned ID,
class StoreDiags : public DiagnosticConsumer {
public:
// The ClangTidyContext populates Source and Name for clang-tidy diagnostics.
- std::vector<Diag> take(const clang::tidy::ClangTidyContext *Tidy = nullptr);
+ std::vector<Diag> take(llvm::StringRef Code,
+ const clang::tidy::ClangTidyContext *Tidy = nullptr);
void BeginSourceFile(const LangOptions &Opts,
const Preprocessor *PP) override;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 4e873f1257a17..41b6977b71930 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -471,7 +471,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
if (!Clang) {
// The last diagnostic contains information about the reason of this
// failure.
- std::vector<Diag> Diags(ASTDiags.take());
+ std::vector<Diag> Diags(ASTDiags.take(Inputs.Contents));
elog("Failed to prepare a compiler instance: {0}",
!Diags.empty() ? static_cast<DiagBase &>(Diags.back()).Message
: "unknown error");
@@ -748,7 +748,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
llvm::append_range(Diags, Patch->patchedDiags());
// Finally, add diagnostics coming from the AST.
{
- std::vector<Diag> D = ASTDiags.take(&*CTContext);
+ std::vector<Diag> D = ASTDiags.take(Inputs.Contents, &*CTContext);
Diags.insert(Diags.end(), D.begin(), D.end());
}
ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index f5e512793e98e..984c9369fe27b 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -661,7 +661,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
log("Built preamble of size {0} for file {1} version {2} in {3} seconds",
BuiltPreamble->getSize(), FileName, Inputs.Version,
PreambleTimer.getTime());
- std::vector<Diag> Diags = PreambleDiagnostics.take();
+ std::vector<Diag> Diags = PreambleDiagnostics.take(Inputs.Contents);
auto Result = std::make_shared<PreambleData>(std::move(*BuiltPreamble));
Result->Version = Inputs.Version;
Result->CompileCommand = Inputs.CompileCommand;
@@ -708,7 +708,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
elog("Could not build a preamble for file {0} version {1}: {2}", FileName,
Inputs.Version, BuiltPreamble.getError().message());
- for (const Diag &D : PreambleDiagnostics.take()) {
+ for (const Diag &D : PreambleDiagnostics.take(Inputs.Contents)) {
if (D.Severity < DiagnosticsEngine::Error)
continue;
// Not an ideal way to show errors, but better than nothing!
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 0661ecb58008e..ee4f786503960 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -918,7 +918,7 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags,
if (!CC1Args.empty())
vlog("Driver produced command: cc1 {0}", printArgv(CC1Args));
std::vector<Diag> CompilerInvocationDiags =
- CompilerInvocationDiagConsumer.take();
+ CompilerInvocationDiagConsumer.take(Inputs.Contents);
if (!Invocation) {
elog("Could not build CompilerInvocation for file {0}", FileName);
// Remove the old AST if it's still in cache.
@@ -995,9 +995,10 @@ void ASTWorker::runWithAST(
// return a compatible preamble as ASTWorker::update blocks.
std::optional<ParsedAST> NewAST;
if (Invocation) {
- NewAST = ParsedAST::build(FileName, FileInputs, std::move(Invocation),
- CompilerInvocationDiagConsumer.take(),
- getPossiblyStalePreamble());
+ NewAST = ParsedAST::build(
+ FileName, FileInputs, std::move(Invocation),
+ CompilerInvocationDiagConsumer.take(FileInputs.Contents),
+ getPossiblyStalePreamble());
++ASTBuildCount;
}
AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
diff --git a/clang-tools-extra/clangd/test/diagnostics-tidy.test b/clang-tools-extra/clangd/test/diagnostics-tidy.test
index e592c9a0be7c3..864d028d8ad4d 100644
--- a/clang-tools-extra/clangd/test/diagnostics-tidy.test
+++ b/clang-tools-extra/clangd/test/diagnostics-tidy.test
@@ -11,7 +11,7 @@
# CHECK-NEXT: "codeDescription": {
# CHECK-NEXT: "href": "https://clang.llvm.org/extra/clang-tidy/checks/bugprone/sizeof-expression.html"
# CHECK-NEXT: },
-# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'?",
+# CHECK-NEXT: "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'? (fixes available)",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 16,
@@ -30,6 +30,82 @@
# CHECK-NEXT: "version": 0
# CHECK-NEXT: }
---
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":1,"character":6},"end":{"line":1,"character":16}},"context":{"diagnostics":[{"range":{"start":{"line":1,"character":6},"end":{"line":1,"character":16}},"severity":2,"message":"Suspicious usage of 'sizeof(K)'; did you mean 'K'? (fixes available)","code":"bugprone-sizeof-expression","source":"clang-tidy"}]}}}
+# CHECK: {
+# CHECK: "result": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "changes": {
+# CHECK-NEXT: "file:///{{.*}}/foo.c": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "newText": " // NOLINT(bugprone-sizeof-expression)",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 17,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 17,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyFix",
+# CHECK-NEXT: "title": "Apply fix: suppress this warning with NOLINT"
+# CHECK-NEXT: },
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "changes": {
+# CHECK-NEXT: "file:///{{.*}}/foo.c": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(bugprone-sizeof-expression)\n",
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyFix",
+# CHECK-NEXT: "title": "Apply fix: suppress this warning with NOLINTNEXTLINE"
+# CHECK-NEXT: },
+# CHECK-NEXT: {
+# CHECK-NEXT: "arguments": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "file": "file:///{{.*}}/foo.c",
+# CHECK-NEXT: "selection": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 16,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 6,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: },
+# CHECK-NEXT: "tweakID": "ExtractVariable"
+# CHECK-NEXT: }
+# CHECK-NEXT: ],
+# CHECK-NEXT: "command": "clangd.applyTweak",
+# CHECK-NEXT: "title": "Extract subexpression to variable"
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+---
{"jsonrpc":"2.0","id":2,"method":"sync","params":null}
---
{"jsonrpc":"2.0","method":"textDocument/didClose","params":{"textDocument":{"uri":"test:///foo.c"}}}
diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
index 03c4f58a49c9c..35695e9b4e995 100644
--- a/clang-tools-extra/clangd/tool/Check.cpp
+++ b/clang-tools-extra/clangd/tool/Check.cpp
@@ -227,7 +227,7 @@ class Checker {
log("Parsing command...");
Invocation =
buildCompilerInvocation(Inputs, CaptureInvocationDiags, &CC1Args);
- auto InvocationDiags = CaptureInvocationDiags.take();
+ auto InvocationDiags = CaptureInvocationDiags.take(Inputs.Contents);
ErrCount += showErrors(InvocationDiags);
log("internal (cc1) args are: {0}", printArgv(CC1Args));
if (!Invocation) {
diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index 95bf5e54fc792..b743f3d312496 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -223,23 +223,37 @@ TEST_F(LSPTest, ClangTidyRename) {
ASSERT_TRUE(Diags && !Diags->empty());
auto RenameDiag = Diags->front();
- auto RenameCommand =
- (*Client
- .call("textDocument/codeAction",
- llvm::json::Object{
- {"textDocument", Client.documentID("foo.cpp")},
- {"context",
- llvm::json::Object{
- {"diagnostics", llvm::json::Array{RenameDiag}}}},
- {"range", Source.range()}})
- .takeValue()
- .getAsArray())[0];
-
- ASSERT_EQ((*RenameCommand.getAsObject())["title"],
+ auto CodeActions =
+ *Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{RenameDiag}}}},
+ {"range", Source.range()}})
+ .takeValue()
+ .getAsArray();
+
+ // Find the rename code action by title.
+ const llvm::json::Value *RenameCommand = nullptr;
+ for (const auto &CA : CodeActions) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->starts_with("Apply fix: change")) {
+ RenameCommand = &CA;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(RenameCommand, nullptr);
+
+ ASSERT_EQ(*RenameCommand->getAsObject()->getString("title"),
"Apply fix: change 'foo' to 'Foo'");
Client.expectServerCall("workspace/applyEdit");
- Client.call("workspace/executeCommand", RenameCommand);
+ Client.call("workspace/executeCommand", *RenameCommand);
Client.sync();
auto Params = Client.takeCallParams("workspace/applyEdit");
@@ -281,6 +295,125 @@ TEST_F(LSPTest, ClangTidyCrash_Issue109367) {
Client.sync();
}
+TEST_F(LSPTest, ClangTidyNolintCodeAction) {
+ // This test requires clang-tidy checks to be linked in.
+ if (!CLANGD_TIDY_CHECKS)
+ return;
+ Annotations Source(R"cpp(
+ int *$diag[[p]] = 0;$comment[[]]
+ )cpp");
+ constexpr auto ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ ClangTidyOpts.Checks = {"-*,modernize-use-nullptr"};
+ };
+ Opts.ClangTidyProvider = ClangTidyProvider;
+ auto &Client = start();
+ Client.didOpen("foo.cpp", Source.code());
+
+ auto Diags = Client.diagnostics("foo.cpp");
+ ASSERT_TRUE(Diags && !Diags->empty());
+ auto UnusedDiag = Diags->front();
+
+ auto CodeActions =
+ Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{UnusedDiag}}}},
+ {"range", Source.range("diag")}})
+ .takeValue();
+
+ // Find the NOLINT code action.
+ const llvm::json::Object *NolintAction = nullptr;
+ for (const auto &CA : *CodeActions.getAsArray()) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->contains("NOLINT") && !Title->contains("NOLINTNEXTLINE")) {
+ NolintAction = Obj;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(NolintAction, nullptr) << "Expected a NOLINT code action";
+ EXPECT_EQ(NolintAction->getString("title"),
+ "Apply fix: suppress this warning with NOLINT");
+
+ auto Uri = [&](llvm::StringRef Path) {
+ return Client.uri(Path).getAsString().value().str();
+ };
+ llvm::json::Array ExpectedArguments = llvm::json::Array{llvm::json::Object{
+ {"changes",
+ llvm::json::Object{{Uri("foo.cpp"),
+ llvm::json::Array{llvm::json::Object{
+ {"range", Source.range("comment")},
+ {"newText", " // NOLINT(modernize-use-nullptr)"},
+ }}}}}}};
+ EXPECT_EQ(*NolintAction->getArray("arguments"), ExpectedArguments);
+}
+
+TEST_F(LSPTest, ClangTidyNolintNextLineCodeAction) {
+ // This test requires clang-tidy checks to be linked in.
+ if (!CLANGD_TIDY_CHECKS)
+ return;
+ Annotations Source(R"cpp(
+$comment[[]] int *$diag[[p]] = 0;
+ )cpp");
+ constexpr auto ClangTidyProvider = [](tidy::ClangTidyOptions &ClangTidyOpts,
+ llvm::StringRef) {
+ ClangTidyOpts.Checks = {"-*,modernize-use-nullptr"};
+ };
+ Opts.ClangTidyProvider = ClangTidyProvider;
+ auto &Client = start();
+ Client.didOpen("foo.cpp", Source.code());
+
+ auto Diags = Client.diagnostics("foo.cpp");
+ ASSERT_TRUE(Diags && !Diags->empty());
+ auto UnusedDiag = Diags->front();
+
+ auto CodeActions =
+ Client
+ .call("textDocument/codeAction",
+ llvm::json::Object{
+ {"textDocument", Client.documentID("foo.cpp")},
+ {"context",
+ llvm::json::Object{
+ {"diagnostics", llvm::json::Array{UnusedDiag}}}},
+ {"range", Source.range("diag")}})
+ .takeValue();
+
+ // Find the NOLINT code action.
+ const llvm::json::Object *NolintAction = nullptr;
+ for (const auto &CA : *CodeActions.getAsArray()) {
+ if (const auto *Obj = CA.getAsObject()) {
+ if (auto Title = Obj->getString("title")) {
+ if (Title->contains("NOLINTNEXTLINE")) {
+ NolintAction = Obj;
+ break;
+ }
+ }
+ }
+ }
+ ASSERT_NE(NolintAction, nullptr) << "Expected a NOLINTNEXTLINE code action";
+ EXPECT_EQ(NolintAction->getString("title"),
+ "Apply fix: suppress this warning with NOLINTNEXTLINE");
+
+ auto Uri = [&](llvm::StringRef Path) {
+ return Client.uri(Path).getAsString().value().str();
+ };
+ llvm::json::Array ExpectedArguments = llvm::json::Array{llvm::json::Object{
+ {"changes",
+ llvm::json::Object{
+ {Uri("foo.cpp"),
+ llvm::json::Array{llvm::json::Object{
+ {"range", Source.range("comment")},
+ {"newText", " // NOLINTNEXTLINE(modernize-use-nullptr)\n"},
+ }}}}}}};
+ EXPECT_EQ(*NolintAction->getArray("arguments"), ExpectedArguments);
+}
+
TEST_F(LSPTest, IncomingCalls) {
Annotations Code(R"cpp(
void calle^e(int);
diff --git a/clang-tools-extra/clangd/unittests/CompilerTests.cpp b/clang-tools-extra/clangd/unittests/CompilerTests.cpp
index 9c8ad8d70b47b..0534a9f711c80 100644
--- a/clang-tools-extra/clangd/unittests/CompilerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompilerTests.cpp
@@ -126,7 +126,7 @@ TEST(BuildCompilerInvocation, SuppressDiags) {
Cfg.Diagnostics.Suppress = {"drv_unknown_argument"};
Wit...
[truncated]
|
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) Clangd Unit TestsClangd Unit Tests._/ClangdTests_exe/DiagnosticTest/ClangTidySelfContainedDiagsClangd Unit Tests._/ClangdTests_exe/DiagnosticTest/ClangTidySelfContainedDiagsFormattingClangd Unit Tests._/ClangdTests_exe/DiagnosticsTest/ClangTidyIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) Clangd Unit TestsClangd Unit Tests._/ClangdTests/DiagnosticTest/ClangTidySelfContainedDiagsClangd Unit Tests._/ClangdTests/DiagnosticTest/ClangTidySelfContainedDiagsFormattingClangd Unit Tests._/ClangdTests/DiagnosticsTest/ClangTidyIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
ArcsinX
reviewed
Mar 26, 2026
Contributor
There was a problem hiding this comment.
A previous PR #114661 attempted to land similar feature, but it ends up being closed by its author.
In that patch, in the comments, there was a suggestion to use "feature module" for this implementation #114661 (review). Can you please address it? Example of feature modules implementation can be found in tests (e.g. here https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp#L27 )
Comment on lines
+674
to
+677
| if (auto NolintFix = makeNolintFix(Code, Diag)) | ||
| Diag.Fixes.push_back(std::move(*NolintFix)); | ||
| if (auto NolintNextLineFix = makeNolintNextLineFix(Code, Diag)) | ||
| Diag.Fixes.push_back(std::move(*NolintNextLineFix)); |
Member
There was a problem hiding this comment.
In one of the code bases I am contributing to, we only use NOLINTNEXTLINE and never use NOLINT.
Would be great to have a setting in the .clangd config controlling which suppression style gets suggested (possible values same-line, previous-line, off or both)
Member
Author
There was a problem hiding this comment.
Cool idea. I'll add config entries as you suggested. Thanks.
Member
Author
Thanks for the feedback. I'm trying to migrate changes in this PR to a feature module. The issue is that Also, I notice that there are currently no in-tree usage of feature module. Is it really preferable to introduce the change as a feature module? |
Contributor
Maybe we can just check that diagnostic ID > DIAG_UPPER_LIMIT, which means that this diagnostic is a custom one. After that we can check diagnostic message and if it ends with
As we can see, with this patch tests fail because of new fixits appearing. Changes in Yes, we have no in-tree feature modules, but personally I think that feature modules is underestimated ability and not well documented (only comments in the source code). Maybe if we implement at least one, we can use feature modules more frequent, thus keeping clangd core logic more clean |
Member
Would it make sense to move all of clang-tidy into a feature module? And make the ClangTidy context an implementation detail of that (That being said, I know next to nothing about feature modules, and hence don't know how much work that refactoring would be or if it's even doable) |
Contributor
Personally, I think this is a great idea. And maybe the reason it wasn't done this way initially is because feature modules were introduced after clang-tidy had already been integrated into clangd. I don't have time to do this by myself right now, but if someone wants to implement it, I'll be happy to review it. |
Member
Author
This sounds like a good idea to me. I'm willing to take some time to investigate into this and I think I would hold this PR on for a while until that approach is attempted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch adds code actions to suppress diagnostics generated by clang-tidy. There are two variants of the code action. The first one suppresses such diagnostics by appending
// NOLINT(check-name)to the end of the source line. The second one suppresses such diagnostics by inserting a source line containing// NOLINTNEXTLINE(check-name)before the source line of the diagnostics.A previous PR #114661 attempted to land similar feature, but it ends up being closed by its author.
Assisted-by: Github Copilot / Claude Opus 4.6