[clang-tidy] Add fix descriptions to clang-tidy checks.

[hokein]

Summary

Motivation/Context: in the code review system integrating with clang-tidy,
clang-tidy doesn't provide a human-readable description of the fix. Usually
developers have to preview a code diff (before vs after apply the fix) to
understand what the fix does before applying a fix.

This patch proposes that each clang-tidy check provides a short and
actional fix description that can be shown in the UI, so that users can know
what the fix does without previewing diff.

This patch extends clang-tidy framework to support fix descriptions (will add implementations for
existing checks in the future). Fix descriptions and fixes are emitted via diagnostic::Note (rather than
attaching the main warning diagnostic).

Before this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}

After:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   diag(loc, "fix description", DiagnosticIDs::Note) << FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}

[hokein]

This is my first attempt, still missing tests, but it should be in a shape to get early feedbacks.

[Eugene.Zelenko]

clang-tidy/ClangTidyCheck.h

109 ↗

(On Diff #192627)

return {} could be used instead.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

266 ↗

(On Diff #192627)

return {} could be used instead.

274 ↗

(On Diff #192627)

return {} could be used instead.

[Eugene.Zelenko]

clang-tidy/ClangTidyCheck.h

107 ↗

(On Diff #192627)

I think Clang and Clang Static Analyzer should be capitalized.

[JonasToth]

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

[Eugene.Zelenko]

In D59932#1446533, @JonasToth wrote:

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

Also Clazy split checks by severity level.

[hokein]

Update the patch:

• move FixDescriptions to tooling diagnostics, YAML serialization support
• add tests

[hokein]

In D59932#1446533, @JonasToth wrote:

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

Thanks. Unfortunately, we have to iterate all checks no matter which solution we use ;(

Adding a severity to checks is an interesting idea, we need to define the semantic of the severity, looks like different analyzers define them in different ways (some defined by check quality, like stable enough/production ready; some defined by importance of the problem that the check find). And the existing modules of clang-tidy checks address this problem in some degree (like bugprone, readability modules). I think it is a separate topic that needs more thoughts and discussions.

clang-tidy/ClangTidyCheck.h

109 ↗

(On Diff #192627)

yes, {} works here, but I'd use "" which is more explicit.

[MyDeveloperDay]

In D59932#1446533, @JonasToth wrote:

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

In a similar vein, I often feel like I'd like to be able to define a AllowFixIt on every checker, so I could turn off the FixIts for things we just want to warn on.

I often run clang-tidy with -fix but I don't want to necessarily want clang-tidy to fix everything I have marked as -check. readability-identifier-naming is one such checker that isn't mature enough to get everything correct, I want to see the warning to prevent new violations from creeping in, but don't actually want it to do a fixit.

However I don't want to have to keep commenting the check out of my .clang-tidy file just so I can run with -fix, it would be great if we could add *.AllowFixIt

i.e.

- key:             <checkername>-naming.AllowFixIt
  value:           'true/false'

To allow us to disable FixIts for certain checkers

- key:             readability-identifier-naming.AllowFixIt
  value:           'false'

Just and idea, sorry for hijacking the thread.

[alexfh]

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

75 ↗

(On Diff #193070)

nit: Use the format that the bugprone-argument-comment check understands: /*FixDescription=*/

Same below.

clang-tools-extra/clang-tidy/ClangTidyCheck.h

109 ↗

(On Diff #193070)

Checks can provide different fixes with different semantics depending on the context. It seems incorrect to assume that there can be one reasonable description for all of them.

[hokein]

clang-tools-extra/clang-tidy/ClangTidyCheck.h

109 ↗

(On Diff #193070)

Yes, as mentioned in the comment, this is the limitation of this approach -- I assume we don't have a large number of these checks, most checks are providing a single fix.

Another alternative is to use some heuristic mechanisms to do a translation from the Replacement, like we do in clangd, https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/Diagnostics.cpp#L350.

[hokein]

As discussed offline, the current approach only works for checks provide a single fix, providing such API is somehow misleading.

Instead, we'd emit the check fix and the fix description via diagnostic::Note, rather than attaching to the main diagnostic of the check:

• match the data model of the checks provide different semantic fixes depending on the context;
• align with the way how existing clang diagnostics emit alternative fixes (via diagnostic::Note);
• open a door to write clang-tidy checks that provide alternative fixes; we don't have these checks at the moment, but some clang diagnostics like clang-diagnostic-parentheses do (and our current implementation just aggregates all the fixes together, which is not correct);

It would require some changes in clang-tidy check side:

Before this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}

After this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   // We might want to introduce an utility method like `diagFix` to save some verbosed words.
   diag(loc, "fix description", DiagnosticIDs::Note) << FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}

An example of unused-using-decls check (clang-tidy command line output)

• before:

1 warning generated.
/tmp/test.cpp:8:12: warning: using decl 'Foo' is unused [misc-unused-using-decls]
using foo::Foo;
~~~~~~~~~~~^~~~

• after:

1 warning generated.
/tmp/test.cpp:8:12: warning: using decl 'Foo' is unused [misc-unused-using-decls]
using foo::Foo;
           ^
/tmp/test.cpp:8:12: note: remove the using
using foo::Foo;
~~~~~~~~~~~^~~~

[hokein]

Emit the check fix description via diagnostic::note.

[hokein]

Running out of time today, the patch is not finished yet, but it should be good for another initial review/comments.

[alexfh]

This looks like a more promising direction. Thanks for the readiness to experiment with this.

See the comments inline.

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

130 ↗

(On Diff #193510)

Could you leave a FIXME here to explore options around interactive fix selection?

132 ↗

(On Diff #193510)

ErrorFix brings more questions than it answers. Maybe SelectedFix, ChosenFix, or just Fix?

133 ↗

(On Diff #193510)

nit: I'd add braces here, since the else branch has them.

144 ↗

(On Diff #193510)

The nesting level starts getting out of control here. I'd try to pull the loop into a function.

187 ↗

(On Diff #193510)

Why Error.Message.Fix? Should this use *SelectedFix instead?

262 ↗

(On Diff #193510)

nit: Early "continue" would make sense here.

if (!Repl.isApplicable())
  continue;
...

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

46 ↗

(On Diff #193510)

Some information from the original comment was lost here:

/// Fixes to apply, grouped by file path.

[hokein]

Fix apply-replacements, and address comments.

[hokein]

Remove a stale comment.

[hokein]

In D59932#1453565, @alexfh wrote:

This looks like a more promising direction. Thanks for the readiness to experiment with this.

See the comments inline.

Thanks for the comments. Now all existing tests are passed, the patch is in a good shape for review.

There is one missing point -- we don't test the fix-description notes in the lit tests, the current test mechanism (CHECK-MESSAGE, CHECK-NOTES) doesn't handle it well, we need to feature it out. I think this can be done in a separate patch.

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

130 ↗

(On Diff #193510)

Done. I moved this code to Diagnostic::getChosenFix() as we have a few places using it.

144 ↗

(On Diff #193510)

Agree, but making such refactoring change in this patch will add noise to the diff, I tend to make the change in this patch as mini as possible.

I think this can be improved in a follow-up change.

187 ↗

(On Diff #193510)

I think we still want to report all the alternative fixes to clients to align with clang's behavior. We only use the SelectedFix when applying the fix.

[alexfh]

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

67–71 ↗

(On Diff #193856)

Do we actually need this method here? This whole structure is sort of a data-only serialization helper, and this method is adding some (arbitrary) logic into it.

[hokein]

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

67–71 ↗

(On Diff #193856)

We have a few places using this method, or we could move this method out of this structure?

[alexfh]

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

67–71 ↗

(On Diff #193856)

I'd move it out closer to the code which uses it and call it "selectFirstFix", for example (the name should be a verb and it should be less vague about what the function is doing).

[hokein]

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

67–71 ↗

(On Diff #193856)

hmm, this function is used in ApplyReplacements.cpp, ClangTidy.cpp, ClangTidyTest.h, ClangTidyDiagnosticConsumer.cpp.

Diagnostic.h seems the most suitable place.

[hokein]

Update and rebase.

[hokein]

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

67–71 ↗

(On Diff #193856)

moved it out of the structure;

[alexfh]

Thanks! The change looks good now.

include/clang/Tooling/Core/Diagnostic.h

struct DiagnosticMessage {
   /// \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 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 llvm::StringMap<Replacements> &Fix,
              const SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
              llvm::StringRef BuildDirectory);
 
   /// Name identifying the Diagnostic.
   std::string DiagnosticName;
 
   /// Message associated to the diagnostic.
   DiagnosticMessage Message;
 
-  /// Fixes to apply, grouped by file path.
-  llvm::StringMap<Replacements> Fix;
-
   /// 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;
 };
 
 /// 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.
+// Return nullptr if no fixes 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

include/clang/Tooling/DiagnosticsYaml.h

 namespace llvm {
 namespace yaml {
 
 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)
+        Fixes.push_back(Replacement);
+    }
+    Io.mapRequired("Replacements", Fixes);
+    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";
+      }
+    }
   }
 };
 
 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), Fix(D.Fix),
-          Notes(D.Notes), DiagLevel(D.DiagLevel),
-          BuildDirectory(D.BuildDirectory) {}
+        : DiagnosticName(D.DiagnosticName), Message(D.Message), Notes(D.Notes),
+          DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory) {}
 
     clang::tooling::Diagnostic denormalize(const IO &) {
-      return clang::tooling::Diagnostic(DiagnosticName, Message, Fix, Notes,
+      return clang::tooling::Diagnostic(DiagnosticName, Message, Notes,
                                         DiagLevel, BuildDirectory);
     }
 
     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;
   };
 
   static void mapping(IO &Io, clang::tooling::Diagnostic &D) {
     MappingNormalization<NormalizedDiagnostic, clang::tooling::Diagnostic> Keys(
         Io, D);
     Io.mapRequired("DiagnosticName", Keys->DiagnosticName);
-    Io.mapRequired("Message", Keys->Message.Message);
-    Io.mapRequired("FileOffset", Keys->Message.FileOffset);
-    Io.mapRequired("FilePath", Keys->Message.FilePath);
+    Io.mapRequired("DiagnosticMessage", Keys->Message);
     Io.mapOptional("Notes", Keys->Notes);
 
     // FIXME: Export properly all the different fields.
-
-    std::vector<clang::tooling::Replacement> Fixes;
-    for (auto &Replacements : Keys->Fix) {
-      for (auto &Replacement : Replacements.second) {
-        Fixes.push_back(Replacement);
-      }
-    }
-    Io.mapRequired("Replacements", Fixes);
-    for (auto &Fix : Fixes) {
-      llvm::Error Err = Keys->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";
-      }
-    }
   }
 };
 
 /// 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);
     Io.mapRequired("Diagnostics", Doc.Diagnostics);
   }
 };
 } // end namespace yaml
 } // end namespace llvm
 
 #endif // LLVM_CLANG_TOOLING_DIAGNOSTICSYAML_H

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/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,

 
 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 llvm::StringMap<Replacements> &Fix,
                        const SmallVector<DiagnosticMessage, 1> &Notes,
                        Level DiagLevel, llvm::StringRef BuildDirectory)
-    : DiagnosticName(DiagnosticName), Message(Message), Fix(Fix), Notes(Notes),
+    : DiagnosticName(DiagnosticName), Message(Message), Notes(Notes),
       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

unittests/Tooling/DiagnosticsYamlTest.cpp

 #include "clang/Tooling/ReplacementsYaml.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 std::string &FilePath,
+                                     const StringMap<Replacements> &Fix) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
 static Diagnostic makeDiagnostic(StringRef DiagnosticName,
                                  const std::string &Message, int FileOffset,
                                  const std::string &FilePath,
                                  const StringMap<Replacements> &Fix) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-                    Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+                    makeMessage(Message, FileOffset, FilePath, Fix), {},
+                    Diagnostic::Warning, "path/to/build/directory");
 }
 
-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));
-
-  StringMap<Replacements> Fix2 = {
-      {"path/to/header.h",
-       Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
-  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-                                           "path/to/header.h", Fix2));
-
-  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"));
-  TUD.Diagnostics.back().Notes.push_back(
-      makeMessage("Note2", 99, "path/to/note2.cpp"));
-
-  std::string YamlContent;
-  raw_string_ostream YamlContentStream(YamlContent);
-
-  yaml::Output YAML(YamlContentStream);
-  YAML << TUD;
-
-  EXPECT_EQ("---\n"
-            "MainSourceFile:  'path/to/source.cpp'\n"
-            "Diagnostics:     \n"
-            "  - DiagnosticName:  'diagnostic#1\'\n"
-            "    Message:         'message #1'\n"
-            "    FileOffset:      55\n"
-            "    FilePath:        'path/to/source.cpp'\n"
-            "    Replacements:    \n"
-            "      - FilePath:        'path/to/source.cpp'\n"
-            "        Offset:          100\n"
-            "        Length:          12\n"
-            "        ReplacementText: 'replacement #1'\n"
-            "  - DiagnosticName:  'diagnostic#2'\n"
-            "    Message:         'message #2'\n"
-            "    FileOffset:      60\n"
-            "    FilePath:        'path/to/header.h'\n"
-            "    Replacements:    \n"
-            "      - FilePath:        'path/to/header.h'\n"
-            "        Offset:          62\n"
-            "        Length:          2\n"
-            "        ReplacementText: 'replacement #2'\n"
-            "  - DiagnosticName:  'diagnostic#3'\n"
-            "    Message:         'message #3'\n"
-            "    FileOffset:      72\n"
-            "    FilePath:        'path/to/source2.cpp'\n"
-            "    Notes:           \n"
-            "      - Message:         Note1\n"
-            "        FilePath:        'path/to/note1.cpp'\n"
-            "        FileOffset:      88\n"
-            "      - Message:         Note2\n"
-            "        FilePath:        'path/to/note2.cpp'\n"
-            "        FileOffset:      99\n"
-            "    Replacements:    []\n"
-            "...\n",
-            YamlContentStream.str());
+static const char *YAMLContent =
+    "---\n"
+    "    DiagnosticMessage: \n"
+    "      Message:         'message #1'\n"
+    "      FilePath:        'path/to/source.cpp'\n"
+    "      FileOffset:      55\n"
+    "      Replacements:    \n"
+    "        - FilePath:        'path/to/source.cpp'\n"
+    "          Offset:          100\n"
+    "          Length:          12\n"
+    "          ReplacementText: 'replacement #1'\n"
+    "    DiagnosticMessage: \n"
+    "      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"
+    "    DiagnosticMessage: \n"
+    "      Message:         'message #3'\n"
+    "      FilePath:        'path/to/source2.cpp'\n"
+    "      FileOffset:      72\n"
+    "      Replacements:    []\n"
+    "        Replacements:    []\n"
+    "        Replacements:    []\n"
+    "...\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));
+
+  StringMap<Replacements> Fix2 = {
+      {"path/to/header.h",
+       Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
+                                           "path/to/header.h", Fix2));
+
+  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", {}));
+  TUD.Diagnostics.back().Notes.push_back(
+      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) {
-  std::string YamlContent = "---\n"
-                            "MainSourceFile:  path/to/source.cpp\n"
-                            "Diagnostics:     \n"
-                            "  - DiagnosticName:  'diagnostic#1'\n"
-                            "    Message:         'message #1'\n"
-                            "    FileOffset:      55\n"
-                            "    FilePath:        path/to/source.cpp\n"
-                            "    Replacements:    \n"
-                            "      - FilePath:        path/to/source.cpp\n"
-                            "        Offset:          100\n"
-                            "        Length:          12\n"
-                            "        ReplacementText: 'replacement #1'\n"
-                            "  - DiagnosticName:  'diagnostic#2'\n"
-                            "    Message:         'message #2'\n"
-                            "    FileOffset:      60\n"
-                            "    FilePath:        path/to/header.h\n"
-                            "    Replacements:    \n"
-                            "      - FilePath:        path/to/header.h\n"
-                            "        Offset:          62\n"
-                            "        Length:          2\n"
-                            "        ReplacementText: 'replacement #2'\n"
-                            "  - DiagnosticName:  'diagnostic#3'\n"
-                            "    Message:         'message #3'\n"
-                            "    FileOffset:      98\n"
-                            "    FilePath:        path/to/source.cpp\n"
-                            "    Notes:\n"
-                            "      - Message:         Note1\n"
-                            "        FilePath:        'path/to/note1.cpp'\n"
-                            "        FileOffset:      66\n"
-                            "      - Message:         Note2\n"
-                            "        FilePath:        'path/to/note2.cpp'\n"
-                            "        FileOffset:      77\n"
-                            "    Replacements:    []\n"
-                            "...\n";
   TranslationUnitDiagnostics TUDActual;
-  yaml::Input YAML(YamlContent);
+  yaml::Input YAML(YAMLContent);
   YAML >> TUDActual;
 
   ASSERT_FALSE(YAML.error());
   ASSERT_EQ(3u, TUDActual.Diagnostics.size());
   EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);
 
   auto getFixes = [](const StringMap<Replacements> &Fix) {
     std::vector<Replacement> Fixes;
     for (auto &Replacements : Fix) {
       for (auto &Replacement : Replacements.second) {
         Fixes.push_back(Replacement);
       }
     }
     return Fixes;
   };
 
   Diagnostic D1 = TUDActual.Diagnostics[0];
   EXPECT_EQ("diagnostic#1", D1.DiagnosticName);
   EXPECT_EQ("message #1", D1.Message.Message);
   EXPECT_EQ(55u, D1.Message.FileOffset);
   EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
-  std::vector<Replacement> Fixes1 = getFixes(D1.Fix);
+  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());
 
   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.Fix);
+  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());
 
   Diagnostic D3 = TUDActual.Diagnostics[2];
   EXPECT_EQ("diagnostic#3", D3.DiagnosticName);
   EXPECT_EQ("message #3", D3.Message.Message);
-  EXPECT_EQ(98u, D3.Message.FileOffset);
-  EXPECT_EQ("path/to/source.cpp", D3.Message.FilePath);
+  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(66u, D3.Notes[0].FileOffset);
+  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(77u, D3.Notes[1].FileOffset);
+  EXPECT_EQ(99u, D3.Notes[1].FileOffset);
   EXPECT_EQ("path/to/note2.cpp", D3.Notes[1].FilePath);
-  std::vector<Replacement> Fixes3 = getFixes(D3.Fix);
+  std::vector<Replacement> Fixes3 = getFixes(D3.Message.Fix);
   EXPECT_TRUE(Fixes3.empty());
 }