[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

52

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

54

headers are paths, not <quoted>, right?

55

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"

60

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 the include stack: Headers.front() is the file declaring the
+  /// symbol, and Headers.back() is the main 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

 //===-- CanonicalIncludes.h - remap #inclue headers--------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CanonicalIncludes.h"
+#include "../Headers.h"
+#include "clang/Driver/Types.h"
 #include "llvm/Support/Regex.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 const char IWYUPragma[] = "// IWYU pragma: private, include ";
 } // namespace
 

 }
 
 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 declaring header.
+  StringRef Header = *I;
+  if (!isLiteralInclude(Header)) {
+    // If Header is not expected be included (e.g. .cc file), we fall back to
+    // the declaring header.
+    StringRef Ext = llvm::sys::path::extension(Header).trim('.');
+    // Include-able headers must have precompile type.
+    if (!Ext.empty() && !driver::types::onlyPrecompileType(
+                            driver::types::lookupTypeForExtension(Ext)))
+      return Headers[0];
+  }
   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;
   }
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
       StringRef Text =
           Lexer::getSourceText(CharSourceRange::getCharRange(Range),
                                PP.getSourceManager(), PP.getLangOpts());
       if (!Text.consume_front(IWYUPragma))
         return false;
       // FIXME(ioeric): resolve the header and store actual file path. For now,
       // we simply assume the written header is suitable to be #included.
       Includes->addMapping(PP.getSourceManager().getFilename(Range.getBegin()),
-                           (Text.startswith("<") || Text.startswith("\""))
-                               ? Text.str()
+                           isLiteralInclude(Text) ? Text.str()
                                                   : ("\"" + Text + "\"").str());
       return false;
     }
 
   private:
     CanonicalIncludes *const Includes;
   };
   return llvm::make_unique<PragmaCommentHandler>(Includes);
 }

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 the #include stack.
+  while (true) {
+    if (!Loc.isValid())
+      break;
+    auto FilePath = SM.getFilename(Loc);
     if (FilePath.empty())
+      break;
+    Headers.push_back(FilePath);
+    if (SM.isInMainFile(Loc))
+      break;
+    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

public:
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
                           const std::vector<std::string> &ExtraArgs = {}) {
     llvm::IntrusiveRefCntPtr<FileManager> Files(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
     auto Factory = llvm::make_unique<SymbolIndexActionFactory>(
         CollectorOpts, PragmaHandler.get());
 
-    std::vector<std::string> Args = {"symbol_collector", "-fsyntax-only",
-                                     "-std=c++11",       "-include",
-                                     TestHeaderName,     TestFileName};
+    std::vector<std::string> Args = {
+        "symbol_collector", "-fsyntax-only", "-xc++",     "-std=c++11",
+        "-include",         TestHeaderName,  TestFileName};
     Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
     tooling::ToolInvocation Invocation(
         Args,
         Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
     InMemoryFileSystem->addFile(TestHeaderName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(HeaderCode));
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, MainFileIsHeaderWhenSkipIncFile) {
+  CollectorOpts.CollectIncludePath = true;
+  CanonicalIncludes Includes;
+  CollectorOpts.Includes = &Includes;
+  TestFileName = testPath("main.h");
+  TestFileURI = URI::createFile(TestFileName).toString();
+  auto IncFile = testPath("test.inc");
+  auto IncURI = URI::createFile(IncFile).toString();
+  InMemoryFileSystem->addFile(IncFile, 0,
+                              llvm::MemoryBuffer::getMemBuffer("class X {};"));
+  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+                     /*ExtraArgs=*/{"-I", testRoot()});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
+                                                  IncludeHeader(TestFileURI))));
+}
+
+TEST_F(SymbolCollectorTest, FallbackToIncFileWhenIncludingFileIsCC) {
+  CollectorOpts.CollectIncludePath = true;
+  CanonicalIncludes Includes;
+  CollectorOpts.Includes = &Includes;
+  auto IncFile = testPath("test.inc");
+  auto IncURI = URI::createFile(IncFile).toString();
+  InMemoryFileSystem->addFile(IncFile, 0,
+                              llvm::MemoryBuffer::getMemBuffer("class X {};"));
+  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+                     /*ExtraArgs=*/{"-I", testRoot()});
+  EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
+                                                  IncludeHeader(IncURI))));
+}
+
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
     // Forward declarations of TagDecls.
     class C;
     struct S;
     union U;