[PATCH] D113974: [clangd] Put additional qualifiers to enum constants not declared as "enum class"

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This is a cheap "fix" of the problem described in
https://github.com/clangd/clangd/issues/39. Currently, for this code:

  c++
  namespace ns {
  enum Color { Green };
  enum class Vehicle { Car };
  }

The following is true:

- `Vehicle::Car` scope is `ns::Vehicle::` - it can't be accessed without spelling full enum class name
- `Color::Green` scope is `ns::` because it can be accessed as `ns::Car`

However, this causes index FuzzyFind to show empty results when querying
`ns::Color::` - `Color::Green` will only show up for `ns::`.

This patch changes the behavior and makes `SymbolCollector` treat plain `enum`
the same way it does handle `enum class`. Ideally, the index would point to the
same symbol for both `ns::Green` and `ns::Color::Green` but that increases
coupling since the enum information has to be propagated to the index builder
which is logically quite far from the `SymbolCollector`.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113974

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1232,7 +1232,7 @@
               UnorderedElementsAre(
                   AllOf(QName("Red"), ForCodeCompletion(true)),
                   AllOf(QName("Color"), ForCodeCompletion(true)),
-                  AllOf(QName("Green"), ForCodeCompletion(true)),
+                  AllOf(QName("Color::Green"), ForCodeCompletion(true)),
                   AllOf(QName("Color2"), ForCodeCompletion(true)),
                   AllOf(QName("Color2::Yellow"), ForCodeCompletion(false)),
                   AllOf(QName("ns"), ForCodeCompletion(true)),
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -250,14 +250,15 @@
       )cpp";
   EXPECT_THAT(getSymbols(TU, "Red"), ElementsAre(QName("Red")));
   EXPECT_THAT(getSymbols(TU, "::Red"), ElementsAre(QName("Red")));
-  EXPECT_THAT(getSymbols(TU, "Green"), ElementsAre(QName("Green")));
-  EXPECT_THAT(getSymbols(TU, "Green"), ElementsAre(QName("Green")));
+  EXPECT_THAT(getSymbols(TU, "Color::Green"),
+              ElementsAre(QName("Color::Green")));
   EXPECT_THAT(getSymbols(TU, "Color2::Yellow"),
               ElementsAre(QName("Color2::Yellow")));
   EXPECT_THAT(getSymbols(TU, "Yellow"), ElementsAre(QName("Color2::Yellow")));
 
   EXPECT_THAT(getSymbols(TU, "ns::Black"), ElementsAre(QName("ns::Black")));
-  EXPECT_THAT(getSymbols(TU, "ns::Blue"), ElementsAre(QName("ns::Blue")));
+  EXPECT_THAT(getSymbols(TU, "ns::Color3::Blue"),
+              ElementsAre(QName("ns::Color3::Blue")));
   EXPECT_THAT(getSymbols(TU, "ns::Color4::White"),
               ElementsAre(QName("ns::Color4::White")));
 }
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -828,6 +828,23 @@
   // FIXME: this returns foo:bar: for objective-C methods, we prefer only foo:
   // for consistency with CodeCompletionString and a clean name/signature split.
   std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+  std::string Scope = S.Scope.str();
+  // Put additional qualifiers for enum constants: technically, they can be
+  // accessed without using the enum name prefix (unless declared as enum
+  // class) but it is more convenient to enhance the scope with the enum type
+  // name.
+  // FIXME: It would be better to just have the SymbolIndex look up the same
+  // enum constant both with and without enum type name but at the time the
+  // index is built the information about ND is already lost.
+  if (const auto *EnumConstant = llvm::dyn_cast<EnumConstantDecl>(&ND)) {
+    if (const auto *II = EnumConstant->getType().getBaseTypeIdentifier()) {
+      std::string EnumScope = II->getName().str() + "::";
+      // For "enum class" the qualified name already has EnumScope.
+      if (!S.Scope.endswith(EnumScope))
+        Scope += EnumScope;
+    }
+  }
+  S.Scope = Scope;
   std::string TemplateSpecializationArgs = printTemplateSpecializationArgs(ND);
   S.TemplateSpecializationArgs = TemplateSpecializationArgs;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D113974.387535.patch
Type: text/x-patch
Size: 3525 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211116/622471dc/attachment.bin>