[clangd] Add quick fix to automatically adds NOLINTNEXTLINE comment #114661
Conversation
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
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
chomosuke
force-pushed
the
clang-tidy-nolint-fix
branch
5 times, most recently
from
581004d to
35cbe23
Compare
chomosuke
force-pushed
the
clang-tidy-nolint-fix
branch
from
35cbe23 to
311c752
Compare
Member
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Richard Li (chomosuke) ChangesFor some other lsp / linters. They will offer a "Ignore this error for this line" code actions for warnings. I find that convenient as I don't have to type the comment myself and copy the name of the diagnostics. This PR implements this for clangd. For all diagnostics from clang-tidy, this PR adds an extra quick fix to insert a // NOLINTNEXTLINE(diag-name) to suppress the diagnostics. This is the first time I work on such a large code base so any feedback and guidance are greatly appreciated. Patch is 31.44 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114661.diff 13 Files Affected:
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/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 9b38be04e7ddd7..7f73b61b12c63f 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"
@@ -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;
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index a59d1e7ac84096..6948e12995bed1 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)";
+
+ // 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 (isNoLintFixes(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) {
@@ -822,8 +833,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->Severity, Info);
+ if (MainFixer) {
+ auto ExtraFixes = MainFixer(*LastDiag, Info);
LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
ExtraFixes.end());
}
@@ -841,8 +852,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->Severity, 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 d4c0478c63a5cf..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 = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
- const clang::Diagnostic &)>;
+ 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/NoLintFixes.cpp b/clang-tools-extra/clangd/NoLintFixes.cpp
new file mode 100644
index 00000000000000..cad828b9e6910a
--- /dev/null
+++ b/clang-tools-extra/clangd/NoLintFixes.cpp
@@ -0,0 +1,99 @@
+//===--- 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> noLintFixes(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 = SrcMgr.getSpellingLoc(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 FileLoc = SrcMgr.getLocForStartOfFile(File);
+ auto CodeTilDiag = toSourceCode(SrcMgr, SourceRange(FileLoc, 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 Regex = std::regex(NoLintFixMsgRegexStr);
+bool isNoLintFixes(const Fix &F) { return std::regex_match(F.Message, Regex); }
+
+} // 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..be67776fcbbf3d
--- /dev/null
+++ b/clang-tools-extra/clangd/NoLintFixes.h
@@ -0,0 +1,36 @@
+//===--- 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> 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 isNoLintFixes(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 045d32afbc938a..044b6581333fce 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"
@@ -655,10 +656,20 @@ 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.contributeMainDiagFixes(
+ [&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.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..94ebf655ca9ca2 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://{{.*}}/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..243dafa17469a0 100644
--- a/clang-tools-extra/clangd/test/fixits-codeaction.test
+++ b/clang-tools-extra/clangd/test/fixits-codeaction.test
@@ -53,6 +53,47 @@
# CHECK-NEXT: "changes": {
# CHECK-NEXT: "file://{{.*}}/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": "(",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
diff --git a/clang-tools-extra/clangd/test/fixits-command-documentchanges.test b/clang-tools-extra/clangd/test/fixits-command-documentchanges.test
index cd636c4df387ad..7cc924923a16a0 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://{{.*}}/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://{{.*}}/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": "(",
# CHE...
[truncated]
|
kadircet
requested changes
Nov 4, 2024
Member
kadircet
left a comment
kadircet
left a comment
There was a problem hiding this comment.
thanks a lot for this patch!
I am not sure if this is a "good" fix-it for C++ ecosystem. As it'll make suppression way easier. Today people needs to decide whether a suppression is warranted, and explain it in a comment. at that point the cost of figuring out how to suppress a finding and typing the characters are definitely not the bottleneck. Whereas after this change people will always have this available with a single click, and they'll consider the issue is "fixed" without any investigation (i know this doesn't sound "right", but speaking from experience, that's how people treat fixes provided by their compiler, even if it just says suppress).
That being said, if majority of people here is convinced this is a useful fix to have in clangd, can we at least use FeatureModules infrastructure instead? https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp shows some of the intended use case. It was meant to provide a way to extend clangd functionality without modifying core logic (as you do here). You can attach an ASTListener to look for all the diagnostics and provide a tweak contributor to attach fixes when an interesting diagnostic is part of the selection.
Author
|
Thank you for the prompt review!! I am currently working on something else now but I'll address this PR asap once I'm done with that :>. And thank you for letting me know about |
Author
|
Hi @kadircet, I have just started looking into I assume I should extend |
Member
|
(sorry for the late reply)
we do have some out-of-tree usages, but nothing in the tree, apart from tests, use this infrastructure yet.
yes, we basically should have a new feature-module, from that new feature-module we can contribute a tweak. this tweak can look at all the diagnostics in the AST, find the ones that intersect with users selection, and if they're from clang-tidy emit a code-action that inserts `` // NOLINTNEXTLINE(check-name)`. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
For some other lsp / linters. They will offer a "Ignore this error for this line" code actions for warnings. I find that convenient as I don't have to type the comment myself and copy the name of the diagnostics.
This PR implements this for clangd. For all diagnostics from clang-tidy, this PR adds an extra quick fix to insert a // NOLINTNEXTLINE(diag-name) to suppress the diagnostics.
This is the first time I work on such a large code base so any feedback and guidance are greatly appreciated.