[libtooling][clang-tidy] Fix diagnostics not respecting and highlighting fed SourceRanges

[whisperity]

Summary

Fixes bug #49000.

Currently, when a diagnostic in Clang-Tidy is produced as diag(Var->getLocation(), "foo %0") << "bar" << Var->getSourceRange();, the source range specified to be highlighted is discarded, i.e.

x.cpp:21:18: warning: foo bar [blah-blah-checker]
void fun(int I, int J, int K) {}
             ^

is created instead of producing a full highlight:

x.cpp:21:18: warning: foo bar [blah-blah-checker]
void fun(int I, int J, int K) {}
         ~~~~^

This creates a discrepancy between warnings generated in Clang::Sema which work properly, and in Tidy.
This patch fixes the issue by making sure Clang-Tidy's logic of decoupling ranges from the SourceManager and rendering them later respects custom ranges that are fed into a diagnostic.

[whisperity]

NFC I originally wrote a Release Notes entry about this, which I forgot to commit before I generated the patch file I uploaded, this is fixed now.

[aaron.ballman]

Thank you for looking into this!

It looks like there are some unit test failures from the change: https://buildkite.com/llvm-project/premerge-checks/builds/29754#a9b04ab5-e326-4ff1-beea-773f21a33349

[whisperity]

In D98635#2626455, @aaron.ballman wrote:

It looks like there are some unit test failures from the change: https://buildkite.com/llvm-project/premerge-checks/builds/29754#a9b04ab5-e326-4ff1-beea-773f21a33349

Strange because I specifically ran both check-clang and check-clang-tools locally, but will look into this.

[whisperity]

In D98635#2626464, @whisperity wrote:

Strange because I specifically ran both check-clang and check-clang-tools locally, but will look into this.

Turns out there is a check-clang-extra-unit too, and it looks like those test TUs either didn't compile for me at all (as in, it was not commanded to compile them), or I messed something up during the rebase.
(But TIL we finally have some semblance of pre-merge checking instead of always reverting commits! 😋)

'll update the patch soon. Gonna do a clean build and a clean check, just in case....

What's the outlook on the executive decision... changing the schema in libToolingCore... is such even allowed? I'm venturing into parts unknown here.

[whisperity]

Fixed a test file that I originally missed to align with the changes.

[aaron.ballman]

In D98635#2626485, @whisperity wrote:

In D98635#2626464, @whisperity wrote:

Strange because I specifically ran both check-clang and check-clang-tools locally, but will look into this.

Turns out there is a check-clang-extra-unit too, and it looks like those test TUs either didn't compile for me at all (as in, it was not commanded to compile them), or I messed something up during the rebase.
(But TIL we finally have some semblance of pre-merge checking instead of always reverting commits! 😋)

Yes, it's actually been catching some useful things lately, too, which is nice!

What's the outlook on the executive decision... changing the schema in libToolingCore... is such even allowed? I'm venturing into parts unknown here.

I think @alexfh will have to make the final call, but I think the changes here look reasonable. I have no idea if we've made stability guarantees about the schema though.

I'm giving my LGTM, but not accepting the review because I'd like to hear from Alex before this lands.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

78

Please use sing back-ticks for non-language constructs.

[njames93]

Again LGTM, but see what alex says.

clang-tools-extra/clang-tidy/ClangTidy.cpp

135–137

nit: Elide braces.

278–280

nit: Elide braces.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

81–84

nit: Elide braces.

[whisperity]

@aaron.ballman Could you please help me understand this pre-merge buildbot? It says that Debian failed, with the following message:

[15/1038] Running lint check for sanitizer sources...
FAILED: projects/compiler-rt/lib/CMakeFiles/SanitizerLintCheck
cd /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/lib && env LLVM_CHECKOUT=/mnt/disks/ssd0/agent/llvm-project/llvm SILENT=1 TMPDIR= PYTHON_EXECUTABLE=/usr/bin/python3.8 COMPILER_RT=/mnt/disks/ssd0/agent/llvm-project/compiler-rt /mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/sanitizer_common/scripts/check_lint.sh
The following differences between the ABI list and the implemented custom wrappers have been found:

Should I be worried because of this, or is this that the patch started building when the main branch was broken? I haven't touched compiler-rt at all. Neither anything that deals with the sanitizers. And this fail is "earlier" than the previous one where I indeed missed a test file's update. (Previous fail was at ninja action multiple hundreds, this is 15.)

And it talks about a lot of stuff like fork and such that is really really out of scope for the patch itself.

[njames93]

In D98635#2629827, @whisperity wrote:

@aaron.ballman Could you please help me understand this pre-merge buildbot? It says that Debian failed, with the following message:

[15/1038] Running lint check for sanitizer sources...
FAILED: projects/compiler-rt/lib/CMakeFiles/SanitizerLintCheck
cd /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/lib && env LLVM_CHECKOUT=/mnt/disks/ssd0/agent/llvm-project/llvm SILENT=1 TMPDIR= PYTHON_EXECUTABLE=/usr/bin/python3.8 COMPILER_RT=/mnt/disks/ssd0/agent/llvm-project/compiler-rt /mnt/disks/ssd0/agent/llvm-project/compiler-rt/lib/sanitizer_common/scripts/check_lint.sh
The following differences between the ABI list and the implemented custom wrappers have been found:

Should I be worried because of this, or is this that the patch started building when the main branch was broken? I haven't touched compiler-rt at all. Neither anything that deals with the sanitizers. And this fail is "earlier" than the previous one where I indeed missed a test file's update. (Previous fail was at ninja action multiple hundreds, this is 15.)

And it talks about a lot of stuff like fork and such that is really really out of scope for the patch itself.

Rule of thumb: If the test failures don't seem related to the contents of your patch, they can usually be disregarded as its likely something on trunk that is causing the failure.

[whisperity]

[NFC] Fixed nits and code formatting.

[whisperity]

Ping!

[aaron.ballman]

This continues to LGTM, so I'm accepting it. If @alexfh has any remaining concerns, hopefully he can raise them quickly or we can handle them post-commit. Thanks!

clang-tools-extra/clang-tidy/ClangTidy.cpp

SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
         Name += "," + llvm::join(Error.EnabledDiagnosticAliases, ",");
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }
       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 &&
           (ChosenFix = getFixIt(Error, ApplyFixes == FB_FixNotes))) {
         for (const auto &FileAndReplacements : *ChosenFix) {
           for (const auto &Repl : FileAndReplacements.second) {
             ++TotalFixes;
             bool CanBeApplied = false;
             if (!Repl.isApplicable())
private:
   }
 
   void reportFix(const DiagnosticBuilder &Diag,
                  const llvm::StringMap<Replacements> &Fix) {
     for (const auto &FileAndReplacements : Fix) {
       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());
       }
     }
   }
 
   void reportNote(const tooling::DiagnosticMessage &Message) {
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
     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;
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
   llvm::StringMap<Replacements> FileReplacements;
   ClangTidyContext &Context;

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 #include "GlobList.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Attr.h"
 #include "clang/Basic/Diagnostic.h"
 #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"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Regex.h"
 #include <tuple>
void emitDiagnosticMessage(FullSourceLoc Loc, PresumedLoc PLoc,
     std::string CheckNameInMessage = " [" + Error.DiagnosticName + "]";
     if (Message.endswith(CheckNameInMessage))
       Message = Message.substr(0, Message.size() - CheckNameInMessage.size());
 
     auto TidyMessage =
         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,
                          DiagnosticsEngine::Level Level,
                          ArrayRef<CharSourceRange> Ranges) override {}
 
   void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
                        SmallVectorImpl<CharSourceRange> &Ranges,

clang-tools-extra/docs/ReleaseNotes.rst

 
 - The `run-clang-tidy.py` helper script is now installed in `bin/` as
   `run-clang-tidy`. It was previously installed in `share/clang/`.
 
 - Added command line option `--fix-notes` to apply fixes found in notes
   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
 ^^^^^^^^^^
 
 - New :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` check.
 
   Finds ``pthread_setcanceltype`` function calls where a thread's cancellation
   type is set to asynchronous.

clang-tools-extra/test/clang-tidy/infrastructure/export-diagnostics.cpp

 // CHECK-YAML-NEXT: MainSourceFile:  '{{.*}}-input.cpp'
 // CHECK-YAML-NEXT: Diagnostics:
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-missing-prototypes
 // CHECK-YAML-NEXT:     DiagnosticMessage:
 // CHECK-YAML-NEXT:       Message:         'no previous prototype for function
 // ''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'
 // CHECK-YAML-NEXT:         FileOffset:      18
 // CHECK-YAML-NEXT:         Replacements:    []
 // CHECK-YAML-NEXT:       - Message:         expanded from here
 // CHECK-YAML-NEXT:         FilePath:        ''
 // CHECK-YAML-NEXT:         FileOffset:      0

 // CHECK-YAML-NEXT:     Level:           Warning
 // CHECK-YAML-NEXT:     BuildDirectory:  '{{.*}}'
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-error
 // CHECK-YAML-NEXT:     DiagnosticMessage:
 // CHECK-YAML-NEXT:       Message:         '''a'' declared as an array with a negative size'
 // CHECK-YAML-NEXT:       FilePath:        '{{.*}}-input.cpp'
 // CHECK-YAML-NEXT:       FileOffset:      41
 // CHECK-YAML-NEXT:       Replacements:    []
-// 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:       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: ...

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

 namespace tooling {
 
 static TUDiagnostics
 makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName,
                   const DiagnosticMessage &Message,
                   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;
 }
 
 // Test to ensure diagnostics with no fixes, will be merged correctly
 // before applying.
 TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
   DiagnosticsEngine Diagnostics(

clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

DiagOnlyCheck(StringRef Name, ClangTidyContext *Context)
               makeRule(returnStmt(), noopEdit(node(RootID)), cat("message")),
               Name, Context) {}
   };
   std::string Input = "int h() { return 5; }";
   std::vector<ClangTidyError> Errors;
   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);
 }
 
 class IntLitCheck : public TransformerClangTidyCheck {
 public:
   IntLitCheck(StringRef Name, ClangTidyContext *Context)

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

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
 
 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 {
   DiagnosticMessage(llvm::StringRef Message = "");
 
   /// Constructs a diagnostic message with anoffset to the diagnostic
   /// within the file where the problem occurred.
   ///
   /// \param Loc Should be a file location, it is not meaningful for a macro
   /// location.
   ///
   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
 /// 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,
-             const SmallVector<FileByteRange, 1> &Ranges);
+             llvm::StringRef BuildDirectory);
 
   /// 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

static void mapping(IO &Io, clang::tooling::DiagnosticMessage &M) {
     for (auto &Fix : Fixes) {
       llvm::Error Err = M.Fix[Fix.getFilePath()].add(Fix);
       if (Err) {
         // FIXME: Implement better conflict handling.
         llvm::errs() << "Fix conflicts with existing fix: "
                      << llvm::toString(std::move(Err)) << "\n";
       }
     }
+    Io.mapOptional("Ranges", M.Ranges);
   }
 };
 
 template <> struct MappingTraits<clang::tooling::Diagnostic> {
   /// Helper to (de)serialize a Diagnostic since we don't have direct
   /// 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),
-          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;
     clang::tooling::DiagnosticMessage Message;
     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("Level", Keys->DiagLevel);
     Io.mapOptional("BuildDirectory", Keys->BuildDirectory);
-    Io.mapOptional("Ranges", Keys->Ranges);
   }
 };
 
 /// Specialized MappingTraits to describe how a
 /// TranslationUnitDiagnostics is (de)serialized.
 template <> struct MappingTraits<clang::tooling::TranslationUnitDiagnostics> {
   static void mapping(IO &Io, clang::tooling::TranslationUnitDiagnostics &Doc) {
     Io.mapRequired("MainSourceFile", Doc.MainSourceFile);

clang/lib/Tooling/Core/Diagnostic.cpp

 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,
-                       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())
     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

 #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) {
+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;
 }
 
 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 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 =
     "---\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"
-    "    Level:           Warning\n"
-    "    BuildDirectory:  'path/to/build/directory'\n"
-    "    Ranges:\n"
-    "      - FilePath:        'path/to/source.cpp'\n"
-    "        FileOffset:      10\n"
-    "        Length:          10\n"
+    "      Ranges:\n"
+    "        - FilePath:        'path/to/source.cpp'\n"
+    "          FileOffset:      10\n"
+    "          Length:          10\n"
+    "    Level:           Warning\n"
+    "    BuildDirectory:  'path/to/build/directory'\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) {
   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, Ranges2));
 
   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);
 
   yaml::Output YAML(YamlContentStream);
   YAML << TUD;
 
   EXPECT_EQ(YAMLContent, YamlContentStream.str());
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());
+  EXPECT_TRUE(D1.Message.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);
+  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);
   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());
+  EXPECT_TRUE(D3.Message.Ranges.empty());
 }