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

[sammccall]

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.

[sammccall]

unittests/clangd/FileIndexTests.cpp

214 ↗

(On Diff #193872)

I suspect we can replace most of these tests with TestTU - here I just changed the ones where the include guard would otherwise be needed.

[ioeric]

clangd/index/SymbolCollector.cpp

157 ↗

(On Diff #193872)

nit: no need for else?

unittests/clangd/FileIndexTests.cpp

214 ↗

(On Diff #193872)

This looks good to me.

I think this test case tried to explicitly test that preamble callback has the expected CanonicalIncludes. Using TestTU seems to achieve the same goal but makes the intention a bit less clear though.

unittests/clangd/TestTU.h

55 ↗

(On Diff #193872)

is this used?

[ioeric]

in case you missed this patch :)

[sammccall]

address review comments

clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h

void addPathSuffixMapping(llvm::StringRef Suffix,
                             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.
-  /// \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:
   /// A map from full include path to a canonical path.
   llvm::StringMap<std::string> FullPathMapping;
   /// A map from a suffix (one or components of a path) to a canonical path.
   llvm::StringMap<std::string> SuffixHeaderMapping;
   /// Maximum number of path components stored in a key of SuffixHeaderMapping.

clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp

 }
 
 void CanonicalIncludes::addSymbolMapping(llvm::StringRef QualifiedName,
                                          llvm::StringRef CanonicalPath) {
   this->SymbolMapping[QualifiedName] = CanonicalPath;
 }
 
 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())
     return MapIt->second;
 
   int Components = 1;
   for (auto It = llvm::sys::path::rbegin(Header),
             End = llvm::sys::path::rend(Header);

clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #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"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {
bool shouldCollectIncludePath(index::SymbolKind Kind) {
   case SK::Variable:
   case SK::EnumConstant:
     return true;
   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.
+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.
 std::pair<SymbolLocation::Position, SymbolLocation::Position>
 getTokenRange(SourceLocation TokLoc, const SourceManager &SM,
               const LangOptions &LangOpts) {
   auto CreatePosition = [&SM](SourceLocation Loc) {
     auto LSPLoc = sourceLocToPosition(SM, Loc);
bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
   const auto *CCS = SymbolCompletion.CreateCodeCompletionStringForMacro(
       *PP, *CompletionAllocator, *CompletionTUInfo);
   std::string Signature;
   std::string SnippetSuffix;
   getSignature(*CCS, &Signature, &SnippetSuffix);
 
   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;
   S.CompletionSnippetSuffix = SnippetSuffix;
   if (!Include.empty())
     S.IncludeHeaders.emplace_back(Include, 1);
 
   Symbols.insert(S);
const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
   std::string ReturnType = getReturnType(*CCS);
   S.ReturnType = ReturnType;
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
     // 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())
     S.IncludeHeaders.emplace_back(Include, 1);
 
   llvm::Optional<OpaqueType> TypeStorage;
   if (S.Flags & Symbol::IndexedForCodeCompletion) {
     TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, DynamicIndexIncludeInsertion) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer::Options Opts = ClangdServer::optsForTest();
   Opts.BuildDynamicSymbolIndex = true;
   ClangdServer Server(CDB, FS, DiagConsumer, Opts);
 
   FS.Files[testPath("foo_header.h")] = R"cpp(
+    #pragma once
     struct Foo {
        // Member doc
        int foo();
     };
   )cpp";
   const std::string FileContent(R"cpp(
     #include "foo_header.h"
     int Foo::foo() {

clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp

TEST(FileIndexTest, IncludeCollected) {
 
   auto Symbols = runFuzzyFind(M, "");
   EXPECT_THAT(Symbols, ElementsAre(_));
   EXPECT_THAT(Symbols.begin()->IncludeHeaders.front().IncludeHeader,
               "<the/good/header.h>");
 }
 
 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>");
 }
 
 TEST(FileIndexTest, TemplateParamsInLabel) {
   auto Source = R"cpp(
 template <class Ty>

 
 TEST(FileIndexTest, CollectMacros) {
   FileIndex M;
   update(M, "f", "#define CLANGD 1");
   EXPECT_THAT(runFuzzyFind(M, ""), Contains(QName("CLANGD")));
 }
 
 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
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
 #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"
 
 #include <memory>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Not;
 using testing::Pair;
 using testing::UnorderedElementsAre;
 using testing::UnorderedElementsAreArray;
 
 // GMock helpers for matching Symbol.
 MATCHER_P(Labeled, Label, "") {
   return (arg.Name + arg.Signature).str() == Label;
 }
 MATCHER_P(ReturnType, D, "") { return arg.ReturnType == D; }
 MATCHER_P(Doc, D, "") { return arg.Documentation == D; }
 MATCHER_P(Snippet, S, "") {
   return (arg.Name + arg.CompletionSnippetSuffix).str() == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
 MATCHER_P(TemplateArgs, TemplArgs, "") {
   return arg.TemplateSpecializationArgs == TemplArgs;
 }
 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);
 }
 MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
   return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
 }
 MATCHER_P(DeclRange, Pos, "") {
public:
   SymbolCollectorTest()
       : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem),
         TestHeaderName(testPath("symbol.h")),
         TestFileName(testPath("symbol.cc")) {
     TestHeaderURI = URI::create(TestHeaderName).toString();
     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(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
     auto Factory = llvm::make_unique<SymbolIndexActionFactory>(
         CollectorOpts, PragmaHandler.get());
 
EXPECT_THAT(Symbols,
                   QName("nx"),
                   AllOf(QName("nx::f"), Labeled("f()"), Snippet("f()")),
                   AllOf(QName("nx::ff"), Labeled("ff(int x, double y)"),
                         Snippet("ff(${1:int x}, ${2:double y})"))));
 }
 
 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,
               UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
 }
 
 #ifndef _WIN32
 TEST_F(SymbolCollectorTest, CanonicalSTLHeader) {
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");
+  // 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;
     union U;
 
     // Canonical declarations.
     class $cdecl[[C]] {};
     struct $sdecl[[S]] {};
EXPECT_THAT(
                 DefRange(Header.range("xdecl"))),
           AllOf(QName("U::y"), DeclURI(TestHeaderURI),
                 DeclRange(Header.range("ydecl")), DefURI(TestHeaderURI),
                 DefRange(Header.range("ydecl")))));
 }
 
 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))));
 }
 
 TEST_F(SymbolCollectorTest, UTF16Character) {
   // ö is 2-bytes.
   Annotations Header(/*Header=*/"class [[pörk]] {};");
TEST_F(SymbolCollectorTest, Origin) {
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            Field(&Symbol::Origin, SymbolOrigin::Static)));
 }
 
 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;
 
     MAC(p);
   )");
 
   Annotations Main(R"(

clang-tools-extra/trunk/unittests/clangd/TestTU.h

struct TestTU {
 
   // Extra arguments for the compiler invocation.
   std::vector<const char *> ExtraArgs;
 
   llvm::Optional<std::string> ClangTidyChecks;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
+  // Simulate a header guard of the header (using an #import directive).
+  bool ImplicitHeaderGuard = true;
+
   ParsedAST build() const;
   SymbolSlab headerSymbols() const;
   std::unique_ptr<SymbolIndex> index() const;
 };
 
 // Look up an index symbol by qualified name, which must be unique.
 const Symbol &findSymbol(const SymbolSlab &, llvm::StringRef QName);
 // Look up an AST symbol by qualified name, which must be unique and top-level.

clang-tools-extra/trunk/unittests/clangd/TestTU.cpp

 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/Utils.h"
 
 namespace clang {
 namespace clangd {
 
 ParsedAST TestTU::build() const {
   std::string FullFilename = testPath(Filename),
-              FullHeaderName = testPath(HeaderFilename);
+              FullHeaderName = testPath(HeaderFilename),
+              ImportThunk = testPath("import_thunk.h");
   std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
+  // We want to implicitly include HeaderFilename without messing up offsets.
+  // -include achieves this, but sometimes we want #import (to simulate a header
+  // guard without messing up offsets). In this case, use an intermediate file.
+  std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
     Cmd.push_back("-include");
-    Cmd.push_back(FullHeaderName.c_str());
+    Cmd.push_back(ImplicitHeaderGuard ? ImportThunk.c_str()
+                                      : FullHeaderName.c_str());
   }
   Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
   ParseInputs Inputs;
   Inputs.CompileCommand.Filename = FullFilename;
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+  Inputs.FS = buildTestFS({{FullFilename, Code},
+                           {FullHeaderName, HeaderCode},
+                           {ImportThunk, ThunkContents}});
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
     Inputs.Opts.SuggestMissingIncludes = true;
   auto CI = buildCompilerInvocation(Inputs);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =