[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D48903#1159985, @simark wrote:

> In https://reviews.llvm.org/D48903#1159303, @ioeric wrote:
>
> > For example, suppose you have header search directories `-I/path/to/include` and `-I.` in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by `FileID` through `SourceManager`. The path you get for a `FileEntry` or `FileID`  would depend on how clang looks up a file and how a file is first opened into `SourceManager`/`FileManager`.  It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future.
>
>
> It's hard to tell without seeing an actual failing use case.


I think the clang-move test you mitigated in https://reviews.llvm.org/D48951 is an example. When setting up compiler instance, it uses `-I.` in the compile command (https://github.com/llvm-mirror/clang-tools-extra/blob/master/unittests/clang-move/ClangMoveTests.cpp#L236), which results in the header paths that start with `./`. If you change replace `.` with the absolute path of the working directory e.g. `-I/usr/include`, the paths you get will start with `/usr/include/`. This is caused by the way preprocessor looks up include headers. We could require users (e.g. the clang-move test writer) to clean up dots, but this has to be enforced somehow by the framework.

> I understand the argument, but I think the necessity to work as closely as `RealFileSystem` as possible is important.  Otherwise, it becomes impossible to reproduce bugs happening in the real world in unittests.

FWIW, I don't think we could reproduce all real world bugs with unit tests ;)  But I agree with you that we should try to converge the behavior if possible, and I support the motivation of this change ;) This change would be perfectly fine as is if in-mem fs is always used directly by users, but the problem is that it's also used inside clang and clang tooling, where users don't control over how files are opened. My point is we should do this in a way that doesn't introduce inconsistency/confusion for users of clang and tooling framework.


Repository:
  rC Clang

https://reviews.llvm.org/D48903