[clangd] Tune down quality score for class constructors so that it's ranked after class types.

[ioeric]

Summary

Currently, class constructors have the same score as the class types, and they
are often ranked before class types. This is often not desireable and can
be annoying when snippet is enabled and constructor signatures are added.

Metrics:

==================================================================================================
                                        OVERALL
==================================================================================================
  Total measurements: 111117 (+0)
  All measurements:
	MRR: 64.06 (+0.20)	Top-5: 75.73% (+0.14%)	Top-100: 93.71% (+0.01%)
  Full identifiers:
	MRR: 98.25 (+0.55)	Top-5: 99.04% (+0.03%)	Top-100: 99.16% (+0.00%)
  Filter length 0-5:
	MRR:      15.23 (+0.02)		50.50 (-0.02)		65.04 (+0.11)		70.75 (+0.19)		74.37 (+0.25)		79.43 (+0.32)
	Top-5:    40.90% (+0.03%)		74.52% (+0.03%)		87.23% (+0.15%)		91.68% (+0.08%)		93.68% (+0.14%)		95.87% (+0.12%)
	Top-100:  68.21% (+0.02%)		96.28% (+0.07%)		98.43% (+0.00%)		98.72% (+0.00%)		98.74% (+0.01%)		98.81% (+0.00%)
==================================================================================================
                                        DEFAULT
==================================================================================================
  Total measurements: 57535 (+0)
  All measurements:
	MRR: 58.07 (+0.37)	Top-5: 69.94% (+0.26%)	Top-100: 90.14% (+0.03%)
  Full identifiers:
	MRR: 97.13 (+1.05)	Top-5: 98.14% (+0.06%)	Top-100: 98.34% (+0.00%)
  Filter length 0-5:
	MRR:      13.91 (+0.00)		38.53 (+0.01)		55.58 (+0.21)		63.63 (+0.30)		69.23 (+0.47)		72.87 (+0.60)
	Top-5:    24.99% (+0.00%)		62.70% (+0.06%)		82.80% (+0.30%)		88.66% (+0.16%)		92.02% (+0.27%)		93.53% (+0.21%)
	Top-100:  51.56% (+0.05%)		93.19% (+0.13%)		97.30% (+0.00%)		97.81% (+0.00%)		97.85% (+0.01%)		97.79% (+0.00%)

Remark:

• The full-id completions have +1.05 MRR improvement.
• There is no noticeable impact on EXPLICIT_MEMBER_ACCESS and WANT_LOCAL.

[ilya-biryukov]

Just a drive-by comment.

clangd/Quality.cpp

211

NIT: This does not seem to change the score, right? Maybe just the assignment or merge with the Unknown case?

[ioeric]

clangd/Quality.cpp

211

Previously, constructors are treated as Function and got 1.1 boost. Now it doesn't get a boost. Merging with Unknown might make sense too, but I wonder if we want to be more explicit. Happy to change.

[sammccall]

No opinion on the boost style thing.

unittests/clangd/QualityTests.cpp

198

duplicates previous line

280

This looks like it duplicates part of the test below, and removes a sensible test for scopes on constructors?

[ioeric]

• revert unintended changes.

[ioeric]

unittests/clangd/QualityTests.cpp

280

Oops, this was an accidental change...

[ilya-biryukov]

clangd/Quality.cpp

211

Have no statement for a no-op instead of "multiply by 1.0" seems clearer.
Other than that, no preferences on my side.

[ioeric]

• Make constructor boost no-op.

clangd/Quality.h

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

clangd/Quality.cpp

#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(ValueDecl, Variable);
     MAP(VarTemplateDecl, Variable);
     MAP(FunctionDecl, Function);
     MAP(FunctionTemplateDecl, Function);
     MAP(Decl, Unknown);
 #undef MAP
   };
   return Switch().Visit(&ND);
categorize(const CodeCompletionResult &R) {
   case CXCursor_ModuleImportDecl:
     return SymbolQualitySignals::Namespace;
   case CXCursor_MacroDefinition:
     return SymbolQualitySignals::Macro;
   case CXCursor_TypeRef:
     return SymbolQualitySignals::Type;
   case CXCursor_MemberRef:
     return SymbolQualitySignals::Variable;
+  case CXCursor_Constructor:
+    return SymbolQualitySignals::Constructor;
   default:
     return SymbolQualitySignals::Keyword;
   }
 }
 
 static SymbolQualitySignals::SymbolCategory
 categorize(const index::SymbolInfo &D) {
   switch (D.Kind) {
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::Constructor:
   case index::SymbolKind::Destructor:
   case index::SymbolKind::ConversionFunction:
     return SymbolQualitySignals::Function;
+  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:
   case index::SymbolKind::Module:
   case index::SymbolKind::Unknown:
case Keyword: // Often relevant, but misses most signals.
     Score *= 4; // FIXME: important keywords should have specific boosts.
     break;
   case Type:
   case Function:
   case Variable:
     Score *= 1.1f;
     break;
   case Namespace:
     Score *= 0.8f;
     break;
   case Macro:
     Score *= 0.2f;
     break;
   case Unknown:
+  case Constructor: // No boost constructors so they are after class types.
     break;
   }
 
   return Score;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const SymbolQualitySignals &S) {
   OS << formatv("=== Symbol quality: {0}\n", S.evaluate());

unittests/clangd/QualityTests.cpp

 //===----------------------------------------------------------------------===//
 
 #include "FileDistance.h"
 #include "Quality.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/Type.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
TEST(QualityTests, SymbolQualitySignalsSanity) {
   EXPECT_LT(ReservedName.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;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
+  Constructor.Category = SymbolQualitySignals::Constructor;
+  Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Constructor.evaluate(), Function.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals Default;
   EXPECT_EQ(Default.evaluate(), 1);
 
   SymbolRelevanceSignals Forbidden;
   Forbidden.Forbidden = true;
TEST(QualityTests, NoBoostForClassConstructor) {
   const NamedDecl *CtorDecl = &findAnyDecl(AST, [](const NamedDecl &ND) {
     return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
            llvm::isa<CXXConstructorDecl>(&ND);
   });
   SymbolRelevanceSignals Ctor;
   Ctor.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
 
   EXPECT_EQ(Cls.Scope, SymbolRelevanceSignals::GlobalScope);
   EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
 }
 
 TEST(QualityTests, IsInstanceMember) {
   auto Header = TestTU::withHeaderCode(R"cpp(
     class Foo {
     public:
       static void foo() {}
 
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) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+    class Foo {
+    public:
+      Foo(int);
+    };
+  )cpp");
+  auto Symbols = Header.headerSymbols();
+  auto AST = Header.build();
+
+  const NamedDecl *CtorDecl = &findAnyDecl(AST, [](const NamedDecl &ND) {
+    return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
+           llvm::isa<CXXConstructorDecl>(&ND);
+  });
+
+  SymbolQualitySignals Q;
+  Q.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+
+  Q.Category = SymbolQualitySignals::Unknown;
+  const Symbol &CtorSym = findSymbol(Symbols, "Foo::Foo");
+  Q.merge(CtorSym);
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang