Fix clang-tidy crash when a single fix is applied on multiple files.

Summary:
tooling::Replacements only holds replacements for a single file, so
this patch makes Fix a map from file paths to tooling::Replacements so that it
can be applied on multiple files.

Reviewers: hokein, alexfh

Subscribers: Prazek, cfe-commits

Differential Revision: https://reviews.llvm.org/D23257

llvm-svn: 278101

6 files changed, 83 insertions, 47 deletions

diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 562e1e80f54..f2ad5b2b9a8 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -126,27 +126,32 @@ public:
       }
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
-      for (const tooling::Replacement &Fix : Error.Fix) {
-        // 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.
-        SourceRange Range;
-        SourceLocation FixLoc;
-        if (Fix.isApplicable()) {
-          SmallString<128> FixAbsoluteFilePath = Fix.getFilePath();
-          Files.makeAbsolutePath(FixAbsoluteFilePath);
-          FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset());
-          SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength());
-          Range = SourceRange(FixLoc, FixEndLoc);
-          Diag << FixItHint::CreateReplacement(Range, Fix.getReplacementText());
-        }
-
-        ++TotalFixes;
-        if (ApplyFixes) {
-          bool Success = Fix.isApplicable() && Fix.apply(Rewrite);
-          if (Success)
-            ++AppliedFixes;
-          FixLocations.push_back(std::make_pair(FixLoc, Success));
+      for (const auto &FileAndReplacements : Error.Fix) {
+        for (const auto &Replacement : FileAndReplacements.second) {
+          // 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.
+          SourceRange Range;
+          SourceLocation FixLoc;
+          if (Replacement.isApplicable()) {
+            SmallString<128> FixAbsoluteFilePath = Replacement.getFilePath();
+            Files.makeAbsolutePath(FixAbsoluteFilePath);
+            FixLoc = getLocation(FixAbsoluteFilePath, Replacement.getOffset());
+            SourceLocation FixEndLoc =
+                FixLoc.getLocWithOffset(Replacement.getLength());
+            Range = SourceRange(FixLoc, FixEndLoc);
+            Diag << FixItHint::CreateReplacement(
+                Range, Replacement.getReplacementText());
+          }
+
+          ++TotalFixes;
+          if (ApplyFixes) {
+            bool Success =
+                Replacement.isApplicable() && Replacement.apply(Rewrite);
+            if (Success)
+              ++AppliedFixes;
+            FixLocations.push_back(std::make_pair(FixLoc, Success));
+          }
         }
       }
     }
@@ -511,9 +516,12 @@ void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
 void exportReplacements(const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS) {
   tooling::TranslationUnitReplacements TUR;
-  for (const ClangTidyError &Error : Errors)
-    TUR.Replacements.insert(TUR.Replacements.end(), Error.Fix.begin(),
-                            Error.Fix.end());
+  for (const ClangTidyError &Error : Errors) {
+    for (const auto &FileAndFixes : Error.Fix)
+      TUR.Replacements.insert(TUR.Replacements.end(),
+                              FileAndFixes.second.begin(),
+                              FileAndFixes.second.end());
+  }
 
   yaml::Output YAML(OS);
   YAML << TUR;
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 940b0634936..8dbae40e4e9 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -77,8 +77,8 @@ protected:
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
-      auto Err =
-          Error.Fix.add(tooling::Replacement(SM, Range, FixIt.CodeToInsert));
+      tooling::Replacement Replacement(SM, Range, FixIt.CodeToInsert);
+      llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
       // FIXME: better error handling.
       if (Err) {
         llvm::errs() << "Fix conflicts with existing fix! "
@@ -495,25 +495,29 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
   std::vector<int> Sizes;
   for (const auto &Error : Errors) {
     int Size = 0;
-    for (const auto &Replace : Error.Fix)
-      Size += Replace.getLength();
+    for (const auto &FileAndReplaces : Error.Fix) {
+      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 &Replace : Errors[I].Fix) {
-      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;
-      FileEvents[FilePath].push_back(
-          Event(Begin, End, Event::ET_Begin, I, Sizes[I]));
-      FileEvents[FilePath].push_back(
-          Event(Begin, End, Event::ET_End, I, Sizes[I]));
+    for (const auto &FileAndReplace : Errors[I].Fix) {
+      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;
+        FileEvents[FilePath].push_back(
+            Event(Begin, End, Event::ET_Begin, I, Sizes[I]));
+        FileEvents[FilePath].push_back(
+            Event(Begin, End, Event::ET_End, I, Sizes[I]));
+      }
     }
   }
 
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index d7e4d2be9aa..9f83aac60ba 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -62,7 +62,8 @@ struct ClangTidyError {
 
   std::string CheckName;
   ClangTidyMessage Message;
-  tooling::Replacements Fix;
+  // Fixes grouped by file path.
+  llvm::StringMap<tooling::Replacements> Fix;
   SmallVector<ClangTidyMessage, 1> Notes;
 
   // A build directory of the diagnostic source file.
diff --git a/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h b/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h
new file mode 100644
index 00000000000..62609e25f68
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/Inputs/modernize-pass-by-value/header-with-fix.h
@@ -0,0 +1,8 @@
+struct S {
+  S(S&&);
+  S(const S&);
+};
+struct Foo {
+  Foo(const S &s);
+  S s;
+};
diff --git a/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp
new file mode 100644
index 00000000000..7980c301c57
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value-multi-fixes.cpp
@@ -0,0 +1,12 @@
+// RUN: cat %S/Inputs/modernize-pass-by-value/header-with-fix.h > %T/pass-by-value-header-with-fix.h
+// RUN: sed -e 's#//.*$##' %s > %t.cpp
+// RUN: clang-tidy %t.cpp -checks='-*,modernize-pass-by-value' -header-filter='.*' -fix -- -std=c++11 -I %T | FileCheck %s -check-prefix=CHECK-MESSAGES -implicit-check-not="{{warning|error}}:"
+// RUN: FileCheck -input-file=%t.cpp %s -check-prefix=CHECK-FIXES
+// RUN: FileCheck -input-file=%T/pass-by-value-header-with-fix.h %s -check-prefix=CHECK-HEADER-FIXES
+
+#include "pass-by-value-header-with-fix.h"
+// CHECK-HEADER-FIXES: Foo(S s);
+Foo::Foo(const S &s) : s(s) {}
+// CHECK-MESSAGES: :9:10: warning: pass by value and use std::move [modernize-pass-by-value]
+// CHECK-FIXES: #include <utility>
+// CHECK-FIXES: Foo::Foo(S s) : s(std::move(s)) {}
diff --git a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
index 0e278f8bd78..6dfce90f435 100644
--- a/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
+++ b/clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
@@ -118,15 +118,18 @@ runCheckOnCode(StringRef Code, std::vector<ClangTidyError> *Errors = nullptr,
 
   DiagConsumer.finish();
   tooling::Replacements Fixes;
-  for (const ClangTidyError &Error : Context.getErrors())
-    for (const auto &Fix : Error.Fix) {
-      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 "";
+  for (const ClangTidyError &Error : Context.getErrors()) {
+    for (const auto &FileAndFixes : Error.Fix) {
+      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 = Context.getErrors();
   auto Result = tooling::applyAllReplacements(Code, Fixes);