[clangd] Add a flag to allow indexing of reserved identifiers

[nridge]

Summary

Some projects, like the Linux kernel, make extensive use of such
identifiers, making it a usability issue if they can't be indexed.

Fixes https://github.com/clangd/clangd/issues/1680

[nridge]

No tests yet, posting for early feedback.

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

233

I'm thinking no: the only time you'd want reserved identifiers from the standard library is if you're working on the standard library itself, but in that case the standard library source files would be part of the project and you'd be indexing them "normally", not via indexStandardLibrary() (so your configuration would be:

Index:
  ReservedIdentifiers: true
  StandardLibrary: false

and the StandardLibrary: false means this codepath is not taken).

clang-tools-extra/clangd/indexer/IndexerMain.cpp

48

I'm thinking we should, so that projects that want to enable this (like e.g. the kernel) can use a remote index if desired.

I'm just not sure if we have the machinery in place to use Config from clangd-indexer.

clang-tools-extra/clangd/refactor/Rename.cpp

239

Would it be inappropriate to set this in the SymbolCollector::Options constructor, instead of every place where we call it?

[sammccall]

Thanks for putting this together, it definitely seems like something people want.

My main concern is that the configuration has the wrong scope.
We're checking whether reserved-ident indexing is on for the TU we're indexing, but really we want to specify which headers we should index reserved-idents from.
If hell freezes over and the linux kernel starts using the C++ standard library one day, we want the preamble index to contain __free_pages() but not std::__variant_impl_helper. But the preamble indexing all runs under the same config.
(Also we more or less assume that all TUs will index a header the same way: if we've indexed foo.h under A.cpp, we won't reindex it under B.cpp even if the config is different. So we get "first-one-wins" behavior...)

I don't think we can afford to re-evaluate the config each time we switch between headers while indexing. (Stupid over-flexible config system is too slow... I've learned my lesson from that one!)
So I'm not really sure what to suggest...

• we could do it this way anyway and accept that it's basically only usable as a global flag: this is probably fine for the linux kernel as it's mostly isolated anyway (no standard library).
• we could add some rules (regexes?) to the config that describe which headers can have interesting symbols. This could be slow too, but at least cacheable, like the self-contained check
• maybe we can come up with some heuristics to improve the "no reserved names" rule, with or without configuration. (Maybe apply it to only -isystem headers? Linux uses angle-quoted includes, but not actually -isystem AFAICS)

What do you think? Is this a real concern or am I missing something?

clang-tools-extra/clangd/ConfigFragment.h

205

nit: it's the *symbols* we're indexing, based on whether their names are reserved identifiers.

207

For some reason I find the name "ReservedNames" a little clearer.

I tend to think of an "identifier" as a type of token at the lexer level, and a "name" as its role at the language level. But I don't think this actually matches any standard terminology, so no need to change it unless it also reads better to you.

[sammccall]

In D153946#4503585, @sammccall wrote:

• maybe we can come up with some heuristics to improve the "no reserved names" rule, with or without configuration. (Maybe apply it to only -isystem headers? Linux uses angle-quoted includes, but not actually -isystem AFAICS)

I tried this out, and it behaves well at least for linux & the C++ stdlib on my system. D155381 if you like the approach.

I have the feeling there may still be some exceptions, but we don't know about them yet and it sidesteps the config questions...

[nridge]

Yeah, I'm happy to go with D155381 instead.

In D153946#4503585, @sammccall wrote:

(Stupid over-flexible config system is too slow... I've learned my lesson from that one!)

The issues with the config system are not entirely obvious to me. If, by chance, you've written some sort of post-mortem / discussion of the shortcomings and how you might design a config system in the future, I'd be interested to read it.

[sammccall]

In D153946#4521576, @nridge wrote:

Yeah, I'm happy to go with D155381 instead.

In D153946#4503585, @sammccall wrote:

(Stupid over-flexible config system is too slow... I've learned my lesson from that one!)

The issues with the config system are not entirely obvious to me. If, by chance, you've written some sort of post-mortem / discussion of the shortcomings and how you might design a config system in the future, I'd be interested to read it.

I haven't, but this is a useful idea and I'd like to.

The main regret is making it too powerful/complicated without properly considering the consequences:

• the if conditions and the way we plug in fragments mean we really need to compute the config for each file we want to consider. Really, probably few conditions are used and something with simpler fs-hierarchical scope would be much easier to cache.
• to avoid having to do too much interpretation of the config as we repeatedly process the same file, we have the compile step - but this makes processing many *different* files in quick succession more expensive

As a result, we can *barely* use it to control background indexing (evaluating the config to know whether each file in the project should be indexed takes nontrivial time) and we can't use it for things like this where we want to configure per-header but this would end up significantly increasing indexing cost.

There are some other things, like the amount of plumbing that's needed for new settings. It would be good to reflect on the alternatives a bit again, though.

[nridge]

Abandoning in favour of D155381.

clang-tools-extra/clangd/Config.h

struct ExternalIndexSpec {
     std::string MountPoint;
   };
   /// Controls index behavior.
   struct {
     /// Whether this TU should be background-indexed.
     BackgroundPolicy Background = BackgroundPolicy::Build;
     ExternalIndexSpec External;
     bool StandardLibrary = true;
+    bool ReservedIdentifiers = false;
   } Index;
 
   enum class IncludesPolicy {
     /// Diagnose missing and unused includes.
     Strict,
     None,
   };
   /// Controls warnings and errors when parsing code.

clang-tools-extra/clangd/ConfigCompile.cpp

void compile(Fragment::IndexBlock &&F) {
     }
     if (F.External)
       compile(std::move(**F.External), F.External->Range);
     if (F.StandardLibrary)
       Out.Apply.push_back(
           [Val(**F.StandardLibrary)](const Params &, Config &C) {
             C.Index.StandardLibrary = Val;
           });
+    if (F.ReservedIdentifiers)
+      Out.Apply.push_back(
+          [Val(**F.ReservedIdentifiers)](const Params &, Config &C) {
+            C.Index.ReservedIdentifiers = Val;
+          });
   }
 
   void compile(Fragment::IndexBlock::ExternalBlock &&External,
                llvm::SMRange BlockRange) {
     if (External.Server && !Trusted) {
       diag(Error,
            "Remote index may not be specified by untrusted configuration. "
            "Copy this into user config to use it.",

clang-tools-extra/clangd/ConfigFragment.h

struct ExternalBlock {
       /// `123.1.1.1:13337`.
       std::optional<Located<std::string>> Server;
       /// Source root governed by this index. Default is the directory
       /// associated with the config fragment. Absolute in case of user config
       /// and relative otherwise. Should always use forward-slashes.
       std::optional<Located<std::string>> MountPoint;
     };
     std::optional<Located<ExternalBlock>> External;
-    // Whether the standard library visible from this file should be indexed.
-    // This makes all standard library symbols available, included or not.
+    /// Whether the standard library visible from this file should be indexed.
+    /// This makes all standard library symbols available, included or not.
     std::optional<Located<bool>> StandardLibrary;
+    /// Whether identifiers reserved for use by the implementation (e.g.
+    /// beginning with two underscores) should be indexed.
+    std::optional<Located<bool>> ReservedIdentifiers;
   };
   IndexBlock Index;
 
   /// Controls behavior of diagnostics (errors and warnings).
   struct DiagnosticsBlock {
     /// Diagnostic codes that should be suppressed.
     ///
     /// Valid values are:

clang-tools-extra/clangd/ConfigYAML.cpp

Dict.handle("External", [&](Node &N) {
       }
       F.External.emplace(std::move(External));
       F.External->Range = N.getSourceRange();
     });
     Dict.handle("StandardLibrary", [&](Node &N) {
       if (auto StandardLibrary = boolValue(N, "StandardLibrary"))
         F.StandardLibrary = *StandardLibrary;
     });
+    Dict.handle("ReservedIdentifiers", [&](Node &N) {
+      if (auto ReservedIdentifiers = boolValue(N, "ReservedIdentifiers"))
+        F.ReservedIdentifiers = *ReservedIdentifiers;
+    });
     Dict.parse(N);
   }
 
   void parse(Fragment::IndexBlock::ExternalBlock &F,
              Located<std::string> ExternalVal) {
     if (!llvm::StringRef(*ExternalVal).equals_insensitive("none")) {
       error("Only scalar value supported for External is 'None'",
             ExternalVal.Range);

clang-tools-extra/clangd/Hover.cpp

void enhanceFromIndex(HoverInfo &Hover, const NamedDecl &ND,
                       const SymbolIndex *Index) {
   assert(&ND == getDeclForComment(&ND));
   // We only add documentation, so don't bother if we already have some.
   if (!Hover.Documentation.empty() || !Index)
     return;
 
   // Skip querying for non-indexable symbols, there's no point.
   // We're searching for symbols that might be indexed outside this main file.
-  if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(),
-                                            SymbolCollector::Options(),
+  SymbolCollector::Options Opts;
+  Opts.CollectReserved = Config::current().Index.ReservedIdentifiers;
+  if (!SymbolCollector::shouldCollectSymbol(ND, ND.getASTContext(), Opts,
                                             /*IsMainFileOnly=*/false))
     return;
   auto ID = getSymbolID(&ND);
   if (!ID)
     return;
   LookupRequest Req;
   Req.IDs.insert(ID);
   Index->lookup(Req, [&](const Symbol &S) {

clang-tools-extra/clangd/XRefs.cpp

 //===--- XRefs.cpp -----------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "XRefs.h"
 #include "AST.h"
+#include "Config.h"
 #include "FindSymbols.h"
 #include "FindTarget.h"
 #include "Headers.h"
 #include "HeuristicResolver.h"
 #include "IncludeCleaner.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "Quality.h"
if (PerToken) {
       if (!Locs.empty() && Locs.front() != Loc)
         Locs.clear(); // First token doesn't match, assume our guess was wrong.
     }
     if (Locs.empty())
       Locs.push_back(Loc);
 
     SymbolCollector::Options CollectorOpts;
     CollectorOpts.CollectMainFileSymbols = true;
+    CollectorOpts.CollectReserved = Config::current().Index.ReservedIdentifiers;
     for (SourceLocation L : Locs) {
       L = SM.getFileLoc(L);
       if (const auto *Tok = TB.spelledTokenAt(L))
         References.push_back(
             {*Tok, Roles,
              SymbolCollector::getRefContainer(ASTNode.Parent, CollectorOpts)});
     }
     return true;

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

if (!Digest)
       return false;
     auto D = ShardVersionsSnapshot.find(*AbsPath);
     if (D != ShardVersionsSnapshot.end() && D->second.Digest == Digest &&
         !D->second.HadErrors)
       return false; // Skip files that haven't changed, without errors.
     return true;
   };
   IndexOpts.CollectMainFileRefs = true;
+  IndexOpts.CollectReserved = Config::current().Index.ReservedIdentifiers;
 
   IndexFileIn Index;
   auto Action = createStaticIndexingAction(
       IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); },
       [&](RefSlab R) { Index.Refs = std::move(R); },
       [&](RelationSlab R) { Index.Relations = std::move(R); },
       [&](IncludeGraph IG) { Index.Sources = std::move(IG); });
 

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

 //===--- FileIndex.cpp - Indexes for files. ------------------------ C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FileIndex.h"
 #include "CollectMacros.h"
+#include "Config.h"
 #include "ParsedAST.h"
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "index/MemIndex.h"
 #include "index/Merge.h"
 #include "index/Ref.h"
 #include "index/Relation.h"
 #include "index/Serialization.h"
SlabTuple indexSymbols(ASTContext &AST, Preprocessor &PP,
   CollectorOpts.CollectIncludePath = true;
   CollectorOpts.Includes = &Includes;
   CollectorOpts.CountReferences = false;
   CollectorOpts.Origin =
       IsIndexMainAST ? SymbolOrigin::Open : SymbolOrigin::Preamble;
   CollectorOpts.CollectMainFileRefs = CollectMainFileRefs;
   // We want stdlib implementation details in the index only if we've opened the
   // file in question. This does means xrefs won't work, though.
-  CollectorOpts.CollectReserved = IsIndexMainAST;
+  CollectorOpts.CollectReserved =
+      IsIndexMainAST || Config::current().Index.ReservedIdentifiers;
 
   index::IndexingOptions IndexOpts;
   // We only need declarations, because we don't count references.
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::DeclarationsOnly;
   // We index function-local classes and its member functions only.
   IndexOpts.IndexFunctionLocals = true;
   if (IsIndexMainAST) {

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

SymbolSlab indexStandardLibrary(llvm::StringRef HeaderSources,
   }
 
   SymbolCollector::Options IndexOpts;
   IndexOpts.Origin = SymbolOrigin::StdLib;
   IndexOpts.CollectMainFileSymbols = false;
   IndexOpts.CollectMainFileRefs = false;
   IndexOpts.CollectMacro = true;
   IndexOpts.StoreAllDocumentation = true;
+  // FIXME: Should we respect the Index.ReservedIdentifiers config here?
   // Sadly we can't use IndexOpts.FileFilter to restrict indexing scope.
   // Files from outside the StdLibLocation may define true std symbols anyway.
   // We end up "blessing" such headers, and can only do that by indexing
   // everything first.
 
   // Refs, relations, include graph in the stdlib mostly aren't useful.
   auto Action = createStaticIndexingAction(
       IndexOpts, [&](SymbolSlab S) { Symbols = std::move(S); }, nullptr,

clang-tools-extra/clangd/indexer/IndexerMain.cpp

 
 class IndexActionFactory : public tooling::FrontendActionFactory {
 public:
   IndexActionFactory(IndexFileIn &Result) : Result(Result) {}
 
   std::unique_ptr<FrontendAction> create() override {
     SymbolCollector::Options Opts;
     Opts.CountReferences = true;
+    // FIXME: Do we have access to Config here? If so, should we respect
+    // Index.ReservedIdentifiers?
     Opts.FileFilter = [&](const SourceManager &SM, FileID FID) {
       const auto F = SM.getFileEntryRefForID(FID);
       if (!F)
         return false; // Skip invalid files.
       auto AbsPath = getCanonicalPath(*F, SM);
       if (!AbsPath)
         return false; // Skip files without absolute path.
       std::lock_guard<std::mutex> Lock(FilesMu);

clang-tools-extra/clangd/refactor/Rename.cpp

 //===--- Rename.cpp - Symbol-rename refactorings -----------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "refactor/Rename.h"
 #include "AST.h"
+#include "Config.h"
 #include "FindTarget.h"
 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "clang/AST/ASTContext.h"
bool DeclaredInMainFile =
       isInsideMainFile(RenameDecl.getBeginLoc(), ASTCtx.getSourceManager());
   bool IsMainFileOnly = true;
   if (MainFileIsHeader)
     // main file is a header, the symbol can't be main file only.
     IsMainFileOnly = false;
   else if (!DeclaredInMainFile)
     IsMainFileOnly = false;
   // If the symbol is not indexable, we disallow rename.
+  SymbolCollector::Options Options;
+  Options.CollectReserved = Config::current().Index.ReservedIdentifiers;
   if (!SymbolCollector::shouldCollectSymbol(
-          RenameDecl, RenameDecl.getASTContext(), SymbolCollector::Options(),
-          IsMainFileOnly))
+          RenameDecl, RenameDecl.getASTContext(), Options, IsMainFileOnly))
     return ReasonToReject::NonIndexable;
 
   return std::nullopt;
 }
 
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
     switch (Reason) {