[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

76

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

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

132

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

133

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

144

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

174

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

249

nit: Early "continue" would make sense here.

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

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

46

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

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

144

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.

174

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

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

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

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

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

moved it out of the structure;

[alexfh]

Thanks! The change looks good now.

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   };
 
   for (const auto &TU : TUs)
     for (const tooling::Replacement &R : TU.Replacements)
       AddToGroup(R, false);
 
   for (const auto &TU : TUDs)
     for (const auto &D : TU.Diagnostics)
-      for (const auto &Fix : D.Fix)
+      if (const auto *ChoosenFix = D.getChosenFix()) {
+        for (const auto &Fix : *ChoosenFix)
           for (const tooling::Replacement &R : Fix.second)
             AddToGroup(R, true);
+      }
 
   // Sort replacements per file to keep consistent behavior when
   // clang-apply-replacements run on differents machine.
   for (auto &FileAndReplacements : GroupedReplacements) {
     llvm::sort(FileAndReplacements.second.begin(),
                FileAndReplacements.second.end());
   }
 

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

 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Rewrite/Frontend/FixItRewriter.h"
 #include "clang/Rewrite/Frontend/FrontendActions.h"
+#include "clang/Tooling/Core/Diagnostic.h"
 #if CLANG_ENABLE_STATIC_ANALYZER
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #endif // CLANG_ENABLE_STATIC_ANALYZER
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "clang/Tooling/Tooling.h"
public:
   AnalyzerDiagnosticConsumer(ClangTidyContext &Context) : Context(Context) {}
 
   void FlushDiagnosticsImpl(std::vector<const ento::PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
     for (const ento::PathDiagnostic *PD : Diags) {
       SmallString<64> CheckName(AnalyzerCheckNamePrefix);
       CheckName += PD->getCheckName();
       Context.diag(CheckName, PD->getLocation().asLocation(),
                    PD->getShortDescription())
           << PD->path.back()->getRanges();
 
       for (const auto &DiagPiece :
            PD->path.flatten(/*ShouldFlattenMacros=*/true)) {
         Context.diag(CheckName, DiagPiece->getLocation().asLocation(),
                      DiagPiece->getString(), DiagnosticIDs::Note)
             << DiagPiece->getRanges();
       }
SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
       std::string Name = Error.DiagnosticName;
       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 auto &FileAndReplacements : Error.Fix) {
+
+      const llvm::StringMap<Replacements> *ChosenFix = Error.getChosenFix();
+      if (ApplyFixes && ChosenFix) {
+        for (const auto &FileAndReplacements : *ChosenFix) {
           for (const auto &Repl : FileAndReplacements.second) {
-          SourceLocation FixLoc;
             ++TotalFixes;
             bool CanBeApplied = false;
             if (Repl.isApplicable()) {
+              SourceLocation FixLoc;
               SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
               Files.makeAbsolutePath(FixAbsoluteFilePath);
-            if (ApplyFixes) {
               tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
                                      Repl.getLength(),
                                      Repl.getReplacementText());
               Replacements &Replacements = FileReplacements[R.getFilePath()];
               llvm::Error Err = Replacements.add(R);
               if (Err) {
                 // FIXME: Implement better conflict handling.
                 llvm::errs() << "Trying to resolve conflict: "
                              << llvm::toString(std::move(Err)) << "\n";
                 unsigned NewOffset =
                     Replacements.getShiftedCodePosition(R.getOffset());
                 unsigned NewLength = Replacements.getShiftedCodePosition(
                                          R.getOffset() + R.getLength()) -
                                      NewOffset;
                 if (NewLength == R.getLength()) {
                   R = Replacement(R.getFilePath(), NewOffset, NewLength,
                                   R.getReplacementText());
                   Replacements = Replacements.merge(tooling::Replacements(R));
                   CanBeApplied = true;
                   ++AppliedFixes;
                 } else {
                   llvm::errs()
                       << "Can't resolve conflict, skipping the replacement.\n";
                 }
-
               } else {
                 CanBeApplied = true;
                 ++AppliedFixes;
               }
-            }
               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());
-          }
-
-          if (ApplyFixes)
               FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
             }
           }
         }
+      }
+      reportFix(Diag, Error.Message.Fix);
+    }
     for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }
     for (const auto &Note : Error.Notes)
       reportNote(Note);
   }
 
SourceLocation getLocation(StringRef FilePath, unsigned Offset) {
     if (FilePath.empty())
       return SourceLocation();
 
     const FileEntry *File = SourceMgr.getFileManager().getFile(FilePath);
     FileID ID = SourceMgr.getOrCreateFileID(File, SrcMgr::C_User);
     return SourceMgr.getLocForStartOfFile(ID).getLocWithOffset(Offset);
   }
 
+  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());
+      }
+    }
+  }
+
   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;
+    reportFix(Diag, Message.Fix);
   }
 
   FileManager Files;
   LangOptions LangOpts; // FIXME: use langopts from each original file
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;

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

 ///    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "clang/AST/ASTDiagnostic.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Tooling/Core/Diagnostic.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include <tuple>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
 namespace {
protected:
   void emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
                          DiagnosticsEngine::Level Level,
                          ArrayRef<CharSourceRange> Ranges) override {}
 
   void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
                        SmallVectorImpl<CharSourceRange> &Ranges,
                        ArrayRef<FixItHint> Hints) override {
     assert(Loc.isValid());
+    tooling::DiagnosticMessage *DiagWithFix =
+        Level == DiagnosticsEngine::Note ? &Error.Notes.back() : &Error.Message;
+
     for (const auto &FixIt : Hints) {
       CharSourceRange Range = FixIt.RemoveRange;
       assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
              "Invalid range in the fix-it hint.");
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
       tooling::Replacement Replacement(Loc.getManager(), Range,
                                        FixIt.CodeToInsert);
-      llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
+      llvm::Error Err =
+          DiagWithFix->Fix[Replacement.getFilePath()].add(Replacement);
       // FIXME: better error handling (at least, don't let other replacements be
       // applied).
       if (Err) {
         llvm::errs() << "Fix conflicts with existing fix! "
                      << llvm::toString(std::move(Err)) << "\n";
         assert(false && "Fix conflicts with existing fix!");
       }
     }
struct Event {
     // belongs.
     unsigned ErrorId;
     // The events will be sorted based on this field.
     std::tuple<unsigned, EventType, int, int, unsigned> Priority;
   };
 
   // Compute error sizes.
   std::vector<int> Sizes;
-  for (const auto &Error : Errors) {
+  std::vector<
+      std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
+      ErrorFixes;
+  for (auto &Error : Errors) {
+    if (const auto *Fix = Error.getChosenFix())
+      ErrorFixes.emplace_back(
+          &Error, const_cast<llvm::StringMap<tooling::Replacements> *>(Fix));
+  }
+  for (const auto &ErrorAndFix : ErrorFixes) {
     int Size = 0;
-    for (const auto &FileAndReplaces : Error.Fix) {
+    for (const auto &FileAndReplaces : *ErrorAndFix.second) {
       for (const auto &Replace : FileAndReplaces.second)
         Size += Replace.getLength();
     }
     Sizes.push_back(Size);
   }
 
   // Build events from error intervals.
   std::map<std::string, std::vector<Event>> FileEvents;
-  for (unsigned I = 0; I < Errors.size(); ++I) {
-    for (const auto &FileAndReplace : Errors[I].Fix) {
+  for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
+    for (const auto &FileAndReplace : *ErrorFixes[I].second) {
       for (const auto &Replace : FileAndReplace.second) {
         unsigned Begin = Replace.getOffset();
         unsigned End = Begin + Replace.getLength();
         const std::string &FilePath = Replace.getFilePath();
         // FIXME: Handle empty intervals, such as those from insertions.
         if (Begin == End)
           continue;
         auto &Events = FileEvents[FilePath];
         Events.emplace_back(Begin, End, Event::ET_Begin, I, Sizes[I]);
         Events.emplace_back(Begin, End, Event::ET_End, I, Sizes[I]);
       }
     }
   }
 
-  std::vector<bool> Apply(Errors.size(), true);
+  std::vector<bool> Apply(ErrorFixes.size(), true);
   for (auto &FileAndEvents : FileEvents) {
     std::vector<Event> &Events = FileAndEvents.second;
     // Sweep.
     std::sort(Events.begin(), Events.end());
     int OpenIntervals = 0;
     for (const auto &Event : Events) {
       if (Event.Type == Event::ET_End)
         --OpenIntervals;
       // This has to be checked after removing the interval from the count if it
       // is an end event, or before adding it if it is a begin event.
       if (OpenIntervals != 0)
         Apply[Event.ErrorId] = false;
       if (Event.Type == Event::ET_Begin)
         ++OpenIntervals;
     }
     assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match");
   }
 
-  for (unsigned I = 0; I < Errors.size(); ++I) {
+  for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
     if (!Apply[I]) {
-      Errors[I].Fix.clear();
-      Errors[I].Notes.emplace_back(
+      ErrorFixes[I].second->clear();
+      ErrorFixes[I].first->Notes.emplace_back(
           "this fix will not be applied because it overlaps with another fix");
     }
   }
 }
 
 namespace {
 struct LessClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {

clang-tools-extra/clang-tidy/add_new_check.py

 }
 
 void %(check_name)s::check(const MatchFinder::MatchResult &Result) {
   // FIXME: Add callback implementation.
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("x");
   if (MatchedDecl->getName().startswith("awesome_"))
     return;
   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
-      << MatchedDecl
+      << MatchedDecl;
+  diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
       << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
 }
 
 } // namespace %(module)s
 } // namespace tidy
 } // namespace clang
 """ % {'check_name': check_name_camel,
        'module': module})

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp

if (Context.UsingTargetDecls.count(D->getCanonicalDecl()) > 0)
       Context.IsUsed = true;
   }
 }
 
 void UnusedUsingDeclsCheck::onEndOfTranslationUnit() {
   for (const auto &Context : Contexts) {
     if (!Context.IsUsed) {
       diag(Context.FoundUsingDecl->getLocation(), "using decl %0 is unused")
-          << Context.FoundUsingDecl
+          << Context.FoundUsingDecl;
+      // Emit a fix and a fix description of the check;
+      diag(Context.FoundUsingDecl->getLocation(),
+           /*FixDescription=*/"remove the using", DiagnosticIDs::Note)
           << FixItHint::CreateRemoval(Context.UsingDeclRange);
     }
   }
   Contexts.clear();
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file1.yaml

 ---
 MainSourceFile:     source1.cpp
 Diagnostics:
   - DiagnosticName: test-basic
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/basic.h
       FileOffset: 242
       Replacements:
         - FilePath:        $(path)/basic.h
           Offset:          242
           Length:          26
           ReplacementText: 'auto & elem : ints'
         - FilePath:        $(path)/basic.h
           Offset:          276
           Length:          22
           ReplacementText: ''
         - FilePath:        $(path)/basic.h
           Offset:          298
           Length:          1
           ReplacementText: elem
         - FilePath:        $(path)/../basic/basic.h
           Offset:          148
           Length:          0
           ReplacementText: 'override '
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml

 ---
 MainSourceFile:     source2.cpp
 Diagnostics:
   - DiagnosticName: test-basic
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/basic.h
       FileOffset: 148
       Replacements:
         - FilePath:        $(path)/../basic/basic.h
           Offset:          298
           Length:          1
           ReplacementText: elem
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml

 ---
 MainSourceFile: source1.cpp
 Diagnostics:
   - DiagnosticName: test-conflict
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/common.h
       FileOffset: 106
       Replacements:
         - FilePath:        $(path)/common.h
           Offset:          106
           Length:          26
           ReplacementText: 'auto & i : ints'
         - FilePath:        $(path)/common.h
           Offset:          140
           Length:          7
           ReplacementText: i
         - FilePath:        $(path)/common.h
           Offset:          160
           Length:          12
           ReplacementText: ''
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml

 ---
 MainSourceFile: source2.cpp
 Diagnostics:
   - DiagnosticName:  test-conflict
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/common.h
       FileOffset: 106
       Replacements:
         - FilePath:        $(path)/common.h
           Offset:          106
           Length:          26
           ReplacementText: 'int & elem : ints'
         - FilePath:        $(path)/common.h
           Offset:          140
           Length:          7
           ReplacementText: elem
         - FilePath:        $(path)/common.h
           Offset:          169
           Length:          1
           ReplacementText: nullptr
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml

 ---
 MainSourceFile: source1.cpp
 Diagnostics:
   - DiagnosticName:  test-conflict
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/common.h
       FileOffset: 169
       Replacements:
         - FilePath:        $(path)/common.h
           Offset:          169
           Length:          0
           ReplacementText: "(int*)"
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/file1.yaml

 ---
 MainSourceFile:      source1.cpp
 Diagnostics:
   - DiagnosticName:  test-crlf
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/crlf.cpp
       FileOffset: 79
       Replacements:
         - FilePath:        $(path)/crlf.cpp
           Offset:          79
           Length:          1
           ReplacementText: nullptr
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/format/no.yaml

 ---
 MainSourceFile:  no.cpp
 Diagnostics:
   - DiagnosticName:  test-no
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/no.cpp
       FileOffset: 94
       Replacements:
         - FilePath:        $(path)/no.cpp
           Offset:          94
           Length:          3
           ReplacementText: 'auto '
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/format/yes.yaml

 # These replacements are out of order on purpose to test that they get sorted
 # so that formatting happens correctly.
 ---
 MainSourceFile:  yes.cpp
 Diagnostics:
   - DiagnosticName:  test-yes
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/yes.cpp
       FileOffset: 494
       Replacements:
         - FilePath:        $(path)/yes.cpp
           Offset:          494
           Length:          1
           ReplacementText: nullptr
         - FilePath:        $(path)/yes.cpp
           Offset:          410
           Length:          1
           ReplacementText: nullptr
         - FilePath:        $(path)/yes.cpp
           Offset:          454
           Length:          1
           ReplacementText: nullptr
         - FilePath:        $(path)/yes.cpp
           Offset:          108
           Length:          38
           ReplacementText: 'auto '
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file1.yaml

 ---
 MainSourceFile:     identical.cpp
 Diagnostics:
   - DiagnosticName: test-identical-insertion
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/identical.cpp
       FileOffset: 12
       Replacements:
         - FilePath:        $(path)/identical.cpp
           Offset:          12
           Length:          0
           ReplacementText: '0'
 ...
 

clang-tools-extra/test/clang-apply-replacements/Inputs/identical/file2.yaml

 ---
 MainSourceFile:     identical.cpp
 Diagnostics:
   - DiagnosticName: test-identical-insertion
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/identical.cpp
       FileOffset: 12
       Replacements:
         - FilePath:        $(path)/identical.cpp
           Offset:          12
           Length:          0
           ReplacementText: '0'
 ...
 

clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml

 ---
 MainSourceFile:     order-dependent.cpp
 Diagnostics:
   - DiagnosticName: test-order-dependent-insertion
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/order-dependent.cpp
       FileOffset: 12
       Replacements:
         - FilePath:        $(path)/order-dependent.cpp
           Offset:          12
           Length:          0
           ReplacementText: '0'
 ...

clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml

 ---
 MainSourceFile:     order-dependent.cpp
 Diagnostics:
   - DiagnosticName: test-order-dependent-insertion
+    DiagnosticMessage:
       Message: Fix
       FilePath: $(path)/order-dependent.cpp
       FileOffset: 12
       Replacements:
         - FilePath:        $(path)/order-dependent.cpp
           Offset:          12
           Length:          0
           ReplacementText: '1'
 ...

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

 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t-input.cpp
 // RUN: clang-tidy %t-input.cpp -checks='-*,google-explicit-constructor,clang-diagnostic-missing-prototypes' -export-fixes=%t.yaml -- -Wmissing-prototypes > %t.msg 2>&1
 // RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s -implicit-check-not='{{warning|error|note}}:'
 // RUN: FileCheck -input-file=%t.yaml -check-prefix=CHECK-YAML %s
 #define X(n) void n ## n() {}
 X(f)
 
 // CHECK-MESSAGES: -input.cpp:2:1: warning: no previous prototype for function 'ff' [clang-diagnostic-missing-prototypes]
 // CHECK-MESSAGES: -input.cpp:1:19: note: expanded from macro 'X'
 // CHECK-MESSAGES: {{^}}note: expanded from here{{$}}
 
 // CHECK-YAML: ---
 // CHECK-YAML-NEXT: MainSourceFile:  '{{.*}}-input.cpp'
 // CHECK-YAML-NEXT: Diagnostics:
 // CHECK-YAML-NEXT:   - DiagnosticName:  clang-diagnostic-missing-prototypes
-// CHECK-YAML-NEXT:     Message:         'no previous prototype for function ''ff'''
-// CHECK-YAML-NEXT:     FileOffset:      30
-// CHECK-YAML-NEXT:     FilePath:        '{{.*}}-input.cpp'
+// 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:     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:     Replacements:    []
+// CHECK-YAML-NEXT:         Replacements:    []
 // CHECK-YAML-NEXT: ...
-

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

 makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName,
                   const DiagnosticMessage &Message,
                   const StringMap<Replacements> &Replacements,
                   StringRef BuildDirectory) {
   TUDiagnostics TUs;
   TUs.push_back({MainSourceFile,
                  {{DiagnosticName,
                    Message,
-                   Replacements,
                    {},
                    Diagnostic::Warning,
                    BuildDirectory}}});
   return TUs;
 }
 
 // Test to ensure diagnostics with no fixes, will be merged correctly
 // before applying.

clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h

 //===--- ClangTidyTest.h - clang-tidy ---------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANGTIDYTEST_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANG_TIDY_CLANGTIDYTEST_H
 
 #include "ClangTidy.h"
 #include "ClangTidyDiagnosticConsumer.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Path.h"
 #include <map>
 #include <memory>
 
 namespace clang {
for (const auto &Error : DiagConsumer.take()) {
       ErrorText += Error.Message.Message + "\n";
     }
     llvm::report_fatal_error(ErrorText);
   }
 
   tooling::Replacements Fixes;
   std::vector<ClangTidyError> Diags = DiagConsumer.take();
   for (const ClangTidyError &Error : Diags) {
-    for (const auto &FileAndFixes : Error.Fix) {
+    if (const auto *ChosenFix = Error.getChosenFix())
+      for (const auto &FileAndFixes : *ChosenFix) {
         for (const auto &Fix : FileAndFixes.second) {
           auto Err = Fixes.add(Fix);
           // FIXME: better error handling. Keep the behavior for now.
           if (Err) {
             llvm::errs() << llvm::toString(std::move(Err)) << "\n";
             return "";
           }
         }
       }
   }
   if (Errors)
     *Errors = std::move(Diags);
   auto Result = tooling::applyAllReplacements(Code, Fixes);
   if (!Result) {
     // FIXME: propogate the error.
     llvm::consumeError(Result.takeError());
     return "";

clang/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);
 
+  // Get the chosen fix to apply for this diagnostic.
+  // FIXME: currently we choose the first non-empty fix, extend it to support
+  // fix selection.
+  const llvm::StringMap<Replacements> *getChosenFix() const;
+
   /// 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.
   ///

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

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/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> *Diagnostic::getChosenFix() const {
+  if (!Message.Fix.empty())
+    return &Message.Fix;
+  auto Iter = llvm::find_if(Notes, [](const tooling::DiagnosticMessage &D) {
+    return !D.Fix.empty();
+  });
+  if (Iter != Notes.end())
+    return &Iter->Fix;
+  return nullptr;
+}
+
 } // end namespace tooling
 } // end namespace clang

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());
 }