[clang-apply-replacements] Added an option to ignore insert conflict.

[Sockke]

Summary

If two different texts are inserted at the same offset, clang-apply-replacements prints the conflict error and discards all fixes. This patch adds support for adjusting conflict offset and keeps running to continually fix them.

https://godbolt.org/z/P938EGoxj doesn't have any fixes when I run run-clang-tidy.py to generate a YAML file with clang-tidy and fix them with clang-apply-replacements. The YAML file has two different header texts insertions at the same offset, unlike clang-tidy with '-fix', clang-apply-replacements does not adjust for this conflict.

[Sockke]

Added an option to ignore insert conflict and this option was enabled by default in run-clang-tidy.py.

[Sockke]

It stands to reason that there should be no insert conflicts in the YAML generated by clang-tidy except for the header insertion. The two checks that caused the conflict should be fixed in clang-tidy.

[MTC]

clang/include/clang/Tooling/Refactoring/AtomicChange.h

119

IMHO, there is no need to rename the method to get the non-const version. The following code is ok enough

Replacements &getReplacements() { return Replaces; }

[Sockke]

Rename the function.

[Sockke]

Friendly ping.

[aaron.ballman]

I don't know enough about this code base to feel comfortable signing off on it, but the changes look reasonable to me FWIW.

[MTC]

In D123924#3482342, @aaron.ballman wrote:

I don't know enough about this code base to feel comfortable signing off on it, but the changes look reasonable to me FWIW.

Thanks, Aaron! I will find another guy to review the change.

[Sockke]

Friendly ping.

[Sockke]

Hi, Could anyone please review this diff?

[aaron.ballman]

I think this has sat for long enough and my LGTM will have to suffice. :-)

[Sockke]

rebased.

clang-tools-extra/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

 /// FileChanges.
 /// \param[in] SM SourceManager required for conflict reporting.
 ///
 /// \returns \parblock
 ///          \li true If all changes were converted successfully.
 ///          \li false If there were conflicts.
 bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
                          FileToChangesMap &FileChanges,
-                         clang::SourceManager &SM);
+                         clang::SourceManager &SM,
+                         bool IgnoreInsertConflict = false);
 
 /// Apply \c AtomicChange on File and rewrite it.
 ///
 /// \param[in] File Path of the file where to apply AtomicChange.
 /// \param[in] Changes to apply.
 /// \param[in] Spec For code cleanup and formatting.
 /// \param[in] Diagnostics DiagnosticsEngine used for error output.
 ///

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

llvm::sort(FileAndReplacements.second.begin(),
                FileAndReplacements.second.end());
   }
 
   return GroupedReplacements;
 }
 
 bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
                          FileToChangesMap &FileChanges,
-                         clang::SourceManager &SM) {
+                         clang::SourceManager &SM, bool IgnoreInsertConflict) {
   auto GroupedReplacements = groupReplacements(TUs, TUDs, SM);
   bool ConflictDetected = false;
 
   // To report conflicting replacements on corresponding file, all replacements
   // are stored into 1 big AtomicChange.
   for (const auto &FileAndReplacements : GroupedReplacements) {
     const FileEntry *Entry = FileAndReplacements.first;
     const SourceLocation BeginLoc =
for (const auto &R : FileAndReplacements.second) {
         // `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";
+        if (IgnoreInsertConflict) {
+          tooling::Replacements &Replacements = FileChange.getReplacements();
+          unsigned NewOffset =
+              Replacements.getShiftedCodePosition(R.getOffset());
+          unsigned NewLength = Replacements.getShiftedCodePosition(
+                                   R.getOffset() + R.getLength()) -
+                               NewOffset;
+          if (NewLength == R.getLength()) {
+            tooling::Replacement RR = tooling::Replacement(
+                R.getFilePath(), NewOffset, NewLength, R.getReplacementText());
+            Replacements = Replacements.merge(tooling::Replacements(RR));
+          } else {
+            llvm::errs()
+                << "Can't resolve conflict, skipping the replacement.\n";
+            ConflictDetected = true;
+          }
+        } else
           ConflictDetected = true;
       }
     }
     FileChanges.try_emplace(Entry,
                             std::vector<tooling::AtomicChange>{FileChange});
   }
 
   return !ConflictDetected;
 }

clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp

const cl::OptionCategory *VisibleCategories[] = {&ReplacementCategory,
                                                  &FormattingCategory};
 
 static cl::opt<bool> RemoveTUReplacementFiles(
     "remove-change-desc-files",
     cl::desc("Remove the change description files regardless of successful\n"
              "merging/replacing."),
     cl::init(false), cl::cat(ReplacementCategory));
 
+static cl::opt<bool> IgnoreInsertConflict(
+    "ignore-insert-conflict",
+    cl::desc("Ignore insert conflict and keep running to fix."),
+    cl::init(false), cl::cat(ReplacementCategory));
+
 static cl::opt<bool> DoFormat(
     "format",
     cl::desc("Enable formatting of code changed by applying replacements.\n"
              "Use -style to choose formatting style.\n"),
     cl::cat(FormattingCategory));
 
 // FIXME: Consider making the default behaviour for finding a style
 // configuration file to start the search anew for every file being changed to
int main(int argc, char **argv) {
   std::unique_ptr<ScopedFileRemover> Remover;
   if (RemoveTUReplacementFiles)
     Remover.reset(new ScopedFileRemover(TUFiles, Diagnostics));
 
   FileManager Files((FileSystemOptions()));
   SourceManager SM(Diagnostics, Files);
 
   FileToChangesMap Changes;
-  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM, IgnoreInsertConflict))
     return 1;
 
   tooling::ApplyChangesSpec Spec;
   Spec.Cleanup = true;
   Spec.Style = FormatStyle;
   Spec.Format = DoFormat ? tooling::ApplyChangesSpec::kAll
                          : tooling::ApplyChangesSpec::kNone;
 

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

else:
     raise SystemExit(
       "error: failed to find {} in $PATH or at {}"
       .format(name, built_path))
 
 
 def apply_fixes(args, clang_apply_replacements_binary, tmpdir):
   """Calls clang-apply-fixes on a given directory."""
   invocation = [clang_apply_replacements_binary]
+  invocation.append('-ignore-insert-conflict')
   if args.format:
     invocation.append('-format')
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
 

clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/file1.yaml

+---
+MainSourceFile:     ignore-conflict.cpp
+Diagnostics:
+  - DiagnosticName: test-ignore-conflict-insertion
+    DiagnosticMessage:
+      Message: Fix
+      FilePath: $(path)/ignore-conflict.cpp
+      FileOffset: 0
+      Replacements:
+        - FilePath:        $(path)/ignore-conflict.cpp
+          Offset:          0
+          Length:          0
+          ReplacementText: "#include <a.h>\n"
+  - DiagnosticName: test-ignore-conflict-insertion
+    DiagnosticMessage:
+      Message: Fix
+      FilePath: $(path)/ignore-conflict.cpp
+      FileOffset: 0
+      Replacements:
+        - FilePath:        $(path)/ignore-conflict.cpp
+          Offset:          0
+          Length:          0
+          ReplacementText: "#include <b.h>\n"
+...

clang-tools-extra/test/clang-apply-replacements/Inputs/ignore-conflict/ignore-conflict.cpp

+class MyType {};
+// CHECK: #include <a.h>
+// CHECK-NEXT: #include <b.h>
+// CEHCK-NEXT: class MyType {};

clang-tools-extra/test/clang-apply-replacements/ignore-conflict.cpp

+// RUN: mkdir -p %T/Inputs/ignore-conflict
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/ignore-conflict/ignore-conflict.cpp > %T/Inputs/ignore-conflict/ignore-conflict.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/ignore-conflict#" %S/Inputs/ignore-conflict/file1.yaml > %T/Inputs/ignore-conflict/file1.yaml
+// RUN: clang-apply-replacements --ignore-insert-conflict %T/Inputs/ignore-conflict
+// RUN: FileCheck -input-file=%T/Inputs/ignore-conflict/ignore-conflict.cpp %S/Inputs/ignore-conflict/ignore-conflict.cpp

clang/include/clang/Tooling/Refactoring/AtomicChange.h

public:
   void addHeader(llvm::StringRef Header);
 
   /// Removes a header from the file that contains the key position.
   void removeHeader(llvm::StringRef Header);
 
   /// Returns a const reference to existing replacements.
   const Replacements &getReplacements() const { return Replaces; }
 
+  Replacements &getReplacements() { return Replaces; }
+
   llvm::ArrayRef<std::string> getInsertedHeaders() const {
     return InsertedHeaders;
   }
 
   llvm::ArrayRef<std::string> getRemovedHeaders() const {
     return RemovedHeaders;
   }