Make llvm::Error generated from replacement interfaces more specific.

[ioeric]

Summary

The new error information contains the type of error (e.g. overlap or bad file path)
and the replacement(s) that is causing the error. This enables us to resolve some errors.
For example, for insertion at the same location conflict, we need to know the
existing replacement which conflicts with the new replacement in order to calculate
the new position to be insert before/after the existing replacement (for merging).

[bkramer]

include/clang/Tooling/Core/Replacement.h

157 ↗

(On Diff #78519)

Can you use pass by value and std::move here?

162 ↗

(On Diff #78519)

Here too.

174 ↗

(On Diff #78519)

This on the other hand should return a const ref.

178 ↗

(On Diff #78519)

This too.

lib/Tooling/Core/Replacement.cpp

157 ↗

(On Diff #78519)

Drop the 's here.

[ioeric]

• Addressed review comments.

[ioeric]

Thanks for the review!

cfe/trunk/include/clang/Tooling/Core/Replacement.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H
 #define LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
 #include <map>
 #include <set>
 #include <string>
+#include <system_error>
 #include <vector>
 
 namespace clang {
 
 class Rewriter;
 
 namespace tooling {
 
void setFromSourceRange(const SourceManager &Sources,
                           StringRef ReplacementText,
                           const LangOptions &LangOpts);
 
   std::string FilePath;
   Range ReplacementRange;
   std::string ReplacementText;
 };
 
+enum class replacement_error {
+  fail_to_apply = 0,
+  wrong_file_path,
+  overlap_conflict,
+  insert_conflict,
+};
+
+/// \brief Carries extra error information in replacement-related llvm::Error,
+/// e.g. fail applying replacements and replacements conflict.
+class ReplacementError : public llvm::ErrorInfo<ReplacementError> {
+public:
+  ReplacementError(replacement_error Err) : Err(Err) {}
+
+  /// \brief Constructs an error related to an existing replacement.
+  ReplacementError(replacement_error Err, Replacement Existing)
+      : Err(Err), ExistingReplacement(std::move(Existing)) {}
+
+  /// \brief Constructs an error related to a new replacement and an existing
+  /// replacement in a set of replacements.
+  ReplacementError(replacement_error Err, Replacement New, Replacement Existing)
+      : Err(Err), NewReplacement(std::move(New)),
+        ExistingReplacement(std::move(Existing)) {}
+
+  std::string message() const override;
+
+  void log(raw_ostream &OS) const override { OS << message(); }
+
+  replacement_error get() const { return Err; }
+
+  static char ID;
+
+  const llvm::Optional<Replacement> &getNewReplacement() const {
+    return NewReplacement;
+  }
+
+  const llvm::Optional<Replacement> &getExistingReplacement() const {
+    return ExistingReplacement;
+  }
+
+private:
+  // Users are not expected to use error_code.
+  std::error_code convertToErrorCode() const override {
+    return llvm::inconvertibleErrorCode();
+  }
+
+  replacement_error Err;
+  // A new replacement, which is to expected be added into a set of
+  // replacements, that is causing problem.
+  llvm::Optional<Replacement> NewReplacement;
+  // An existing replacement in a replacements set that is causing problem.
+  llvm::Optional<Replacement> ExistingReplacement;
+};
+
 /// \brief Less-than operator between two Replacements.
 bool operator<(const Replacement &LHS, const Replacement &RHS);
 
 /// \brief Equal-to operator between two Replacements.
 bool operator==(const Replacement &LHS, const Replacement &RHS);
 
 /// \brief Maintains a set of replacements that are conflict-free.
 /// Two replacements are considered conflicts if they overlap or have the same

cfe/trunk/lib/Tooling/Core/Replacement.cpp

 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_os_ostream.h"
 
 namespace clang {
 namespace tooling {
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement()
-  : FilePath(InvalidLocation) {}
+Replacement::Replacement() : FilePath(InvalidLocation) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
                          StringRef ReplacementText)
     : FilePath(FilePath), ReplacementRange(Offset, Length),
       ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
                          unsigned Length, StringRef ReplacementText) {

 Replacement
 Replacements::getReplacementInChangedCode(const Replacement &R) const {
   unsigned NewStart = getShiftedCodePosition(R.getOffset());
   unsigned NewEnd = getShiftedCodePosition(R.getOffset() + R.getLength());
   return Replacement(R.getFilePath(), NewStart, NewEnd - NewStart,
                      R.getReplacementText());
 }
 
-static llvm::Error makeConflictReplacementsError(const Replacement &New,
-                                                 const Replacement &Existing) {
-  return llvm::make_error<llvm::StringError>(
-      "New replacement:\n" + New.toString() +
-          "\nconflicts with existing replacement:\n" + Existing.toString(),
-      llvm::inconvertibleErrorCode());
+static std::string getReplacementErrString(replacement_error Err) {
+  switch (Err) {
+  case replacement_error::fail_to_apply:
+    return "Failed to apply a replacement.";
+  case replacement_error::wrong_file_path:
+    return "The new replacement's file path is different from the file path of "
+           "existing replacements";
+  case replacement_error::overlap_conflict:
+    return "The new replacement overlaps with an existing replacement.";
+  case replacement_error::insert_conflict:
+    return "The new insertion has the same insert location as an existing "
+           "replacement.";
+  }
+  llvm_unreachable("A value of replacement_error has no message.");
+}
+
+std::string ReplacementError::message() const {
+  std::string Message = getReplacementErrString(Err);
+  if (NewReplacement.hasValue())
+    Message += "\nNew replacement: " + NewReplacement->toString();
+  if (ExistingReplacement.hasValue())
+    Message += "\nExisting replacement: " + ExistingReplacement->toString();
+  return Message;
 }
 
+char ReplacementError::ID = 0;
+
 Replacements Replacements::getCanonicalReplacements() const {
   std::vector<Replacement> NewReplaces;
   // Merge adjacent replacements.
   for (const auto &R : Replaces) {
     if (NewReplaces.empty()) {
       NewReplaces.push_back(R);
       continue;
     }
Replacements::mergeIfOrderIndependent(const Replacement &R) const {
   // This is equivalent to applying `R` first and then `Replaces`.
   auto MergeShiftedReplaces = Rs.merge(ReplacesShiftedByRs);
 
   // Since empty or segmented replacements around existing replacements might be
   // produced above, we need to compare replacements in canonical forms.
   if (MergeShiftedRs.getCanonicalReplacements() ==
       MergeShiftedReplaces.getCanonicalReplacements())
     return MergeShiftedRs;
-  return makeConflictReplacementsError(R, *Replaces.begin());
+  return llvm::make_error<ReplacementError>(replacement_error::overlap_conflict,
+                                            R, *Replaces.begin());
 }
 
 llvm::Error Replacements::add(const Replacement &R) {
   // Check the file path.
   if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath())
-    return llvm::make_error<llvm::StringError>(
-        "All replacements must have the same file path. New replacement: " +
-            R.getFilePath() + ", existing replacements: " +
-            Replaces.begin()->getFilePath() + "\n",
-        llvm::inconvertibleErrorCode());
+    return llvm::make_error<ReplacementError>(
+        replacement_error::wrong_file_path, R, *Replaces.begin());
 
   // Special-case header insertions.
   if (R.getOffset() == UINT_MAX) {
     Replaces.insert(R);
     return llvm::Error::success();
   }
 
   // This replacement cannot conflict with replacements that end before
llvm::Error Replacements::add(const Replacement &R) {
   // If `I` starts at the same offset as `R`, `R` must be an insertion.
   if (I != Replaces.end() && R.getOffset() == I->getOffset()) {
     assert(R.getLength() == 0);
     // `I` is also an insertion, `R` and `I` conflict.
     if (I->getLength() == 0) {
       // Check if two insertions are order-indepedent: if inserting them in
       // either order produces the same text, they are order-independent.
       if ((R.getReplacementText() + I->getReplacementText()).str() !=
-          (I->getReplacementText() + R.getReplacementText()).str()) {
-        return makeConflictReplacementsError(R, *I);
-      }
+          (I->getReplacementText() + R.getReplacementText()).str())
+        return llvm::make_error<ReplacementError>(
+            replacement_error::insert_conflict, R, *I);
       // If insertions are order-independent, we can merge them.
       Replacement NewR(
           R.getFilePath(), R.getOffset(), 0,
           (R.getReplacementText() + I->getReplacementText()).str());
       Replaces.erase(I);
       Replaces.insert(std::move(NewR));
       return llvm::Error::success();
     }
llvm::Expected<std::string> applyAllReplacements(StringRef Code,
   InMemoryFileSystem->addFile(
       "<stdin>", 0, llvm::MemoryBuffer::getMemBuffer(Code, "<stdin>"));
   FileID ID = SourceMgr.createFileID(Files.getFile("<stdin>"), SourceLocation(),
                                      clang::SrcMgr::C_User);
   for (auto I = Replaces.rbegin(), E = Replaces.rend(); I != E; ++I) {
     Replacement Replace("<stdin>", I->getOffset(), I->getLength(),
                         I->getReplacementText());
     if (!Replace.apply(Rewrite))
-      return llvm::make_error<llvm::StringError>(
-          "Failed to apply replacement: " + Replace.toString(),
-          llvm::inconvertibleErrorCode());
+      return llvm::make_error<ReplacementError>(
+          replacement_error::fail_to_apply, Replace);
   }
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   Rewrite.getEditBuffer(ID).write(OS);
   OS.flush();
   return Result;
 }
 

cfe/trunk/unittests/Tooling/RefactoringTest.cpp

 TEST_F(ReplacementTest, ReturnsInvalidPath) {
   Replacement Replace1(Context.Sources, SourceLocation(), 0, "");
   EXPECT_TRUE(Replace1.getFilePath().empty());
 
   Replacement Replace2;
   EXPECT_TRUE(Replace2.getFilePath().empty());
 }
 
+// Checks that an llvm::Error instance contains a ReplacementError with expected
+// error code, expected new replacement, and expected existing replacement.
+static bool checkReplacementError(
+    llvm::Error&& Error, replacement_error ExpectedErr,
+    llvm::Optional<Replacement> ExpectedExisting,
+    llvm::Optional<Replacement> ExpectedNew) {
+  if (!Error) {
+    llvm::errs() << "Error is a success.";
+    return false;
+  }
+  std::string ErrorMessage;
+  llvm::raw_string_ostream OS(ErrorMessage);
+  llvm::handleAllErrors(std::move(Error), [&](const ReplacementError &RE) {
+    llvm::errs() << "Handling error...\n";
+    if (ExpectedErr != RE.get())
+      OS << "Unexpected error code: " << int(RE.get()) << "\n";
+    if (ExpectedExisting != RE.getExistingReplacement()) {
+      OS << "Expected Existing != Actual Existing.\n";
+      if (ExpectedExisting.hasValue())
+        OS << "Expected existing replacement: " << ExpectedExisting->toString()
+           << "\n";
+      if (RE.getExistingReplacement().hasValue())
+        OS << "Actual existing replacement: "
+           << RE.getExistingReplacement()->toString() << "\n";
+    }
+    if (ExpectedNew != RE.getNewReplacement()) {
+      OS << "Expected New != Actual New.\n";
+      if (ExpectedNew.hasValue())
+        OS << "Expected new replacement: " << ExpectedNew->toString() << "\n";
+      if (RE.getNewReplacement().hasValue())
+        OS << "Actual new replacement: " << RE.getNewReplacement()->toString()
+           << "\n";
+    }
+  });
+  OS.flush();
+  if (ErrorMessage.empty()) return true;
+  llvm::errs() << ErrorMessage;
+  return false;
+}
+
 TEST_F(ReplacementTest, FailAddReplacements) {
   Replacements Replaces;
   Replacement Deletion("x.cc", 0, 10, "3");
   auto Err = Replaces.add(Deletion);
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
-  Err = Replaces.add(Replacement("x.cc", 0, 2, "a"));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
-  Err = Replaces.add(Replacement("x.cc", 2, 2, "a"));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
-  Err = Replaces.add(Replacement("y.cc", 20, 2, ""));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
+
+  Replacement OverlappingReplacement("x.cc", 0, 2, "a");
+  Err = Replaces.add(OverlappingReplacement);
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::overlap_conflict,
+                                    Deletion, OverlappingReplacement));
+
+  Replacement ContainedReplacement("x.cc", 2, 2, "a");
+  Err = Replaces.add(Replacement(ContainedReplacement));
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::overlap_conflict,
+                                    Deletion, ContainedReplacement));
+
+  Replacement WrongPathReplacement("y.cc", 20, 2, "");
+  Err = Replaces.add(WrongPathReplacement);
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::wrong_file_path,
+                                    Deletion, WrongPathReplacement));
+
   EXPECT_EQ(1u, Replaces.size());
   EXPECT_EQ(Deletion, *Replaces.begin());
 }
 
 TEST_F(ReplacementTest, DeletionInReplacements) {
   Replacements Replaces;
   Replacement R("x.cc", 0, 10, "3");
   auto Err = Replaces.add(R);
TEST_F(ReplacementTest, FailAddRegression) {
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
   Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
 
   // Make sure we find the overlap with the first entry when inserting a
   // replacement that ends exactly at the seam of the existing replacements.
-  Err = Replaces.add(Replacement("x.cc", 5, 5, "fail"));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
+  Replacement OverlappingReplacement("x.cc", 5, 5, "fail");
+  Err = Replaces.add(OverlappingReplacement);
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::overlap_conflict,
+                                    *Replaces.begin(), OverlappingReplacement));
 
   Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
 }
 
 TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) {
   Replacements Replaces;
TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) {
   EXPECT_EQ(Replaces.size(), 2u);
 }
 
 TEST_F(ReplacementTest, AddInsertAtOtherInsertWhenOderIndependent) {
   Replacements Replaces;
   auto Err = Replaces.add(Replacement("x.cc", 10, 0, "a"));
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
-  Err = Replaces.add(Replacement("x.cc", 10, 0, "b"));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
+  Replacement ConflictInsertion("x.cc", 10, 0, "b");
+  Err = Replaces.add(ConflictInsertion);
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::insert_conflict,
+                                    *Replaces.begin(), ConflictInsertion));
 
   Replaces.clear();
   Err = Replaces.add(Replacement("x.cc", 10, 0, "a"));
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));
   Err = Replaces.add(Replacement("x.cc", 10, 0, "aa"));
   EXPECT_TRUE(!Err);
   llvm::consumeError(std::move(Err));

 }
 
 TEST_F(ReplacementTest, FailOrderDependentReplacements) {
   FileID ID = Context.createInMemoryFile("input.cpp",
                                          "line1\nline2\nline3\nline4");
   auto Replaces = toReplacements({Replacement(
       Context.Sources, Context.getLocation(ID, 2, 1), 5, "other")});
 
-  auto Err = Replaces.add(Replacement(
-      Context.Sources, Context.getLocation(ID, 2, 1), 5, "rehto"));
-  EXPECT_TRUE((bool)Err);
-  llvm::consumeError(std::move(Err));
+  Replacement ConflictReplacement(Context.Sources,
+                                  Context.getLocation(ID, 2, 1), 5, "rehto");
+  auto Err = Replaces.add(ConflictReplacement);
+  EXPECT_TRUE(checkReplacementError(std::move(Err),
+                                    replacement_error::overlap_conflict,
+                                    *Replaces.begin(), ConflictReplacement));
 
   EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
   EXPECT_EQ("line1\nother\nline3\nline4", Context.getRewrittenText(ID));
 }
 
 TEST_F(ReplacementTest, InvalidSourceLocationFailsApplyAll) {
   Replacements Replaces =
       toReplacements({Replacement(Context.Sources, SourceLocation(), 5, "2")});