Skip to content

Commit f52c645

Browse files
chomosukechomosuke
committed
Add quick fix to automatically adds NOLINTNEXTLINE comment above clang-tidy warnings
1 parent 7603fea commit f52c645

13 files changed

+492
-32
lines changed

‎clang-tools-extra/clangd/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ add_clang_library(clangDaemon STATIC
9898
InlayHints.cpp
9999
JSONTransport.cpp
100100
ModulesBuilder.cpp
101+
NoLintFixes.cpp
101102
PathMapping.cpp
102103
Protocol.cpp
103104
Quality.cpp

‎clang-tools-extra/clangd/ClangdServer.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "Format.h"
1616
#include "HeaderSourceSwitch.h"
1717
#include "InlayHints.h"
18+
#include "NoLintFixes.h"
1819
#include "ParsedAST.h"
1920
#include "Preamble.h"
2021
#include "Protocol.h"
@@ -661,7 +662,7 @@ tryConvertToRename(const Diag *Diag, const Fix &Fix) {
661662
bool IsClangTidyRename = Diag->Source == Diag::ClangTidy &&
662663
Diag->Name == "readability-identifier-naming" &&
663664
!Fix.Edits.empty();
664-
if (IsClangTidyRename && Diag->InsideMainFile) {
665+
if (IsClangTidyRename && !isNoLintFixes(Fix) && Diag->InsideMainFile) {
665666
ClangdServer::CodeActionResult::Rename R;
666667
R.NewName = Fix.Edits.front().newText;
667668
R.FixMessage = Fix.Message;

‎clang-tools-extra/clangd/Diagnostics.cpp‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "Diagnostics.h"
1010
#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
1111
#include "Compiler.h"
12+
#include "NoLintFixes.h"
1213
#include "Protocol.h"
1314
#include "SourceCode.h"
1415
#include "support/Logger.h"
@@ -311,8 +312,18 @@ std::string mainMessage(const Diag &D, const ClangdDiagnosticOptions &Opts) {
311312
std::string Result;
312313
llvm::raw_string_ostream OS(Result);
313314
OS << D.Message;
314-
if (Opts.DisplayFixesCount && !D.Fixes.empty())
315-
OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)";
315+
316+
// NOLINT fixes are somewhat not real fixes and to say "(fix available)" when
317+
// the fixes is just to suppress could be misleading.
318+
int RealFixCount = D.Fixes.size();
319+
for (auto const &Fix : D.Fixes) {
320+
if (isNoLintFixes(Fix)) {
321+
RealFixCount--;
322+
}
323+
}
324+
325+
if (Opts.DisplayFixesCount && RealFixCount > 0)
326+
OS << " (" << (RealFixCount > 1 ? "fixes" : "fix") << " available)";
316327
// If notes aren't emitted as structured info, add them to the message.
317328
if (!Opts.EmitRelatedLocations)
318329
for (auto &Note : D.Notes) {
@@ -822,8 +833,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
822833
LastDiagOriginallyError = OriginallyError;
823834
if (!Info.getFixItHints().empty())
824835
AddFix(true /* try to invent a message instead of repeating the diag */);
825-
if (Fixer) {
826-
auto ExtraFixes = Fixer(LastDiag->Severity, Info);
836+
if (MainFixer) {
837+
auto ExtraFixes = MainFixer(*LastDiag, Info);
827838
LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
828839
ExtraFixes.end());
829840
}
@@ -841,8 +852,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
841852
return;
842853

843854
// Give include-fixer a chance to replace a note with a fix.
844-
if (Fixer) {
845-
auto ReplacementFixes = Fixer(LastDiag->Severity, Info);
855+
if (NoteFixer) {
856+
auto ReplacementFixes = NoteFixer(*LastDiag, Info);
846857
if (!ReplacementFixes.empty()) {
847858
assert(Info.getNumFixItHints() == 0 &&
848859
"Include-fixer replaced a note with clang fix-its attached!");

‎clang-tools-extra/clangd/Diagnostics.h‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,15 +147,18 @@ class StoreDiags : public DiagnosticConsumer {
147147
const clang::Diagnostic &Info) override;
148148

149149
/// When passed a main diagnostic, returns fixes to add to it.
150+
using MainDiagFixer =
151+
std::function<std::vector<Fix>(const Diag &, const clang::Diagnostic &)>;
150152
/// When passed a note diagnostic, returns fixes to replace it with.
151-
using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
152-
const clang::Diagnostic &)>;
153+
using NoteDiagFixer =
154+
std::function<std::vector<Fix>(const Diag &, const clang::Diagnostic &)>;
153155
using LevelAdjuster = std::function<DiagnosticsEngine::Level(
154156
DiagnosticsEngine::Level, const clang::Diagnostic &)>;
155157
using DiagCallback =
156158
std::function<void(const clang::Diagnostic &, clangd::Diag &)>;
157159
/// If set, possibly adds fixes for diagnostics using \p Fixer.
158-
void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
160+
void contributeMainDiagFixes(MainDiagFixer Fixer) { this->MainFixer = Fixer; }
161+
void contributeNoteDiagFixes(NoteDiagFixer Fixer) { this->NoteFixer = Fixer; }
159162
/// If set, this allows the client of this class to adjust the level of
160163
/// diagnostics, such as promoting warnings to errors, or ignoring
161164
/// diagnostics.
@@ -167,7 +170,8 @@ class StoreDiags : public DiagnosticConsumer {
167170
private:
168171
void flushLastDiag();
169172

170-
DiagFixer Fixer = nullptr;
173+
MainDiagFixer MainFixer = nullptr;
174+
NoteDiagFixer NoteFixer = nullptr;
171175
LevelAdjuster Adjuster = nullptr;
172176
DiagCallback DiagCB = nullptr;
173177
std::vector<Diag> Output;

‎clang-tools-extra/clangd/NoLintFixes.cpp‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
//===--- NoLintFixes.cpp -------------------------------------------*-
2+
// C++-*-===//
3+
//
4+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
5+
// See https://llvm.org/LICENSE.txt for license information.
6+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "NoLintFixes.h"
11+
#include "../clang-tidy/ClangTidyCheck.h"
12+
#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
13+
#include "../clang-tidy/ClangTidyModule.h"
14+
#include "AST.h"
15+
#include "Diagnostics.h"
16+
#include "FeatureModule.h"
17+
#include "SourceCode.h"
18+
#include "clang/AST/ASTContext.h"
19+
#include "clang/Basic/Diagnostic.h"
20+
#include "clang/Basic/DiagnosticIDs.h"
21+
#include "clang/Basic/LLVM.h"
22+
#include "clang/Basic/SourceLocation.h"
23+
#include "clang/Basic/SourceManager.h"
24+
#include "clang/Frontend/CompilerInstance.h"
25+
#include "clang/Frontend/CompilerInvocation.h"
26+
#include "clang/Lex/Lexer.h"
27+
#include "clang/Serialization/ASTWriter.h"
28+
#include "llvm/ADT/STLFunctionalExtras.h"
29+
#include "llvm/ADT/SmallVector.h"
30+
#include "llvm/ADT/StringRef.h"
31+
#include <cassert>
32+
#include <cctype>
33+
#include <optional>
34+
#include <regex>
35+
#include <string>
36+
#include <vector>
37+
38+
namespace clang {
39+
namespace clangd {
40+
41+
std::vector<Fix> noLintFixes(const clang::tidy::ClangTidyContext &CTContext,
42+
const clang::Diagnostic &Info, const Diag &Diag) {
43+
auto RuleName = CTContext.getCheckName(Diag.ID);
44+
if (
45+
// If this isn't a clang-tidy diag
46+
RuleName.empty() ||
47+
// NOLINT does not work on Serverity Error or above
48+
Diag.Severity >= DiagnosticsEngine::Error ||
49+
// No point adding extra fixes if the Diag is for a different file
50+
!Diag.InsideMainFile) {
51+
return {};
52+
}
53+
54+
auto &SrcMgr = Info.getSourceManager();
55+
auto DiagLoc = SrcMgr.getSpellingLoc(Info.getLocation());
56+
57+
auto F = Fix{};
58+
F.Message = llvm::formatv("ignore [{0}] for this line", RuleName);
59+
auto &E = F.Edits.emplace_back();
60+
61+
auto File = SrcMgr.getFileID(DiagLoc);
62+
auto FileLoc = SrcMgr.getLocForStartOfFile(File);
63+
auto CodeTilDiag = toSourceCode(SrcMgr, SourceRange(FileLoc, DiagLoc));
64+
65+
auto StartCurLine = CodeTilDiag.find_last_of('\n') + 1;
66+
auto CurLine = CodeTilDiag.substr(StartCurLine);
67+
auto Indent = CurLine.take_while([](char C) { return std::isspace(C); });
68+
69+
auto ExistingNoLintNextLineInsertPos = std::optional<int>();
70+
if (StartCurLine > 0) {
71+
auto StartPrevLine = CodeTilDiag.find_last_of('\n', StartCurLine - 1) + 1;
72+
auto PrevLine =
73+
CodeTilDiag.substr(StartPrevLine, StartCurLine - StartPrevLine - 1);
74+
auto NLPos = PrevLine.find("NOLINTNEXTLINE(");
75+
if (NLPos != StringRef::npos) {
76+
ExistingNoLintNextLineInsertPos =
77+
std::make_optional(PrevLine.find(")", NLPos));
78+
}
79+
}
80+
81+
auto InsertPos = sourceLocToPosition(SrcMgr, DiagLoc);
82+
if (ExistingNoLintNextLineInsertPos) {
83+
E.newText = llvm::formatv(", {0}", RuleName);
84+
InsertPos.line -= 1;
85+
InsertPos.character = *ExistingNoLintNextLineInsertPos;
86+
} else {
87+
E.newText = llvm::formatv("{0}// NOLINTNEXTLINE({1})\n", Indent, RuleName);
88+
InsertPos.character = 0;
89+
}
90+
E.range = {InsertPos, InsertPos};
91+
92+
return {F};
93+
}
94+
95+
const auto Regex = std::regex(NoLintFixMsgRegexStr);
96+
bool isNoLintFixes(const Fix &F) { return std::regex_match(F.Message, Regex); }
97+
98+
} // namespace clangd
99+
} // namespace clang

‎clang-tools-extra/clangd/NoLintFixes.h‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
//===--- NoLintFixes.h ------------------------------------------*- C++-*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H
11+
12+
#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
13+
#include "../clang-tidy/ClangTidyModule.h"
14+
#include "Diagnostics.h"
15+
#include "FeatureModule.h"
16+
#include "clang/Basic/Diagnostic.h"
17+
#include <cassert>
18+
#include <vector>
19+
20+
namespace clang {
21+
namespace clangd {
22+
23+
/// Suggesting to insert "\\ NOLINTNEXTLINE(...)" to suppress clang-tidy
24+
/// diagnostics.
25+
std::vector<Fix> noLintFixes(const clang::tidy::ClangTidyContext &CTContext,
26+
const clang::Diagnostic &Info, const Diag &Diag);
27+
28+
const auto NoLintFixMsgRegexStr = std::string("ignore \\[.*\\] for this line");
29+
30+
/// Check if a fix created by clangTidyNoLintFixes().
31+
bool isNoLintFixes(const Fix &F);
32+
33+
} // namespace clangd
34+
} // namespace clang
35+
36+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_NOLINTFIXES_H

‎clang-tools-extra/clangd/ParsedAST.cpp‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "HeuristicResolver.h"
2424
#include "IncludeCleaner.h"
2525
#include "IncludeFixer.h"
26+
#include "NoLintFixes.h"
2627
#include "Preamble.h"
2728
#include "SourceCode.h"
2829
#include "TidyProvider.h"
@@ -655,10 +656,20 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
655656
: Symbol::Include;
656657
FixIncludes.emplace(Filename, Inserter, *Inputs.Index,
657658
/*IndexRequestLimit=*/5, Directive);
658-
ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,
659-
const clang::Diagnostic &Info) {
660-
return FixIncludes->fix(DiagLevl, Info);
661-
});
659+
ASTDiags.contributeMainDiagFixes(
660+
[&FixIncludes, &CTContext](const Diag &Diag,
661+
const clang::Diagnostic &Info) {
662+
auto Fixes = std::vector<Fix>();
663+
auto NoLintFixes = noLintFixes(*CTContext, Info, Diag);
664+
Fixes.insert(Fixes.end(), NoLintFixes.begin(), NoLintFixes.end());
665+
auto IncludeFixes = FixIncludes->fix(Diag.Severity, Info);
666+
Fixes.insert(Fixes.end(), IncludeFixes.begin(), IncludeFixes.end());
667+
return Fixes;
668+
});
669+
ASTDiags.contributeNoteDiagFixes(
670+
[&FixIncludes](const Diag &Diag, const clang::Diagnostic &Info) {
671+
return FixIncludes->fix(Diag.Severity, Info);
672+
});
662673
Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder());
663674
}
664675
}

‎clang-tools-extra/clangd/test/fixits-codeaction-documentchanges.test‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,53 @@
5454
# CHECK-NEXT: {
5555
# CHECK-NEXT: "edits": [
5656
# CHECK-NEXT: {
57+
# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n",
58+
# CHECK-NEXT: "range": {
59+
# CHECK-NEXT: "end": {
60+
# CHECK-NEXT: "character": 0,
61+
# CHECK-NEXT: "line": 0
62+
# CHECK-NEXT: },
63+
# CHECK-NEXT: "start": {
64+
# CHECK-NEXT: "character": 0,
65+
# CHECK-NEXT: "line": 0
66+
# CHECK-NEXT: }
67+
# CHECK-NEXT: }
68+
# CHECK-NEXT: }
69+
# CHECK-NEXT: ],
70+
# CHECK-NEXT: "textDocument": {
71+
# CHECK-NEXT: "uri": "file:///clangd-test/foo.c",
72+
# CHECK-NEXT: "version": 1
73+
# CHECK-NEXT: }
74+
# CHECK-NEXT: }
75+
# CHECK-NEXT: ]
76+
# CHECK-NEXT: },
77+
# CHECK-NEXT: "kind": "quickfix",
78+
# CHECK-NEXT: "title": "ignore [clang-diagnostic-parentheses] for this line"
79+
# CHECK-NEXT: },
80+
# CHECK-NEXT: {
81+
# CHECK-NEXT: "diagnostics": [
82+
# CHECK-NEXT: {
83+
# CHECK-NEXT: "code": "-Wparentheses",
84+
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
85+
# CHECK-NEXT: "range": {
86+
# CHECK-NEXT: "end": {
87+
# CHECK-NEXT: "character": 37,
88+
# CHECK-NEXT: "line": 0
89+
# CHECK-NEXT: },
90+
# CHECK-NEXT: "start": {
91+
# CHECK-NEXT: "character": 32,
92+
# CHECK-NEXT: "line": 0
93+
# CHECK-NEXT: }
94+
# CHECK-NEXT: },
95+
# CHECK-NEXT: "severity": 2,
96+
# CHECK-NEXT: "source": "clang"
97+
# CHECK-NEXT: }
98+
# CHECK-NEXT: ],
99+
# CHECK-NEXT: "edit": {
100+
# CHECK-NEXT: "documentChanges": [
101+
# CHECK-NEXT: {
102+
# CHECK-NEXT: "edits": [
103+
# CHECK-NEXT: {
57104
# CHECK-NEXT: "newText": "(",
58105
# CHECK-NEXT: "range": {
59106
# CHECK-NEXT: "end": {

‎clang-tools-extra/clangd/test/fixits-codeaction.test‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,47 @@
5151
# CHECK-NEXT: ],
5252
# CHECK-NEXT: "edit": {
5353
# CHECK-NEXT: "changes": {
54+
# CHECK-NEXT: "file:///clangd-test/foo.c": [
55+
# CHECK-NEXT: {
56+
# CHECK-NEXT: "newText": "// NOLINTNEXTLINE(clang-diagnostic-parentheses)\n",
57+
# CHECK-NEXT: "range": {
58+
# CHECK-NEXT: "end": {
59+
# CHECK-NEXT: "character": 0,
60+
# CHECK-NEXT: "line": 0
61+
# CHECK-NEXT: },
62+
# CHECK-NEXT: "start": {
63+
# CHECK-NEXT: "character": 0,
64+
# CHECK-NEXT: "line": 0
65+
# CHECK-NEXT: }
66+
# CHECK-NEXT: }
67+
# CHECK-NEXT: }
68+
# CHECK-NEXT: ]
69+
# CHECK-NEXT: }
70+
# CHECK-NEXT: },
71+
# CHECK-NEXT: "kind": "quickfix",
72+
# CHECK-NEXT: "title": "ignore [clang-diagnostic-parentheses] for this line"
73+
# CHECK-NEXT: },
74+
# CHECK-NEXT: {
75+
# CHECK-NEXT: "diagnostics": [
76+
# CHECK-NEXT: {
77+
# CHECK-NEXT: "code": "-Wparentheses",
78+
# CHECK-NEXT: "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
79+
# CHECK-NEXT: "range": {
80+
# CHECK-NEXT: "end": {
81+
# CHECK-NEXT: "character": 37,
82+
# CHECK-NEXT: "line": 0
83+
# CHECK-NEXT: },
84+
# CHECK-NEXT: "start": {
85+
# CHECK-NEXT: "character": 32,
86+
# CHECK-NEXT: "line": 0
87+
# CHECK-NEXT: }
88+
# CHECK-NEXT: },
89+
# CHECK-NEXT: "severity": 2,
90+
# CHECK-NEXT: "source": "clang"
91+
# CHECK-NEXT: }
92+
# CHECK-NEXT: ],
93+
# CHECK-NEXT: "edit": {
94+
# CHECK-NEXT: "changes": {
5495
# CHECK-NEXT: "file://{{.*}}/foo.c": [
5596
# CHECK-NEXT: {
5697
# CHECK-NEXT: "newText": "(",

0 commit comments

Comments
 (0)