Index: include/clang/Tooling/Core/Replacement.h =================================================================== --- include/clang/Tooling/Core/Replacement.h +++ include/clang/Tooling/Core/Replacement.h @@ -158,14 +158,18 @@ /// \brief Adds a new replacement \p R to the current set of replacements. /// \p R must have the same file path as all existing replacements. - /// Returns true if the replacement is successfully inserted; otherwise, + /// Returns `success` if the replacement is successfully inserted; otherwise, /// it returns an llvm::Error, i.e. there is a conflict between R and the - /// existing replacements or R's file path is different from the filepath of - /// existing replacements. Callers must explicitly check the Error returned. - /// This prevents users from adding order-dependent replacements. To control - /// the order in which order-dependent replacements are applied, use - /// merge({R}) with R referring to the changed code after applying all - /// existing replacements. + /// existing replacements (i.e. they are order-dependent) or R's file path is + /// different from the filepath of existing replacements. Callers must + /// explicitly check the Error returned. This prevents users from adding + /// order-dependent replacements. To control the order in which + /// order-dependent replacements are applied, use merge({R}) with R referring + /// to the changed code after applying all existing replacements. + /// Two replacements are considered order-independent if they: + /// - don't overlap (being directly adjacent is fine) and + /// - aren't both inserts at the same code location. + /// Note: two insertions at the same offset are considered order-dependent. /// Replacements with offset UINT_MAX are special - we do not detect conflicts /// for such replacements since users may add them intentionally as a special /// category of replacements. Index: lib/Tooling/Core/Replacement.cpp =================================================================== --- lib/Tooling/Core/Replacement.cpp +++ lib/Tooling/Core/Replacement.cpp @@ -137,6 +137,14 @@ ReplacementText); } +llvm::Error makeConflictReplacementsError(const Replacement &New, + const Replacement &Existing) { + return llvm::make_error( + "New replacement:\n" + New.toString() + + "\nconflicts with existing replacement:\n" + Existing.toString(), + llvm::inconvertibleErrorCode()); +} + llvm::Error Replacements::add(const Replacement &R) { // Check the file path. if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath()) @@ -163,11 +171,22 @@ // entries that start at the end can still be conflicting if R is an // insertion. auto I = Replaces.lower_bound(AtEnd); - // If it starts at the same offset as R (can only happen if R is an - // insertion), we have a conflict. In that case, increase I to fall through - // to the conflict check. - if (I != Replaces.end() && R.getOffset() == I->getOffset()) - ++I; + // 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) + return makeConflictReplacementsError(R, *I); + // Insertion `R` is adjacent to a non-insertion replacement `I`, so they + // are order-independent. It is safe to assume that `R` will not conflict + // with any replacement before `I` since all replacements before `I` must + // either end before `R` or end at `R` but has length > 0 (if the + // replacement before `I` is an insertion at `R`, it would have been `I` + // since it is a lower bound of `AtEnd` and ordered before the current `I` + // in the set). + Replaces.insert(R); + return llvm::Error::success(); + } // I is the smallest iterator whose entry cannot overlap. // If that is begin(), there are no overlaps. @@ -178,16 +197,19 @@ --I; // If the previous entry does not overlap, we know that entries before it // can also not overlap. - if (R.getOffset() != I->getOffset() && - !Range(R.getOffset(), R.getLength()) + if (!Range(R.getOffset(), R.getLength()) .overlapsWith(Range(I->getOffset(), I->getLength()))) { + // If `R` and `I` do not have the same offset, it is safe to add `R` since + // it must come after `I`. Otherwise: + // - If `R` is an insertion, `I` must not be an insertion since it would + // have come after `AtEnd` if it has length 0. + // - If `R` is not an insertion, `I` must be an insertion; otherwise, `R` + // and `I` would have overlapped. + // In either case, we can safely insert `R`. Replaces.insert(R); return llvm::Error::success(); } - return llvm::make_error( - "New replacement:\n" + R.toString() + - "\nconflicts with existing replacement:\n" + I->toString(), - llvm::inconvertibleErrorCode()); + return makeConflictReplacementsError(R, *I); } namespace { Index: unittests/Tooling/RefactoringTest.cpp =================================================================== --- unittests/Tooling/RefactoringTest.cpp +++ unittests/Tooling/RefactoringTest.cpp @@ -115,15 +115,16 @@ llvm::consumeError(std::move(Err)); } -TEST_F(ReplacementTest, FailAddOverlappingInsertions) { +TEST_F(ReplacementTest, AddAdjacentInsertionAndReplacement) { Replacements Replaces; // Test adding an insertion at the offset of an existing replacement. auto Err = Replaces.add(Replacement("x.cc", 10, 3, "replace")); EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); Err = Replaces.add(Replacement("x.cc", 10, 0, "insert")); - EXPECT_TRUE((bool)Err); + EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); + EXPECT_EQ(Replaces.size(), 2u); Replaces.clear(); // Test overlap with an existing insertion. @@ -131,8 +132,9 @@ EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); Err = Replaces.add(Replacement("x.cc", 10, 3, "replace")); - EXPECT_TRUE((bool)Err); + EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); + EXPECT_EQ(Replaces.size(), 2u); } TEST_F(ReplacementTest, FailAddRegression) { @@ -157,14 +159,24 @@ llvm::consumeError(std::move(Err)); } -TEST_F(ReplacementTest, FailAddInsertAtOffsetOfReplacement) { +TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) { Replacements Replaces; auto Err = Replaces.add(Replacement("x.cc", 10, 2, "")); EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); Err = Replaces.add(Replacement("x.cc", 10, 0, "")); - EXPECT_TRUE((bool)Err); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + EXPECT_EQ(Replaces.size(), 2u); + + Replaces.clear(); + Err = Replaces.add(Replacement("x.cc", 10, 0, "")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 2, "")); + EXPECT_TRUE(!Err); llvm::consumeError(std::move(Err)); + EXPECT_EQ(Replaces.size(), 2u); } TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) { @@ -175,6 +187,38 @@ Err = Replaces.add(Replacement("x.cc", 10, 0, "b")); EXPECT_TRUE((bool)Err); llvm::consumeError(std::move(Err)); + + Replaces.clear(); + Err = Replaces.add(Replacement("x.cc", 10, 0, "")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 0, "")); + EXPECT_TRUE((bool)Err); + llvm::consumeError(std::move(Err)); + + Replaces.clear(); + Err = Replaces.add(Replacement("x.cc", 10, 0, "")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 3, "")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 0, "")); + EXPECT_TRUE((bool)Err); + llvm::consumeError(std::move(Err)); +} + +TEST_F(ReplacementTest, InsertBetweenAdjacentReplacements) { + Replacements Replaces; + auto Err = Replaces.add(Replacement("x.cc", 10, 5, "a")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 8, 2, "a")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); + Err = Replaces.add(Replacement("x.cc", 10, 0, "b")); + EXPECT_TRUE(!Err); + llvm::consumeError(std::move(Err)); } TEST_F(ReplacementTest, CanApplyReplacements) { @@ -189,6 +233,29 @@ EXPECT_EQ("line1\nreplaced\nother\nline4", Context.getRewrittenText(ID)); } +// Verifies that replacement/deletion is applied before insertion at the same +// offset. +TEST_F(ReplacementTest, InsertAndDelete) { + FileID ID = Context.createInMemoryFile("input.cpp", + "line1\nline2\nline3\nline4"); + Replacements Replaces = toReplacements( + {Replacement(Context.Sources, Context.getLocation(ID, 2, 1), 6, ""), + Replacement(Context.Sources, Context.getLocation(ID, 2, 1), 0, + "other\n")}); + EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite)); + EXPECT_EQ("line1\nother\nline3\nline4", Context.getRewrittenText(ID)); +} + +TEST_F(ReplacementTest, AdjacentReplacements) { + FileID ID = Context.createInMemoryFile("input.cpp", + "ab"); + Replacements Replaces = toReplacements( + {Replacement(Context.Sources, Context.getLocation(ID, 1, 1), 1, "x"), + Replacement(Context.Sources, Context.getLocation(ID, 1, 2), 1, "y")}); + EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite)); + EXPECT_EQ("xy", Context.getRewrittenText(ID)); +} + TEST_F(ReplacementTest, SkipsDuplicateReplacements) { FileID ID = Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4");