Summary: Instead of dropping all the ranges associated with a Diagnostic when converting them to a ClangTidy error, instead attach them to the ClangTidyError, so they can be consumed by other APIs.

[alexfh]

include/clang/Tooling/DiagnosticsYaml.h

75 ↗

(On Diff #227644)

We should serialize the ranges too.

[compositeprimes]

Updated to serialize the ranges in yaml. This required making a few abstractions around the representation of CharSourceRange.

[alexfh]

Thanks for the update!

Please upload patches with full context. That makes navigating the code much easier during reviews. See https://llvm.org/docs/Phabricator.html

A few more comments inline.

include/clang/Tooling/Core/Diagnostic.h

62–70 ↗

(On Diff #244737)

Why is this needed? Shouldn't LLVM_YAML_IS_SEQUENCE_VECTOR be enough to allow for SmallVector<DiagnosticAssociatedRange, ...> to be yaml serializable? Seems to work with DiagnosticMessage and Diagnostic::Notes.

lib/Tooling/Core/Diagnostic.cpp

51 ↗

(On Diff #244737)

With emplace_back constructor parameters can be used directly:

Ranges.emplace_back(Sources, Range);

That's the whole point of emplace_back.

[compositeprimes]

Removed unnecessary DiagnosticAssociatedRanges struct

[compositeprimes]

Sorry about the lack of context on the last upload, this one should have it all

include/clang/Tooling/Core/Diagnostic.h

62–70 ↗

(On Diff #244737)

Yeah, you're right it's not really needed. I had been trying to make it easier to convert a vector<CharSourceRange> to vector<AssociatedRange>, but it's really not that hard as is.

[alexfh]

include/clang/Tooling/Core/Diagnostic.h

51 ↗

(On Diff #245207)

This comment was lost by Phabricator (or I just wanted to leave it without actually doing it?).

The name assumes a specific usage. I'd try to make it more generic in case there's another use case for the struct, for example: ByteRange (as opposed to TextRange, which would be {start line, start column, end line, end column}) or FileByteRange (if we want to stress that it contains a file name).

[compositeprimes]

Added tests for Yaml serialization

include/clang/Tooling/Core/Diagnostic.h

51 ↗

(On Diff #245207)

Ah good idea, I like that name, Done

[alexfh]

LG. Thanks!

Do you have commit rights or should someone help you committing the patch?

[compositeprimes]

I don't have commit rights (this is my first llvm commit!), so could someone help out? Thanks!

[alexfh]

In D69782#1886766, @compositeprimes wrote:

I don't have commit rights (this is my first llvm commit!), so could someone help out? Thanks!

Fixed include paths and a broken test and committed as b26c88e3c6e08f8f78ab4291bc85b5685241f493.

[alexfh]

Next step is to plumb this to clang-tidy.

clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName,
                   const StringMap<Replacements> &Replacements,
                   StringRef BuildDirectory) {
   TUDiagnostics TUs;
   TUs.push_back({MainSourceFile,
                  {{DiagnosticName,
                    Message,
                    {},
                    Diagnostic::Warning,
-                   BuildDirectory}}});
+                   BuildDirectory,
+                   {}}}});
   return TUs;
 }
 
 // Test to ensure diagnostics with no fixes, will be merged correctly
 // before applying.
 TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
   DiagnosticsEngine Diagnostics(

clang/include/clang/Tooling/Core/Diagnostic.h

struct DiagnosticMessage {
   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;
+};
+
 /// Represents the diagnostic with the level of severity and possible
 /// fixes to be applied.
 struct Diagnostic {
   enum Level {
     Warning = DiagnosticsEngine::Warning,
     Error = DiagnosticsEngine::Error
   };
 
   Diagnostic() = default;
 
   Diagnostic(llvm::StringRef DiagnosticName, Level DiagLevel,
              StringRef BuildDirectory);
 
   Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,
              const SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
-             llvm::StringRef BuildDirectory);
+             llvm::StringRef BuildDirectory,
+             const SmallVector<FileByteRange, 1> &Ranges);
 
   /// Name identifying the Diagnostic.
   std::string DiagnosticName;
 
   /// Message associated to the diagnostic.
   DiagnosticMessage Message;
 
   /// Potential notes about the diagnostic.
   SmallVector<DiagnosticMessage, 1> Notes;
 
   /// Diagnostic level. Can indicate either an error or a warning.
   Level DiagLevel;
 
   /// A build directory of the diagnostic source file.
   ///
   /// It's an absolute path which is `directory` field of the source file in
   /// compilation database. If users don't specify the compilation database
   /// directory, it is the current directory where clang-tidy runs.
   ///
   /// 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.
 struct TranslationUnitDiagnostics {
   /// Name of the main source for the translation unit.
   std::string MainSourceFile;
   std::vector<Diagnostic> Diagnostics;
 };
 
 /// Get the first fix to apply for this diagnostic.
 /// \returns nullptr if no fixes are attached to the diagnostic.
 const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D);
 
 } // end namespace tooling
 } // end namespace clang
 #endif // LLVM_CLANG_TOOLING_CORE_DIAGNOSTIC_H

clang/include/clang/Tooling/DiagnosticsYaml.h

 
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <string>
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::Diagnostic)
 LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::DiagnosticMessage)
+LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::FileByteRange)
 
 namespace llvm {
 namespace yaml {
 
+template <> struct MappingTraits<clang::tooling::FileByteRange> {
+  static void mapping(IO &Io, clang::tooling::FileByteRange &R) {
+    Io.mapRequired("FilePath", R.FilePath);
+    Io.mapRequired("FileOffset", R.FileOffset);
+    Io.mapRequired("Length", R.Length);
+  }
+};
+
 template <> struct MappingTraits<clang::tooling::DiagnosticMessage> {
   static void mapping(IO &Io, clang::tooling::DiagnosticMessage &M) {
     Io.mapRequired("Message", M.Message);
     Io.mapOptional("FilePath", M.FilePath);
     Io.mapOptional("FileOffset", M.FileOffset);
     std::vector<clang::tooling::Replacement> Fixes;
     for (auto &Replacements : M.Fix) {
       for (auto &Replacement : Replacements.second)
template <> struct MappingTraits<clang::tooling::Diagnostic> {
   /// access to its data members.
   class NormalizedDiagnostic {
   public:
     NormalizedDiagnostic(const IO &)
         : DiagLevel(clang::tooling::Diagnostic::Level::Warning) {}
 
     NormalizedDiagnostic(const IO &, const clang::tooling::Diagnostic &D)
         : DiagnosticName(D.DiagnosticName), Message(D.Message), Notes(D.Notes),
-          DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory) {}
+          DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory),
+          Ranges(D.Ranges) {}
 
     clang::tooling::Diagnostic denormalize(const IO &) {
       return clang::tooling::Diagnostic(DiagnosticName, Message, Notes,
-                                        DiagLevel, BuildDirectory);
+                                        DiagLevel, BuildDirectory, Ranges);
     }
 
     std::string DiagnosticName;
     clang::tooling::DiagnosticMessage Message;
     llvm::StringMap<clang::tooling::Replacements> Fix;
     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) {
     MappingNormalization<NormalizedDiagnostic, clang::tooling::Diagnostic> Keys(
         Io, D);
     Io.mapRequired("DiagnosticName", Keys->DiagnosticName);
     Io.mapRequired("DiagnosticMessage", Keys->Message);
     Io.mapOptional("Notes", Keys->Notes);
+    Io.mapOptional("Ranges", Keys->Ranges);
 
     // FIXME: Export properly all the different fields.
   }
 };
 
 /// Specialized MappingTraits to describe how a
 /// TranslationUnitDiagnostics is (de)serialized.
 template <> struct MappingTraits<clang::tooling::TranslationUnitDiagnostics> {

clang/lib/Tooling/Core/Diagnostic.cpp

 //===--- Diagnostic.cpp - Framework for clang diagnostics tools ----------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 //  Implements classes to support/store diagnostics refactoring.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
 
 namespace clang {
 namespace tooling {
 
 DiagnosticMessage::DiagnosticMessage(llvm::StringRef Message)
     : Message(Message), FileOffset(0) {}
 
 DiagnosticMessage::DiagnosticMessage(llvm::StringRef Message,
                                      const SourceManager &Sources,
                                      SourceLocation Loc)
     : Message(Message), FileOffset(0) {
   assert(Loc.isValid() && Loc.isFileID());
   FilePath = std::string(Sources.getFilename(Loc));
 
   // Don't store offset in the scratch space. It doesn't tell anything to the
   // user. Moreover, it depends on the history of macro expansions and thus
   // prevents deduplication of warnings in headers.
   if (!FilePath.empty())
     FileOffset = Sources.getFileOffset(Loc);
 }
 
+FileByteRange::FileByteRange(
+    const SourceManager &Sources, CharSourceRange Range)
+    : FileOffset(0), Length(0) {
+  FilePath = std::string(Sources.getFilename(Range.getBegin()));
+  if (!FilePath.empty()) {
+    FileOffset = Sources.getFileOffset(Range.getBegin());
+    Length = Sources.getFileOffset(Range.getEnd()) - FileOffset;
+  }
+}
+
 Diagnostic::Diagnostic(llvm::StringRef DiagnosticName,
                        Diagnostic::Level DiagLevel, StringRef BuildDirectory)
     : DiagnosticName(DiagnosticName), DiagLevel(DiagLevel),
       BuildDirectory(BuildDirectory) {}
 
 Diagnostic::Diagnostic(llvm::StringRef DiagnosticName,
                        const DiagnosticMessage &Message,
                        const SmallVector<DiagnosticMessage, 1> &Notes,
-                       Level DiagLevel, llvm::StringRef BuildDirectory)
+                       Level DiagLevel, llvm::StringRef BuildDirectory,
+                       const SmallVector<FileByteRange, 1> &Ranges)
     : DiagnosticName(DiagnosticName), Message(Message), Notes(Notes),
-      DiagLevel(DiagLevel), BuildDirectory(BuildDirectory) {}
+      DiagLevel(DiagLevel), BuildDirectory(BuildDirectory), Ranges(Ranges) {}
 
 const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D) {
    if (!D.Message.Fix.empty())
     return &D.Message.Fix;
   auto Iter = llvm::find_if(D.Notes, [](const tooling::DiagnosticMessage &D) {
     return !D.Fix.empty();
   });
   if (Iter != D.Notes.end())
     return &Iter->Fix;
   return nullptr;
 }
 
 } // end namespace tooling
 } // end namespace clang

clang/unittests/Tooling/DiagnosticsYamlTest.cpp

 //===- unittests/Tooling/DiagnosticsYamlTest.cpp - Serialization tests ---===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Tests for serialization of Diagnostics.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/ReplacementsYaml.h"
+#include "llvm/ADT/SmallVector.h"
 #include "gtest/gtest.h"
 
 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) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
   DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
+static FileByteRange makeByteRange(int FileOffset,
+                                   int Length,
+                                   const std::string &FilePath) {
+  FileByteRange Range;
+  Range.FileOffset = FileOffset;
+  Range.Length = Length;
+  Range.FilePath = FilePath;
+  return Range;
+}
+
 static Diagnostic makeDiagnostic(StringRef DiagnosticName,
                                  const std::string &Message, int FileOffset,
                                  const std::string &FilePath,
-                                 const StringMap<Replacements> &Fix) {
+                                 const StringMap<Replacements> &Fix,
+                                 const SmallVector<FileByteRange, 1> &Ranges) {
   return Diagnostic(DiagnosticName,
                     makeMessage(Message, FileOffset, FilePath, Fix), {},
-                    Diagnostic::Warning, "path/to/build/directory");
+                    Diagnostic::Warning, "path/to/build/directory", Ranges);
 }
 
 static const char *YAMLContent =
     "---\n"
     "MainSourceFile:  'path/to/source.cpp'\n"
     "Diagnostics:\n"
     "  - DiagnosticName:  'diagnostic#1\'\n"
     "    DiagnosticMessage:\n"
static const char *YAMLContent =
     "      Message:         'message #2'\n"
     "      FilePath:        'path/to/header.h'\n"
     "      FileOffset:      60\n"
     "      Replacements:\n"
     "        - FilePath:        'path/to/header.h'\n"
     "          Offset:          62\n"
     "          Length:          2\n"
     "          ReplacementText: 'replacement #2'\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"
     "      FilePath:        'path/to/source2.cpp'\n"
     "      FileOffset:      72\n"
     "      Replacements:    []\n"
     "    Notes:\n"
     "      - Message:         Note1\n"

 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
 
   StringMap<Replacements> Fix1 = {
       {"path/to/source.cpp",
        Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
-                                           "path/to/source.cpp", Fix1));
+                                           "path/to/source.cpp", Fix1, {}));
 
   StringMap<Replacements> Fix2 = {
       {"path/to/header.h",
        Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
+  SmallVector<FileByteRange, 1> Ranges2 =
+      {makeByteRange(10, 10, "path/to/source.cpp")};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-                                           "path/to/header.h", Fix2));
+                                           "path/to/header.h", Fix2, Ranges2));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-                                           "path/to/source2.cpp", {}));
+                                           "path/to/source2.cpp", {}, {}));
   TUD.Diagnostics.back().Notes.push_back(
       makeMessage("Note1", 88, "path/to/note1.cpp", {}));
   TUD.Diagnostics.back().Notes.push_back(
       makeMessage("Note2", 99, "path/to/note2.cpp", {}));
 
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
 
TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
   EXPECT_EQ(55u, D1.Message.FileOffset);
   EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
   std::vector<Replacement> Fixes1 = getFixes(D1.Message.Fix);
   ASSERT_EQ(1u, Fixes1.size());
   EXPECT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());
   EXPECT_EQ(100u, Fixes1[0].getOffset());
   EXPECT_EQ(12u, Fixes1[0].getLength());
   EXPECT_EQ("replacement #1", Fixes1[0].getReplacementText());
+  EXPECT_TRUE(D1.Ranges.empty());
 
   Diagnostic D2 = TUDActual.Diagnostics[1];
   EXPECT_EQ("diagnostic#2", D2.DiagnosticName);
   EXPECT_EQ("message #2", D2.Message.Message);
   EXPECT_EQ(60u, D2.Message.FileOffset);
   EXPECT_EQ("path/to/header.h", D2.Message.FilePath);
   std::vector<Replacement> Fixes2 = getFixes(D2.Message.Fix);
   ASSERT_EQ(1u, Fixes2.size());
   EXPECT_EQ("path/to/header.h", Fixes2[0].getFilePath());
   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);
 
   Diagnostic D3 = TUDActual.Diagnostics[2];
   EXPECT_EQ("diagnostic#3", D3.DiagnosticName);
   EXPECT_EQ("message #3", D3.Message.Message);
   EXPECT_EQ(72u, D3.Message.FileOffset);
   EXPECT_EQ("path/to/source2.cpp", D3.Message.FilePath);
   EXPECT_EQ(2u, D3.Notes.size());
   EXPECT_EQ("Note1", D3.Notes[0].Message);
   EXPECT_EQ(88u, D3.Notes[0].FileOffset);
   EXPECT_EQ("path/to/note1.cpp", D3.Notes[0].FilePath);
   EXPECT_EQ("Note2", D3.Notes[1].Message);
   EXPECT_EQ(99u, D3.Notes[1].FileOffset);
   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());
 }