[FileManager] fillRealPathName even if we aren't opening the file

[jkorous]

Summary

This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All

rdar://47536127

[jkorous]

@sammccall I think you touched this part of the code recently in r352079 (revert of r347205).

[arphaman]

lib/Basic/FileManager.cpp

271

I don't really understand what this comment is trying to say. Can you rephrase it?

unittests/Basic/FileManagerTest.cpp

349

Please leave this comment out

[jkorous]

Thanks for taking a look!

lib/Basic/FileManager.cpp

214

If we can't stat the file we return here.

271

Please see the above comment (line 214).

What I meant is that it's guaranteed that the file exists as we would have returned otherwise. Does that make sense?

Any suggestion how to rephrase the comment? (Or shall I just remove it?)

[jkorous]

unittests/Basic/FileManagerTest.cpp

349

Sure, no problem.

Just so I understand it - when it is appropriate to quote the radar? I see a lot of them in clang/test. Are these just from the "ancient" history?

[arphaman]

lib/Basic/FileManager.cpp

271

The throwback here is unnecessary. You can just say that RealFilePath should still be filled even for stats.

unittests/Basic/FileManagerTest.cpp

357

NIT: You might want to make OpenFile = false explicit to emphasize the stat.

[arphaman]

unittests/Basic/FileManagerTest.cpp

349

There are some radars but that doesn't mean we should still be adding them. They don't provide any context or value for the developers here, especially in a comment. For a unit test like this the name should generally provide some form of self-contained description of the scenario, which it does here. I would recommend leaving the radar number in a commit message instead.

[jkorous]

• comments
• explicit openFile = false in test

[jkorous]

Updated the patch.

unittests/Basic/FileManagerTest.cpp

349

Got it. Thanks!

[arphaman]

LGTM

lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   // Check to see if the file exists.
   std::unique_ptr<llvm::vfs::File> F;
   FileData Data;
   if (getStatValue(InterndFileName, Data, true, openFile ? &F : nullptr)) {
     // There's no real file at the given path.
     if (!CacheFailure)
       SeenFileEntries.erase(Filename);
 
     return nullptr;
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
 
const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
   if (UFE.File) {
     if (auto PathName = UFE.File->getName())
       fillRealPathName(&UFE, *PathName);
+  } else if (!openFile) {
+    // We should still fill the path even if we aren't opening the file.
+    fillRealPathName(&UFE, InterndFileName);
   }
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;

unittests/Basic/FileManagerTest.cpp

#endif
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
   SmallString<64> ExpectedResult = CustomWorkingDir;
 
   llvm::sys::path::append(ExpectedResult, "tmp", "test");
   EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);
 }
 
+TEST_F(FileManagerTest, getFileDontOpenRealPath) {
+  auto statCache = llvm::make_unique<FakeStatCache>();
+  statCache->InjectDirectory("/tmp/abc", 42);
+  SmallString<64> Path("/tmp/abc/foo.cpp");
+  statCache->InjectFile(Path.str().str().c_str(), 43);
+  manager.setStatCache(std::move(statCache));
+
+  const FileEntry *file = manager.getFile(Path, /*openFile=*/false);
+
+  ASSERT_TRUE(file != nullptr);
+
+  ASSERT_EQ(file->tryGetRealPathName(), Path);
+}
+
 } // anonymous namespace