[clangd] Allow indexing of __reserved_names outside system headers

[sammccall]

Summary

The special handling for these names was added in
https://github.com/llvm/llvm-project/commit/055d8090d1d5137dab88533995e0c5d9b5390c28
and the motivation was the C++ standard library.

It turns out some projects like using these names anyway, in particular the
linux kernel. D153946 proposed making this a config option, but there are
some implementation issues with the config system.

As an alternative, this patch tweaks the heuristic so we only drop these symbols
in system headers. This does the right thing for linux and the C++ standard
library, at least.

Fixes https://github.com/clangd/clangd/issues/1680
Fixes https://github.com/llvm/llvm-project/issues/63862

[nridge]

Thanks! I agree that this no-configuration approach is nicer.

clang-tools-extra/clangd/index/SymbolCollector.h

struct Options {
     /// Collect symbols local to main-files, such as static functions, symbols
     /// inside an anonymous namespace, function-local classes and its member
     /// functions.
     bool CollectMainFileSymbols = true;
     /// Collect references to main-file symbols.
     bool CollectMainFileRefs = false;
     /// Collect symbols with reserved names, like __Vector_base.
     /// This does not currently affect macros (many like _WIN32 are important!)
+    /// This only affects system headers.
     bool CollectReserved = false;
     /// If set to true, SymbolCollector will collect doc for all symbols.
     /// Note that documents of symbols being indexed for completion will always
     /// be collected regardless of this option.
     bool StoreAllDocumentation = false;
     /// If this is set, only collect symbols/references from a file if
     /// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
     std::function<bool(const SourceManager &, FileID)> FileFilter = nullptr;

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

default:
     if (!isa<RecordDecl>(DeclCtx))
       return false;
   }
 
   // Avoid indexing internal symbols in protobuf generated headers.
   if (isPrivateProtoDecl(ND))
     return false;
   if (!Opts.CollectReserved &&
-      (hasReservedName(ND) || hasReservedScope(*ND.getDeclContext())))
+      (hasReservedName(ND) || hasReservedScope(*ND.getDeclContext())) &&
+      ASTCtx.getSourceManager().isInSystemHeader(ND.getLocation()))
     return false;
 
   return true;
 }
 
 const Decl *
 SymbolCollector::getRefContainer(const Decl *Enclosing,
                                  const SymbolCollector::Options &Opts) {

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

bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
     // This allows to override the "-xc++" with something else, i.e.
     // -xobjective-c++.
     Args.push_back(TestFileName);
 
     tooling::ToolInvocation Invocation(
         Args, Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
-    InMemoryFileSystem->addFile(TestHeaderName, 0,
-                                llvm::MemoryBuffer::getMemBuffer(HeaderCode));
-    InMemoryFileSystem->addFile(TestFileName, 0,
-                                llvm::MemoryBuffer::getMemBuffer(MainCode));
+    // Multiple calls to runSymbolCollector with different contents will fail
+    // to update the filesystem! Why are we sharing one across tests, anyway?
+    EXPECT_TRUE(InMemoryFileSystem->addFile(
+        TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode)));
+    EXPECT_TRUE(InMemoryFileSystem->addFile(
+        TestFileName, 0, llvm::MemoryBuffer::getMemBuffer(MainCode)));
     Invocation.run();
     Symbols = Factory->Collector->takeSymbols();
     Refs = Factory->Collector->takeRefs();
     Relations = Factory->Collector->takeRelations();
     return true;
   }
 
 protected:
EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(qName("X"), refCount(1)),
                                             AllOf(qName("C"), refCount(0))));
 }
 
 TEST_F(SymbolCollectorTest, Origin) {
   CollectorOpts.Origin = SymbolOrigin::Static;
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            Field(&Symbol::Origin, SymbolOrigin::Static)));
+  InMemoryFileSystem = new llvm::vfs::InMemoryFileSystem;
+  CollectorOpts.CollectMacro = true;
   runSymbolCollector("#define FOO", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
                            Field(&Symbol::Origin, SymbolOrigin::Static)));
 }
 
 TEST_F(SymbolCollectorTest, CollectMacros) {
   CollectorOpts.CollectIncludePath = true;
   Annotations Header(R"(
TEST_F(SymbolCollectorTest, NoCrashOnObjCMethodCStyleParam) {
   TU.build();
   // We mostly care about not crashing.
   EXPECT_THAT(TU.headerSymbols(),
               UnorderedElementsAre(qName("Foo"), qName("Foo::fun:")));
 }
 
 TEST_F(SymbolCollectorTest, Reserved) {
   const char *Header = R"cpp(
+    #pragma once
     void __foo();
     namespace _X { int secret; }
   )cpp";
 
   CollectorOpts.CollectReserved = true;
-  runSymbolCollector("", Header);
+  runSymbolCollector(Header, "");
   EXPECT_THAT(Symbols, UnorderedElementsAre(qName("__foo"), qName("_X"),
                                             qName("_X::secret")));
 
   CollectorOpts.CollectReserved = false;
-  runSymbolCollector("", Header); //
+  runSymbolCollector(Header, "");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(qName("__foo"), qName("_X"),
+                                            qName("_X::secret")));
+
+  // Ugly: for some reason we reuse the test filesystem across tests.
+  // You can't overwrite the same filename with new content!
+  InMemoryFileSystem = new llvm::vfs::InMemoryFileSystem;
+  runSymbolCollector("#pragma GCC system_header\n" + std::string(Header), "");
   EXPECT_THAT(Symbols, IsEmpty());
 }
 
 TEST_F(SymbolCollectorTest, Concepts) {
   const char *Header = R"cpp(
     template <class T>
     concept A = sizeof(T) <= 8;
   )cpp";