[VFS] Emit an error when a file isn't located in any directory.

[vsapsai]

Summary

Orphaned files prevent us from building a file system tree and cause the
assertion

Assertion failed: (NewParentE && "Parent entry must exist"), function uniqueOverlayTree, file clang/lib/Basic/VirtualFileSystem.cpp, line 1303.

rdar://problem/28990865

[vsapsai]

clang/lib/Basic/VirtualFileSystem.cpp

1423

Not happy with the error message but didn't come up with anything better. Suggestions are welcome.

[vsapsai]

Need to double check what tests we have when using relative path names at the root level. I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

[bruno]

Hi Volodymyr, thanks for improving this.

Need to double check what tests we have when using relative path names at the root level.

Another interesting place to look at is unittests/Basic/VirtualFileSystemTest.cpp

I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

Makes sense. My general impression is that relative paths don't make sense here either, but you can address that in a follow up patch (giving time for any potential counterexample on this).

clang/lib/Basic/VirtualFileSystem.cpp

1423

What about the message you used in the comment: file entry is at the root level, outside of any directory ?

[vsapsai]

In D49518#1168038, @bruno wrote:

Hi Volodymyr, thanks for improving this.

Need to double check what tests we have when using relative path names at the root level.

Another interesting place to look at is unittests/Basic/VirtualFileSystemTest.cpp

Thanks for the pointer. Probably I'll move the added test there as it doesn't need full file system interaction and doesn't need to execute entire clang_cc1.

I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

Makes sense. My general impression is that relative paths don't make sense here either, but you can address that in a follow up patch (giving time for any potential counterexample on this).

Based on the code, relative paths won't work for file lookup. That is so because in RedirectingFileSystem::lookupPath we convert paths to absolute

if (std::error_code EC = makeAbsolute(Path))
  return EC;
// ...
sys::path::const_iterator Start = sys::path::begin(Path);
sys::path::const_iterator End = sys::path::end(Path);

and textually compare them with VFS entry name.

Having an error for relative paths makes the change cleaner, so I'll include it in this patch.

[vsapsai]

• Make diagnostics more general, use unit tests.

[bruno]

LGTM

clang/lib/Basic/VirtualFileSystem.cpp

1391

Reads better :)

[vsapsai]

Thanks for the review, Bruno.

clang/lib/Basic/VirtualFileSystem.cpp

case EK_File: {
       auto *DE = dyn_cast<RedirectingDirectoryEntry>(NewParentE);
       DE->addContent(llvm::make_unique<RedirectingFileEntry>(
           Name, FE->getExternalContentsPath(), FE->getUseName()));
       break;
     }
     }
   }
 
-  std::unique_ptr<Entry> parseEntry(yaml::Node *N, RedirectingFileSystem *FS) {
+  std::unique_ptr<Entry> parseEntry(yaml::Node *N, RedirectingFileSystem *FS,
+                                    bool IsRootEntry) {
     auto *M = dyn_cast<yaml::MappingNode>(N);
     if (!M) {
       error(N, "expected mapping node for file or directory entry");
       return nullptr;
     }
 
     KeyStatusPair Fields[] = {
       KeyStatusPair("name", true),
       KeyStatusPair("type", true),
       KeyStatusPair("contents", false),
       KeyStatusPair("external-contents", false),
       KeyStatusPair("use-external-name", false),
     };
 
     DenseMap<StringRef, KeyStatus> Keys(std::begin(Fields), std::end(Fields));
 
     bool HasContents = false; // external or otherwise
     std::vector<std::unique_ptr<Entry>> EntryArrayContents;
     std::string ExternalContentsPath;
     std::string Name;
+    yaml::Node *NameValueNode;
     auto UseExternalName = RedirectingFileEntry::NK_NotSet;
     EntryKind Kind;
 
     for (auto &I : *M) {
       StringRef Key;
       // Reuse the buffer for key and value, since we don't look at key after
       // parsing value.
       SmallString<256> Buffer;
       if (!parseScalarString(I.getKey(), Key, Buffer))
         return nullptr;
 
       if (!checkDuplicateOrUnknownKey(I.getKey(), Key, Keys))
         return nullptr;
 
       StringRef Value;
       if (Key == "name") {
         if (!parseScalarString(I.getValue(), Value, Buffer))
           return nullptr;
 
+        NameValueNode = I.getValue();
         if (FS->UseCanonicalizedPaths) {
           SmallString<256> Path(Value);
           // Guarantee that old YAML files containing paths with ".." and "."
           // are properly canonicalized before read into the VFS.
           Path = sys::path::remove_leading_dotslash(Path);
           sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
           Name = Path.str();
         } else {
for (auto &I : *M) {
         auto *Contents = dyn_cast<yaml::SequenceNode>(I.getValue());
         if (!Contents) {
           // FIXME: this is only for directories, what about files?
           error(I.getValue(), "expected array");
           return nullptr;
         }
 
         for (auto &I : *Contents) {
-          if (std::unique_ptr<Entry> E = parseEntry(&I, FS))
+          if (std::unique_ptr<Entry> E =
+                  parseEntry(&I, FS, /*IsRootEntry*/ false))
             EntryArrayContents.push_back(std::move(E));
           else
             return nullptr;
         }
       } else if (Key == "external-contents") {
         if (HasContents) {
           error(I.getKey(),
                 "entry already has 'contents' or 'external-contents'");
std::unique_ptr<Entry> parseEntry(yaml::Node *N, RedirectingFileSystem *FS,
 
     // check invalid configuration
     if (Kind == EK_Directory &&
         UseExternalName != RedirectingFileEntry::NK_NotSet) {
       error(N, "'use-external-name' is not supported for directories");
       return nullptr;
     }
 
+    if (IsRootEntry && !sys::path::is_absolute(Name)) {
+      assert(NameValueNode && "Name presence should be checked earlier");
+      error(NameValueNode,
+            "entry with relative path at the root level is not discoverable");
+      return nullptr;
+    }
+
     // Remove trailing slash(es), being careful not to remove the root path
     StringRef Trimmed(Name);
     size_t RootPathLen = sys::path::root_path(Trimmed).size();
     while (Trimmed.size() > RootPathLen &&
            sys::path::is_separator(Trimmed.back()))
       Trimmed = Trimmed.slice(0, Trimmed.size()-1);
     // Get the last component
     StringRef LastComponent = sys::path::filename(Trimmed);
case EK_Directory:
       break;
     }
 
     StringRef Parent = sys::path::parent_path(Trimmed);
     if (Parent.empty())
       return Result;
 
     // if 'name' contains multiple components, create implicit directory entries
     for (sys::path::reverse_iterator I = sys::path::rbegin(Parent),
                                      E = sys::path::rend(Parent);
          I != E; ++I) {
       std::vector<std::unique_ptr<Entry>> Entries;
       Entries.push_back(std::move(Result));
       Result = llvm::make_unique<RedirectingDirectoryEntry>(
           *I, std::move(Entries),
           Status("", getNextVirtualUniqueID(), std::chrono::system_clock::now(),
                  0, 0, 0, file_type::directory_file, sys::fs::all_all));
for (auto &I : *Top) {
       if (Key == "roots") {
         auto *Roots = dyn_cast<yaml::SequenceNode>(I.getValue());
         if (!Roots) {
           error(I.getValue(), "expected array");
           return false;
         }
 
         for (auto &I : *Roots) {
-          if (std::unique_ptr<Entry> E = parseEntry(&I, FS))
+          if (std::unique_ptr<Entry> E =
+                  parseEntry(&I, FS, /*IsRootEntry*/ true))
             RootEntries.push_back(std::move(E));
           else
             return false;
         }
       } else if (Key == "version") {
         StringRef VersionString;
         SmallString<4> Storage;
         if (!parseScalarString(I.getValue(), VersionString, Storage))

clang/unittests/Basic/VirtualFileSystemTest.cpp

vfs::recursive_directory_iterator I = vfs::recursive_directory_iterator(
                                         *O, "//root", EC), E;
   ASSERT_FALSE(EC);
   for (int l = 0; I != E; I.increment(EC), ++l) {
     ASSERT_FALSE(EC);
     EXPECT_EQ(I.level(), l);
   }
   EXPECT_EQ(I, E);
 }
+
+TEST_F(VFSFromYAMLTest, RelativePaths) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
+  // Filename at root level without a parent directory.
+  IntrusiveRefCntPtr<vfs::FileSystem> FS = getFromYAMLString(
+      "{ 'roots': [\n"
+      "  { 'type': 'file', 'name': 'file-not-in-directory.h',\n"
+      "    'external-contents': '//root/external/file'\n"
+      "  }\n"
+      "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative file path.
+  FS = getFromYAMLString(
+      "{ 'roots': [\n"
+      "  { 'type': 'file', 'name': 'relative/file/path.h',\n"
+      "    'external-contents': '//root/external/file'\n"
+      "  }\n"
+      "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative directory path.
+  FS = getFromYAMLString(
+       "{ 'roots': [\n"
+       "  { 'type': 'directory', 'name': 'relative/directory/path.h',\n"
+       "    'contents': []\n"
+       "  }\n"
+       "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  EXPECT_EQ(3, NumDiagnostics);
+}