[clangd] Skip inline namespace when collecting scopes for index symbols.

[ioeric]

Summary

Some STL symbols are defined in inline namespaces. For example,

namespace std {
inline namespace __cxx11 {
 typedef ... string;
}
}

Currently, this will be std::__cxx11::string; however, std::string is desired.

Inline namespaces are treated as transparent scopes. This
reflects the way they're most commonly used for lookup. Ideally we'd
include them, but at query time it's hard to find all the inline
namespaces to query: the preamble doesn't have a dedicated list.

[hokein]

clangd/index/SymbolCollector.cpp

69 ↗

(On Diff #132398)

There is a SuppressUnwrittenScope option in PrintingPolicy, I think we can probably use printQualifiedName with our customized policy (setting SuppressUnwrittenScope to true) here.

195 ↗

(On Diff #132398)

Consider moving to shouldFilterDecl? We also have a check if (ND->getDeclName().isEmpty()) there, which I assume does similar thing.

[sammccall]

Nice catch, and nice fix! Might be worth adding a motivating example to the patch description.

clangd/index/SymbolCollector.cpp

68 ↗

(On Diff #132398)

I'd expand on this for inline namespaces a little, because it's non-obvious. e.g.

// Inline namespaces are treated as transparent scopes.
// This reflects the way they're most commonly used for lookup.
// Ideally we'd include them, but at query time it's hard to find all the inline
// namespaces to query: the preamble doesn't have a dedicated list.

Conversely, I don't think you need to explicitly mention unscoped enums anymore, because the behavior for transparent scopes is the obvious one, and we shouldn't need any special code handling enums (see below suggestions).

71 ↗

(On Diff #132398)

if the condition is !Context->isTranslationUnit() you can skip the break inside, which I think reads more naturally. You'll never reach null - only TU can have a null parent I think.

71 ↗

(On Diff #132398)

uber-nit: I think DC is pretty common in clang to refer to a DeclContext - once I got used to it, it seems less ambiguous than Context. Up to you though.

73 ↗

(On Diff #132398)

I'm not sure this is always correct: at least clang accepts this code:

namespace X { extern "C++" { int y; }}

and you'll emit "y" instead of "X::y".

I think the check you want is

if (Context->isTransparentContext() || Context->isInlineNamespace())
  continue;

isTransparentContext will handle the Namespace and Enum cases as you do below, including the enum/enum class distinction.

(The code you have below is otherwise correct, I think - but a reader needs to think about more separate cases in order to see that)

76 ↗

(On Diff #132398)

With the changes suggested above, I think we only get to this point in these cases:

1. non-inline namespace
2. decl is in some other named scope (class, scoped enum, ...)

Currently case 2 is symbols we're not indexing: shouldFilterDecl() should be false. So this is a programming error. So I think we just want

Contexts.push_back(cast<NamespaceDecl>(Context)->getName());

which includes an assertion. Returning Expected seems weird here - we should never hit it unless a precondition is violated.

90 ↗

(On Diff #132398)

(nit: might be slightly more obvious just to write the for loop and avoid the special case, up to you)

113 ↗

(On Diff #132398)

Hmm, if this is ever hot-path, we may want to eventually combine "determine scope" and "shouldFilter" somewhat. shouldFilterDecl is doing much the same upwards-scope-traversal, and it seems pretty redundant.

Nothing to do for now though.

195 ↗

(On Diff #132398)

hmm, what case is this handling? should shouldFilterDecl catch it?

unittests/clangd/SymbolCollectorTests.cpp

326 ↗

(On Diff #132398)

you could consider modifying one of the testcases to have a weird linkage-spec-inside-namespace thing I mentioned :-)

[sammccall]

Doh, nevermind - SuppressUnwrittenScopes is way simpler. Thanks @hokein for catching!

[ilya-biryukov]

clangd/index/SymbolCollector.cpp

74 ↗

(On Diff #132398)

I may not know enough about the AST, sorry if the question is obvious.
TranslationUnitDecl is the root of the tree, but why should we stop at LinkageSpecDecl?

This code is probably going away per @hokein's comments.

195 ↗

(On Diff #132398)

Why do we skip names without identifiers? AFAIK, they are perfectly reasonable C++ entities: overloaded operators, constructors, etc.

[ioeric]

Addressed review comments.

clangd/index/SymbolCollector.cpp

69 ↗

(On Diff #132398)

This is perfect! Thank you!

73 ↗

(On Diff #132398)

In namespace X { extern "C++" { int y; }}, we would still want y instead of X::y since C-style symbol doesn't have scope. printQualifiedName also does the same thing printing y; I've added a test case for extern C.

I also realized we've been dropping C symbols in shouldFilterDecl and fixed it in the same patch.

74 ↗

(On Diff #132398)

Symbols in LinkageSpecDecl (i.e. extern "C") are C style symbols and do not have scopes. Also see my reply to Sam's related comment.

195 ↗

(On Diff #132398)

getName crashes for NamedDecl without identifier. I thought symbols without identifier are not interesting for global code completion, so I added this filter to avoid crash. But on a second thought, these symbols would still be useful for go-to-definition, for example.

This is no longer needed with printQualifiedName though.

[ioeric]

• clang-format

[ilya-biryukov]

clangd/index/SymbolCollector.cpp

73 ↗

(On Diff #132398)

I think we want X::y, not y.

Lookup still finds it inside the namespace and does not find it in the global scope. So for our purposes they are actually inside the namespace and have the qualified name of this namespace. Here's an example:

namespace ns {
extern "C" int foo();
}

void test() {
  ns::foo(); // ok
  foo(); // error
  ::foo(); // error
}

Note, however, that the tricky bit there is probably merging of the symbols, as it means symbols with the same USR (they are the same for all extern "c" declarations with the same name, right?) can have different qualified names and we won't know which one to choose.

namespace a {
 extern "C" int foo();
}
namespace b {
  extern "C" int foo(); // probably same USR, different qname. Also, possibly different types.
}

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

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
+#include "Logger.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/USRGeneration.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
bool shouldFilterDecl(const NamedDecl *ND, ASTContext *ASTCtx,
   // violations.
   if (ND->isInAnonymousNamespace())
     return true;
 
   // We only want:
   //   * symbols in namespaces or translation unit scopes (e.g. no class
   //     members)
   //   * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
-  auto InTopLevelScope =
-      hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl()));
+  auto InTopLevelScope = hasDeclContext(
+      anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
   if (match(decl(allOf(Opts.IndexMainFiles
                            ? decl()
                            : decl(unless(isExpansionInMainFile())),
                        anyOf(InTopLevelScope,
                              hasDeclContext(enumDecl(InTopLevelScope,
                                                      unless(isScoped())))))),
             *ND, *ASTCtx)
           .empty())
if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
     if (index::generateUSRForDecl(ND, USR))
       return true;
 
     auto ID = SymbolID(USR);
     if (Symbols.find(ID) != nullptr)
       return true;
 
     auto &SM = ND->getASTContext().getSourceManager();
-    std::string QName = ND->getQualifiedNameAsString();
+
+    std::string QName;
+    llvm::raw_string_ostream OS(QName);
+    PrintingPolicy Policy(ASTCtx->getLangOpts());
+    // Note that inline namespaces are treated as transparent scopes. This
+    // reflects the way they're most commonly used for lookup. Ideally we'd
+    // include them, but at query time it's hard to find all the inline
+    // namespaces to query: the preamble doesn't have a dedicated list.
+    Policy.SuppressUnwrittenScope = true;
+    ND->printQualifiedName(OS, Policy);
+    OS.flush();
 
     Symbol S;
     S.ID = std::move(ID);
     std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
     S.SymInfo = index::getSymbolInfo(D);
     std::string FilePath;
     S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, FilePath);
 

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

const std::string Header = R"(
     enum {
       Black
     };
     }
   )";
   runSymbolCollector(Header, /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
                                             QName("Green"), QName("Color2"),
-                                            QName("ns"),
-                                            QName("ns::Black")));
+                                            QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
   CollectorOpts.IndexMainFiles = false;
   const std::string Header = R"(
     struct {
       int a;
     } Foo;
TEST_F(SymbolCollectorTest, IgnoreClassMembers) {
   const std::string Main = R"(
     void Foo::g() {}
     void Foo::ssf() {}
   )";
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo")));
 }
 
+TEST_F(SymbolCollectorTest, Scopes) {
+  const std::string Header = R"(
+    namespace na {
+    class Foo {};
+    namespace nb {
+    class Bar {};
+    }
+    }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("na"), QName("na::nb"),
+                                   QName("na::Foo"), QName("na::nb::Bar")));
+}
+
+TEST_F(SymbolCollectorTest, ExternC) {
+  const std::string Header = R"(
+    extern "C" { class Foo {}; }
+    namespace na {
+    extern "C" { class Bar {}; }
+    }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("na"), QName("Foo"), QName("Bar")));
+}
+
+TEST_F(SymbolCollectorTest, SkipInlineNamespace) {
+  const std::string Header = R"(
+    namespace na {
+    inline namespace nb {
+    class Foo {};
+    }
+    }
+    namespace na {
+    // This is still inlined.
+    namespace nb {
+    class Bar {};
+    }
+    }
+  )";
+  runSymbolCollector(Header, /*Main=*/"");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(QName("na"), QName("na::nb"),
+                                   QName("na::Foo"), QName("na::Bar")));
+}
+
 TEST_F(SymbolCollectorTest, SymbolWithDocumentation) {
   const std::string Header = R"(
     namespace nx {
     /// Foo comment.
     int ff(int x, double y) { return 0; }
     }
   )";
   runSymbolCollector(Header, /*Main=*/"");