[clangd] Penalize destructor and overloaded operators in code completion.

[hokein]

clangd/Quality.h

74

Maybe name it OverloadedOperator to avoid confusion with other non-overloaded operators like ?:

unittests/clangd/QualityTests.cpp

428

The test name *Quality is a bit confusing. At the first glance, I'd expect to see some tests about ranking, but it just verifies that the new symbol category is added, maybe name it something like OperatorSymbolCategory.

[ioeric]

• Rename test name.

clangd/Quality.h

74

Ternary operator is not a symbol AFAIK? It's an expression. Even if it's an operator, I think we would still want to categorize it as operator.

clangd/Quality.h

struct SymbolQualitySignals {
 
   enum SymbolCategory {
     Unknown = 0,
     Variable,
     Macro,
     Type,
     Function,
     Constructor,
+    Destructor,
     Namespace,
     Keyword,
+    Operator,
   } Category = Unknown;
 
   void merge(const CodeCompletionResult &SemaCCResult);
   void merge(const Symbol &IndexResult);
 
   // Condense these signals down to a single number, higher is better.
   float evaluate() const;
 };

clangd/Quality.cpp

if (R.ShadowDecl) {
     const auto Loc = SourceMgr.getExpansionLoc(R.ShadowDecl->getLocation());
     if (SourceMgr.isWrittenInMainFile(Loc))
       return true;
   }
   return false;
 }
 
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
+  if (const auto *FD = dyn_cast<FunctionDecl>(&ND)) {
+    if (FD->isOverloadedOperator())
+      return SymbolQualitySignals::Operator;
+  }
   class Switch
       : public ConstDeclVisitor<Switch, SymbolQualitySignals::SymbolCategory> {
   public:
 #define MAP(DeclType, Category)                                                \
   SymbolQualitySignals::SymbolCategory Visit##DeclType(const DeclType *) {     \
     return SymbolQualitySignals::Category;                                     \
   }
     MAP(NamespaceDecl, Namespace);
     MAP(NamespaceAliasDecl, Namespace);
     MAP(TypeDecl, Type);
     MAP(TypeAliasTemplateDecl, Type);
     MAP(ClassTemplateDecl, Type);
     MAP(CXXConstructorDecl, Constructor);
+    MAP(CXXDestructorDecl, Destructor);
     MAP(ValueDecl, Variable);
     MAP(VarTemplateDecl, Variable);
     MAP(FunctionDecl, Function);
     MAP(FunctionTemplateDecl, Function);
     MAP(Decl, Unknown);
 #undef MAP
   };
   return Switch().Visit(&ND);
case index::SymbolKind::TypeAlias:
     return SymbolQualitySignals::Type;
   case index::SymbolKind::Function:
   case index::SymbolKind::ClassMethod:
   case index::SymbolKind::InstanceMethod:
   case index::SymbolKind::StaticMethod:
   case index::SymbolKind::InstanceProperty:
   case index::SymbolKind::ClassProperty:
   case index::SymbolKind::StaticProperty:
-  case index::SymbolKind::Destructor:
   case index::SymbolKind::ConversionFunction:
     return SymbolQualitySignals::Function;
+  case index::SymbolKind::Destructor:
+    return SymbolQualitySignals::Destructor;
   case index::SymbolKind::Constructor:
     return SymbolQualitySignals::Constructor;
   case index::SymbolKind::Variable:
   case index::SymbolKind::Field:
   case index::SymbolKind::EnumConstant:
   case index::SymbolKind::Parameter:
     return SymbolQualitySignals::Variable;
   case index::SymbolKind::Using:
float SymbolQualitySignals::evaluate() const {
   case Function:
   case Variable:
     Score *= 1.1f;
     break;
   case Namespace:
     Score *= 0.8f;
     break;
   case Macro:
+  case Destructor:
+  case Operator:
     Score *= 0.5f;
     break;
-  case Unknown:
   case Constructor: // No boost constructors so they are after class types.
+  case Unknown:
     break;
   }
 
   return Score;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) {
   OS << formatv("=== Symbol quality: {0}\n", S.evaluate());

unittests/clangd/QualityTests.cpp

TEST(QualityTests, SymbolQualitySignalsSanity) {
   EXPECT_LT(ImplementationDetail.evaluate(), Default.evaluate());
 
   SymbolQualitySignals WithReferences, ManyReferences;
   WithReferences.References = 20;
   ManyReferences.References = 1000;
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function,
+      Destructor, Operator;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
   Constructor.Category = SymbolQualitySignals::Constructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Operator.Category = SymbolQualitySignals::Operator;
   Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Operator.evaluate(), Default.evaluate());
   EXPECT_LT(Constructor.evaluate(), Function.evaluate());
+  EXPECT_LT(Destructor.evaluate(), Constructor.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals Default;
   EXPECT_EQ(Default.evaluate(), 1);
 
   SymbolRelevanceSignals Forbidden;
   Forbidden.Forbidden = true;
TEST(QualityTests, IsInstanceMember) {
   EXPECT_FALSE(Rel.IsInstanceMember);
   Rel.merge(CodeCompletionResult(Bar, /*Priority=*/0));
   EXPECT_TRUE(Rel.IsInstanceMember);
   Rel.IsInstanceMember = false;
   Rel.merge(CodeCompletionResult(Tpl, /*Priority=*/0));
   EXPECT_TRUE(Rel.IsInstanceMember);
 }
 
-TEST(QualityTests, ConstructorQuality) {
+TEST(QualityTests, ConstructorDestructor) {
   auto Header = TestTU::withHeaderCode(R"cpp(
     class Foo {
     public:
       Foo(int);
+      ~Foo();
     };
   )cpp");
   auto Symbols = Header.headerSymbols();
   auto AST = Header.build();
 
   const NamedDecl *CtorDecl = &findDecl(AST, [](const NamedDecl &ND) {
     return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
            isa<CXXConstructorDecl>(&ND);
   });
+  const NamedDecl *DtorDecl = &findDecl(AST, [](const NamedDecl &ND) {
+    return (ND.getQualifiedNameAsString() == "Foo::~Foo") &&
+           isa<CXXDestructorDecl>(&ND);
+  });
 
-  SymbolQualitySignals Q;
-  Q.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  SymbolQualitySignals CtorQ;
+  CtorQ.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
 
-  Q.Category = SymbolQualitySignals::Unknown;
+  CtorQ.Category = SymbolQualitySignals::Unknown;
   const Symbol &CtorSym = findSymbol(Symbols, "Foo::Foo");
-  Q.merge(CtorSym);
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  CtorQ.merge(CtorSym);
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
+
+  SymbolQualitySignals DtorQ;
+  DtorQ.merge(CodeCompletionResult(DtorDecl, /*Priority=*/0));
+  EXPECT_EQ(DtorQ.Category, SymbolQualitySignals::Destructor);
+}
+
+TEST(QualityTests, Operator) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+    class Foo {
+    public:
+      bool operator<(const Foo& f1, const Foo& f2);
+    };
+  )cpp");
+  auto AST = Header.build();
+
+  const NamedDecl *Operator = &findDecl(AST, [](const NamedDecl &ND) {
+    if (const auto *OD = dyn_cast<FunctionDecl>(&ND))
+      if (OD->isOverloadedOperator())
+        return true;
+    return false;
+  });
+  SymbolQualitySignals Q;
+  Q.merge(CodeCompletionResult(Operator, /*Priority=*/0));
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Operator);
 }
 
 TEST(QualityTests, ItemWithFixItsRankedDown) {
   CodeCompleteOptions Opts;
   Opts.IncludeFixIts = true;
 
   auto Header = TestTU::withHeaderCode(R"cpp(
         int x;