https://github.com/chomosuke updated https://github.com/llvm/llvm-project/pull/111640
>From e1c2a46487c42c17dc0bbfab56cde194c15e14b3 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Fri, 16 Aug 2024 13:31:21 +0000 Subject: [PATCH 01/15] Fixing one error --- clang-tools-extra/clangd/ClangdServer.cpp | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 9b38be04e7ddd7..e08ce12223f6b0 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -44,6 +44,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" +#include "llvm/Support/Format.h" #include "llvm/Support/Path.h" #include "llvm/Support/raw_ostream.h" #include <algorithm> @@ -672,6 +673,21 @@ tryConvertToRename(const Diag *Diag, const Fix &Fix) { return std::nullopt; } +// Add NOLINT insert as code actions +std::optional<Fix> tryAddClangTidySuppression(const Diag *Diag) { + if (Diag->Source == Diag::ClangTidy) { + Fix F; + F.Message = llvm::formatv("ignore [{0}] for this line", Diag->Name); + TextEdit &E = F.Edits.emplace_back(); + E.newText = llvm::formatv("// NOLINTNEXTLINE({0})\n", Diag->Name); + Position InsertPos = Diag->Range.start; + InsertPos.character = 0; + E.range = {InsertPos, InsertPos}; + return F; + } + return std::nullopt; +} + } // namespace void ClangdServer::codeAction(const CodeActionInputs &Params, @@ -701,7 +717,7 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, return nullptr; }; for (const auto &DiagRef : Params.Diagnostics) { - if (const auto *Diag = FindMatchedDiag(DiagRef)) + if (const auto *Diag = FindMatchedDiag(DiagRef)) { for (const auto &Fix : Diag->Fixes) { if (auto Rename = tryConvertToRename(Diag, Fix)) { Result.Renames.emplace_back(std::move(*Rename)); @@ -709,6 +725,11 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, Result.QuickFixes.push_back({DiagRef, Fix}); } } + + if (auto Fix = tryAddClangTidySuppression(Diag)) { + Result.QuickFixes.push_back({DiagRef, std::move(*Fix)}); + } + } } } >From 433040437830935d7a0822984cd5de53213b8af3 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Sun, 18 Aug 2024 17:15:21 +0000 Subject: [PATCH 02/15] Revert "Fixing one error" This reverts commit 17989044ad480628a2f7814674dbe7cd985abd17. --- clang-tools-extra/clangd/ClangdServer.cpp | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index e08ce12223f6b0..9b38be04e7ddd7 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -44,7 +44,6 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" -#include "llvm/Support/Format.h" #include "llvm/Support/Path.h" #include "llvm/Support/raw_ostream.h" #include <algorithm> @@ -673,21 +672,6 @@ tryConvertToRename(const Diag *Diag, const Fix &Fix) { return std::nullopt; } -// Add NOLINT insert as code actions -std::optional<Fix> tryAddClangTidySuppression(const Diag *Diag) { - if (Diag->Source == Diag::ClangTidy) { - Fix F; - F.Message = llvm::formatv("ignore [{0}] for this line", Diag->Name); - TextEdit &E = F.Edits.emplace_back(); - E.newText = llvm::formatv("// NOLINTNEXTLINE({0})\n", Diag->Name); - Position InsertPos = Diag->Range.start; - InsertPos.character = 0; - E.range = {InsertPos, InsertPos}; - return F; - } - return std::nullopt; -} - } // namespace void ClangdServer::codeAction(const CodeActionInputs &Params, @@ -717,7 +701,7 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, return nullptr; }; for (const auto &DiagRef : Params.Diagnostics) { - if (const auto *Diag = FindMatchedDiag(DiagRef)) { + if (const auto *Diag = FindMatchedDiag(DiagRef)) for (const auto &Fix : Diag->Fixes) { if (auto Rename = tryConvertToRename(Diag, Fix)) { Result.Renames.emplace_back(std::move(*Rename)); @@ -725,11 +709,6 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, Result.QuickFixes.push_back({DiagRef, Fix}); } } - - if (auto Fix = tryAddClangTidySuppression(Diag)) { - Result.QuickFixes.push_back({DiagRef, std::move(*Fix)}); - } - } } } >From 7b64a8fafd93af944f72b987d7abf4e167f03205 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Mon, 19 Aug 2024 15:17:05 +0000 Subject: [PATCH 03/15] inserted NOLINT with indent Retrieved CurLine and PrevLine --- clang-tools-extra/clangd/Diagnostics.cpp | 4 +- clang-tools-extra/clangd/Diagnostics.h | 4 +- clang-tools-extra/clangd/ParsedAST.cpp | 57 ++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index a59d1e7ac84096..47810248f093e4 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -823,7 +823,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); if (Fixer) { - auto ExtraFixes = Fixer(LastDiag->Severity, Info); + auto ExtraFixes = Fixer(*LastDiag, Info); LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(), ExtraFixes.end()); } @@ -842,7 +842,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, // Give include-fixer a chance to replace a note with a fix. if (Fixer) { - auto ReplacementFixes = Fixer(LastDiag->Severity, Info); + auto ReplacementFixes = Fixer(*LastDiag, Info); if (!ReplacementFixes.empty()) { assert(Info.getNumFixItHints() == 0 && "Include-fixer replaced a note with clang fix-its attached!"); diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index d4c0478c63a5cf..4fe41fecd8ae5c 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -148,8 +148,8 @@ class StoreDiags : public DiagnosticConsumer { /// When passed a main diagnostic, returns fixes to add to it. /// When passed a note diagnostic, returns fixes to replace it with. - using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level, - const clang::Diagnostic &)>; + using DiagFixer = + std::function<std::vector<Fix>(const Diag &, const clang::Diagnostic &)>; using LevelAdjuster = std::function<DiagnosticsEngine::Level( DiagnosticsEngine::Level, const clang::Diagnostic &)>; using DiagCallback = diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 045d32afbc938a..4cac2091864c66 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -67,6 +67,7 @@ #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include <cassert> +#include <cctype> #include <cstddef> #include <iterator> #include <memory> @@ -401,6 +402,48 @@ filterFastTidyChecks(const tidy::ClangTidyCheckFactories &All, } // namespace +std::vector<Fix> +clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, + const clang::Diagnostic &Info, const Diag &Diag) { + auto RuleName = CTContext.getCheckName(Info.getID()); + if (RuleName.empty()) { + return {}; + } + if (!Diag.InsideMainFile) { + return {}; + } + auto &SrcMgr = Info.getSourceManager(); + auto &DiagLoc = Info.getLocation(); + + auto F = Fix{}; + F.Message = llvm::formatv("ignore [{0}] for this line", RuleName); + auto &E = F.Edits.emplace_back(); + + auto File = SrcMgr.getFileID(DiagLoc); + auto CodeTilDiag = toSourceCode( + SrcMgr, SourceRange(SrcMgr.getLocForStartOfFile(File), DiagLoc)); + + auto StartCurLine = CodeTilDiag.find_last_of('\n') + 1; + auto CurLine = CodeTilDiag.substr(StartCurLine); + elog("CurLine: '{0}'", CurLine); + auto Indent = CurLine.take_while([](char C) { return std::isspace(C); }); + + if (StartCurLine > 0) { + auto StartPrevLine = CodeTilDiag.find_last_of('\n', StartCurLine - 1) + 1; + auto PrevLine = + CodeTilDiag.substr(StartPrevLine, StartCurLine - StartPrevLine - 1); + elog("PrevLine: '{0}'", PrevLine); + } + + E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName); + + auto InsertPos = sourceLocToPosition(SrcMgr, DiagLoc); + InsertPos.character = 0; + E.range = {InsertPos, InsertPos}; + + return {F}; +} + std::optional<ParsedAST> ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::unique_ptr<clang::CompilerInvocation> CI, @@ -655,10 +698,16 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : Symbol::Include; FixIncludes.emplace(Filename, Inserter, *Inputs.Index, /*IndexRequestLimit=*/5, Directive); - ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl, - const clang::Diagnostic &Info) { - return FixIncludes->fix(DiagLevl, Info); - }); + ASTDiags.contributeFixes( + [&FixIncludes, &CTContext](const Diag &Diag, + const clang::Diagnostic &Info) { + auto Fixes = std::vector<Fix>(); + auto NoLintFixes = clangTidyNoLintFixes(*CTContext, Info, Diag); + Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); + auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); + Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); + return Fixes; + }); Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } } >From 5895f196a99711cde27c6f81668d6cd7dad2a39e Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Mon, 19 Aug 2024 19:38:05 +0000 Subject: [PATCH 04/15] Fixed converting 'ignore ... for this line' Fix into rename bug --- clang-tools-extra/clangd/ClangdServer.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 9b38be04e7ddd7..38808c5549608c 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -701,14 +701,21 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, return nullptr; }; for (const auto &DiagRef : Params.Diagnostics) { - if (const auto *Diag = FindMatchedDiag(DiagRef)) - for (const auto &Fix : Diag->Fixes) { - if (auto Rename = tryConvertToRename(Diag, Fix)) { + if (const auto *Diag = FindMatchedDiag(DiagRef)) { + auto It = Diag->Fixes.begin(); + if (It != Diag->Fixes.end()) { + if (auto Rename = tryConvertToRename(Diag, *It)) { + // Only try to convert the first Fix to rename as subsequent Fixes + // might be "ignore [readability-identifier-naming] for this + // line". Result.Renames.emplace_back(std::move(*Rename)); - } else { - Result.QuickFixes.push_back({DiagRef, Fix}); + It++; } } + for (; It != Diag->Fixes.end(); It++) { + Result.QuickFixes.push_back({DiagRef, *It}); + } + } } } >From 1a589eee1871dc97234caf9557e1e024219d9ee6 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Mon, 19 Aug 2024 19:48:25 +0000 Subject: [PATCH 05/15] Adding to exsiting NOLINTNEXTLINE if possible --- clang-tools-extra/clangd/ParsedAST.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 4cac2091864c66..253f44020011b6 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -425,20 +425,29 @@ clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, auto StartCurLine = CodeTilDiag.find_last_of('\n') + 1; auto CurLine = CodeTilDiag.substr(StartCurLine); - elog("CurLine: '{0}'", CurLine); auto Indent = CurLine.take_while([](char C) { return std::isspace(C); }); + auto ExistingNoLintNextLineInsertPos = std::optional<int>(); if (StartCurLine > 0) { auto StartPrevLine = CodeTilDiag.find_last_of('\n', StartCurLine - 1) + 1; auto PrevLine = CodeTilDiag.substr(StartPrevLine, StartCurLine - StartPrevLine - 1); - elog("PrevLine: '{0}'", PrevLine); + auto NLPos = PrevLine.find("NOLINTNEXTLINE("); + if (NLPos != StringRef::npos) { + ExistingNoLintNextLineInsertPos = + std::make_optional(PrevLine.find(")", NLPos)); + } } - E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName); - auto InsertPos = sourceLocToPosition(SrcMgr, DiagLoc); - InsertPos.character = 0; + if (ExistingNoLintNextLineInsertPos) { + E.newText = llvm::formatv(", {0}", RuleName); + InsertPos.line -= 1; + InsertPos.character = *ExistingNoLintNextLineInsertPos; + } else { + E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName); + InsertPos.character = 0; + } E.range = {InsertPos, InsertPos}; return {F}; >From 260745de470ebf488af9a880d016fcf597a3345e Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Thu, 3 Oct 2024 06:31:02 +0000 Subject: [PATCH 06/15] No NOLINT fix for Error Diagnostic --- clang-tools-extra/clangd/ParsedAST.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 253f44020011b6..98c158ab9d5a38 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -405,13 +405,12 @@ filterFastTidyChecks(const tidy::ClangTidyCheckFactories &All, std::vector<Fix> clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, const clang::Diagnostic &Info, const Diag &Diag) { - auto RuleName = CTContext.getCheckName(Info.getID()); - if (RuleName.empty()) { - return {}; - } - if (!Diag.InsideMainFile) { + auto RuleName = CTContext.getCheckName(Diag.ID); + if (RuleName.empty() || Diag.Severity >= DiagnosticsEngine::Error || + !Diag.InsideMainFile) { return {}; } + auto &SrcMgr = Info.getSourceManager(); auto &DiagLoc = Info.getLocation(); >From d618955bdf2c10cc3189b9953e0d4b3bbb9c39f6 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Fri, 4 Oct 2024 04:52:00 +0000 Subject: [PATCH 07/15] added comment for return {} branch --- clang-tools-extra/clangd/ParsedAST.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 98c158ab9d5a38..4e713258c58213 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -406,7 +406,12 @@ std::vector<Fix> clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, const clang::Diagnostic &Info, const Diag &Diag) { auto RuleName = CTContext.getCheckName(Diag.ID); - if (RuleName.empty() || Diag.Severity >= DiagnosticsEngine::Error || + if ( + // If this isn't a clang-tidy diag + RuleName.empty() || + // NOLINT does not work on Serverity Error or above + Diag.Severity >= DiagnosticsEngine::Error || + // No point adding extra fixes if the Diag is for a different file !Diag.InsideMainFile) { return {}; } >From 2992a2071e8e27e527f9e5c52eb0a602dc3eb30f Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Fri, 4 Oct 2024 20:44:21 +0000 Subject: [PATCH 08/15] found '(fix available)' insertion code --- clang-tools-extra/clangd/Diagnostics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 47810248f093e4..ee7d36e029fcde 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -312,7 +312,7 @@ std::string mainMessage(const Diag &D, const ClangdDiagnosticOptions &Opts) { llvm::raw_string_ostream OS(Result); OS << D.Message; if (Opts.DisplayFixesCount && !D.Fixes.empty()) - OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)"; + OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)"; // TODO // If notes aren't emitted as structured info, add them to the message. if (!Opts.EmitRelatedLocations) for (auto &Note : D.Notes) { >From 330d8518541f28e6aa7291de6ef5bac1fa63b946 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Sat, 5 Oct 2024 01:41:49 +0000 Subject: [PATCH 09/15] Don't display (fix available) when there's only NOLINT fix --- clang-tools-extra/clangd/CMakeLists.txt | 1 + clang-tools-extra/clangd/Diagnostics.cpp | 18 +++- clang-tools-extra/clangd/NoLintFixes.cpp | 102 +++++++++++++++++++++++ clang-tools-extra/clangd/NoLintFixes.h | 35 ++++++++ clang-tools-extra/clangd/ParsedAST.cpp | 57 +------------ 5 files changed, 153 insertions(+), 60 deletions(-) create mode 100644 clang-tools-extra/clangd/NoLintFixes.cpp create mode 100644 clang-tools-extra/clangd/NoLintFixes.h diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt index d797ddce8c44d1..2a6433a5c3effd 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -98,6 +98,7 @@ add_clang_library(clangDaemon STATIC InlayHints.cpp JSONTransport.cpp ModulesBuilder.cpp + NoLintFixes.cpp PathMapping.cpp Protocol.cpp Quality.cpp diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index ee7d36e029fcde..f345f7c88542b3 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -9,6 +9,7 @@ #include "Diagnostics.h" #include "../clang-tidy/ClangTidyDiagnosticConsumer.h" #include "Compiler.h" +#include "NoLintFixes.h" #include "Protocol.h" #include "SourceCode.h" #include "support/Logger.h" @@ -311,8 +312,18 @@ std::string mainMessage(const Diag &D, const ClangdDiagnosticOptions &Opts) { std::string Result; llvm::raw_string_ostream OS(Result); OS << D.Message; - if (Opts.DisplayFixesCount && !D.Fixes.empty()) - OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)"; // TODO + + // NOLINT fixes are somewhat not real fixes and to say "(fix available)" when + // the fixes is just to suppress could be misleading. + int RealFixCount = D.Fixes.size(); + for (auto const &Fix : D.Fixes) { + if (isClangTidyNoLintFixes(Fix)) { + RealFixCount--; + } + } + + if (Opts.DisplayFixesCount && RealFixCount > 0) + OS << " (" << (RealFixCount > 1 ? "fixes" : "fix") << " available)"; // If notes aren't emitted as structured info, add them to the message. if (!Opts.EmitRelatedLocations) for (auto &Note : D.Notes) { @@ -795,8 +806,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, } if (Message.empty()) // either !SyntheticMessage, or we failed to make one. Info.FormatDiagnostic(Message); - LastDiag->Fixes.push_back( - Fix{std::string(Message), std::move(Edits), {}}); + LastDiag->Fixes.push_back(Fix{std::string(Message), std::move(Edits), {}}); return true; }; diff --git a/clang-tools-extra/clangd/NoLintFixes.cpp b/clang-tools-extra/clangd/NoLintFixes.cpp new file mode 100644 index 00000000000000..2ba86b611a8612 --- /dev/null +++ b/clang-tools-extra/clangd/NoLintFixes.cpp @@ -0,0 +1,102 @@ +//===--- NoLintFixes.cpp -------------------------------------------*- +// C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "NoLintFixes.h" +#include "../clang-tidy/ClangTidyCheck.h" +#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" +#include "../clang-tidy/ClangTidyModule.h" +#include "AST.h" +#include "Diagnostics.h" +#include "FeatureModule.h" +#include "SourceCode.h" +#include "clang/AST/ASTContext.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/DiagnosticIDs.h" +#include "clang/Basic/LLVM.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Frontend/CompilerInvocation.h" +#include "clang/Lex/Lexer.h" +#include "clang/Serialization/ASTWriter.h" +#include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include <cassert> +#include <cctype> +#include <optional> +#include <regex> +#include <string> +#include <vector> + +namespace clang { +namespace clangd { + +std::vector<Fix> +clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, + const clang::Diagnostic &Info, const Diag &Diag) { + auto RuleName = CTContext.getCheckName(Diag.ID); + if ( + // If this isn't a clang-tidy diag + RuleName.empty() || + // NOLINT does not work on Serverity Error or above + Diag.Severity >= DiagnosticsEngine::Error || + // No point adding extra fixes if the Diag is for a different file + !Diag.InsideMainFile) { + return {}; + } + + auto &SrcMgr = Info.getSourceManager(); + auto &DiagLoc = Info.getLocation(); + + auto F = Fix{}; + F.Message = llvm::formatv("ignore [{0}] for this line", RuleName); + auto &E = F.Edits.emplace_back(); + + auto File = SrcMgr.getFileID(DiagLoc); + auto CodeTilDiag = toSourceCode( + SrcMgr, SourceRange(SrcMgr.getLocForStartOfFile(File), DiagLoc)); + + auto StartCurLine = CodeTilDiag.find_last_of('\n') + 1; + auto CurLine = CodeTilDiag.substr(StartCurLine); + auto Indent = CurLine.take_while([](char C) { return std::isspace(C); }); + + auto ExistingNoLintNextLineInsertPos = std::optional<int>(); + if (StartCurLine > 0) { + auto StartPrevLine = CodeTilDiag.find_last_of('\n', StartCurLine - 1) + 1; + auto PrevLine = + CodeTilDiag.substr(StartPrevLine, StartCurLine - StartPrevLine - 1); + auto NLPos = PrevLine.find("NOLINTNEXTLINE("); + if (NLPos != StringRef::npos) { + ExistingNoLintNextLineInsertPos = + std::make_optional(PrevLine.find(")", NLPos)); + } + } + + auto InsertPos = sourceLocToPosition(SrcMgr, DiagLoc); + if (ExistingNoLintNextLineInsertPos) { + E.newText = llvm::formatv(", {0}", RuleName); + InsertPos.line -= 1; + InsertPos.character = *ExistingNoLintNextLineInsertPos; + } else { + E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName); + InsertPos.character = 0; + } + E.range = {InsertPos, InsertPos}; + + return {F}; +} + +const auto MsgRegex = std::regex("ignore \\[.*\\] for this line"); +bool isClangTidyNoLintFixes(const Fix &F) { + return std::regex_match(F.Message, MsgRegex); +} + +} // namespace clangd +} // namespace clang diff --git a/clang-tools-extra/clangd/NoLintFixes.h b/clang-tools-extra/clangd/NoLintFixes.h new file mode 100644 index 00000000000000..dabe154585e0fb --- /dev/null +++ b/clang-tools-extra/clangd/NoLintFixes.h @@ -0,0 +1,35 @@ +//===--- NoLintFixes.h ------------------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H + +#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" +#include "../clang-tidy/ClangTidyModule.h" +#include "Diagnostics.h" +#include "FeatureModule.h" +#include "clang/Basic/Diagnostic.h" +#include <cassert> +#include <vector> + +namespace clang { +namespace clangd { + +/// Suggesting to insert "\\ NOLINTNEXTLINE(...)" to suppress clang-tidy +/// diagnostics. +std::vector<Fix> +clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, + const clang::Diagnostic &Info, const Diag &Diag); + +/// Check if a fix created by clangTidyNoLintFixes(). +bool isClangTidyNoLintFixes(const Fix &F); + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 4e713258c58213..eb267c66e9e953 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -23,6 +23,7 @@ #include "HeuristicResolver.h" #include "IncludeCleaner.h" #include "IncludeFixer.h" +#include "NoLintFixes.h" #include "Preamble.h" #include "SourceCode.h" #include "TidyProvider.h" @@ -67,7 +68,6 @@ #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include <cassert> -#include <cctype> #include <cstddef> #include <iterator> #include <memory> @@ -402,61 +402,6 @@ filterFastTidyChecks(const tidy::ClangTidyCheckFactories &All, } // namespace -std::vector<Fix> -clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, - const clang::Diagnostic &Info, const Diag &Diag) { - auto RuleName = CTContext.getCheckName(Diag.ID); - if ( - // If this isn't a clang-tidy diag - RuleName.empty() || - // NOLINT does not work on Serverity Error or above - Diag.Severity >= DiagnosticsEngine::Error || - // No point adding extra fixes if the Diag is for a different file - !Diag.InsideMainFile) { - return {}; - } - - auto &SrcMgr = Info.getSourceManager(); - auto &DiagLoc = Info.getLocation(); - - auto F = Fix{}; - F.Message = llvm::formatv("ignore [{0}] for this line", RuleName); - auto &E = F.Edits.emplace_back(); - - auto File = SrcMgr.getFileID(DiagLoc); - auto CodeTilDiag = toSourceCode( - SrcMgr, SourceRange(SrcMgr.getLocForStartOfFile(File), DiagLoc)); - - auto StartCurLine = CodeTilDiag.find_last_of('\n') + 1; - auto CurLine = CodeTilDiag.substr(StartCurLine); - auto Indent = CurLine.take_while([](char C) { return std::isspace(C); }); - - auto ExistingNoLintNextLineInsertPos = std::optional<int>(); - if (StartCurLine > 0) { - auto StartPrevLine = CodeTilDiag.find_last_of('\n', StartCurLine - 1) + 1; - auto PrevLine = - CodeTilDiag.substr(StartPrevLine, StartCurLine - StartPrevLine - 1); - auto NLPos = PrevLine.find("NOLINTNEXTLINE("); - if (NLPos != StringRef::npos) { - ExistingNoLintNextLineInsertPos = - std::make_optional(PrevLine.find(")", NLPos)); - } - } - - auto InsertPos = sourceLocToPosition(SrcMgr, DiagLoc); - if (ExistingNoLintNextLineInsertPos) { - E.newText = llvm::formatv(", {0}", RuleName); - InsertPos.line -= 1; - InsertPos.character = *ExistingNoLintNextLineInsertPos; - } else { - E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName); - InsertPos.character = 0; - } - E.range = {InsertPos, InsertPos}; - - return {F}; -} - std::optional<ParsedAST> ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::unique_ptr<clang::CompilerInvocation> CI, >From 6411c80be90b606449f6ab8bb50f21c7c9631ede Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Sat, 5 Oct 2024 10:14:30 +0000 Subject: [PATCH 10/15] changed withFix to containsFix in DiagnosticsTest to ignore NOLINT fix --- .../clangd/unittests/DiagnosticsTests.cpp | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 7a47d6ebebf3b2..da5786a4de2931 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -69,6 +69,11 @@ ::testing::Matcher<const Diag &> withFix(::testing::Matcher<Fix> FixMatcher1, return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2)); } +::testing::Matcher<const Diag &> +containsFix(::testing::Matcher<Fix> FixMatcher) { + return Field(&Diag::Fixes, Contains(FixMatcher)); +} + ::testing::Matcher<const Diag &> withID(unsigned ID) { return Field(&Diag::ID, ID); } @@ -1290,6 +1295,7 @@ TEST(IncludeFixerTest, IncompleteType) { {"call_incomplete_argument", "int m(ns::X); int i = m([[*x]]);"}, {"switch_incomplete_class_type", "void a() { [[switch]](*x) {} }"}, {"delete_incomplete_class_type", "void f() { [[delete]] *x; }"}, + // TODO: Add to test case {"-Wdelete-incomplete", "void f() { [[delete]] x; }"}, {"dereference_incomplete_type", R"cpp(void f() { asm("" : "=r"([[*]]x)::); })cpp"}, @@ -1297,11 +1303,12 @@ TEST(IncludeFixerTest, IncompleteType) { for (auto Case : Tests) { Annotations Main(Case.second); TU.Code = Main.code().str() + "\n // error-ok"; - EXPECT_THAT( - TU.build().getDiagnostics(), - ElementsAre(AllOf(diagName(Case.first), hasRange(Main.range()), - withFix(Fix(Range{}, "#include \"x.h\"\n", - "Include \"x.h\" for symbol ns::X"))))) + // TODO: maybe only do containsFix on -Wdelete-incomplete + EXPECT_THAT(TU.build().getDiagnostics(), + ElementsAre(AllOf( + diagName(Case.first), hasRange(Main.range()), + containsFix(Fix(Range{}, "#include \"x.h\"\n", + "Include \"x.h\" for symbol ns::X"))))) << Case.second; } } @@ -1662,8 +1669,9 @@ TEST(IncludeFixerTest, HeaderNamedInDiag) { "with type 'int (const char *, ...)'; ISO C99 " "and later do not support implicit function " "declarations"), - withFix(Fix(Test.range("insert"), "#include <stdio.h>\n", - "Include <stdio.h> for symbol printf"))))); + // TODO: Add to test case + containsFix(Fix(Test.range("insert"), "#include <stdio.h>\n", + "Include <stdio.h> for symbol printf"))))); TU.ExtraArgs = {"-xc", "-std=c89"}; EXPECT_THAT( @@ -1671,8 +1679,8 @@ TEST(IncludeFixerTest, HeaderNamedInDiag) { ElementsAre(AllOf( Diag(Test.range(), "implicitly declaring library function 'printf' " "with type 'int (const char *, ...)'"), - withFix(Fix(Test.range("insert"), "#include <stdio.h>\n", - "Include <stdio.h> for symbol printf"))))); + containsFix(Fix(Test.range("insert"), "#include <stdio.h>\n", + "Include <stdio.h> for symbol printf"))))); } TEST(IncludeFixerTest, CImplicitFunctionDecl) { @@ -1698,15 +1706,15 @@ TEST(IncludeFixerTest, CImplicitFunctionDecl) { Diag(Test.range(), "call to undeclared function 'foo'; ISO C99 and later do not " "support implicit function declarations"), - withFix(Fix(Range{}, "#include \"foo.h\"\n", - "Include \"foo.h\" for symbol foo"))))); + containsFix(Fix(Range{}, "#include \"foo.h\"\n", + "Include \"foo.h\" for symbol foo"))))); TU.ExtraArgs = {"-std=c89", "-Wall"}; EXPECT_THAT(TU.build().getDiagnostics(), ElementsAre(AllOf( Diag(Test.range(), "implicit declaration of function 'foo'"), - withFix(Fix(Range{}, "#include \"foo.h\"\n", - "Include \"foo.h\" for symbol foo"))))); + containsFix(Fix(Range{}, "#include \"foo.h\"\n", + "Include \"foo.h\" for symbol foo"))))); } TEST(DiagsInHeaders, DiagInsideHeader) { @@ -1930,10 +1938,10 @@ TEST(ParsedASTTest, ModuleSawDiag) { TestTU TU; auto AST = TU.build(); - #if 0 +#if 0 EXPECT_THAT(AST.getDiagnostics(), testing::Contains(Diag(Code.range(), KDiagMsg.str()))); - #endif +#endif } TEST(Preamble, EndsOnNonEmptyLine) { >From afc8df3ca22308dfc0c338705f0711842841c33e Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Sat, 5 Oct 2024 12:02:13 +0000 Subject: [PATCH 11/15] Filtered NOLINT fix in ClangTidyRename test --- clang-tools-extra/clangd/Diagnostics.cpp | 2 +- clang-tools-extra/clangd/NoLintFixes.cpp | 8 ++++---- clang-tools-extra/clangd/NoLintFixes.h | 9 +++++---- clang-tools-extra/clangd/ParsedAST.cpp | 2 +- .../clangd/unittests/ClangdLSPServerTests.cpp | 16 +++++++++++++--- 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index f345f7c88542b3..c1a4fc5c9ca7b1 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -317,7 +317,7 @@ std::string mainMessage(const Diag &D, const ClangdDiagnosticOptions &Opts) { // the fixes is just to suppress could be misleading. int RealFixCount = D.Fixes.size(); for (auto const &Fix : D.Fixes) { - if (isClangTidyNoLintFixes(Fix)) { + if (isNoLintFixes(Fix)) { RealFixCount--; } } diff --git a/clang-tools-extra/clangd/NoLintFixes.cpp b/clang-tools-extra/clangd/NoLintFixes.cpp index 2ba86b611a8612..80e2976c8bea30 100644 --- a/clang-tools-extra/clangd/NoLintFixes.cpp +++ b/clang-tools-extra/clangd/NoLintFixes.cpp @@ -39,7 +39,7 @@ namespace clang { namespace clangd { std::vector<Fix> -clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, +noLintFixes(const clang::tidy::ClangTidyContext &CTContext, const clang::Diagnostic &Info, const Diag &Diag) { auto RuleName = CTContext.getCheckName(Diag.ID); if ( @@ -93,9 +93,9 @@ clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, return {F}; } -const auto MsgRegex = std::regex("ignore \\[.*\\] for this line"); -bool isClangTidyNoLintFixes(const Fix &F) { - return std::regex_match(F.Message, MsgRegex); +const auto Regex = std::regex(NoLintFixMsgRegexStr); +bool isNoLintFixes(const Fix &F) { + return std::regex_match(F.Message, Regex); } } // namespace clangd diff --git a/clang-tools-extra/clangd/NoLintFixes.h b/clang-tools-extra/clangd/NoLintFixes.h index dabe154585e0fb..be67776fcbbf3d 100644 --- a/clang-tools-extra/clangd/NoLintFixes.h +++ b/clang-tools-extra/clangd/NoLintFixes.h @@ -22,12 +22,13 @@ namespace clangd { /// Suggesting to insert "\\ NOLINTNEXTLINE(...)" to suppress clang-tidy /// diagnostics. -std::vector<Fix> -clangTidyNoLintFixes(const clang::tidy::ClangTidyContext &CTContext, - const clang::Diagnostic &Info, const Diag &Diag); +std::vector<Fix> noLintFixes(const clang::tidy::ClangTidyContext &CTContext, + const clang::Diagnostic &Info, const Diag &Diag); + +const auto NoLintFixMsgRegexStr = std::string("ignore \\[.*\\] for this line"); /// Check if a fix created by clangTidyNoLintFixes(). -bool isClangTidyNoLintFixes(const Fix &F); +bool isNoLintFixes(const Fix &F); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index eb267c66e9e953..7e46cb62c18857 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -660,7 +660,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, [&FixIncludes, &CTContext](const Diag &Diag, const clang::Diagnostic &Info) { auto Fixes = std::vector<Fix>(); - auto NoLintFixes = clangTidyNoLintFixes(*CTContext, Info, Diag); + auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp index 49a94045ea4878..3593fd15d022ed 100644 --- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp @@ -15,6 +15,7 @@ #include "FeatureModule.h" #include "LSPBinder.h" #include "LSPClient.h" +#include "NoLintFixes.h" #include "TestFS.h" #include "support/Function.h" #include "support/Logger.h" @@ -32,6 +33,7 @@ #include "llvm/Testing/Support/SupportHelpers.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include <algorithm> #include <cassert> #include <condition_variable> #include <cstddef> @@ -39,6 +41,7 @@ #include <memory> #include <mutex> #include <optional> +#include <regex> #include <thread> #include <utility> @@ -222,7 +225,7 @@ TEST_F(LSPTest, ClangTidyRename) { ASSERT_TRUE(Diags && !Diags->empty()); auto RenameDiag = Diags->front(); - auto RenameCommand = + auto Fixes = (*Client .call("textDocument/codeAction", llvm::json::Object{ @@ -232,9 +235,16 @@ TEST_F(LSPTest, ClangTidyRename) { {"diagnostics", llvm::json::Array{RenameDiag}}}}, {"range", Source.range()}}) .takeValue() - .getAsArray())[0]; + .getAsArray()); + const auto NoLintRegex = std::regex("Apply fix: " + NoLintFixMsgRegexStr); + const auto &RenameCommand = *std::find_if( + Fixes.begin(), Fixes.end(), [&](decltype(Fixes)::value_type &Fix) { - ASSERT_EQ((*RenameCommand.getAsObject())["title"], "change 'foo' to 'Foo'"); + return !std::regex_match((*Fix.getAsObject())["title"].getAsString()->data(), NoLintRegex); + }); + + ASSERT_EQ((*RenameCommand.getAsObject()).get("title")->getAsString(), + "change 'foo' to 'Foo'"); Client.expectServerCall("workspace/applyEdit"); Client.call("workspace/executeCommand", RenameCommand); >From 4e29be27679cfdd112eb65ad85a3b60a026db407 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Mon, 7 Oct 2024 03:51:46 +0000 Subject: [PATCH 12/15] moved assert for Include-fixer to correct place --- clang-tools-extra/clangd/Diagnostics.cpp | 2 -- clang-tools-extra/clangd/ParsedAST.cpp | 26 +++++++++++++++--------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index c1a4fc5c9ca7b1..da40afbc3b22a5 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -854,8 +854,6 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, if (Fixer) { auto ReplacementFixes = Fixer(*LastDiag, Info); if (!ReplacementFixes.empty()) { - assert(Info.getNumFixItHints() == 0 && - "Include-fixer replaced a note with clang fix-its attached!"); LastDiag->Fixes.insert(LastDiag->Fixes.end(), ReplacementFixes.begin(), ReplacementFixes.end()); return; diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 7e46cb62c18857..949e54bb71ace5 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -656,16 +656,22 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : Symbol::Include; FixIncludes.emplace(Filename, Inserter, *Inputs.Index, /*IndexRequestLimit=*/5, Directive); - ASTDiags.contributeFixes( - [&FixIncludes, &CTContext](const Diag &Diag, - const clang::Diagnostic &Info) { - auto Fixes = std::vector<Fix>(); - auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); - Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); - auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); - Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); - return Fixes; - }); + ASTDiags.contributeFixes([&FixIncludes, + &CTContext](const Diag &Diag, + const clang::Diagnostic &Info) { + auto Fixes = std::vector<Fix>(); + auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); + + // Ensures that if clang later introduces its own fix-it for includes it + // will get on our radar. + assert((IncludeFixes.empty() || Info.getNumFixItHints() == 0) && + "Include-fixer replaced a note with clang fix-its attached!"); + + Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); + auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); + Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); + return Fixes; + }); Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } } >From 1cc47619bd7100b9d7c464ff488ee1af90fb58e7 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Mon, 7 Oct 2024 03:59:59 +0000 Subject: [PATCH 13/15] better deal with converting fixes to rename while avoiding NoLint fixes --- clang-tools-extra/clangd/ClangdServer.cpp | 24 +++++++++-------------- clang-tools-extra/clangd/ParsedAST.cpp | 5 +++-- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 38808c5549608c..925a015ae341bc 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -15,6 +15,7 @@ #include "Format.h" #include "HeaderSourceSwitch.h" #include "InlayHints.h" +#include "NoLintFixes.h" #include "ParsedAST.h" #include "Preamble.h" #include "Protocol.h" @@ -62,8 +63,8 @@ namespace clangd { namespace { // Tracks number of times a tweak has been offered. -static constexpr trace::Metric TweakAvailable( - "tweak_available", trace::Metric::Counter, "tweak_id"); +static constexpr trace::Metric + TweakAvailable("tweak_available", trace::Metric::Counter, "tweak_id"); // Update the FileIndex with new ASTs and plumb the diagnostics responses. struct UpdateIndexCallbacks : public ParsingCallbacks { @@ -661,7 +662,7 @@ tryConvertToRename(const Diag *Diag, const Fix &Fix) { bool IsClangTidyRename = Diag->Source == Diag::ClangTidy && Diag->Name == "readability-identifier-naming" && !Fix.Edits.empty(); - if (IsClangTidyRename && Diag->InsideMainFile) { + if (IsClangTidyRename && !isNoLintFixes(Fix) && Diag->InsideMainFile) { ClangdServer::CodeActionResult::Rename R; R.NewName = Fix.Edits.front().newText; R.FixMessage = Fix.Message; @@ -701,21 +702,14 @@ void ClangdServer::codeAction(const CodeActionInputs &Params, return nullptr; }; for (const auto &DiagRef : Params.Diagnostics) { - if (const auto *Diag = FindMatchedDiag(DiagRef)) { - auto It = Diag->Fixes.begin(); - if (It != Diag->Fixes.end()) { - if (auto Rename = tryConvertToRename(Diag, *It)) { - // Only try to convert the first Fix to rename as subsequent Fixes - // might be "ignore [readability-identifier-naming] for this - // line". + if (const auto *Diag = FindMatchedDiag(DiagRef)) + for (const auto &Fix : Diag->Fixes) { + if (auto Rename = tryConvertToRename(Diag, Fix)) { Result.Renames.emplace_back(std::move(*Rename)); - It++; + } else { + Result.QuickFixes.push_back({DiagRef, Fix}); } } - for (; It != Diag->Fixes.end(); It++) { - Result.QuickFixes.push_back({DiagRef, *It}); - } - } } } diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 949e54bb71ace5..095c3b02e3613a 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -660,16 +660,17 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, &CTContext](const Diag &Diag, const clang::Diagnostic &Info) { auto Fixes = std::vector<Fix>(); - auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); + auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); // Ensures that if clang later introduces its own fix-it for includes it // will get on our radar. assert((IncludeFixes.empty() || Info.getNumFixItHints() == 0) && "Include-fixer replaced a note with clang fix-its attached!"); - Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); + auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); + return Fixes; }); Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); >From a917bea81357b9c21007f414a5338cc60d418eeb Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Wed, 9 Oct 2024 00:14:04 +0000 Subject: [PATCH 14/15] Revert "moved assert for Include-fixer to correct place" This reverts commit 039ecf375d0d71524b1c3a904f93e57a10807b41. --- clang-tools-extra/clangd/Diagnostics.cpp | 2 ++ clang-tools-extra/clangd/ParsedAST.cpp | 27 +++++++++--------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index da40afbc3b22a5..c1a4fc5c9ca7b1 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -854,6 +854,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, if (Fixer) { auto ReplacementFixes = Fixer(*LastDiag, Info); if (!ReplacementFixes.empty()) { + assert(Info.getNumFixItHints() == 0 && + "Include-fixer replaced a note with clang fix-its attached!"); LastDiag->Fixes.insert(LastDiag->Fixes.end(), ReplacementFixes.begin(), ReplacementFixes.end()); return; diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 095c3b02e3613a..7e46cb62c18857 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -656,23 +656,16 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : Symbol::Include; FixIncludes.emplace(Filename, Inserter, *Inputs.Index, /*IndexRequestLimit=*/5, Directive); - ASTDiags.contributeFixes([&FixIncludes, - &CTContext](const Diag &Diag, - const clang::Diagnostic &Info) { - auto Fixes = std::vector<Fix>(); - - auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); - // Ensures that if clang later introduces its own fix-it for includes it - // will get on our radar. - assert((IncludeFixes.empty() || Info.getNumFixItHints() == 0) && - "Include-fixer replaced a note with clang fix-its attached!"); - Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); - - auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); - Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); - - return Fixes; - }); + ASTDiags.contributeFixes( + [&FixIncludes, &CTContext](const Diag &Diag, + const clang::Diagnostic &Info) { + auto Fixes = std::vector<Fix>(); + auto NoLintFixes = noLintFixes(*CTContext, Info, Diag); + Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end()); + auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info); + Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); + return Fixes; + }); Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } } >From e8c2afa814747650138907eadf2c5743720e3d10 Mon Sep 17 00:00:00 2001 From: chomosuke <a13323...@gmail.com> Date: Wed, 9 Oct 2024 01:07:54 +0000 Subject: [PATCH 15/15] fixed all tests --- clang-tools-extra/clangd/Diagnostics.cpp | 8 +-- clang-tools-extra/clangd/Diagnostics.h | 10 ++- clang-tools-extra/clangd/ParsedAST.cpp | 7 ++- .../fixits-codeaction-documentchanges.test | 47 ++++++++++++++ .../clangd/test/fixits-codeaction.test | 41 ++++++++++++ .../test/fixits-command-documentchanges.test | 62 +++++++++++++++++++ .../clangd/test/fixits-command.test | 50 +++++++++++++++ 7 files changed, 217 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index c1a4fc5c9ca7b1..e42da2a9135e9b 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -832,8 +832,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, LastDiagOriginallyError = OriginallyError; if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); - if (Fixer) { - auto ExtraFixes = Fixer(*LastDiag, Info); + if (MainFixer) { + auto ExtraFixes = MainFixer(*LastDiag, Info); LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(), ExtraFixes.end()); } @@ -851,8 +851,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, return; // Give include-fixer a chance to replace a note with a fix. - if (Fixer) { - auto ReplacementFixes = Fixer(*LastDiag, Info); + if (NoteFixer) { + auto ReplacementFixes = NoteFixer(*LastDiag, Info); if (!ReplacementFixes.empty()) { assert(Info.getNumFixItHints() == 0 && "Include-fixer replaced a note with clang fix-its attached!"); diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index 4fe41fecd8ae5c..1665cc0aa0a6a7 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -147,15 +147,18 @@ class StoreDiags : public DiagnosticConsumer { const clang::Diagnostic &Info) override; /// When passed a main diagnostic, returns fixes to add to it. + using MainDiagFixer = + std::function<std::vector<Fix>(const Diag &, const clang::Diagnostic &)>; /// When passed a note diagnostic, returns fixes to replace it with. - using DiagFixer = + using NoteDiagFixer = std::function<std::vector<Fix>(const Diag &, const clang::Diagnostic &)>; using LevelAdjuster = std::function<DiagnosticsEngine::Level( DiagnosticsEngine::Level, const clang::Diagnostic &)>; using DiagCallback = std::function<void(const clang::Diagnostic &, clangd::Diag &)>; /// If set, possibly adds fixes for diagnostics using \p Fixer. - void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; } + void contributeMainDiagFixes(MainDiagFixer Fixer) { this->MainFixer = Fixer; } + void contributeNoteDiagFixes(NoteDiagFixer Fixer) { this->NoteFixer = Fixer; } /// If set, this allows the client of this class to adjust the level of /// diagnostics, such as promoting warnings to errors, or ignoring /// diagnostics. @@ -167,7 +170,8 @@ class StoreDiags : public DiagnosticConsumer { private: void flushLastDiag(); - DiagFixer Fixer = nullptr; + MainDiagFixer MainFixer = nullptr; + NoteDiagFixer NoteFixer = nullptr; LevelAdjuster Adjuster = nullptr; DiagCallback DiagCB = nullptr; std::vector<Diag> Output; diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 7e46cb62c18857..cfd5b53b3e292d 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -656,7 +656,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : Symbol::Include; FixIncludes.emplace(Filename, Inserter, *Inputs.Index, /*IndexRequestLimit=*/5, Directive); - ASTDiags.contributeFixes( + ASTDiags.contributeMainDiagFixes( [&FixIncludes, &CTContext](const Diag &Diag, const clang::Diagnostic &Info) { auto Fixes = std::vector<Fix>(); @@ -666,6 +666,11 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end()); return Fixes; }); + ASTDiags.contributeNoteDiagFixes( + [&FixIncludes](const Diag &Diag, + const clang::Diagnostic &Info) { + return FixIncludes->fix(Diag.Severity, Info); + }); Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } } diff --git a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test index 7a591319a74054..3481377a8d1dfa 100644 --- a/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test +++ b/clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test @@ -54,6 +54,53 @@ # CHECK-NEXT: { # CHECK-NEXT: "edits": [ # CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "textDocument": { +# CHECK-NEXT: "uri": "file:///clangd-test/foo.c", +# CHECK-NEXT: "version": 1 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: }, +# CHECK-NEXT: "kind": "quickfix", +# CHECK-NEXT: "title": "ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "diagnostics": [ +# CHECK-NEXT: { +# CHECK-NEXT: "code": "-Wparentheses", +# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 37, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 32, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: }, +# CHECK-NEXT: "severity": 2, +# CHECK-NEXT: "source": "clang" +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "edit": { +# CHECK-NEXT: "documentChanges": [ +# CHECK-NEXT: { +# CHECK-NEXT: "edits": [ +# CHECK-NEXT: { # CHECK-NEXT: "newText": "(", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { diff --git a/clang-tools-extra/clangd/test/fixits-codeaction.test b/clang-tools-extra/clangd/test/fixits-codeaction.test index 75d0fb012ffc81..6e844fbed5b74a 100644 --- a/clang-tools-extra/clangd/test/fixits-codeaction.test +++ b/clang-tools-extra/clangd/test/fixits-codeaction.test @@ -51,6 +51,47 @@ # CHECK-NEXT: ], # CHECK-NEXT: "edit": { # CHECK-NEXT: "changes": { +# CHECK-NEXT: "file:///clangd-test/foo.c": [ +# CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: }, +# CHECK-NEXT: "kind": "quickfix", +# CHECK-NEXT: "title": "ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "diagnostics": [ +# CHECK-NEXT: { +# CHECK-NEXT: "code": "-Wparentheses", +# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 37, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 32, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: }, +# CHECK-NEXT: "severity": 2, +# CHECK-NEXT: "source": "clang" +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "edit": { +# CHECK-NEXT: "changes": { # CHECK-NEXT: "file://{{.*}}/foo.c": [ # CHECK-NEXT: { # CHECK-NEXT: "newText": "(", diff --git a/clang-tools-extra/clangd/test/fixits-command-documentchanges.test b/clang-tools-extra/clangd/test/fixits-command-documentchanges.test index cd636c4df387ad..4fe127776c825b 100644 --- a/clang-tools-extra/clangd/test/fixits-command-documentchanges.test +++ b/clang-tools-extra/clangd/test/fixits-command-documentchanges.test @@ -37,6 +37,37 @@ # CHECK-NEXT: { # CHECK-NEXT: "edits": [ # CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "textDocument": { +# CHECK-NEXT: "uri": "file:///clangd-test/foo.c", +# CHECK-NEXT: "version": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "command": "clangd.applyFix", +# CHECK-NEXT: "title": "Apply fix: ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "arguments": [ +# CHECK-NEXT: { +# CHECK-NEXT: "documentChanges": [ +# CHECK-NEXT: { +# CHECK-NEXT: "edits": [ +# CHECK-NEXT: { # CHECK-NEXT: "newText": "(", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { @@ -119,6 +150,37 @@ # CHECK-NEXT: { # CHECK-NEXT: "edits": [ # CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "textDocument": { +# CHECK-NEXT: "uri": "file:///clangd-test/foo.c", +# CHECK-NEXT: "version": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "command": "clangd.applyFix", +# CHECK-NEXT: "title": "Apply fix: ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "arguments": [ +# CHECK-NEXT: { +# CHECK-NEXT: "documentChanges": [ +# CHECK-NEXT: { +# CHECK-NEXT: "edits": [ +# CHECK-NEXT: { # CHECK-NEXT: "newText": "(", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { diff --git a/clang-tools-extra/clangd/test/fixits-command.test b/clang-tools-extra/clangd/test/fixits-command.test index 62b5a6152d2cf3..8cc6daee48d971 100644 --- a/clang-tools-extra/clangd/test/fixits-command.test +++ b/clang-tools-extra/clangd/test/fixits-command.test @@ -34,6 +34,31 @@ # CHECK-NEXT: "arguments": [ # CHECK-NEXT: { # CHECK-NEXT: "changes": { +# CHECK-NEXT: "file:///clangd-test/foo.c": [ +# CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "command": "clangd.applyFix", +# CHECK-NEXT: "title": "Apply fix: ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "arguments": [ +# CHECK-NEXT: { +# CHECK-NEXT: "changes": { # CHECK-NEXT: "file://{{.*}}/foo.c": [ # CHECK-NEXT: { # CHECK-NEXT: "newText": "(", @@ -104,6 +129,31 @@ # CHECK-NEXT: "arguments": [ # CHECK-NEXT: { # CHECK-NEXT: "changes": { +# CHECK-NEXT: "file:///clangd-test/foo.c": [ +# CHECK-NEXT: { +# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n", +# CHECK-NEXT: "range": { +# CHECK-NEXT: "end": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: }, +# CHECK-NEXT: "start": { +# CHECK-NEXT: "character": 0, +# CHECK-NEXT: "line": 0 +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: } +# CHECK-NEXT: ], +# CHECK-NEXT: "command": "clangd.applyFix", +# CHECK-NEXT: "title": "Apply fix: ignore [clang-diagnostic-parentheses] for this line" +# CHECK-NEXT: }, +# CHECK-NEXT: { +# CHECK-NEXT: "arguments": [ +# CHECK-NEXT: { +# CHECK-NEXT: "changes": { # CHECK-NEXT: "file://{{.*}}/foo.c": [ # CHECK-NEXT: { # CHECK-NEXT: "newText": "(", _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits