[clangd] Handle enumerators in named, unscoped enums similarly to scoped enums

[malaperle]

Summary

For enumerators in unscoped enums that have names, even if they are not
scoped, we add the enum name to the scope so that users can find the
enumerators when fully qualifying them, for example: MyEnum::Enumerator.
This makes it more consistent with how the code is visually laid out.

Signed-off-by: Marc-Andre Laperle <[e-mail]>

[ilya-biryukov]

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

[malaperle]

In D47223#1109247, @ilya-biryukov wrote:

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

I'll wait to see what others think before changing it. But I feel it's a bit odd that completion and workspace symbols would be inconsistent. I'd rather have it that A, ::A, and Foo::A work for both completion and workspace. Maybe it would complicate things too much...

[ilya-biryukov]

But I feel it's a bit odd that completion and workspace symbols would be inconsistent.

It does not seem that odd to me. Completion is something that should follow the language rules more closely, i.e. we don't want to deviate from sema completions too much. While workspaceSymbol is a user-facing feature and we probably want users to type as little as possible there.
E.g. not showing namespace_name::A in completion seems bad to me, since people are used to referring to the enumerators this way and sema completion gives you those results.

[ioeric]

In D47223#1110571, @malaperle wrote:

In D47223#1109247, @ilya-biryukov wrote:

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

It's one of those cases where code completion and workspace symbol search seem to want different results :-(
I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to Index::fuzzyFind on whether we need to match enum scopes or not.
+@ioeric, +@sammccall, WDYT?

I'll wait to see what others think before changing it. But I feel it's a bit odd that completion and workspace symbols would be inconsistent. I'd rather have it that A, ::A, and Foo::A work for both completion and workspace. Maybe it would complicate things too much...

Having "wrong" names in workspaceSymbol (e.g. ::A instead of ::Foo::A) and missing results in code completions (e.g. ::A missing in ::^) seem equally bad.

I think the fundamental problem here is that C++ symbols (esp. enum constants) can have different names (e.g. ns::Foo::A and ns::A). We want ns::Foo::A for workspaceSymbols, but we also want ns::A for index-based code completions (sema completion would give us ns::Foo::A right? ). One possible solution would be adding a short name in Symbol (e.g. ns::A is a short name for ns::Foo::A), and index/fuzzyFind can match on both names.

clangd/index/SymbolCollector.cpp

365

drive-by nit: please pull this to a helper function.

[sammccall]

In D47223#1110571, @malaperle wrote:

In D47223#1109247, @ilya-biryukov wrote:

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

/// foo.h
enum Foo {
  A, B, C
};

/// foo.cpp
#include "foo.h"

int a = ^ // <-- A, B, C should be in completion list here.

Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.

I don't understand how this would still work (at least when using the index). The index call would have Scopes set to the enclosing scopes {""}, which is equivalent to a search for ::A. Maybe I'm missing something.

In D47223#1112118, @ioeric wrote:

I think the fundamental problem here is that C++ symbols (esp. enum constants) can have different names (e.g. ns::Foo::A and ns::A).

Yup. There are a few other instances of this:

1. decls aliased with using decls - we model these as duplicate symbols
2. decls aliased with using directives - we ask clang to resolve the namespaces in scope when searching
3. inline namespaces - we mostly get away with ignoring them

AFAICS none of these are ideal for enums. (2 fails because the list of namespaces would get too long).
This is messy :-(

[ilya-biryukov]

So to keep completion working, maybe we can give add enumerators with full qualification and have a flag that determines whether the last component of the qualifier can be ignored?
This would give a simple implementation path for now, and will add a semantic signal that we need to support enumerators, at least.
For example,

enum En {
  A,
};

enum class ScopedEn {
  A
};

will produce symbols with

[ { Scope = "En::", 
    InsideUnscopedEnum=true, 
    Name="A"}, 
  { Scope = "ScopedEn::",
    InsideUnscopedEnum = false,
    Name = "A" } ]

fuzzyFind will have 2 variants:

• For code completion, it will return En::A only when queries with ::A (e.g. in global scope).
• For workspace symbol, it can return En::A for A and En::A. For ::A it can return empty results.

Scoped enums work the same as before. Other case (using decls and inline namespaces) will also work as before.

clangd/index/SymbolCollector.cpp

 // heuristic.
 bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
   using namespace clang::ast_matchers;
   return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
          llvm::isa<TagDecl>(&ND) &&
          match(decl(isExpansionInMainFile()), ND, ND.getASTContext()).empty();
 }
 
+std::string getQualifiedName(const NamedDecl &ND) {
+  std::string QName;
+  llvm::raw_string_ostream OS(QName);
+  PrintingPolicy Policy(ND.getASTContext().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();
+  assert(!StringRef(QName).startswith("::"));
+  return QName;
+}
+
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
 
 void SymbolCollector::initialize(ASTContext &Ctx) {
   ASTCtx = &Ctx;
   CompletionAllocator = std::make_shared<GlobalCodeCompletionAllocator>();
   CompletionTUInfo =
void SymbolCollector::finish() {
   ReferencedDecls.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND,
                                               SymbolID ID) {
   auto &Ctx = ND.getASTContext();
   auto &SM = Ctx.getSourceManager();
 
-  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();
-  assert(!StringRef(QName).startswith("::"));
+  std::string QName = getQualifiedName(ND);
 
   Symbol S;
   S.ID = std::move(ID);
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+
+  using namespace clang::ast_matchers;
+  // For enumerators in unscoped enums that have names, even if they are not
+  // scoped, we add the enum name to the scope so that users can find the
+  // enumerators when fully qualifying them, for example: MyEnum::Enumerator.
+  auto InUnscopedEnum =
+      match(decl(hasDeclContext(enumDecl(unless(isScoped())).bind("enum"))), ND,
+            *ASTCtx);
+  std::string EnumQName;
+  if (!InUnscopedEnum.empty()) {
+    auto Enum = InUnscopedEnum[0].getNodeAs<EnumDecl>("enum");
+    if (Enum->getDeclName()) {
+      EnumQName = getQualifiedName(*Enum);
+      EnumQName += "::";
+      S.Scope = EnumQName;
+    }
+  }
+
   S.SymInfo = index::getSymbolInfo(&ND);
   std::string FileURI;
   if (auto DeclLoc =
           getSymbolLocation(ND, SM, Opts, ASTCtx->getLangOpts(), FileURI))
     S.CanonicalDeclaration = *DeclLoc;
 
   // Add completion info.
   // FIXME: we may want to choose a different redecl, or combine from several.

unittests/clangd/SymbolCollectorTests.cpp

const std::string Header = R"(
     };
     namespace ns {
     enum {
       Black
     };
     }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-                                            QName("Green"), QName("Color2"),
-                                            QName("ns"), QName("ns::Black")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+                           QName("Red"), QName("Color"), QName("Color::Green"),
+                           QName("Color2"), QName("ns"), QName("ns::Black")));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreNamelessSymbols) {
   const std::string Header = R"(
     struct {
       int a;
     } Foo;
   )";
const std::string Header =
            class TopLevel {};
            enum Kind {
              KIND_OK,
              Kind_Not_Ok,
            };
            bool operator<(const TopLevel &, const TopLevel &);
          })";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
-                                   QName("nx::Kind"), QName("nx::KIND_OK"),
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::TopLevel"),
+                                            QName("nx::Kind"),
+                                            QName("nx::Kind::KIND_OK"),
                                             QName("nx::operator<")));
 }
 
 TEST_F(SymbolCollectorTest, DoubleCheckProtoHeaderComment) {
   TestHeaderName = testPath("x.proto.h");
   const std::string Header = R"(
   namespace nx {
     class Top_Level {};
     enum Kind {
       Kind_Fine
     };
   }
   )";
   runSymbolCollector(Header, /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
-                                   QName("nx::Kind"), QName("nx::Kind_Fine")));
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("nx"), QName("nx::Top_Level"),
+                                            QName("nx::Kind"),
+                                            QName("nx::Kind::Kind_Fine")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang