[clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

sam-mccall · sam-mccall · commit 62e2472321b7 · 2019-04-17T10:36:02.000Z
Summary: We do have some reports of include insertion behaving badly in some codebases. Requiring header guards both makes sense in principle, and is likely to disable this "nice-to-have" feature in codebases where headers don't follow the expected pattern. With this we can drop some other heuristics, such as looking at file extensions to detect known non-headers - implementation files have no guards. One wrinkle here is #import - objc headers may not have guards because they're intended to be used via #import. If the header is the main file or is #included, we won't collect locations - merge should take care of this if we see the file #imported somewhere. Seems likely to be OK. Headers which have a canonicalization (stdlib, IWYU) are exempt from this check. *.inc files continue to be handled by looking up to the including file. This patch also adds *.def here - tablegen wants this pattern too. In terms of code structure, the division between SymbolCollector and CanonicalIncludes has shifted: SymbolCollector is responsible for more. This is because SymbolCollector has all the SourceManager/HeaderSearch access needed for checking for guards, and we interleave these checks with the *.def checks in a loop (potentially). We could hand all the info into CanonicalIncludes and put the logic there if that's preferable. Reviewers: ioeric Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D60316 llvm-svn: 358571
diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
@@ -37,31 +37,12 @@ void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
 }
 
 llvm::StringRef
-CanonicalIncludes::mapHeader(llvm::ArrayRef<std::string> Headers,
+CanonicalIncludes::mapHeader(llvm::StringRef Header,
                              llvm::StringRef QualifiedName) const {
-  assert(!Headers.empty());
+  assert(!Header.empty());
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
     return SE->second;
-  // Find the first header such that the extension is not '.inc', and isn't a
-  // recognized non-header file
-  auto I = llvm::find_if(Headers, [](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.
-  llvm::StringRef Header = *I;
-  // If Header is not expected be included (e.g. .cc file), we fall back to
-  // the declaring header.
-  llvm::StringRef Ext = llvm::sys::path::extension(Header).trim('.');
-  // Include-able headers must have precompile type. Treat files with
-  // non-recognized extenstions (TY_INVALID) as headers.
-  auto ExtType = driver::types::lookupTypeForExtension(Ext);
-  if ((ExtType != driver::types::TY_INVALID) &&
-      !driver::types::onlyPrecompileType(ExtType))
-    return Headers[0];
 
   auto MapIt = FullPathMapping.find(Header);
   if (MapIt != FullPathMapping.end())
diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.h b/clang-tools-extra/clangd/index/CanonicalIncludes.h
@@ -50,9 +50,9 @@ class CanonicalIncludes {
                         llvm::StringRef CanonicalPath);
 
   /// 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,
+  /// \p Header is the file the declaration was reachable from.
+  /// Header itself will be returned if there is no relevant mapping.
+  llvm::StringRef mapHeader(llvm::StringRef Header,
                             llvm::StringRef QualifiedName) const;
 
 private:
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -25,6 +25,7 @@
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/USRGeneration.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -127,34 +128,46 @@ bool shouldCollectIncludePath(index::SymbolKind Kind) {
   }
 }
 
-/// 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.
+bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
+                           const Preprocessor &PP) {
+  const FileEntry *FE = SM.getFileEntryForID(FID);
+  if (!FE)
+    return false;
+  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
+}
+
+/// Gets a canonical include (URI of the header or <header> or "header") for
+/// header of \p FID (which should usually be the *expansion* file).
+/// Returns None if includes should not be inserted for this file.
 llvm::Optional<std::string>
 getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
-                 SourceLocation Loc, const SymbolCollector::Options &Opts) {
-  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 None;
-  llvm::StringRef Header = Headers[0];
+                 const Preprocessor &PP, FileID FID,
+                 const SymbolCollector::Options &Opts) {
+  const FileEntry *FE = SM.getFileEntryForID(FID);
+  if (!FE || FE->getName().empty())
+    return llvm::None;
+  llvm::StringRef Filename = FE->getName();
+  // If a file is mapped by canonical headers, use that mapping, regardless
+  // of whether it's an otherwise-good header (header guards etc).
   if (Opts.Includes) {
-    Header = Opts.Includes->mapHeader(Headers, QName);
-    if (Header.startswith("<") || Header.startswith("\""))
-      return Header.str();
+    llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
+    // If we had a mapping, always use it.
+    if (Canonical.startswith("<") || Canonical.startswith("\""))
+      return Canonical.str();
+    if (Canonical != Filename)
+      return toURI(SM, Canonical, Opts);
+  }
+  if (!isSelfContainedHeader(FID, SM, PP)) {
+    // A .inc or .def file is often included into a real header to define
+    // symbols (e.g. LLVM tablegen files).
+    if (Filename.endswith(".inc") || Filename.endswith(".def"))
+      return getIncludeHeader(QName, SM, PP,
+                              SM.getFileID(SM.getIncludeLoc(FID)), Opts);
+    // Conservatively refuse to insert #includes to files without guards.
+    return llvm::None;
   }
-  return toURI(SM, Header, Opts);
+  // Standard case: just insert the file itself.
+  return toURI(SM, Filename, Opts);
 }
 
 // Return the symbol range of the token at \p TokLoc.
@@ -439,8 +452,9 @@ bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
-    if (auto Header = getIncludeHeader(Name->getName(), SM,
-                                       SM.getExpansionLoc(DefLoc), Opts))
+    if (auto Header =
+            getIncludeHeader(Name->getName(), SM, *PP,
+                             SM.getDecomposedExpansionLoc(DefLoc).first, Opts))
       Include = std::move(*Header);
   }
   S.Signature = Signature;
@@ -588,7 +602,8 @@ const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
     // Use the expansion location to get the #include header since this is
     // where the symbol is exposed.
     if (auto Header = getIncludeHeader(
-            QName, SM, SM.getExpansionLoc(ND.getLocation()), Opts))
+            QName, SM, *PP,
+            SM.getDecomposedExpansionLoc(ND.getLocation()).first, Opts))
       Include = std::move(*Header);
   }
   if (!Include.empty())
diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -684,6 +684,7 @@ TEST(CompletionTest, DynamicIndexIncludeInsertion) {
   ClangdServer Server(CDB, FS, DiagConsumer, Opts);
 
   FS.Files[testPath("foo_header.h")] = R"cpp(
+    #pragma once
     struct Foo {
        // Member doc
        int foo();
diff --git a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
@@ -212,33 +212,11 @@ TEST(FileIndexTest, IncludeCollected) {
 }
 
 TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
-  FileIndex Index;
-  const std::string Header = R"cpp(
-    class Foo {};
-  )cpp";
-  auto MainFile = testPath("foo.cpp");
-  auto HeaderFile = testPath("algorithm");
-  std::vector<const char *> Cmd = {"clang", "-xc++", MainFile.c_str(),
-                                   "-include", HeaderFile.c_str()};
-  // Preparse ParseInputs.
-  ParseInputs PI;
-  PI.CompileCommand.Directory = testRoot();
-  PI.CompileCommand.Filename = MainFile;
-  PI.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
-  PI.Contents = "";
-  PI.FS = buildTestFS({{MainFile, ""}, {HeaderFile, Header}});
+  TestTU TU;
+  TU.HeaderCode = "class Foo{};";
+  TU.HeaderFilename = "algorithm";
 
-  // Prepare preamble.
-  auto CI = buildCompilerInvocation(PI);
-  auto PreambleData =
-      buildPreamble(MainFile, *buildCompilerInvocation(PI),
-                    /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
-                    /*StoreInMemory=*/true,
-                    [&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP,
-                        const CanonicalIncludes &Includes) {
-                      Index.updatePreamble(MainFile, Ctx, PP, Includes);
-                    });
-  auto Symbols = runFuzzyFind(Index, "");
+  auto Symbols = runFuzzyFind(*TU.index(), "");
   EXPECT_THAT(Symbols, ElementsAre(_));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders.front().IncludeHeader,
               "<algorithm>");
@@ -369,50 +347,23 @@ TEST(FileIndexTest, CollectMacros) {
 }
 
 TEST(FileIndexTest, ReferencesInMainFileWithPreamble) {
-  const std::string Header = R"cpp(
-    class Foo {};
-  )cpp";
+  TestTU TU;
+  TU.HeaderCode = "class Foo{};";
   Annotations Main(R"cpp(
     #include "foo.h"
     void f() {
       [[Foo]] foo;
     }
   )cpp");
-  auto MainFile = testPath("foo.cpp");
-  auto HeaderFile = testPath("foo.h");
-  std::vector<const char *> Cmd = {"clang", "-xc++", MainFile.c_str()};
-  // Preparse ParseInputs.
-  ParseInputs PI;
-  PI.CompileCommand.Directory = testRoot();
-  PI.CompileCommand.Filename = MainFile;
-  PI.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
-  PI.Contents = Main.code();
-  PI.FS = buildTestFS({{MainFile, Main.code()}, {HeaderFile, Header}});
-
-  // Prepare preamble.
-  auto CI = buildCompilerInvocation(PI);
-  auto PreambleData =
-      buildPreamble(MainFile, *buildCompilerInvocation(PI),
-                    /*OldPreamble=*/nullptr, tooling::CompileCommand(), PI,
-                    /*StoreInMemory=*/true,
-                    [&](ASTContext &Ctx, std::shared_ptr<Preprocessor> PP,
-                        const CanonicalIncludes &) {});
-  // Build AST for main file with preamble.
-  auto AST =
-      ParsedAST::build(createInvocationFromCommandLine(Cmd), PreambleData,
-                       llvm::MemoryBuffer::getMemBufferCopy(Main.code()), PI.FS,
-                       /*Index=*/nullptr, ParseOptions());
-  ASSERT_TRUE(AST);
+  TU.Code = Main.code();
+  auto AST = TU.build();
   FileIndex Index;
-  Index.updateMain(MainFile, *AST);
-
-  auto Foo = findSymbol(TestTU::withHeaderCode(Header).headerSymbols(), "Foo");
-  RefsRequest Request;
-  Request.IDs.insert(Foo.ID);
+  Index.updateMain(testPath(TU.Filename), AST);
 
   // Expect to see references in main file, references in headers are excluded
   // because we only index main AST.
-  EXPECT_THAT(getRefs(Index, Foo.ID), RefsAre({RefRange(Main.range())}));
+  EXPECT_THAT(getRefs(Index, findSymbol(TU.headerSymbols(), "Foo").ID),
+              RefsAre({RefRange(Main.range())}));
 }
 
 } // namespace
diff --git a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-more-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -33,7 +34,7 @@ namespace {
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Not;
@@ -58,6 +59,7 @@ MATCHER_P(DeclURI, P, "") {
   return StringRef(arg.CanonicalDeclaration.FileURI) == P;
 }
 MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(IncludeHeader, "") { return !arg.IncludeHeaders.empty(); }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
          (arg.IncludeHeaders.begin()->IncludeHeader == P);
@@ -249,6 +251,8 @@ class SymbolCollectorTest : public ::testing::Test {
     TestFileURI = URI::create(TestFileName).toString();
   }
 
+  // Note that unlike TestTU, no automatic header guard is added.
+  // HeaderCode should start with #pragma once to be treated as modular.
   bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
                           const std::vector<std::string> &ExtraArgs = {}) {
     llvm::IntrusiveRefCntPtr<FileManager> Files(
@@ -920,7 +924,7 @@ TEST_F(SymbolCollectorTest, Snippet) {
 
 TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
   CollectorOpts.CollectIncludePath = true;
-  runSymbolCollector("class Foo {};", /*Main=*/"");
+  runSymbolCollector("#pragma once\nclass Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("Foo"), DeclURI(TestHeaderURI))));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders,
@@ -1020,53 +1024,54 @@ TEST_F(SymbolCollectorTest, SkipIncFileWhenCanonicalizeHeaders) {
 
 TEST_F(SymbolCollectorTest, MainFileIsHeaderWhenSkipIncFile) {
   CollectorOpts.CollectIncludePath = true;
-  CanonicalIncludes Includes;
-  CollectorOpts.Includes = &Includes;
-  TestFileName = testPath("main.h");
+  // To make this case as hard as possible, we won't tell clang main is a
+  // header. No extension, no -x c++-header.
+  TestFileName = testPath("no_ext_main");
   TestFileURI = URI::create(TestFileName).toString();
   auto IncFile = testPath("test.inc");
   auto IncURI = URI::create(IncFile).toString();
   InMemoryFileSystem->addFile(IncFile, 0,
                               llvm::MemoryBuffer::getMemBuffer("class X {};"));
-  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+  runSymbolCollector("", R"cpp(
+    // Can't use #pragma once in a main file clang doesn't think is a header.
+    #ifndef MAIN_H_
+    #define MAIN_H_
+    #include "test.inc"
+    #endif
+  )cpp",
                      /*ExtraArgs=*/{"-I", testRoot()});
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
                                                   IncludeHeader(TestFileURI))));
 }
 
-TEST_F(SymbolCollectorTest, MainFileIsHeaderWithoutExtensionWhenSkipIncFile) {
+TEST_F(SymbolCollectorTest, IncFileInNonHeader) {
   CollectorOpts.CollectIncludePath = true;
-  CanonicalIncludes Includes;
-  CollectorOpts.Includes = &Includes;
-  TestFileName = testPath("no_ext_main");
+  TestFileName = testPath("main.cc");
   TestFileURI = URI::create(TestFileName).toString();
   auto IncFile = testPath("test.inc");
   auto IncURI = URI::create(IncFile).toString();
   InMemoryFileSystem->addFile(IncFile, 0,
                               llvm::MemoryBuffer::getMemBuffer("class X {};"));
-  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+  runSymbolCollector("", R"cpp(
+    #include "test.inc"
+  )cpp",
                      /*ExtraArgs=*/{"-I", testRoot()});
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
-                                                  IncludeHeader(TestFileURI))));
+                                                  Not(IncludeHeader()))));
 }
 
-TEST_F(SymbolCollectorTest, FallbackToIncFileWhenIncludingFileIsCC) {
-  CollectorOpts.CollectIncludePath = true;
-  CanonicalIncludes Includes;
-  CollectorOpts.Includes = &Includes;
-  auto IncFile = testPath("test.inc");
-  auto IncURI = URI::create(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, NonModularHeader) {
+  auto TU = TestTU::withHeaderCode("int x();");
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
+
+  TU.ImplicitHeaderGuard = false;
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader())));
 }
 
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
+    #pragma once
     // Forward declarations of TagDecls.
     class C;
     struct S;
@@ -1100,7 +1105,8 @@ TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
 
 TEST_F(SymbolCollectorTest, ClassForwardDeclarationIsCanonical) {
   CollectorOpts.CollectIncludePath = true;
-  runSymbolCollector(/*Header=*/"class X;", /*Main=*/"class X {};");
+  runSymbolCollector(/*Header=*/"#pragma once\nclass X;",
+                     /*Main=*/"class X {};");
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(
                            QName("X"), DeclURI(TestHeaderURI),
                            IncludeHeader(TestHeaderURI), DefURI(TestFileURI))));
@@ -1167,6 +1173,7 @@ TEST_F(SymbolCollectorTest, Origin) {
 TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
+    #pragma once
     #define X 1
     #define $mac[[MAC]](x) int x
     #define $used[[USED]](y) float y;
diff --git a/clang-tools-extra/unittests/clangd/TestTU.cpp b/clang-tools-extra/unittests/clangd/TestTU.cpp
diff --git a/clang-tools-extra/unittests/clangd/TestTU.h b/clang-tools-extra/unittests/clangd/TestTU.h
