Add document outline symbols from unnamed contexts, e.g. extern "C".

[ilya-golovenko]

Summary

It is necessary to traverse children of unnamed declaration contexts
to get symbols which are currently missing in document outline, e.g.:

extern "C" {
void foo();
}

[ilya-golovenko]

Trigger new build

[kadircet]

clang-tools-extra/clangd/FindSymbols.cpp

198–199

this will result in traversal of other declcontexts like block/capturedecls.

i think we should rather change the VisitKind to {No, Decl, Children} and then make shouldVisit return a VisitKindSet and take a Decl* instead of a NamedDecl.
Later on we should first check for LinkageSpecDecl and return Children only while keeping the rest of the logic the same (i.e. try to cast to nameddecl and return No if it fails.
Finally logic in here should also be adjusted accordingly to visit children and the decl itself separately.

Do you have other cases apart from extern'd symbols?

[ilya-golovenko]

clang-tools-extra/clangd/FindSymbols.cpp

198–199

I have only C++20 ExportDecl in mind. Initially I wanted to make changes similar like you propose, but tried to minimize the diff :-) I will re-work the fix according to your proposal.

[ilya-golovenko]

Changes afte code review, add test for export context

[ilya-golovenko]

@kadircet I decided to add OnlyChildren to enum VisitKind instead of making a VisitKindSet - this helped to minimize the changes in shouldVisit(). What do you think?

[ilya-golovenko]

Remove invalid comment

[kadircet]

In D84839#2187222, @ilya-golovenko wrote:

@kadircet I decided to add OnlyChildren to enum VisitKind instead of making a VisitKindSet - this helped to minimize the changes in shouldVisit(). What do you think?

SGTM.

clang-tools-extra/clangd/FindSymbols.cpp

203–212

ah this is complicated now, but at least for the sake of nestedness:

if (Visit == OnlyChildren)
  return traverseChildren;

// We must have the Decl in the result set.
auto *ND = llvm::cast<NamedDecl>(D);
auto Sym = declToSym(AST.getASTContext(), *ND);
if (!Sym) return;
Results.push_back(std::move(*Sym));

if(Visit == OnlyDecl) return;

assert(Visit == DeclAndChildren && "Unexpected VisitKind");
traverseChildren(Result.back()->children);

[ilya-golovenko]

Simplify logic in traverseDecl method

[kadircet]

thanks, lgtm!

let me know if i should land this for you

[ilya-golovenko]

In D84839#2189120, @kadircet wrote:

thanks, lgtm!

let me know if i should land this for you

Thank you much for the review.

I don't have commit access so could you please land it for me:
Ilya Golovenko <[e-mail]>

clang-tools-extra/clangd/FindSymbols.cpp

public:
   std::vector<DocumentSymbol> build() {
     std::vector<DocumentSymbol> Results;
     for (auto &TopLevel : AST.getLocalTopLevelDecls())
       traverseDecl(TopLevel, Results);
     return Results;
   }
 
 private:
-  enum class VisitKind { No, OnlyDecl, DeclAndChildren };
+  enum class VisitKind { No, OnlyDecl, OnlyChildren, DeclAndChildren };
 
   void traverseDecl(Decl *D, std::vector<DocumentSymbol> &Results) {
     if (auto *Templ = llvm::dyn_cast<TemplateDecl>(D)) {
       // TemplatedDecl might be null, e.g. concepts.
       if (auto *TD = Templ->getTemplatedDecl())
         D = TD;
     }
-    auto *ND = llvm::dyn_cast<NamedDecl>(D);
-    if (!ND)
-      return;
-    VisitKind Visit = shouldVisit(ND);
+
+    VisitKind Visit = shouldVisit(D);
     if (Visit == VisitKind::No)
       return;
-    llvm::Optional<DocumentSymbol> Sym = declToSym(AST.getASTContext(), *ND);
-    if (!Sym)
-      return;
+    if (Visit == VisitKind::OnlyChildren) {
+      traverseChildren(D, Results);
+    } else {
+      auto *ND = llvm::cast<NamedDecl>(D);
+      if (auto Sym = declToSym(AST.getASTContext(), *ND)) {
         if (Visit == VisitKind::DeclAndChildren)
-      traverseChildren(D, Sym->children);
+          traverseChildren(ND, Sym->children);
         Results.push_back(std::move(*Sym));
       }
+    }
+  }
 
   void traverseChildren(Decl *D, std::vector<DocumentSymbol> &Results) {
     auto *Scope = llvm::dyn_cast<DeclContext>(D);
     if (!Scope)
       return;
     for (auto *C : Scope->decls())
       traverseDecl(C, Results);
   }
 
-  VisitKind shouldVisit(NamedDecl *D) {
+  VisitKind shouldVisit(Decl *D) {
     if (D->isImplicit())
       return VisitKind::No;
 
+    if (llvm::isa<LinkageSpecDecl>(D) || llvm::isa<ExportDecl>(D))
+      return VisitKind::OnlyChildren;
+
+    if (!llvm::isa<NamedDecl>(D))
+      return VisitKind::No;
+
     if (auto Func = llvm::dyn_cast<FunctionDecl>(D)) {
       // Some functions are implicit template instantiations, those should be
       // ignored.
       if (auto *Info = Func->getTemplateSpecializationInfo()) {
         if (!Info->isExplicitInstantiationOrSpecialization())
           return VisitKind::No;
       }
       // Only visit the function itself, do not visit the children (i.e.
if (auto *TemplSpec = llvm::dyn_cast<VarTemplateSpecializationDecl>(D)) {
                    : VisitKind::OnlyDecl;
       return VisitKind::No;
     }
     // For all other cases, visit both the children and the decl.
     return VisitKind::DeclAndChildren;
   }
 
   ParsedAST &AST;
-};
+}; // namespace
 
 std::vector<DocumentSymbol> collectDocSymbols(ParsedAST &AST) {
   return DocumentOutline(AST).build();
 }
 } // namespace
 
 llvm::Expected<std::vector<DocumentSymbol>> getDocumentSymbols(ParsedAST &AST) {
   return collectDocSymbols(AST);
 }
 
 } // namespace clangd
 } // namespace clang

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

TU.AdditionalFiles["foo.h"] = R"cpp(
       )cpp";
   TU.Code = R"cpp(
       #include "foo.h"
       )cpp";
 
   EXPECT_THAT(getSymbols(TU.build()), IsEmpty());
 }
 
+TEST(DocumentSymbols, ExternContext) {
+  TestTU TU;
+  TU.Code = R"cpp(
+      extern "C" {
+      void foo();
+      class Foo {};
+      }
+      namespace ns {
+        extern "C" {
+        void bar();
+        class Bar {};
+        }
+      })cpp";
+
+  EXPECT_THAT(getSymbols(TU.build()),
+              ElementsAre(WithName("foo"), WithName("Foo"),
+                          AllOf(WithName("ns"),
+                                Children(WithName("bar"), WithName("Bar")))));
+}
+
+TEST(DocumentSymbols, ExportContext) {
+  TestTU TU;
+  TU.ExtraArgs = {"-std=c++20"};
+  TU.Code = R"cpp(
+      export module test;
+      export {
+      void foo();
+      class Foo {};
+      })cpp";
+
+  EXPECT_THAT(getSymbols(TU.build()),
+              ElementsAre(WithName("foo"), WithName("Foo")));
+}
+
 TEST(DocumentSymbols, NoLocals) {
   TestTU TU;
   TU.Code = R"cpp(
       void test(int FirstParam, int SecondParam) {
         struct LocalClass {};
         int local_var;
       })cpp";
   EXPECT_THAT(getSymbols(TU.build()), ElementsAre(WithName("test")));