Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp =================================================================== --- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp +++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp @@ -204,19 +204,13 @@ FileChange.replace(SM, BeginLoc.getLocWithOffset(R.getOffset()), R.getLength(), R.getReplacementText()); if (Err) { - // FIXME: This will report conflicts by pair using a file+offset format - // which is not so much human readable. - // A first improvement could be to translate offset to line+col. For - // this and without loosing error message some modifications arround - // `tooling::ReplacementError` are need (access to - // `getReplacementErrString`). - // A better strategy could be to add a pretty printer methods for - // conflict reporting. Methods that could be parameterized to report a - // conflict in different format, file+offset, file+line+col, or even - // more human readable using VCS conflict markers. - // For now, printing directly the error reported by `AtomicChange` is - // the easiest solution. - errs() << llvm::toString(std::move(Err)) << "\n"; + Err = llvm::handleErrors(std::move(Err), + [&](const tooling::ReplacementError &RE) { + RE.reportConflict(errs(), SM); + errs() << "\n"; + }); + if (Err) + errs() << llvm::toString(std::move(Err)) << "\n"; ConflictDetected = true; } } Index: clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/expected.txt =================================================================== --- clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/expected.txt +++ clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/expected.txt @@ -1,12 +1,30 @@ -The new replacement overlaps with an existing replacement. -New replacement: $(path)/common.h: 106:+26:"int & elem : ints" -Existing replacement: $(path)/common.h: 106:+26:"auto & i : ints" -The new replacement overlaps with an existing replacement. -New replacement: $(path)/common.h: 140:+7:"i" -Existing replacement: $(path)/common.h: 140:+7:"elem" -The new replacement overlaps with an existing replacement. -New replacement: $(path)/common.h: 169:+0:"(int*)" -Existing replacement: $(path)/common.h: 160:+12:"" -The new replacement overlaps with an existing replacement. -New replacement: $(path)/common.h: 169:+1:"nullptr" -Existing replacement: $(path)/common.h: 160:+12:"" +$(path)/common.h:8:8-8:34 +<<<<<<< Existing replacement + for (auto & i : ints) { +======= + for (int & elem : ints) { +>>>>>>> New replacement + +$(path)/common.h:9:5-9:12 +<<<<<<< Existing replacement + elem = t; +======= + i = t; +>>>>>>> New replacement + +$(path)/common.h:12:3-13:1 +<<<<<<< Existing replacement + +======= + int *i = (int*)0; + +>>>>>>> New replacement + +$(path)/common.h:12:3-13:1 +<<<<<<< Existing replacement + +======= + int *i = nullptr; + +>>>>>>> New replacement + Index: clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt =================================================================== --- clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt +++ clang-tools-extra/test/clang-apply-replacements/Inputs/order-dependent/expected.txt @@ -1,3 +1,7 @@ -The new insertion has the same insert location as an existing replacement. -New replacement: $(path)/order-dependent.cpp: 12:+0:"1" -Existing replacement: $(path)/order-dependent.cpp: 12:+0:"0" +$(path)/order-dependent.cpp:1:13-1:13 +<<<<<<< Existing replacement +class MyType0 {}; +======= +class MyType1 {}; +>>>>>>> New replacement + Index: clang/include/clang/Tooling/Core/Replacement.h =================================================================== --- clang/include/clang/Tooling/Core/Replacement.h +++ clang/include/clang/Tooling/Core/Replacement.h @@ -129,6 +129,14 @@ /// Returns a human readable string representation. std::string toString() const; + /// Print a human readable string representation where offset is converted + /// in line+col. + void print(raw_ostream &OS, const SourceManager &SM) const; + + /// Returns a human readable string representation where offset is converted + /// in line+col. + std::string printToString(const SourceManager &SM) const; + private: void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start, unsigned Length, StringRef ReplacementText); @@ -181,6 +189,9 @@ return ExistingReplacement; } + /// Print a replacement conflict using conflict marker format. + void reportConflict(raw_ostream &OS, SourceManager &SM) const; + private: // Users are not expected to use error_code. std::error_code convertToErrorCode() const override { @@ -279,7 +290,7 @@ const_iterator end() const { return Replaces.end(); } - const_reverse_iterator rbegin() const { return Replaces.rbegin(); } + const_reverse_iterator rbegin() const { return Replaces.rbegin(); } const_reverse_iterator rend() const { return Replaces.rend(); } Index: clang/lib/Tooling/Core/Replacement.cpp =================================================================== --- clang/lib/Tooling/Core/Replacement.cpp +++ clang/lib/Tooling/Core/Replacement.cpp @@ -40,7 +40,7 @@ using namespace clang; using namespace tooling; -static const char * const InvalidLocation = ""; +static const char *const InvalidLocation = ""; Replacement::Replacement() : FilePath(InvalidLocation) {} @@ -61,9 +61,7 @@ setFromSourceRange(Sources, Range, ReplacementText, LangOpts); } -bool Replacement::isApplicable() const { - return FilePath != InvalidLocation; -} +bool Replacement::isApplicable() const { return FilePath != InvalidLocation; } bool Replacement::apply(Rewriter &Rewrite) const { SourceManager &SM = Rewrite.getSourceMgr(); @@ -72,9 +70,8 @@ return false; FileID ID = SM.getOrCreateFileID(*Entry, SrcMgr::C_User); - const SourceLocation Start = - SM.getLocForStartOfFile(ID). - getLocWithOffset(ReplacementRange.getOffset()); + const SourceLocation Start = SM.getLocForStartOfFile(ID).getLocWithOffset( + ReplacementRange.getOffset()); // ReplaceText returns false on success. // ReplaceText only fails if the source location is not a file location, in // which case we already returned false earlier. @@ -92,6 +89,27 @@ return Stream.str(); } +void Replacement::print(raw_ostream &OS, const SourceManager &SM) const { + auto Entry = SM.getFileManager().getFile(FilePath); + assert(File); + FileID ID = SM.translateFile(*Entry); + auto PrintOffsetPosition = [&](unsigned int Offset) { + OS << SM.getLineNumber(ID, Offset) << ":" << SM.getColumnNumber(ID, Offset); + }; + OS << FilePath << ":"; + PrintOffsetPosition(getOffset()); + OS << "-"; + PrintOffsetPosition(getOffset() + getLength()); + OS << ":\"" << ReplacementText << "\""; +} + +std::string Replacement::printToString(const SourceManager &SM) const { + std::string Result; + llvm::raw_string_ostream Stream(Result); + print(Stream, SM); + return Result; +} + namespace clang { namespace tooling { @@ -138,7 +156,8 @@ SourceLocation SpellingEnd = Sources.getSpellingLoc(Range.getEnd()); std::pair Start = Sources.getDecomposedLoc(SpellingBegin); std::pair End = Sources.getDecomposedLoc(SpellingEnd); - if (Start.first != End.first) return -1; + if (Start.first != End.first) + return -1; if (Range.isTokenRange()) End.second += Lexer::MeasureTokenLength(SpellingEnd, Sources, LangOpts); return End.second - Start.second; @@ -186,6 +205,75 @@ return Message; } +void ReplacementError::reportConflict(raw_ostream &OS, + SourceManager &SM) const { + if (Err == replacement_error::fail_to_apply || + Err == replacement_error::wrong_file_path) { + OS << getReplacementErrString(Err); + if (NewReplacement.hasValue()) { + OS << "\nNew replacement: "; + NewReplacement->print(OS, SM); + } + if (ExistingReplacement.hasValue()) { + OS << "\nExisting replacement: "; + ExistingReplacement->print(OS, SM); + } + return; + } + + auto Entry = SM.getFileManager().getFile(NewReplacement->getFilePath()); + if (!Entry) + return; + + FileID FID = SM.getOrCreateFileID(*Entry, SrcMgr::C_User); + + // Compute the min an max offset of between the conflicting replacements. + // This range is enlarge to the corresponding line begin and end, then the + // text is extract. + unsigned int OffsetMin = + std::min(ExistingReplacement->getOffset(), NewReplacement->getOffset()); + unsigned int OffsetMax = std::max( + ExistingReplacement->getOffset() + ExistingReplacement->getLength(), + NewReplacement->getOffset() + NewReplacement->getLength()); + unsigned int BeginOffset = + SM.getDecomposedLoc( + SM.translateLineCol(FID, SM.getLineNumber(FID, OffsetMin), 1)) + .second; + unsigned int EndOffset = + SM.getDecomposedLoc( + SM.translateLineCol(FID, SM.getLineNumber(FID, OffsetMax), 0)) + .second; + StringRef Text = SM.getBuffer(FID)->getBuffer().slice(BeginOffset, EndOffset); + + auto ReplacementText = [&](const tooling::Replacement &R) { + unsigned int StartOffset = R.getOffset() - BeginOffset; + unsigned int EndOffset = R.getOffset() + R.getLength() - BeginOffset; + std::string Buffer = Text; + Buffer.erase(Buffer.begin() + StartOffset, Buffer.begin() + EndOffset); + Buffer.insert(StartOffset, R.getReplacementText()); + return Buffer; + }; + + auto PrintOffsetPosition = [&](unsigned int Offset) { + OS << SM.getLineNumber(FID, Offset) << ":" + << SM.getColumnNumber(FID, Offset); + }; + + // Apply existing and new replacement into the extracted text and print then + // within the conflict marker format and prefix everthing with filepath + file + // location range of the conflict. + OS << NewReplacement->getFilePath() << ":"; + PrintOffsetPosition(OffsetMin); + OS << "-"; + PrintOffsetPosition(OffsetMax); + OS << "\n" + << "<<<<<<< Existing replacement\n" + << ReplacementText(*ExistingReplacement) << "\n" + << "=======\n" + << ReplacementText(*NewReplacement) << "\n" + << ">>>>>>> New replacement\n"; +} + char ReplacementError::ID = 0; Replacements Replacements::getCanonicalReplacements() const { @@ -367,8 +455,8 @@ public: MergedReplacement(const Replacement &R, bool MergeSecond, int D) : MergeSecond(MergeSecond), Delta(D), FilePath(R.getFilePath()), - Offset(R.getOffset() + (MergeSecond ? 0 : Delta)), Length(R.getLength()), - Text(R.getReplacementText()) { + Offset(R.getOffset() + (MergeSecond ? 0 : Delta)), + Length(R.getLength()), Text(R.getReplacementText()) { Delta += MergeSecond ? 0 : Text.size() - Length; DeltaFirst = MergeSecond ? Text.size() - Length : 0; } @@ -577,7 +665,7 @@ } llvm::Expected applyAllReplacements(StringRef Code, - const Replacements &Replaces) { + const Replacements &Replaces) { if (Replaces.empty()) return Code.str(); @@ -592,8 +680,7 @@ InMemoryFileSystem->addFile( "", 0, llvm::MemoryBuffer::getMemBuffer(Code, "")); FileID ID = SourceMgr.createFileID(*Files.getFile(""), - SourceLocation(), - clang::SrcMgr::C_User); + SourceLocation(), clang::SrcMgr::C_User); for (auto I = Replaces.rbegin(), E = Replaces.rend(); I != E; ++I) { Replacement Replace("", I->getOffset(), I->getLength(), I->getReplacementText()); Index: clang/unittests/Tooling/RefactoringTest.cpp =================================================================== --- clang/unittests/Tooling/RefactoringTest.cpp +++ clang/unittests/Tooling/RefactoringTest.cpp @@ -24,10 +24,13 @@ #include "clang/Frontend/FrontendAction.h" #include "clang/Frontend/TextDiagnosticPrinter.h" #include "clang/Rewrite/Core/Rewriter.h" +#include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Refactoring/AtomicChange.h" #include "clang/Tooling/Tooling.h" #include "llvm/ADT/SmallString.h" +#include "llvm/Support/Error.h" #include "llvm/Support/VirtualFileSystem.h" +#include "llvm/Support/raw_ostream.h" #include "gtest/gtest.h" namespace clang { @@ -58,8 +61,8 @@ } TEST_F(ReplacementTest, CanReplaceTextAtPosition) { - FileID ID = Context.createInMemoryFile("input.cpp", - "line1\nline2\nline3\nline4"); + FileID ID = + Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4"); SourceLocation Location = Context.getLocation(ID, 2, 3); Replacement Replace(createReplacement(Location, 12, "x")); EXPECT_TRUE(Replace.apply(Context.Rewrite)); @@ -67,8 +70,8 @@ } TEST_F(ReplacementTest, CanReplaceTextAtPositionMultipleTimes) { - FileID ID = Context.createInMemoryFile("input.cpp", - "line1\nline2\nline3\nline4"); + FileID ID = + Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4"); SourceLocation Location1 = Context.getLocation(ID, 2, 3); Replacement Replace1(createReplacement(Location1, 12, "x\ny\n")); EXPECT_TRUE(Replace1.apply(Context.Rewrite)); @@ -135,7 +138,8 @@ } }); OS.flush(); - if (ErrorMessage.empty()) return true; + if (ErrorMessage.empty()) + return true; llvm::errs() << ErrorMessage; return false; } @@ -430,8 +434,8 @@ } TEST_F(ReplacementTest, CanApplyReplacements) { - FileID ID = Context.createInMemoryFile("input.cpp", - "line1\nline2\nline3\nline4"); + FileID ID = + Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4"); Replacements Replaces = toReplacements({Replacement(Context.Sources, Context.getLocation(ID, 2, 1), 5, "replaced"), @@ -444,8 +448,8 @@ // 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"); + 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, @@ -455,8 +459,7 @@ } TEST_F(ReplacementTest, AdjacentReplacements) { - FileID ID = Context.createInMemoryFile("input.cpp", - "ab"); + 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")}); @@ -465,8 +468,8 @@ } TEST_F(ReplacementTest, AddDuplicateReplacements) { - FileID ID = Context.createInMemoryFile("input.cpp", - "line1\nline2\nline3\nline4"); + FileID ID = + Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4"); auto Replaces = toReplacements({Replacement( Context.Sources, Context.getLocation(ID, 2, 1), 5, "replaced")}); @@ -485,8 +488,8 @@ } TEST_F(ReplacementTest, FailOrderDependentReplacements) { - FileID ID = Context.createInMemoryFile("input.cpp", - "line1\nline2\nline3\nline4"); + FileID ID = + Context.createInMemoryFile("input.cpp", "line1\nline2\nline3\nline4"); auto Replaces = toReplacements({Replacement( Context.Sources, Context.getLocation(ID, 2, 1), 5, "other")}); @@ -585,9 +588,9 @@ class FlushRewrittenFilesTest : public ::testing::Test { public: - FlushRewrittenFilesTest() {} + FlushRewrittenFilesTest() {} - ~FlushRewrittenFilesTest() override { + ~FlushRewrittenFilesTest() override { for (llvm::StringMap::iterator I = TemporaryFiles.begin(), E = TemporaryFiles.end(); I != E; ++I) { @@ -646,8 +649,7 @@ } namespace { -template -class TestVisitor : public clang::RecursiveASTVisitor { +template class TestVisitor : public clang::RecursiveASTVisitor { public: bool runOver(StringRef Code) { return runToolOnCode(std::make_unique(this), Code); @@ -689,8 +691,8 @@ }; } // end namespace -void expectReplacementAt(const Replacement &Replace, - StringRef File, unsigned Offset, unsigned Length) { +void expectReplacementAt(const Replacement &Replace, StringRef File, + unsigned Offset, unsigned Length) { ASSERT_TRUE(Replace.isApplicable()); EXPECT_EQ(File, Replace.getFilePath()); EXPECT_EQ(Offset, Replace.getOffset()); @@ -740,7 +742,7 @@ TEST(Replacement, TemplatedFunctionCall) { CallToFVisitor CallToF; EXPECT_TRUE(CallToF.runOver( - "template void F(); void G() { F(); }")); + "template void F(); void G() { F(); }")); expectReplacementAt(CallToF.Replace, "input.cc", 43, 8); } @@ -749,14 +751,15 @@ public: bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLoc) { if (NNSLoc.getNestedNameSpecifier()) { - if (const NamespaceDecl* NS = NNSLoc.getNestedNameSpecifier()->getAsNamespace()) { + if (const NamespaceDecl *NS = + NNSLoc.getNestedNameSpecifier()->getAsNamespace()) { if (NS->getName() == "a") { Replace = Replacement(*SM, &NNSLoc, "", Context->getLangOpts()); } } } - return TestVisitor::TraverseNestedNameSpecifierLoc( - NNSLoc); + return TestVisitor< + NestedNameSpecifierAVisitor>::TraverseNestedNameSpecifierLoc(NNSLoc); } Replacement Replace; }; @@ -896,7 +899,8 @@ } TEST(Range, MergeRangesAfterReplacements) { - std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)}; + std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), + Range(0, 1)}; Replacements Replaces = toReplacements({Replacement("foo", 1, 3, ""), Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")}); @@ -1091,19 +1095,19 @@ } class AtomicChangeTest : public ::testing::Test { - protected: - void SetUp() override { - DefaultFileID = Context.createInMemoryFile("input.cpp", DefaultCode); - DefaultLoc = Context.Sources.getLocForStartOfFile(DefaultFileID) - .getLocWithOffset(20); - assert(DefaultLoc.isValid() && "Default location must be valid."); - } +protected: + void SetUp() override { + DefaultFileID = Context.createInMemoryFile("input.cpp", DefaultCode); + DefaultLoc = Context.Sources.getLocForStartOfFile(DefaultFileID) + .getLocWithOffset(20); + assert(DefaultLoc.isValid() && "Default location must be valid."); + } - RewriterTestContext Context; - std::string DefaultCode = std::string(100, 'a'); - unsigned DefaultOffset = 20; - SourceLocation DefaultLoc; - FileID DefaultFileID; + RewriterTestContext Context; + std::string DefaultCode = std::string(100, 'a'); + unsigned DefaultOffset = 20; + SourceLocation DefaultLoc; + FileID DefaultFileID; }; TEST_F(AtomicChangeTest, AtomicChangeToYAML) { @@ -1112,7 +1116,7 @@ Change.insert(Context.Sources, DefaultLoc, "aa", /*InsertAfter=*/false); ASSERT_TRUE(!Err); Err = Change.insert(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb", - /*InsertAfter=*/false); + /*InsertAfter=*/false); ASSERT_TRUE(!Err); Change.addHeader("a.h"); Change.removeHeader("b.h"); @@ -1161,10 +1165,10 @@ "...\n"; AtomicChange ExpectedChange(Context.Sources, DefaultLoc); llvm::Error Err = ExpectedChange.insert(Context.Sources, DefaultLoc, "aa", - /*InsertAfter=*/false); + /*InsertAfter=*/false); ASSERT_TRUE(!Err); Err = ExpectedChange.insert(Context.Sources, DefaultLoc.getLocWithOffset(10), - "bb", /*InsertAfter=*/false); + "bb", /*InsertAfter=*/false); ASSERT_TRUE(!Err); ExpectedChange.addHeader("a.h"); @@ -1209,6 +1213,35 @@ EXPECT_EQ(Change.getReplacements().size(), 1u); } +TEST_F(AtomicChangeTest, ReplaceReportConflict) { + AtomicChange Change(Context.Sources, DefaultLoc); + llvm::Error Err = Change.replace(Context.Sources, DefaultLoc, 2, "bb"); + ASSERT_TRUE(!Err); + EXPECT_EQ(Change.getReplacements().size(), 1u); + EXPECT_EQ(*Change.getReplacements().begin(), + Replacement(Context.Sources, DefaultLoc, 2, "bb")); + + // Add a new replacement that conflicts with the existing one. + Err = Change.replace(Context.Sources, DefaultLoc, 2, "cc"); + EXPECT_TRUE((bool)Err); + std::string ConflictMessage; + llvm::consumeError( + llvm::handleErrors(std::move(Err), [&](const ReplacementError &RE) { + llvm::raw_string_ostream OS(ConflictMessage); + RE.reportConflict(OS, Context.Sources); + })); + llvm::errs() << ConflictMessage; + EXPECT_STREQ("input.cpp:1:21-1:23\n" + "<<<<<<< Existing replacement\n" + "aaaaaaaaaaaaaaaaaaaabbaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + "=======\n" + "aaaaaaaaaaaaaaaaaaaaccaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n" + ">>>>>>> New replacement\n", + ConflictMessage.c_str()); +} + TEST_F(AtomicChangeTest, ReplaceWithRange) { AtomicChange Change(Context.Sources, DefaultLoc); SourceLocation End = DefaultLoc.getLocWithOffset(20); @@ -1256,7 +1289,7 @@ // Invalid location. Err = Change.insert(Context.Sources, SourceLocation(), "a", - /*InsertAfter=*/false); + /*InsertAfter=*/false); ASSERT_TRUE((bool)Err); EXPECT_TRUE(checkReplacementError( std::move(Err), replacement_error::wrong_file_path,