[-Wunsafe-buffer-usage] Filter out conflicting fix-its

Two fix-its conflict if they have overlapping source ranges. We shall
not emit conflicting fix-its.  This patch checks conflicts in fix-its
generated for one variable (including variable declaration fix-its and
variable usage fix-its). If there is any, we do NOT emit any fix-it
for that variable.

Reviewed by: NoQ

Differential revision: https://reviews.llvm.org/D141338

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

@@ -52,6 +52,12 @@ class UnsafeBufferUsageHandler {
 // through the handler class.
 void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
 
+namespace internal {
+// Tests if any two `FixItHint`s in `FixIts` conflict.  Two `FixItHint`s
+// conflict if they have overlapping source ranges.
+bool anyConflict(const llvm::SmallVectorImpl<FixItHint> &FixIts,
+                 const SourceManager &SM);
+} // namespace internal
 } // end namespace clang
 
 #endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -694,6 +694,37 @@ groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
   return FixablesForUnsafeVars;
 }
 
+bool clang::internal::anyConflict(
+    const SmallVectorImpl<FixItHint> &FixIts, const SourceManager &SM) {
+  // A simple interval overlap detection algorithm.  Sorts all ranges by their
+  // begin location then finds the first overlap in one pass.
+  std::vector<const FixItHint *> All; // a copy of `FixIts`
+
+  for (const FixItHint &H : FixIts)
+    All.push_back(&H);
+  std::sort(All.begin(), All.end(),
+            [&SM](const FixItHint *H1, const FixItHint *H2) {
+              return SM.isBeforeInTranslationUnit(H1->RemoveRange.getBegin(),
+                                                  H2->RemoveRange.getBegin());
+            });
+
+  const FixItHint *CurrHint = nullptr;
+
+  for (const FixItHint *Hint : All) {
+    if (!CurrHint ||
+        SM.isBeforeInTranslationUnit(CurrHint->RemoveRange.getEnd(),
+                                     Hint->RemoveRange.getBegin())) {
+      // Either to initialize `CurrHint` or `CurrHint` does not
+      // overlap with `Hint`:
+      CurrHint = Hint;
+    } else
+      // In case `Hint` overlaps the `CurrHint`, we found at least one
+      // conflict:
+      return true;
+  }
+  return false;
+}
+
 std::optional<FixItList>
 ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
@@ -948,8 +979,12 @@ getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S,
     else
       FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
                                    FixItsForVD.begin(), FixItsForVD.end());
-    // Fix-it shall not overlap with macros or/and templates:
-    if (overlapWithMacro(FixItsForVariable[VD])) {
+    // We conservatively discard fix-its of a variable if
+    // a fix-it overlaps with macros; or
+    // a fix-it conflicts with another one
+    if (overlapWithMacro(FixItsForVariable[VD]) ||
+        clang::internal::anyConflict(FixItsForVariable[VD],
+                                     Ctx.getSourceManager())) {      
       FixItsForVariable.erase(VD);
     }
   }

clang/unittests/Analysis/CMakeLists.txt

@@ -9,6 +9,7 @@ add_clang_unittest(ClangAnalysisTests
   CloneDetectionTest.cpp
   ExprMutationAnalyzerTest.cpp
   MacroExpansionContextTest.cpp
+  UnsafeBufferUsageTest.cpp
   )
 
 clang_target_link_libraries(ClangAnalysisTests

clang/unittests/Analysis/UnsafeBufferUsageTest.cpp

@@ -0,0 +1,60 @@
+#include "gtest/gtest.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
+
+using namespace clang;
+
+namespace {
+// The test fixture.
+class UnsafeBufferUsageTest : public ::testing::Test {
+protected:
+  UnsafeBufferUsageTest()
+      : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+        Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+        SourceMgr(Diags, FileMgr) {}
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+};
+} // namespace
+
+TEST_F(UnsafeBufferUsageTest, FixItHintsConflict) {
+  const FileEntry *DummyFile = FileMgr.getVirtualFile("<virtual>", 100, 0);
+  FileID DummyFileID = SourceMgr.getOrCreateFileID(DummyFile, SrcMgr::C_User);
+  SourceLocation StartLoc = SourceMgr.getLocForStartOfFile(DummyFileID);
+
+#define MkDummyHint(Begin, End)                                                \
+  FixItHint::CreateReplacement(SourceRange(StartLoc.getLocWithOffset((Begin)), \
+                                           StartLoc.getLocWithOffset((End))),  \
+                               "dummy")
+
+  FixItHint H1 = MkDummyHint(0, 5);
+  FixItHint H2 = MkDummyHint(6, 10);
+  FixItHint H3 = MkDummyHint(20, 25);
+  llvm::SmallVector<FixItHint> Fixes;
+
+  // Test non-overlapping fix-its:
+  Fixes = {H1, H2, H3};
+  EXPECT_FALSE(internal::anyConflict(Fixes, SourceMgr));
+  Fixes = {H3, H2, H1}; // re-order
+  EXPECT_FALSE(internal::anyConflict(Fixes, SourceMgr));
+
+  // Test overlapping fix-its:
+  Fixes = {H1, H2, H3, MkDummyHint(0, 4) /* included in H1 */};
+  EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+  Fixes = {H1, H2, H3, MkDummyHint(10, 15) /* overlaps H2 */};
+  EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+  Fixes = {H1, H2, H3, MkDummyHint(7, 23) /* overlaps H2, H3 */};
+  EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+
+  Fixes = {H1, H2, H3, MkDummyHint(2, 23) /* overlaps H1, H2, and H3 */};
+  EXPECT_TRUE(internal::anyConflict(Fixes, SourceMgr));
+}