[clang-tools-extra] 3b677b8 - [libtooling][clang-tidy] Fix diagnostics not highlighting fed SourceRanges
via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 10 07:44:35 PDT 2021
Author: Whisperity
Date: 2021-04-10T16:43:44+02:00
New Revision: 3b677b81cec7b3c5132aee8fccc30252d87deb69
URL: https://github.com/llvm/llvm-project/commit/3b677b81cec7b3c5132aee8fccc30252d87deb69
DIFF: https://github.com/llvm/llvm-project/commit/3b677b81cec7b3c5132aee8fccc30252d87deb69.diff
LOG: [libtooling][clang-tidy] Fix diagnostics not highlighting fed SourceRanges
Fixes bug http://bugs.llvm.org/show_bug.cgi?id=49000.
This patch allows Clang-Tidy checks to do
diag(X->getLocation(), "text") << Y->getSourceRange();
and get the highlight of `Y` as expected:
warning: text [blah-blah]
xxx(something)
^ ~~~~~~~~~
Reviewed-By: aaron.ballman, njames93
Differential Revision: http://reviews.llvm.org/D98635
Added:
Modified:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
clang/include/clang/Tooling/Core/Diagnostic.h
clang/include/clang/Tooling/DiagnosticsYaml.h
clang/lib/Tooling/Core/Diagnostic.cpp
clang/unittests/Tooling/DiagnosticsYamlTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 88ba4bf63e09f..73d66b980a5e1 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -132,6 +132,8 @@ class ErrorReporter {
}
auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
<< Message.Message << Name;
+ for (const FileByteRange &FBR : Error.Message.Ranges)
+ Diag << getRange(FBR);
// FIXME: explore options to support interactive fix selection.
const llvm::StringMap<Replacements> *ChosenFix;
if (ApplyFixes != FB_NoFix &&
@@ -257,17 +259,13 @@ class ErrorReporter {
for (const auto &Repl : FileAndReplacements.second) {
if (!Repl.isApplicable())
continue;
- SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
- Files.makeAbsolutePath(FixAbsoluteFilePath);
- SourceLocation FixLoc =
- getLocation(FixAbsoluteFilePath, Repl.getOffset());
- SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength());
- // Retrieve the source range for applicable fixes. Macro definitions
- // on the command line have locations in a virtual buffer and don't
- // have valid file paths and are therefore not applicable.
- CharSourceRange Range =
- CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
- Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText());
+ FileByteRange FBR;
+ FBR.FilePath = Repl.getFilePath().str();
+ FBR.FileOffset = Repl.getOffset();
+ FBR.Length = Repl.getLength();
+
+ Diag << FixItHint::CreateReplacement(getRange(FBR),
+ Repl.getReplacementText());
}
}
}
@@ -277,9 +275,22 @@ class ErrorReporter {
auto Diag =
Diags.Report(Loc, Diags.getCustomDiagID(DiagnosticsEngine::Note, "%0"))
<< Message.Message;
+ for (const FileByteRange &FBR : Message.Ranges)
+ Diag << getRange(FBR);
reportFix(Diag, Message.Fix);
}
+ CharSourceRange getRange(const FileByteRange &Range) {
+ SmallString<128> AbsoluteFilePath{Range.FilePath};
+ Files.makeAbsolutePath(AbsoluteFilePath);
+ SourceLocation BeginLoc = getLocation(AbsoluteFilePath, Range.FileOffset);
+ SourceLocation EndLoc = BeginLoc.getLocWithOffset(Range.Length);
+ // Retrieve the source range for applicable highlights and fixes. Macro
+ // definition on the command line have locations in a virtual buffer and
+ // don't have valid file paths and are therefore not applicable.
+ return CharSourceRange::getCharRange(BeginLoc, EndLoc);
+ }
+
FileManager Files;
LangOptions LangOpts; // FIXME: use langopts from each original file
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 9e45f0aea798f..0590715562d08 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -25,6 +25,7 @@
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Lex/Lexer.h"
#include "clang/Tooling/Core/Diagnostic.h"
#include "clang/Tooling/Core/Replacement.h"
#include "llvm/ADT/STLExtras.h"
@@ -62,15 +63,31 @@ class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
Loc.isValid()
? tooling::DiagnosticMessage(Message, Loc.getManager(), Loc)
: tooling::DiagnosticMessage(Message);
+
+ // Make sure that if a TokenRange is receieved from the check it is unfurled
+ // into a real CharRange for the diagnostic printer later.
+ // Whatever we store here gets decoupled from the current SourceManager, so
+ // we **have to** know the exact position and length of the highlight.
+ const auto &ToCharRange = [this, &Loc](const CharSourceRange &SourceRange) {
+ if (SourceRange.isCharRange())
+ return SourceRange;
+ SourceLocation End = Lexer::getLocForEndOfToken(
+ SourceRange.getEnd(), 1, Loc.getManager(), LangOpts);
+ return CharSourceRange::getCharRange(SourceRange.getBegin(), End);
+ };
+
if (Level == DiagnosticsEngine::Note) {
Error.Notes.push_back(TidyMessage);
+ for (const CharSourceRange &SourceRange : Ranges)
+ Error.Notes.back().Ranges.emplace_back(Loc.getManager(),
+ ToCharRange(SourceRange));
return;
}
assert(Error.Message.Message.empty() && "Overwriting a diagnostic message");
Error.Message = TidyMessage;
- for (const CharSourceRange &SourceRange : Ranges) {
- Error.Ranges.emplace_back(Loc.getManager(), SourceRange);
- }
+ for (const CharSourceRange &SourceRange : Ranges)
+ Error.Message.Ranges.emplace_back(Loc.getManager(),
+ ToCharRange(SourceRange));
}
void emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2e38c7d86dc9c..61097d1fd6242 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -74,6 +74,13 @@ Improvements to clang-tidy
attached to warnings. These are typically cases where we are less confident
the fix will have the desired effect.
+- libToolingCore and Clang-Tidy was refactored and now checks can produce
+ highlights (`^~~~~` under fragments of the source code) in diagnostics.
+ Existing and new checks in the future can be expected to start implementing
+ this functionality.
+ This change only affects the visual rendering of diagnostics, and does not
+ alter the behavior of generated fixes.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
index 13d7684597e5a..af8272ffcc632 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp
@@ -22,7 +22,7 @@ int a[-1];
// ''ff'''
// CHECK-YAML-NEXT: FilePath: '{{.*}}-input.cpp'
// CHECK-YAML-NEXT: FileOffset: 30
-// CHECK-YAML-NEXT: Replacements: []
+// CHECK-YAML-NEXT: Replacements: []
// CHECK-YAML-NEXT: Notes:
// CHECK-YAML-NEXT: - Message: 'expanded from macro ''X'''
// CHECK-YAML-NEXT: FilePath: '{{.*}}-input.cpp'
@@ -52,10 +52,10 @@ int a[-1];
// CHECK-YAML-NEXT: FilePath: '{{.*}}-input.cpp'
// CHECK-YAML-NEXT: FileOffset: 41
// CHECK-YAML-NEXT: Replacements: []
+// CHECK-YAML-NEXT: Ranges:
+// CHECK-YAML-NEXT: - FilePath: '{{.*}}-input.cpp'
+// CHECK-YAML-NEXT: FileOffset: 41
+// CHECK-YAML-NEXT: Length: 1
// CHECK-YAML-NEXT: Level: Error
// CHECK-YAML-NEXT: BuildDirectory: '{{.*}}'
-// CHECK-YAML-NEXT: Ranges:
-// CHECK-YAML-NEXT: - FilePath: '{{.*}}-input.cpp'
-// CHECK-YAML-NEXT: FileOffset: 41
-// CHECK-YAML-NEXT: Length: 1
// CHECK-YAML-NEXT: ...
diff --git a/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp b/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
index ceb18168636fb..4fe57e618004d 100644
--- a/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
+++ b/clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
@@ -23,13 +23,9 @@ makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName,
const StringMap<Replacements> &Replacements,
StringRef BuildDirectory) {
TUDiagnostics TUs;
- TUs.push_back({MainSourceFile,
- {{DiagnosticName,
- Message,
- {},
- Diagnostic::Warning,
- BuildDirectory,
- {}}}});
+ TUs.push_back(
+ {MainSourceFile,
+ {{DiagnosticName, Message, {}, Diagnostic::Warning, BuildDirectory}}});
return TUs;
}
diff --git a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
index 24b6bea98787a..53ea4f5977587 100644
--- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -84,7 +84,7 @@ TEST(TransformerClangTidyCheckTest, DiagnosticsCorrectlyGenerated) {
EXPECT_EQ(Input, test::runCheckOnCode<DiagOnlyCheck>(Input, &Errors));
EXPECT_EQ(Errors.size(), 1U);
EXPECT_EQ(Errors[0].Message.Message, "message");
- EXPECT_THAT(Errors[0].Ranges, testing::IsEmpty());
+ EXPECT_THAT(Errors[0].Message.Ranges, testing::IsEmpty());
// The diagnostic is anchored to the match, "return 5".
EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
diff --git a/clang/include/clang/Tooling/Core/Diagnostic.h b/clang/include/clang/Tooling/Core/Diagnostic.h
index 123874f9ccf7a..f68e0a682cd21 100644
--- a/clang/include/clang/Tooling/Core/Diagnostic.h
+++ b/clang/include/clang/Tooling/Core/Diagnostic.h
@@ -26,6 +26,17 @@
namespace clang {
namespace tooling {
+/// Represents a range within a specific source file.
+struct FileByteRange {
+ FileByteRange() = default;
+
+ FileByteRange(const SourceManager &Sources, CharSourceRange Range);
+
+ std::string FilePath;
+ unsigned FileOffset;
+ unsigned Length;
+};
+
/// Represents the diagnostic message with the error message associated
/// and the information on the location of the problem.
struct DiagnosticMessage {
@@ -39,23 +50,17 @@ struct DiagnosticMessage {
///
DiagnosticMessage(llvm::StringRef Message, const SourceManager &Sources,
SourceLocation Loc);
+
std::string Message;
std::string FilePath;
unsigned FileOffset;
/// Fixes for this diagnostic, grouped by file path.
llvm::StringMap<Replacements> Fix;
-};
-
-/// Represents a range within a specific source file.
-struct FileByteRange {
- FileByteRange() = default;
- FileByteRange(const SourceManager &Sources, CharSourceRange Range);
-
- std::string FilePath;
- unsigned FileOffset;
- unsigned Length;
+ /// Extra source ranges associated with the note, in addition to the location
+ /// of the Message itself.
+ llvm::SmallVector<FileByteRange, 1> Ranges;
};
/// Represents the diagnostic with the level of severity and possible
@@ -73,8 +78,7 @@ struct Diagnostic {
Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,
const SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
- llvm::StringRef BuildDirectory,
- const SmallVector<FileByteRange, 1> &Ranges);
+ llvm::StringRef BuildDirectory);
/// Name identifying the Diagnostic.
std::string DiagnosticName;
@@ -96,10 +100,6 @@ struct Diagnostic {
///
/// Note: it is empty in unittest.
std::string BuildDirectory;
-
- /// Extra source ranges associated with the diagnostic (in addition to the
- /// location of the Message above).
- SmallVector<FileByteRange, 1> Ranges;
};
/// Collection of Diagnostics generated from a single translation unit.
diff --git a/clang/include/clang/Tooling/DiagnosticsYaml.h b/clang/include/clang/Tooling/DiagnosticsYaml.h
index 38fbcfc1da95b..31bc2989b883e 100644
--- a/clang/include/clang/Tooling/DiagnosticsYaml.h
+++ b/clang/include/clang/Tooling/DiagnosticsYaml.h
@@ -54,6 +54,7 @@ template <> struct MappingTraits<clang::tooling::DiagnosticMessage> {
<< llvm::toString(std::move(Err)) << "\n";
}
}
+ Io.mapOptional("Ranges", M.Ranges);
}
};
@@ -67,12 +68,11 @@ template <> struct MappingTraits<clang::tooling::Diagnostic> {
NormalizedDiagnostic(const IO &, const clang::tooling::Diagnostic &D)
: DiagnosticName(D.DiagnosticName), Message(D.Message), Notes(D.Notes),
- DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory),
- Ranges(D.Ranges) {}
+ DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory) {}
clang::tooling::Diagnostic denormalize(const IO &) {
return clang::tooling::Diagnostic(DiagnosticName, Message, Notes,
- DiagLevel, BuildDirectory, Ranges);
+ DiagLevel, BuildDirectory);
}
std::string DiagnosticName;
@@ -80,7 +80,6 @@ template <> struct MappingTraits<clang::tooling::Diagnostic> {
SmallVector<clang::tooling::DiagnosticMessage, 1> Notes;
clang::tooling::Diagnostic::Level DiagLevel;
std::string BuildDirectory;
- SmallVector<clang::tooling::FileByteRange, 1> Ranges;
};
static void mapping(IO &Io, clang::tooling::Diagnostic &D) {
@@ -91,7 +90,6 @@ template <> struct MappingTraits<clang::tooling::Diagnostic> {
Io.mapOptional("Notes", Keys->Notes);
Io.mapOptional("Level", Keys->DiagLevel);
Io.mapOptional("BuildDirectory", Keys->BuildDirectory);
- Io.mapOptional("Ranges", Keys->Ranges);
}
};
diff --git a/clang/lib/Tooling/Core/Diagnostic.cpp b/clang/lib/Tooling/Core/Diagnostic.cpp
index b0c4ea8c56088..fb3358024692d 100644
--- a/clang/lib/Tooling/Core/Diagnostic.cpp
+++ b/clang/lib/Tooling/Core/Diagnostic.cpp
@@ -53,10 +53,9 @@ Diagnostic::Diagnostic(llvm::StringRef DiagnosticName,
Diagnostic::Diagnostic(llvm::StringRef DiagnosticName,
const DiagnosticMessage &Message,
const SmallVector<DiagnosticMessage, 1> &Notes,
- Level DiagLevel, llvm::StringRef BuildDirectory,
- const SmallVector<FileByteRange, 1> &Ranges)
+ Level DiagLevel, llvm::StringRef BuildDirectory)
: DiagnosticName(DiagnosticName), Message(Message), Notes(Notes),
- DiagLevel(DiagLevel), BuildDirectory(BuildDirectory), Ranges(Ranges) {}
+ DiagLevel(DiagLevel), BuildDirectory(BuildDirectory) {}
const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D) {
if (!D.Message.Fix.empty())
diff --git a/clang/unittests/Tooling/DiagnosticsYamlTest.cpp b/clang/unittests/Tooling/DiagnosticsYamlTest.cpp
index acfefa505edbc..0686b22691251 100644
--- a/clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ b/clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,14 +20,16 @@ using namespace llvm;
using namespace clang::tooling;
using clang::tooling::Diagnostic;
-static DiagnosticMessage makeMessage(const std::string &Message, int FileOffset,
- const std::string &FilePath,
- const StringMap<Replacements> &Fix) {
+static DiagnosticMessage
+makeMessage(const std::string &Message, int FileOffset,
+ const std::string &FilePath, const StringMap<Replacements> &Fix,
+ const SmallVector<FileByteRange, 1> &Ranges) {
DiagnosticMessage DiagMessage;
DiagMessage.Message = Message;
DiagMessage.FileOffset = FileOffset;
DiagMessage.FilePath = FilePath;
DiagMessage.Fix = Fix;
+ DiagMessage.Ranges = Ranges;
return DiagMessage;
}
@@ -47,8 +49,8 @@ static Diagnostic makeDiagnostic(StringRef DiagnosticName,
const StringMap<Replacements> &Fix,
const SmallVector<FileByteRange, 1> &Ranges) {
return Diagnostic(DiagnosticName,
- makeMessage(Message, FileOffset, FilePath, Fix), {},
- Diagnostic::Warning, "path/to/build/directory", Ranges);
+ makeMessage(Message, FileOffset, FilePath, Fix, Ranges), {},
+ Diagnostic::Warning, "path/to/build/directory");
}
static const char *YAMLContent =
@@ -77,12 +79,12 @@ static const char *YAMLContent =
" Offset: 62\n"
" Length: 2\n"
" ReplacementText: 'replacement #2'\n"
+ " Ranges:\n"
+ " - FilePath: 'path/to/source.cpp'\n"
+ " FileOffset: 10\n"
+ " Length: 10\n"
" Level: Warning\n"
" BuildDirectory: 'path/to/build/directory'\n"
- " Ranges:\n"
- " - FilePath: 'path/to/source.cpp'\n"
- " FileOffset: 10\n"
- " Length: 10\n"
" - DiagnosticName: 'diagnostic#3'\n"
" DiagnosticMessage:\n"
" Message: 'message #3'\n"
@@ -123,9 +125,9 @@ TEST(DiagnosticsYamlTest, serializesDiagnostics) {
TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
"path/to/source2.cpp", {}, {}));
TUD.Diagnostics.back().Notes.push_back(
- makeMessage("Note1", 88, "path/to/note1.cpp", {}));
+ makeMessage("Note1", 88, "path/to/note1.cpp", {}, {}));
TUD.Diagnostics.back().Notes.push_back(
- makeMessage("Note2", 99, "path/to/note2.cpp", {}));
+ makeMessage("Note2", 99, "path/to/note2.cpp", {}, {}));
std::string YamlContent;
raw_string_ostream YamlContentStream(YamlContent);
@@ -166,7 +168,7 @@ TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
EXPECT_EQ(100u, Fixes1[0].getOffset());
EXPECT_EQ(12u, Fixes1[0].getLength());
EXPECT_EQ("replacement #1", Fixes1[0].getReplacementText());
- EXPECT_TRUE(D1.Ranges.empty());
+ EXPECT_TRUE(D1.Message.Ranges.empty());
Diagnostic D2 = TUDActual.Diagnostics[1];
EXPECT_EQ("diagnostic#2", D2.DiagnosticName);
@@ -179,10 +181,10 @@ TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
EXPECT_EQ(62u, Fixes2[0].getOffset());
EXPECT_EQ(2u, Fixes2[0].getLength());
EXPECT_EQ("replacement #2", Fixes2[0].getReplacementText());
- EXPECT_EQ(1u, D2.Ranges.size());
- EXPECT_EQ("path/to/source.cpp", D2.Ranges[0].FilePath);
- EXPECT_EQ(10u, D2.Ranges[0].FileOffset);
- EXPECT_EQ(10u, D2.Ranges[0].Length);
+ EXPECT_EQ(1u, D2.Message.Ranges.size());
+ EXPECT_EQ("path/to/source.cpp", D2.Message.Ranges[0].FilePath);
+ EXPECT_EQ(10u, D2.Message.Ranges[0].FileOffset);
+ EXPECT_EQ(10u, D2.Message.Ranges[0].Length);
Diagnostic D3 = TUDActual.Diagnostics[2];
EXPECT_EQ("diagnostic#3", D3.DiagnosticName);
@@ -198,5 +200,5 @@ TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
EXPECT_EQ("path/to/note2.cpp", D3.Notes[1].FilePath);
std::vector<Replacement> Fixes3 = getFixes(D3.Message.Fix);
EXPECT_TRUE(Fixes3.empty());
- EXPECT_TRUE(D3.Ranges.empty());
+ EXPECT_TRUE(D3.Message.Ranges.empty());
}
More information about the cfe-commits
mailing list