[clang][tidy] Ensure rewriter has the correct CWD

[jansvoboda11]

This patch replaces use of the deprecated FileEntry::getName() with FileEntryRef::getName(). This means the code now uses the path that was used to register file entry in SourceManager instead of the absolute path that happened to be used in the last call to FileManager::getFile() some place else.

This caused some test failures due to the fact that some paths can be relative and thus rely on the VFS CWD. The CWD can change for each TU, so when we run clang-tidy on a compilation database and try to perform all the replacements at the end, relative paths won't resolve the same. This patch takes care to reinstate the correct CWD and make the path reported by FileEntryRef absolute before passing it to llvm::writeToOutput().

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tidy

Changes

This patch replaces use of the deprecated FileEntry::getName() with FileEntryRef::getName(). This code now uses the path that was used to register file entry in SourceManager instead of the path that happened to be used in the last call to FileManager::getFile() some place else.

This caused some test failures due to the fact that the FileManager's CWD does not match what the real file system considers to be a CWD. This patch takes care to use the FileManager to make the file path absolute before passing it off to the real FS.

---

Full diff: https://github.com/llvm/llvm-project/pull/67839.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+8)
• (modified) clang/lib/Rewrite/Rewriter.cpp (+7-6)

diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 695bfd6e2edf7ef..426bb94f835a0de 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -227,6 +227,14 @@ class ErrorReporter {
           llvm::errs() << "Can't apply replacements for file " << File << "\n";
         }
       }
+
+      auto BuildDir = Context.getCurrentBuildDirectory();
+      if (!BuildDir.empty())
+        Rewrite.getSourceMgr()
+            .getFileManager()
+            .getVirtualFileSystem()
+            .setCurrentWorkingDirectory(BuildDir);
+
       if (Rewrite.overwriteChangedFiles()) {
         llvm::errs() << "clang-tidy failed to apply suggested fixes.\n";
       } else {
diff --git a/clang/lib/Rewrite/Rewriter.cpp b/clang/lib/Rewrite/Rewriter.cpp
index ef2858990dd954e..0896221dd0bdeb5 100644
--- a/clang/lib/Rewrite/Rewriter.cpp
+++ b/clang/lib/Rewrite/Rewriter.cpp
@@ -412,12 +412,13 @@ bool Rewriter::overwriteChangedFiles() {
   unsigned OverwriteFailure = Diag.getCustomDiagID(
       DiagnosticsEngine::Error, "unable to overwrite file %0: %1");
   for (buffer_iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
-    const FileEntry *Entry = getSourceMgr().getFileEntryForID(I->first);
-    if (auto Error =
-            llvm::writeToOutput(Entry->getName(), [&](llvm::raw_ostream &OS) {
-              I->second.write(OS);
-              return llvm::Error::success();
-            })) {
+    OptionalFileEntryRef Entry = getSourceMgr().getFileEntryRefForID(I->first);
+    llvm::SmallString<128> Path(Entry->getName());
+    getSourceMgr().getFileManager().makeAbsolutePath(Path);
+    if (auto Error = llvm::writeToOutput(Path, [&](llvm::raw_ostream &OS) {
+          I->second.write(OS);
+          return llvm::Error::success();
+        })) {
       Diag.Report(OverwriteFailure)
           << Entry->getName() << llvm::toString(std::move(Error));
       AllWritten = false;

[PiotrZSL]

Clang part looks fine.
For a clang-tidy part, is there a way to test this part ? What changes because we use now a absolute build directory.
I'm missing some tests for that part.

[jansvoboda11]

Clang part looks fine. For a clang-tidy part, is there a way to test this part ? What changes because we use now a absolute build directory. I'm missing some tests for that part.

So the clang-tidy part is necessary to keep the clang-tidy/infrastructure/clang-tidy-run-with-database.cpp test passing. It uses compilation database that has entries like this:

{
  "directory": "<absolute-dir>/b",
  "command": "clang++ -o test.o ../b/c.cpp",
  "file": "<absolute-dir>/b/c.cpp"
}

I think this already provides good enough code coverage for this PR. What do you think?

What I'd like to get clarified in this PR is which directory to use as the CWD.
Is using ClangTidyContext::getCurrentBuildDirectory() okay, or do we need to look somewhere else?

[jansvoboda11]

Ping @PiotrZSL.

[PiotrZSL]

LGTM