[clangd] Avoid crashes in override completions

[ilya-biryukov]

Summary

NamedDecl::getName cannot be called on non-identifier names.

[ilya-biryukov]

unittests/clangd/CodeCompleteTests.cpp

1770

Was wondering if testing for crashes LG this way, or adding more assertions might make sense too

[ioeric]

unittests/clangd/CodeCompleteTests.cpp

1770

Hmm, I think this is okay, but I'd probably do some sanity check on the results, just to make it look less odd ;)

[ilya-biryukov]

unittests/clangd/CodeCompleteTests.cpp

1770

Exactly my feelings: this looks odd.
However, couldn't come up with a decent sanity check at this point.
The reason is: we don't store enough information to tell override completion from non-override ones.

It means I can assert something like Not(Contains(Labelled("~Base() override"))), but lots of broken outputs can still make the test pass, e.g.:

• void ~Base() override
• ~Derived() override
• ...

Will probably keep as this and think how to factor out overriden completions from the results better...

clangd/CodeComplete.cpp

getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
   const auto *CR = llvm::dyn_cast<CXXRecordDecl>(DC);
   // If not inside a class/struct/union return empty.
   if (!CR)
     return {};
   // First store overrides within current class.
   // These are stored by name to make querying fast in the later step.
   llvm::StringMap<std::vector<FunctionDecl *>> Overrides;
   for (auto *Method : CR->methods()) {
-    if (!Method->isVirtual())
+    if (!Method->isVirtual() || !Method->getIdentifier())
       continue;
     Overrides[Method->getName()].push_back(Method);
   }
 
   std::vector<CodeCompletionResult> Results;
   for (const auto &Base : CR->bases()) {
     const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
     if (!BR)
       continue;
     for (auto *Method : BR->methods()) {
-      if (!Method->isVirtual())
+      if (!Method->isVirtual() || !Method->getIdentifier())
         continue;
       const auto it = Overrides.find(Method->getName());
       bool IsOverriden = false;
       if (it != Overrides.end()) {
         for (auto *MD : it->second) {
           // If the method in current body is not an overload of this virtual
-          // function, that it overrides this one.
+          // function, then it overrides this one.
           if (!S->IsOverload(MD, Method, false)) {
             IsOverriden = true;
             break;
           }
         }
       }
       if (!IsOverriden)
         Results.emplace_back(Method, 0);

unittests/clangd/CodeCompleteTests.cpp

TEST(CompletionTest, SuggestOverrides) {
   )cpp");
   const auto Results = completions(Text);
   EXPECT_THAT(Results.Completions,
               AllOf(Contains(Labeled("void vfunc(bool param, int p) override")),
                     Contains(Labeled("void ttt(bool param) const override")),
                     Not(Contains(Labeled("void vfunc(bool param) override")))));
 }
 
+TEST(CompletionTest, OverridesNonIdentName) {
+  // Check the completions call does not crash.
+  completions(R"cpp(
+    struct Base {
+      virtual ~Base() = 0;
+      virtual operator int() = 0;
+      virtual Base& operator+(Base&) = 0;
+    };
+
+    struct Derived : Base {
+      ^
+    };
+  )cpp");
+}
+
 TEST(SpeculateCompletionFilter, Filters) {
   Annotations F(R"cpp($bof^
       $bol^
       ab$ab^
       x.ab$dot^
       x.$dotempty^
       x::ab$scoped^
       x::$scopedempty^