[PATCH] D42640: [clangd] collect symbol #include & insert #include in global code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG apart from the .inc handling (happy to chat more)



================
Comment at: clangd/index/CanonicalIncludes.h:52
+  /// a canonical header name.
+  llvm::StringRef mapHeader(llvm::StringRef Header) const;
+
----------------
ioeric wrote:
> sammccall wrote:
> > So I'm a bit concerned this is too narrow an interface, and we really want to deal with SourceLocation here which would give us the include stack.
> > 
> > Evidence #1: .inc handling really is the same job, but because this class has a single-file interface, we have to push it into the caller.
> > Evidence #2: I was thinking about a more... principled way of determining system headers. One option would be to have a whitelist of public headers, and walk up the include stack to see if you hit one. (I'm not 100% sure this would work, but still...) This isn't implementable with the current interface.
> > 
> > One compromise would be to pass in a stack<StringRef> or something. Efficiency doesn't really matter because the caller can cache based on the top element.
> > Evidence #1: .inc handling really is the same job, but because this class has a single-file interface, we have to push it into the caller.
> I think this would depend on how you define the scope of this class. `.inc` handling is a subtle case that I'm a bit hesitated to build into the interface here.
> 
> > Evidence #2: ....
> This is actually very similar to how the hardcoded mapping was generated. I had a tool that examined include stacks for a library (e.g. STL) and applied a similar heuristic - treat the last header in the include stack within the library boundary as the "exporting" public header for a leaf include header, if there is no other public header that has shorter distance to that include. For example, if we see a stack like `stl/bits/internal.h -> stl/bits/another.h -> stl/public_1.h -> user_code.cc`, we say `public_1.h` exports `bits/internal.h` and add a mapping from `bits/internal.h$` to `public_1.h`. But if we see another (shorter) include stack like `stl/bits/internal.h -> stl/public_2.h -> user_code.cc`, we say `stl/public_2.h` exports `stl/bits/internal.h`. This heuristic works well for many cases. However, this may produce wrong mappings when an internal header is used by multiple public headers. For example, if we have two include stacks with the same length e.g. `bits/internal.h -> public_1.h -> user.cc` and `bits/inernal.h -> public_2.h -> user.cc`, the result mapping would depend on the order in which we see these two stacks; thus, we had to do some manual adjustment to make sure bits/internal.h is mapped to the correct header according to the standard.
> 
> I am happy to discuss better solution here. But within the scope of this patch, I'd prefer to stick to interfaces that work well for the current working solution instead of designing for potential future solutions. I should be easy to iterate on the interfaces as these interfaces aren't going to be widely used in clangd after all. WDYT?
> I think this would depend on how you define the scope of this class. .inc handling is a subtle case that I'm a bit hesitated to build into the interface here.

Sure it's subtle, but it's clearly in the scope of determining what the canonical header is for a symbol, which is the job of this class. We wouldn't be building it into the interface - on the contrary, the *current* proposed interface codifies *not* handling .inc files.

But you're right that we should check in something that handles most cases.

My preference would be to drop `.inc` from this patch until we can incorporate it into the design, but I'm also OK with a FIXME to move it.


================
Comment at: unittests/clangd/HeadersTests.cpp:29
+  // absolute path.
+  std::string addToSubdir(PathRef File, llvm::StringRef Code = "") {
+    assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
----------------
This test would be clearer to me if you removed this helper and just did

```FS.Files["sub/bar.h"] = ...```

in the test.

Can we change `buildTestFS` in `TestFS.cpp` to call `getVirtualTestFilePath` on relative paths to allow this?

(I can do this as a followup if you like, but it seems like a trivial change)


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:50
 MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
+MATCHER(HasIncludeHeader, "") {
+  return arg.Detail && !arg.Detail->IncludeHeader.empty();
----------------
nit: this is only used once, as Not(HasIncludeHeader()). Just use IncludeHeader("")?
The slight difference in detail handling doesn't seem to matter (I'm not even sure if it's exactly the right assertion)


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:624
+  TestHeaderURI = URI::createFile(TestHeaderName).toString();
+  runSymbolCollector("class Foo {};", /*Main=*/"");
+  EXPECT_THAT(Symbols,
----------------
Took me a while to understand this test, and still not sure I get it. Maybe "class string" here?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640