[clang-tidy] Prevent a remove only fixit causing a conflict when enclosed by another fixit

[njames93]

Summary

Say you have some text 'AAABBBCCCDDD' and there are 2 fix its on that text. Fix-it A just wants to remove all occurances of 'C' and fix-it B wants to replace the entire string with 'XXXXXXXXXXXX'.
These 2 fix its currently get diagnosed as conflicting, and prevent the entire fix it from applying(plus any from the same warning).
However Fix-it A can be disabled (while leaving any other fix its in the same warning active) without causing any conflicts.

Note I haven't included any specific test cases, but running the clang-tools unittests and regression tests yielded no failures.

[merge_guards_bot]

Unit tests: pass. 62056 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[Eugene.Zelenko]

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

733

Please don't use auto unless type is spelled in same statement or iterator.

737

Please don't use auto unless type is spelled in same statement or iterator.

[njames93]

• remove unnecessary auto

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

733

What if the type is `llvm::SmallDenseSet<const clang::tooling::Replacement *, 4, llvm::DenseMapInfo<const clang::tooling::Replacement *> >`

[merge_guards_bot]

Unit tests: pass. 62056 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• Small edge case tweak

[merge_guards_bot]

Unit tests: fail. 62126 tests passed, 5 failed and 807 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[JonasToth]

We need tests for that. There should be tests for the exporting-feature. Maybe they could provide a good starting point on adding tests. (not sure if there is unit-test code for isolated testing, but some tests do exist!)

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

 #include "ClangTidyOptions.h"
 #include "GlobList.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Attr.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "clang/Tooling/Core/Diagnostic.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include <tuple>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
 namespace {
void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
   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) {
+          unsigned ErrorSize, const tooling::Replacement &Replacement)
+        : Type(Type), ErrorId(ErrorId), Replacement(&Replacement) {
       // 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
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 replacement this event relates to.
+    const tooling::Replacement *Replacement;
     // The events will be sorted based on this field.
     std::tuple<unsigned, EventType, int, int, unsigned> Priority;
   };
 
   // Compute error sizes.
   std::vector<int> Sizes;
   std::vector<
       std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
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]);
+        Events.emplace_back(Begin, End, Event::ET_Begin, I, Sizes[I], Replace);
+        Events.emplace_back(Begin, End, Event::ET_End, I, Sizes[I], Replace);
       }
     }
   }
 
   std::vector<bool> Apply(ErrorFixes.size(), true);
   for (auto &FileAndEvents : FileEvents) {
+    llvm::SmallSet<const Event *, 4> RemoveOnlyEvents;
     std::vector<Event> &Events = FileAndEvents.second;
+    using ReplacementPtrSet = llvm::SmallDenseSet<const tooling::Replacement *>;
+    llvm::SmallDenseMap<unsigned, ReplacementPtrSet> DiscardableReplacements;
     // 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)
+      if (OpenIntervals != 0) {
+        if (Event.Replacement->getReplacementText().empty()) {
+          if (Event.Type == Event::ET_Begin) {
+            // Starting a removal only fix-it is OK inside another fix-it.
+            // But need to discard this specific fix-it from its parent so it
+            // wont get executed later and cause a conflict.
+            RemoveOnlyEvents.insert(&Event);
+          } else if (Event.Type == Event::ET_End) {
+            // End of a Remove only fix-it, Remove it from the set.
+            bool Found = false;
+            for (const auto *RemoveOnlyEvent : RemoveOnlyEvents) {
+              if (RemoveOnlyEvent->Replacement == Event.Replacement) {
+                assert(!Found && "Event should only appear in set once");
+                RemoveOnlyEvents.erase(RemoveOnlyEvent);
+                Found = true;
+              }
+            }
+            DiscardableReplacements[Event.ErrorId].insert(Event.Replacement);
+            assert(Found && "Event didn't appear in set");
+          }
+        } else {
+          // This isnt a text removal only change, so must be a conflict.
           Apply[Event.ErrorId] = false;
+        }
+      } else {
+        decltype(RemoveOnlyEvents)::size_type Size = RemoveOnlyEvents.size();
+        assert(Size < 2 && "Once OpenIntervals is `0` this set should contain "
+                           "no more than 1 event");
+        if (Size) {
+          // The remove only fix-it is overlapping another even that we have
+          // already disabled. So no need to discard this fix-it
+          RemoveOnlyEvents.clear();
+        }
+      }
       if (Event.Type == Event::ET_Begin)
         ++OpenIntervals;
     }
     assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match");
+    for (const auto &ErrorAndDiscarded : DiscardableReplacements) {
+      unsigned ErrorIndex = ErrorAndDiscarded.first;
+      const ReplacementPtrSet &Discarded = ErrorAndDiscarded.second;
+      if (!Apply[ErrorIndex])
+        continue; // The whole error has already been discarded.
+      tooling::Replacements NewReplacements;
+      const tooling::Replacements &CurReplacements =
+          ErrorFixes[ErrorIndex].second->lookup(FileAndEvents.first);
+      for (const tooling::Replacement &Replacement : CurReplacements) {
+        // Comparing the pointer here is fine as they are pointing to the same
+        // Replacement.
+        if (Discarded.count(&Replacement))
+          continue;
+        if (NewReplacements.add(Replacement)) {
+          // This should never fire as we have just tested
+          // but leave the check in just in case.
+          Apply[ErrorIndex] = false;
+        }
+      }
+      ErrorFixes[ErrorIndex].second->insert_or_assign(
+          FileAndEvents.first, std::move(NewReplacements));
+    }
   }
 
   for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
     if (!Apply[I]) {
       ErrorFixes[I].second->clear();
       ErrorFixes[I].first->Notes.emplace_back(
           "this fix will not be applied because it overlaps with another fix");
     }