llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: Sirui Mu (Lancern) <details> <summary>Changes</summary> 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 --- 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: - (modified) clang-tools-extra/clangd/Diagnostics.cpp (+71-1) - (modified) clang-tools-extra/clangd/Diagnostics.h (+2-1) - (modified) clang-tools-extra/clangd/ParsedAST.cpp (+2-2) - (modified) clang-tools-extra/clangd/Preamble.cpp (+2-2) - (modified) clang-tools-extra/clangd/TUScheduler.cpp (+5-4) - (modified) clang-tools-extra/clangd/test/diagnostics-tidy.test (+77-1) - (modified) clang-tools-extra/clangd/tool/Check.cpp (+1-1) - (modified) clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp (+147-14) - (modified) clang-tools-extra/clangd/unittests/CompilerTests.cpp (+1-1) - (modified) clang-tools-extra/clangd/unittests/TestTU.cpp (+1-1) ``````````diff 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] `````````` </details> https://github.com/llvm/llvm-project/pull/188796 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
