[clang-apply-replacements] Correctly handle relative paths

[avogelsgesang]

Summary

The fixes from the YAML file can refer to relative paths.
Those relative paths are meant to be resolved relative to the
corresponding build directory.
However, clang-apply-replacements currently interprets all
paths relative to its own working directory. This causes issues,
e.g., when clang-apply-replacements is run from outside of
the original build directory.

This commit adjusts clang-apply-replacements to take the build
directory into account when resolving relative file paths.

[avogelsgesang]

Remove unrelated changes

[avogelsgesang]

Adding reviewers based on git log

[ymandel]

I'm not really familiar w/ this code. But, if njames93 doesn't respond, then I'll give it a go.

[ymandel]

Overall, this looks ok, but I don't feel like I'm qualified to approve this patch, since I'm not really familiar w/ how this tool is used in practice. Some comments nonetheless:

Those relative paths are meant to be resolved relative to the corresponding build directory.

Is this behavior documented somewhere?

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

156

let's use an optional -- when populated, we have to copy it anyhow, so there's nothing gained by passing by pointer, and it comes at the cost of a nullable pointer.

159

Why are you capturing this as a reference? This is a subtle and IMO error prone pattern, since it's not obvious in the code below that you're mutating a deeply nested element of the filesystem. Can you instead use a local variable and just set this right before you need it?

162

assuming this is now an optional.

[avogelsgesang]

Those relative paths are meant to be resolved relative to the corresponding build directory.

Is this behavior documented somewhere?

I couldn't find this documented anywhere. My assumption is based on behavior which I observed from clang-tidy. Without this patch, clang-apply-fixes failed to apply the changes exported by clang-tidy --export-fixes due to its inability to find some source files referred to through relative paths

It seems the clang::tooling::Diagnostic::BuildDirectory was introduced in https://reviews.llvm.org/D26137. Maybe @alexfh who reviewed that patch can provide some more context here?

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

159

Why are you capturing this as a reference?

Mostly to keep the code shorter. Otherwise, I would have to write

auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
if (buildDir)
  SM.getFileManager().getFileSystemOpts().WorkingDir = *buildDir;
// [...]
WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;

which isn't really DRY.

Can you instead use a local variable and just set this right before you need it?

I think I don't understand the alternative you are proposing here. Can you provide an example?

[avogelsgesang]

Use llvm::Optional instead of pointer as suggested by @ymandel

[avogelsgesang]

@alexfh, @ymandel: Do you know who could review this change?

[ymandel]

Overall, looks good to me. But, I'd like to get a more experienced reviewer to confirm they're comfortable with the new behavior.

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

159

What you wrote is what I had in mind, though the last line, i think, would be:
SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
?

That said, I see your point. The problem is not in your code so much as in the underlying (lack of) API for setting and restoring the working directory. Honestly, the need to manually restore bothers me more than the reference, now that I consider it in more detail. Perhaps another reviewer will have a better suggestion.

[hokein]

this change looks good to me, I will leave the final stamp for @ymandel.

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

159

rather than keeping the code short, I would be more explicit on setting and restoring the working directory, it is easier for readers to understand the intention (the FileManager's getter API which returns a mutable reference seems a bit weird to me).

[ymandel]

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

159

Ok, let's do that then. It's "wordy" but it will be very clear what's going on, and seems the best we can do given the odd API.

[avogelsgesang]

Don't update WorkingDir through local reference; as requested @ymandel and @hokein

[avogelsgesang]

Thanks for the reviews, @ymandel and @hokein!

What are the next steps to get this landed?
(I am a first time contributor and have no commit access)

[ymandel]

I can land it for you. should be sometime this afternoon, but ping me tomorrow if you don't see any activity.

[dexonsmith]

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

179

This looks incorrect.

The FileManager is not designed to have the working directory change and it's not sound to change/restore it. This is because it caches the meaning of every relative-path lookup.

Consider:

/dir1/file1.h
/dir2/file1.h
/dir2/file2.h

if you do:

FM.WorkingDir = "/dir1";
FM.getFile("file1.h"); // returns /dir1/file1.h
FM.WorkingDir = "/dir2";
FM.getFile("file1.h"); // returns /dir1/file1.h !!!
FM.getFile("file2.h"); // returns /dir2/file2.h

See also the comment at https://reviews.llvm.org/D122549#3412965.

I don't know anything about clang-apply-replacements, but I have a couple of ideas for handling this correctly:

• Change the FileManager so that it's constructed with the working directory set to the build directory (don't save+restore, just make it correct from the start).
• If there are multiple concepts of a "build directory", use a different FileManager for each one.

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

groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
   // same TU.
   // FIXME: Find an efficient way to deduplicate on diagnostics level.
   llvm::DenseMap<const FileEntry *,
                  std::map<tooling::Replacement,
                           const tooling::TranslationUnitDiagnostics *>>
       DiagReplacements;
 
   auto AddToGroup = [&](const tooling::Replacement &R,
-                        const tooling::TranslationUnitDiagnostics *SourceTU) {
+                        const tooling::TranslationUnitDiagnostics *SourceTU,
+                        const std::string *buildDir) {
     // Use the file manager to deduplicate paths. FileEntries are
     // automatically canonicalized.
+    auto &WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
+    auto PrevWorkingDir = WorkingDir;
+    if (buildDir) {
+      WorkingDir = *buildDir;
+    }
     if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
       if (SourceTU) {
         auto &Replaces = DiagReplacements[*Entry];
         auto It = Replaces.find(R);
         if (It == Replaces.end())
           Replaces.emplace(R, SourceTU);
         else if (It->second != SourceTU)
           // This replacement is a duplicate of one suggested by another TU.
           return;
       }
       GroupedReplacements[*Entry].push_back(R);
     } else if (Warned.insert(R.getFilePath()).second) {
       errs() << "Described file '" << R.getFilePath()
              << "' doesn't exist. Ignoring...\n";
     }
+    WorkingDir = PrevWorkingDir;
   };
 
   for (const auto &TU : TUs)
     for (const tooling::Replacement &R : TU.Replacements)
-      AddToGroup(R, nullptr);
+      AddToGroup(R, nullptr, nullptr);
 
   for (const auto &TU : TUDs)
     for (const auto &D : TU.Diagnostics)
       if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
         for (const auto &Fix : *ChoosenFix)
           for (const tooling::Replacement &R : Fix.second)
-            AddToGroup(R, &TU);
+            AddToGroup(R, &TU, &D.BuildDirectory);
       }
 
   // Sort replacements per file to keep consistent behavior when
   // clang-apply-replacements run on differents machine.
   for (auto &FileAndReplacements : GroupedReplacements) {
     llvm::sort(FileAndReplacements.second.begin(),
                FileAndReplacements.second.end());
   }

clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h

+#ifndef BASIC_H
+#define BASIC_H
+
+
+class Parent {
+public:
+  virtual void func() {}
+};
+
+class Derived : public Parent {
+public:
+  virtual void func() {}
+  // CHECK: virtual void func() override {}
+};
+
+extern void ext(int (&)[5], const Parent &);
+
+void func(int t) {
+  int ints[5];
+  for (unsigned i = 0; i < 5; ++i) {
+    int &e = ints[i];
+    e = t;
+    // CHECK: for (auto & elem : ints) {
+    // CHECK-NEXT: elem = t;
+  }
+
+  Derived d;
+
+  ext(ints, d);
+}
+
+#endif // BASIC_H

clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml

+---
+MainSourceFile:     source1.cpp
+Diagnostics:
+  - BuildDirectory: $(path)/subdir/
+    DiagnosticName: test-relative-path
+    DiagnosticMessage:
+      Message: Fix
+      FilePath: ../relative-path.h
+      FileOffset: 242
+      Replacements:
+        - FilePath:        ../basic.h
+          Offset:          242
+          Length:          26
+          ReplacementText: 'auto & elem : ints'
+        - FilePath:        $(path)/basic.h
+          Offset:          276
+          Length:          22
+          ReplacementText: ''
+        - FilePath:        ../basic.h
+          Offset:          298
+          Length:          1
+          ReplacementText: elem
+        - FilePath:        ../../relative-paths/basic.h
+          Offset:          148
+          Length:          0
+          ReplacementText: 'override '
+...

clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml

+---
+MainSourceFile:     source2.cpp
+Diagnostics:
+  - BuildDirectory: $(path)
+    DiagnosticName: test-relative-path
+    DiagnosticMessage:
+      Message: Fix
+      FilePath: ./basic.h
+      FileOffset: 148
+      Replacements:
+        - FilePath:        ./basic.h
+          Offset:          298
+          Length:          1
+          ReplacementText: elem
+...

clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp

+// RUN: mkdir -p %T/Inputs/relative-paths
+// RUN: mkdir -p %T/Inputs/relative-paths/subdir
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/relative-paths/basic.h > %T/Inputs/relative-paths/basic.h
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file1.yaml > %T/Inputs/relative-paths/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file2.yaml > %T/Inputs/relative-paths/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/relative-paths
+// RUN: FileCheck -input-file=%T/Inputs/relative-paths/basic.h %S/Inputs/relative-paths/basic.h