[clangd] Skip .inc headers when canonicalizing header #include.

[ioeric]

Summary

This assumes that .inc files are supposed to be included via headers
that include them.

[sammccall]

n

clangd/index/CanonicalIncludes.cpp

50

nit: here and elsewhere, you probably want "declaring" rather than defining

clangd/index/CanonicalIncludes.h

52

This might be clearer slightly more concretely:

\p Headers is the include stack: Headers.front() is the file declaring the symbol, and Headers.back() is the main file

(or "included by the main file", but having it be main-file might be neater)

54

Nit: "this" is a bit ambiguous here, the parameter or the algorithm?
Consider just dropping this new part of the comment, it's kind of an implementation detail (hiding behind the word "canonical")

clangd/index/SymbolCollector.cpp

209

(as above, maybe want to include the main file for simplicity/symmetry)

[ioeric]

• Include main file in the include stack when mapping headers.

clangd/index/SymbolCollector.cpp

209

Thanks! As you pointed out offline, we would need main file for correctness as well, as a main file could the exporting header.

[sammccall]

clangd/index/CanonicalIncludes.cpp

52

headers are paths, not <quoted>, right?

53

This logic could be merged with the .inc logic, right?
"Find the first header such that the extension is not '.inc', and isn't a recognized non-header file"

58

lookupTypeForExtension can return TY_INVALID, which you have to check for (onlyPrecompileType will assert).

I think we should be conservative and accept TY_INVALID as possible headers - our goal here really is to exclude "obvious" non-headers like cpp.

(in this case you can probably drop the explicit empty check)

[ioeric]

• addressed comments.

[malaperle]

I think this might have broken the "shared lib" build? Could you double check that you don't need to add "clangDriver" ?

../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:61: error: undefined reference to 'clang::driver::types::lookupTypeForExtension(llvm::StringRef)'
../tools/clang/tools/extra/clangd/index/CanonicalIncludes.cpp:63: error: undefined reference to 'clang::driver::types::onlyPrecompileType(clang::driver::types::ID)'

Thanks!

---

From: cfe-commits <[e-mail]> on behalf of Eric Liu via Phabricator via cfe-commits <[e-mail]>
Sent: Thursday, May 24, 2018 10:44:26 AM
To: [e-mail]; [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: [PATCH] D47187: [clangd] Skip .inc headers when canonicalizing header #include.

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333188: [clangd] Skip .inc headers when canonicalizing header #include. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:

rL LLVM

https://reviews.llvm.org/D47187

Files:

clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

clangd/index/CanonicalIncludes.h

public:
   /// Maps all files matching \p RE to \p CanonicalPath
   void addRegexMapping(llvm::StringRef RE, llvm::StringRef CanonicalPath);
 
   /// Sets the canonical include for any symbol with \p QualifiedName.
   /// Symbol mappings take precedence over header mappings.
   void addSymbolMapping(llvm::StringRef QualifiedName,
                         llvm::StringRef CanonicalPath);
 
-  /// Returns the canonical include for symbol with \p QualifiedName, which is
-  /// declared in \p Header
-  llvm::StringRef mapHeader(llvm::StringRef Header,
+  /// Returns the canonical include for symbol with \p QualifiedName. \p Headers
+  /// is a stack of #includes that starts from the last included header (i.e.
+  /// header that defines the symbol) to the header file that is directly
+  /// included by the main file. When mapping headers, this skips files that are
+  /// not supposed to be #included directly e.g. .inc file.
+  llvm::StringRef mapHeader(llvm::ArrayRef<std::string> Headers,
                             llvm::StringRef QualifiedName) const;
 
 private:
   // A map from header patterns to header names. This needs to be mutable so
   // that we can match again a Regex in a const function member.
   // FIXME(ioeric): All the regexes we have so far are suffix matches. The
   // performance could be improved by allowing only suffix matches instead of
   // arbitrary regexes.

clangd/index/CanonicalIncludes.cpp

 }
 
 void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
                                          llvm::StringRef CanonicalPath) {
   this->SymbolMapping[QualifiedName] = CanonicalPath;
 }
 
 llvm::StringRef
-CanonicalIncludes::mapHeader(llvm::StringRef Header,
+CanonicalIncludes::mapHeader(llvm::ArrayRef<std::string> Headers,
                              llvm::StringRef QualifiedName) const {
+  assert(!Headers.empty());
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
     return SE->second;
   std::lock_guard<std::mutex> Lock(RegexMutex);
+  auto I = std::find_if(Headers.begin(), Headers.end(),
+                        [](llvm::StringRef Include) {
+                          // Skip .inc file whose including header file should
+                          // be #included instead.
+                          return !Include.endswith(".inc");
+                        });
+  if (I == Headers.end())
+    return Headers[0]; // Fallback to the defining header.
+  llvm::StringRef Header = *I;
   for (auto &Entry : RegexHeaderMappingTable) {
 #ifndef NDEBUG
     std::string Dummy;
     assert(Entry.first.isValid(Dummy) && "Regex should never be invalid!");
 #endif
     if (Entry.first.match(Header))
       return Entry.second;
   }
   return Header;
 }
 
 std::unique_ptr<CommentHandler>
 collectIWYUHeaderMaps(CanonicalIncludes *Includes) {
   class PragmaCommentHandler : public clang::CommentHandler {
   public:

clangd/index/SymbolCollector.cpp

bool shouldCollectIncludePath(index::SymbolKind Kind) {
   default:
     return false;
   }
 }
 
 /// Gets a canonical include (URI of the header or <header>  or "header") for
 /// header of \p Loc.
 /// Returns None if fails to get include header for \p Loc.
-/// FIXME: we should handle .inc files whose symbols are expected be exported by
-/// their containing headers.
 llvm::Optional<std::string>
 getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
                  SourceLocation Loc, const SymbolCollector::Options &Opts) {
-  llvm::StringRef FilePath = SM.getFilename(Loc);
+  std::vector<std::string> Headers;
+  // Collect all headers in the #include stack.
+  while (true) {
+    if (!Loc.isValid() || SM.isInMainFile(Loc))
+      break;
+    auto FilePath = SM.getFilename(Loc);
     if (FilePath.empty())
+      break;
+    Headers.push_back(FilePath);
+    Loc = SM.getIncludeLoc(SM.getFileID(Loc));
+  }
+  if (Headers.empty())
     return llvm::None;
+  llvm::StringRef Header = Headers[0];
   if (Opts.Includes) {
-    llvm::StringRef Mapped = Opts.Includes->mapHeader(FilePath, QName);
-    if (Mapped != FilePath)
-      return (Mapped.startswith("<") || Mapped.startswith("\""))
-                 ? Mapped.str()
-                 : ("\"" + Mapped + "\"").str();
+    Header = Opts.Includes->mapHeader(Headers, QName);
+    if (Header.startswith("<") || Header.startswith("\""))
+      return Header.str();
   }
-
-  return toURI(SM, SM.getFilename(Loc), Opts);
+  return toURI(SM, Header, Opts);
 }
 
 // Return the symbol location of the given declaration `D`.
 //
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
 llvm::Optional<SymbolLocation> getSymbolLocation(

unittests/clangd/SymbolCollectorTests.cpp

const std::string Header = R"(
     class Foo {};
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("Foo"), DeclURI(TestHeaderURI),
                                  IncludeHeader("\"the/good/header.h\""))));
 }
 
+TEST_F(SymbolCollectorTest, SkipIncFileWhenCanonicalizeHeaders) {
+  CollectorOpts.CollectIncludePath = true;
+  CanonicalIncludes Includes;
+  Includes.addMapping(TestHeaderName, "<canonical>");
+  CollectorOpts.Includes = &Includes;
+  auto IncFile = testPath("test.inc");
+  auto IncURI = URI::createFile(IncFile).toString();
+  InMemoryFileSystem->addFile(IncFile, 0,
+                              llvm::MemoryBuffer::getMemBuffer("class X {};"));
+  runSymbolCollector("#include \"test.inc\"\nclass Y {};", /*Main=*/"",
+                     /*ExtraArgs=*/{"-I", testRoot()});
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
+                                         IncludeHeader("<canonical>")),
+                                   AllOf(QName("Y"), DeclURI(TestHeaderURI),
+                                         IncludeHeader("<canonical>"))));
+}
+
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
     // Forward declarations of TagDecls.
     class C;
     struct S;
     union U;