Fix overlapping replacements in clang-tidy.

Summary: Prevent clang-tidy from applying fixes to errors that overlap with other errors' fixes, with one exception: if one fix is completely contained inside another one, then we can apply the big one.

Reviewers: bkramer, klimek

Subscribers: djasper, cfe-commits, alexfh

Differential Revision: http://reviews.llvm.org/D13516

llvm-svn: 250509

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

@@ -22,8 +22,8 @@
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "llvm/ADT/SmallString.h"
-#include <set>
 #include <tuple>
+#include <vector>
 using namespace clang;
 using namespace tidy;
 
@@ -146,8 +146,7 @@ static llvm::Regex ConsumeGlob(StringRef &GlobList) {
 }
 
 GlobList::GlobList(StringRef Globs)
-    : Positive(!ConsumeNegativeIndicator(Globs)),
+    : Positive(!ConsumeNegativeIndicator(Globs)), Regex(ConsumeGlob(Globs)),
-      Regex(ConsumeGlob(Globs)),
       NextGlob(Globs.empty() ? nullptr : new GlobList(Globs)) {}
 
 bool GlobList::contains(StringRef S, bool Contains) {
@@ -222,9 +221,7 @@ const ClangTidyOptions &ClangTidyContext::getOptions() const {
   return CurrentOptions;
 }
 
-void ClangTidyContext::setCheckProfileData(ProfileData *P) {
+void ClangTidyContext::setCheckProfileData(ProfileData *P) { Profile = P; }
-  Profile = P;
-}
 
 GlobList &ClangTidyContext::getChecksFilter() {
   assert(CheckFilter != nullptr);
@@ -296,16 +293,16 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
       // This is a compiler diagnostic without a warning option. Assign check
       // name based on its level.
       switch (DiagLevel) {
-        case DiagnosticsEngine::Error:
+      case DiagnosticsEngine::Error:
-        case DiagnosticsEngine::Fatal:
+      case DiagnosticsEngine::Fatal:
-          CheckName = "clang-diagnostic-error";
+        CheckName = "clang-diagnostic-error";
-          break;
+        break;
-        case DiagnosticsEngine::Warning:
+      case DiagnosticsEngine::Warning:
-          CheckName = "clang-diagnostic-warning";
+        CheckName = "clang-diagnostic-warning";
-          break;
+        break;
-        default:
+      default:
-          CheckName = "clang-diagnostic-unknown";
+        CheckName = "clang-diagnostic-unknown";
-          break;
+        break;
       }
     }
 
@@ -340,7 +337,7 @@ bool ClangTidyDiagnosticConsumer::passesLineFilter(StringRef FileName,
                                                    unsigned LineNumber) const {
   if (Context.getGlobalOptions().LineFilter.empty())
     return true;
-  for (const FileFilter& Filter : Context.getGlobalOptions().LineFilter) {
+  for (const FileFilter &Filter : Context.getGlobalOptions().LineFilter) {
     if (FileName.endswith(Filter.Name)) {
       if (Filter.LineRanges.empty())
         return true;
@@ -398,26 +395,147 @@ llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
   return HeaderFilter.get();
 }
 
+void ClangTidyDiagnosticConsumer::removeIncompatibleErrors(
+    SmallVectorImpl<ClangTidyError> &Errors) const {
+  // Each error is modelled as the set of intervals in which it applies
+  // replacements. To detect overlapping replacements, we use a sweep line
+  // algorithm over these sets of intervals.
+  // An event here consists of the opening or closing of an interval. During the
+  // proccess, we maintain a counter with the amount of open intervals. If we
+  // find an endpoint of an interval and this counter is different from 0, it
+  // means that this interval overlaps with another one, so we set it as
+  // inapplicable.
+  struct Event {
+    // An event can be either the begin or the end of an interval.
+    enum EventType {
+      ET_Begin = 1,
+      ET_End = -1,
+    };
+
+    Event(unsigned Begin, unsigned End, EventType Type, unsigned ErrorId,
+          unsigned ErrorSize)
+        : Type(Type), ErrorId(ErrorId) {
+      // The events are going to be sorted by their position. In case of draw:
+      //
+      // * If an interval ends at the same position at which other interval
+      //   begins, this is not an overlapping, so we want to remove the ending
+      //   interval before adding the starting one: end events have higher
+      //   priority than begin events.
+      //
+      // * If we have several begin points at the same position, we will mark as
+      //   inapplicable the ones that we proccess later, so the first one has to
+      //   be the one with the latest end point, because this one will contain
+      //   all the other intervals. For the same reason, if we have several end
+      //   points in the same position, the last one has to be the one with the
+      //   earliest begin point. In both cases, we sort non-increasingly by the
+      //   position of the complementary.
+      //
+      // * In case of two equal intervals, the one whose error is bigger can
+      //   potentially contain the other one, so we want to proccess its begin
+      //   points before and its end points later.
+      //
+      // * Finally, if we have two equal intervals whose errors have the same
+      //   size, none of them will be strictly contained inside the other.
+      //   Sorting by ErrorId will guarantee that the begin point of the first
+      //   one will be proccessed before, disallowing the second one, and the
+      //   end point of the first one will also be proccessed before,
+      //   disallowing the first one.
+      if (Type == ET_Begin)
+        Priority = std::make_tuple(Begin, Type, -End, -ErrorSize, ErrorId);
+      else
+        Priority = std::make_tuple(End, Type, -Begin, ErrorSize, ErrorId);
+    }
+
+    bool operator<(const Event &Other) const {
+      return Priority < Other.Priority;
+    }
+
+    // Determines if this event is the begin or the end of an interval.
+    EventType Type;
+    // The index of the error to which the interval that generated this 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) {
+    int Size = 0;
+    for (const auto &Replace : Error.Fix)
+      Size += Replace.getLength();
+    Sizes.push_back(Size);
+  }
+
+  // Build events from error intervals.
+  std::vector<Event> Events;
+  for (unsigned I = 0; I < Errors.size(); ++I) {
+    for (const auto &Replace : Errors[I].Fix) {
+      unsigned Begin = Replace.getOffset();
+      unsigned End = Begin + Replace.getLength();
+      // FIXME: Handle empty intervals, such as those from insertions.
+      if (Begin == End)
+        continue;
+      Events.push_back(Event(Begin, End, Event::ET_Begin, I, Sizes[I]));
+      Events.push_back(Event(Begin, End, Event::ET_End, I, Sizes[I]));
+    }
+  }
+  std::sort(Events.begin(), Events.end());
+
+  // Sweep.
+  std::vector<bool> Apply(Errors.size(), true);
+  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) {
+    if (!Apply[I]) {
+      Errors[I].Fix.clear();
+      Errors[I].Notes.push_back(
+          ClangTidyMessage("this fix will not be applied because"
+                           " it overlaps with another fix"));
+    }
+  }
+}
+
 namespace {
 struct LessClangTidyError {
-  bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+  bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
-    const ClangTidyMessage &M1 = LHS->Message;
+    const ClangTidyMessage &M1 = LHS.Message;
-    const ClangTidyMessage &M2 = RHS->Message;
+    const ClangTidyMessage &M2 = RHS.Message;
 
     return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
            std::tie(M2.FilePath, M2.FileOffset, M2.Message);
   }
 };
+struct EqualClangTidyError {
+  static LessClangTidyError Less;
+  bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
+    return !Less(LHS, RHS) && !Less(RHS, LHS);
+  }
+};
 } // end anonymous namespace
 
 // Flushes the internal diagnostics buffer to the ClangTidyContext.
 void ClangTidyDiagnosticConsumer::finish() {
   finalizeLastError();
-  std::set<const ClangTidyError*, LessClangTidyError> UniqueErrors;
-  for (const ClangTidyError &Error : Errors)
-    UniqueErrors.insert(&Error);
 
-  for (const ClangTidyError *Error : UniqueErrors)
+  std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
-    Context.storeError(*Error);
+  Errors.erase(std::unique(Errors.begin(), Errors.end(), EqualClangTidyError()),
+               Errors.end());
+  removeIncompatibleErrors(Errors);
+
+  for (const ClangTidyError &Error : Errors)
+    Context.storeError(Error);
   Errors.clear();
 }

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

@@ -179,8 +179,8 @@ public:
   ///
   /// Setting a non-null pointer here will enable profile collection in
   /// clang-tidy.
-  void setCheckProfileData(ProfileData* Profile);
+  void setCheckProfileData(ProfileData *Profile);
-  ProfileData* getCheckProfileData() const { return Profile; }
+  ProfileData *getCheckProfileData() const { return Profile; }
 
 private:
   // Calls setDiagnosticsEngine() and storeError().
@@ -231,9 +231,11 @@ public:
 private:
   void finalizeLastError();
 
+  void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors) const;
+
   /// \brief Returns the \c HeaderFilter constructed for the options set in the
   /// context.
-  llvm::Regex* getHeaderFilter();
+  llvm::Regex *getHeaderFilter();
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.

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

@@ -77,11 +77,12 @@ public:
     auto *VD = Result.Nodes.getNodeAs<VarDecl>(BoundDecl);
     std::string NewName = newName(VD->getName());
 
-    auto Diag = diag(VD->getLocation(), "refactor")
+    auto Diag = diag(VD->getLocation(), "refactor %0 into %1")
+                << VD->getName() << NewName
                 << FixItHint::CreateReplacement(
-                    CharSourceRange::getTokenRange(VD->getLocation(),
+                       CharSourceRange::getTokenRange(VD->getLocation(),
-                                                   VD->getLocation()),
+                                                      VD->getLocation()),
-                    NewName);
+                       NewName);
 
     class UsageVisitor : public RecursiveASTVisitor<UsageVisitor> {
     public:
@@ -281,7 +282,7 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) {
 
   // Apply the UseCharCheck together with the IfFalseCheck.
   //
-  // The 'If' fix is bigger, so that is the one that has to be applied.
+  // The 'If' fix contains the other, so that is the one that has to be applied.
   // } else if (int a = 0) {
   //            ^^^ -> char
   //            ~~~~~~~~~ -> false
@@ -294,7 +295,9 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) {
   }
 })";
   Res = runCheckOnCode<UseCharCheck, IfFalseCheck>(Code);
-  // FIXME: EXPECT_EQ(CharIfFix, Res);
+  EXPECT_EQ(CharIfFix, Res);
+  Res = runCheckOnCode<IfFalseCheck, UseCharCheck>(Code);
+  EXPECT_EQ(CharIfFix, Res);
 
   // Apply the IfFalseCheck with the StartsWithPotaCheck.
   //
@@ -303,7 +306,7 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) {
   //          ^^^^^^ -> tomato
   //     ~~~~~~~~~~~~~~~ -> false
   //
-  // But the refactoring is bigger here:
+  // But the refactoring is the one that contains the other here:
   // char potato = 0;
   //      ^^^^^^ -> tomato
   // if (potato) potato;
@@ -318,60 +321,87 @@ TEST(OverlappingReplacementsTest, ReplacementInsideOtherReplacement) {
   }
 })";
   Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
-  // FIXME: EXPECT_EQ(IfStartsFix, Res);
+  EXPECT_EQ(IfStartsFix, Res);
-
+  Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck>(Code);
-  // Silence warnings.
+  EXPECT_EQ(IfStartsFix, Res);
-  (void)CharIfFix;
-  (void)IfStartsFix;
 }
 
-TEST(OverlappingReplacementsTest, ApplyFullErrorOrNothingWhenOverlapping) {
+TEST(OverlappingReplacements, TwoReplacementsInsideOne) {
+  std::string Res;
+  const char Code[] =
+      R"(void f() {
+  if (int potato = 0) {
+    int a = 0;
+  }
+})";
+
+  // The two smallest replacements should not be applied.
+  // if (int potato = 0) {
+  //         ^^^^^^ -> tomato
+  //     *** -> char
+  //     ~~~~~~~~~~~~~~ -> false
+  // But other errors from the same checks should not be affected.
+  //   int a = 0;
+  //   *** -> char
+  const char Fix[] =
+      R"(void f() {
+  if (false) {
+    char a = 0;
+  }
+})";
+  Res = runCheckOnCode<UseCharCheck, IfFalseCheck, StartsWithPotaCheck>(Code);
+  EXPECT_EQ(Fix, Res);
+  Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck, UseCharCheck>(Code);
+  EXPECT_EQ(Fix, Res);
+}
+
+TEST(OverlappingReplacementsTest,
+     ApplyAtMostOneOfTheChangesWhenPartialOverlapping) {
+  std::string Res;
+  const char Code[] =
+      R"(void f() {
+  if (int potato = 0) {
+    int a = potato;
+  }
+})";
+
+  // These two replacements overlap, but none of them is completely contained
+  // inside the other.
+  // if (int potato = 0) {
+  //         ^^^^^^ -> tomato
+  //     ~~~~~~~~~~~~~~ -> false
+  //   int a = potato;
+  //           ^^^^^^ -> tomato
+  //
+  // The 'StartsWithPotaCheck' fix has endpoints inside the 'IfFalseCheck' fix,
+  // so it is going to be set as inapplicable. The 'if' fix will be applied.
+  const char IfFix[] =
+      R"(void f() {
+  if (false) {
+    int a = potato;
+  }
+})";
+  Res = runCheckOnCode<IfFalseCheck, StartsWithPotaCheck>(Code);
+  EXPECT_EQ(IfFix, Res);
+}
+
+TEST(OverlappingReplacementsTest, TwoErrorsHavePerfectOverlapping) {
   std::string Res;
   const char Code[] =
       R"(void f() {
   int potato = 0;
   potato += potato * potato;
-  if (char this_name_make_this_if_really_long = potato) potato;
+  if (char a = potato) potato;
 })";
 
-  // StartsWithPotaCheck will try to refactor 'potato' into 'tomato',
+  // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', and
-  // and EndsWithTatoCheck will try to use 'pomelo'. We have to apply
+  // EndsWithTatoCheck will try to use 'pomelo'. Both fixes have the same set of
-  // either all conversions from one check, or all from the other.
+  // ranges. This is a corner case of one error completely containing another:
-  const char StartsFix[] =
+  // the other completely contains the first one as well. Both errors are
-      R"(void f() {
+  // discarded.
-  int tomato = 0;
+
-  tomato += tomato * tomato;
-  if (char this_name_make_this_if_really_long = tomato) tomato;
-})";
-  const char EndsFix[] =
-      R"(void f() {
-  int pomelo = 0;
-  pomelo += pomelo * pomelo;
-  if (char this_name_make_this_if_really_long = pomelo) pomelo;
-})";
-  // In case of overlapping, we will prioritize the biggest fix. However, these
-  // two fixes have the same size and position, so we don't know yet which one
-  // will have preference.
   Res = runCheckOnCode<StartsWithPotaCheck, EndsWithTatoCheck>(Code);
-  // FIXME: EXPECT_TRUE(Res == StartsFix || Res == EndsFix);
+  EXPECT_EQ(Code, Res);
-
-  // StartsWithPotaCheck will try to refactor 'potato' into 'tomato', but
-  // replacing the 'if' condition is a bigger change than all the refactoring
-  // changes together (48 vs 36), so this is the one that is going to be
-  // applied.
-  const char IfFix[] =
-      R"(void f() {
-  int potato = 0;
-  potato += potato * potato;
-  if (true) potato;
-})";
-  Res = runCheckOnCode<StartsWithPotaCheck, IfFalseCheck>(Code);
-  // FIXME: EXPECT_EQ(IfFix, Res);
-
-  // Silence warnings.
-  (void)StartsFix;
-  (void)EndsFix;
-  (void)IfFix;
 }
 
 } // namespace test