Fix replacements for files with relative paths are not applied.

[arielbernal]

Summary

D1771 was supposed to fix this but it didn't, make_absolute can't be use to check if a file is virtual. I didn't break anything either.
This patch exposes virtual entries in the FileManager to check if a filepath is a virtual file that has already been allocated.

[klimek]

I don't understand that yet. Why wouldn't make_absolute just do nothing for virtual files anyway?

[arielbernal]

A better look at make_absolute showed me that it doesn't check for the filepath to exists it just gets the current path (PWD) and then uses the current path for getting the root and relative paths, and append them to the file name. So you can actually get an absolute path to a virtual file :).
I think virtual files shouldn't be made absolute unless someone explicitly create them that way.

[arielbernal]

The test I created in D1771 is irrelevant to the patch, it doesn't hurt and in fact there should be a test to exercise the use of relative paths in a compilation database but clang-check is not creating any replacement. I'm now adding another test for this patch.

[arielbernal]

Added unittest

[klimek]

include/clang/Basic/FileManager.h

278 ↗

(On Diff #4641)

If we decided we need this (see further down), this'll need approval from Doug...

lib/Tooling/Refactoring.cpp

108

I'd always use {} if there's a more complex statement inside.

122

Can't we instead just check whether the relative file exists?
If the file is relative, and it can be accessed via the filesystem, use make_absolute; otherwise, simply use the entry name.

[arielbernal]

include/clang/Basic/FileManager.h

278 ↗

(On Diff #4641)

if I use exists on the relative file then this won't be needed anymore.

lib/Tooling/Refactoring.cpp

108

yes sure

122

no problem I'll check that and upload another patch.

[arielbernal]

Addressed comments and used exists instead of isVirtualFile.

[klimek]

Apart from my remaining nits, LG. Thanks!

lib/Tooling/Refactoring.cpp

120

Did you change files to file on purpose? I think this makes the sentence incorrect ;)

unittests/Tooling/RefactoringTest.cpp

290

I'd EXPECT_EQ(0, chdir(...)), as the type of the chdir is not bool, and EXPECT_FALSE reads like we'd expect the command to fail.

[arielbernal]

Fixed test since it broke OSx build.

[arielbernal]

unittests/Tooling/RefactoringTest.cpp

290

Disregard this line, it should't be here

[klimek]

unittests/Tooling/RefactoringTest.cpp

290

It's still in the updated patch, though :)

[arielbernal]

Removed wrong setting of the current path

[klimek]

lg

lib/Tooling/Refactoring.cpp

 }
 
 void Replacement::setFromSourceLocation(SourceManager &Sources,
                                         SourceLocation Start, unsigned Length,
                                         StringRef ReplacementText) {
   const std::pair<FileID, unsigned> DecomposedLocation =
       Sources.getDecomposedLoc(Start);
   const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
   if (Entry != NULL) {
     // Make FilePath absolute so replacements can be applied correctly when
-    // relative paths for files are used.
+    // relative paths for files are used. But we don't want to change virtual
+    // files.
+    if (llvm::sys::fs::exists(Entry->getName())) {
       llvm::SmallString<256> FilePath(Entry->getName());
-    llvm::error_code EC = llvm::sys::fs::make_absolute(FilePath);
-    this->FilePath = EC ? FilePath.c_str() : Entry->getName();
+      llvm::sys::fs::make_absolute(FilePath);
+      this->FilePath = FilePath.c_str();
+    }
+    else {
+      this->FilePath = Entry->getName();
+    }
   } else {
     this->FilePath = InvalidLocation;
   }
   this->ReplacementRange = Range(DecomposedLocation.second, Length);
   this->ReplacementText = ReplacementText;
 }
 
 // FIXME: This should go into the Lexer, but we need to figure out how
 // to handle ranges for refactoring in general first - there is no obvious
 // good way how to integrate this into the Lexer yet.
 static int getRangeSize(SourceManager &Sources, const CharSourceRange &Range) {

unittests/Tooling/RefactoringTest.cpp

FileID createFile(llvm::StringRef Name, llvm::StringRef Content) {
     assert(File != NULL);
 
     StringRef Found = TemporaryFiles.GetOrCreateValue(Name, Path.str()).second;
     assert(Found == Path);
     (void)Found;
     return Context.Sources.createFileID(File, SourceLocation(), SrcMgr::C_User);
   }
 
+  StringRef getFilePath(llvm::StringRef Name) {
+    return TemporaryFiles.lookup(Name);
+  }
+
   std::string getFileContentFromDisk(llvm::StringRef Name) {
     std::string Path = TemporaryFiles.lookup(Name);
     assert(!Path.empty());
     // We need to read directly from the FileManager without relaying through
     // a FileEntry, as otherwise we'd read through an already opened file
     // descriptor, which might not see the changes made.
     // FIXME: Figure out whether there is a way to get the SourceManger to
     // reopen the file.
TEST_F(FlushRewrittenFilesTest, StoresChangesOnDisk) {
   Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, 2, 1),
                               5, "replaced"));
   EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
   EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
   EXPECT_EQ("line1\nreplaced\nline3\nline4",
             getFileContentFromDisk("input.cpp"));
 }
 
+TEST_F(FlushRewrittenFilesTest, GetFilePath) {
+  // Create a temporary file.
+  createFile("input.cpp", "line1\nline2\nline3\nline4");
+  StringRef FilePath = getFilePath("input.cpp");
+  StringRef TempPath = llvm::sys::path::parent_path(FilePath);
+  StringRef TempFile = llvm::sys::path::filename(FilePath);
+
+  // Save current path.
+  SmallString<512> CurrentPath;
+  llvm::sys::fs::current_path(CurrentPath);
+
+  // Change directory to the temporary directory.
+  EXPECT_EQ(0, chdir(TempPath.str().c_str()));
+
+  // Get a FileEntry from the current directory.
+  FileManager Files((FileSystemOptions()));
+  const FileEntry *Entry = Files.getFile(TempFile);
+  ASSERT_TRUE(Entry != NULL);
+
+  FileID ID = Context.Sources.createFileID(Entry, SourceLocation(),
+               SrcMgr::C_User);
+
+  Replacement R = Replacement(Context.Sources, Context.getLocation(ID, 2, 1),
+                              5, "replaced");
+
+  // Change back to the original path so we can verify that replacements
+  // are being applied independently of the location.
+  EXPECT_EQ(0, chdir(CurrentPath.c_str()));
+
+  // We expect that the file path of the replacement is using an absolute path
+  EXPECT_TRUE(llvm::sys::fs::equivalent(R.getFilePath(),
+      getFilePath("input.cpp")));
+
+  // Apply replacements.
+  Replacements Replaces;
+  Replaces.insert(R);
+  EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
+  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+  EXPECT_EQ("line1\nreplaced\nline3\nline4",
+                 getFileContentFromDisk("input.cpp"));
+}
+
 namespace {
 template <typename T>
 class TestVisitor : public clang::RecursiveASTVisitor<T> {
 public:
   bool runOver(StringRef Code) {
     return runToolOnCode(new TestAction(this), Code);
   }