[CodeComplete] Improve overload handling for C++ qualified and ref-qualified methods.

[sammccall]

Summary

• when a method is not available because of the target value kind (e.g. an && method on a Foo& variable), then don't offer it.
• when a method is effectively shadowed by another method from the same class with a) an identical argument list and b) superior qualifiers, then don't offer it.

[sammccall]

Remove debug

[ilya-biryukov]

lib/Sema/SemaCodeComplete.cpp

1302

Won't this crash on Method->getName()? Could we add a test for that?
Completing in a class that has a user-defined operator==() would probably do the trick.

1310

Could you add a comment that we modify previously added results here?
It's obvious from the code, but not something I would normally expect so having a separate comment looks useful.

[sammccall]

Handle methods without "normal" identifier names.
Add comments.
Trigger in both directions (dominated and dominated-by).

[sammccall]

Oops, we weren't handling the case when the newly-added overload is dominated by an existing one. Fixed.

lib/Sema/SemaCodeComplete.cpp

1302

Right! Fixed and test added. We now no longer hash the strings which is nice too.

[ilya-biryukov]

LGTM

lib/Sema/SemaCodeComplete.cpp

1308

A leftover debug output. Please remove before landing

lib/Sema/SemaCodeComplete.cpp

 //===----------------------------------------------------------------------===//
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/QualTypeNames.h"
+#include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
private:
   /// Closely matching the preferred type gives a boost to a result's
   /// priority.
   CanQualType PreferredType;
 
   /// A list of shadow maps, which is used to model name hiding at
   /// different levels of, e.g., the inheritance hierarchy.
   std::list<ShadowMap> ShadowMaps;
 
+  /// Overloaded C++ member functions found by SemaLookup.
+  /// Used to determine when one overload is dominated by another.
+  llvm::DenseMap<std::pair<DeclContext *, StringRef>, ShadowMapEntry>
+      OverloadMap;
+
   /// If we're potentially referring to a C++ member function, the set
   /// of qualifiers applied to the object type.
   Qualifiers ObjectTypeQualifiers;
+  /// The kind of the object expression, for rvalue/lvalue overloads.
+  ExprValueKind ObjectKind;
 
   /// Whether the \p ObjectTypeQualifiers field is active.
   bool HasObjectTypeQualifiers;
 
   /// The selector that we prefer.
   Selector PreferredSelector;
 
   /// The completion context in which we are gathering results.
public:
 
   /// Set the cv-qualifiers on the object type, for us in filtering
   /// calls to member functions.
   ///
   /// When there are qualifiers in this set, they will be used to filter
   /// out member functions that aren't available (because there will be a
   /// cv-qualifier mismatch) or prefer functions with an exact qualifier
   /// match.
-  void setObjectTypeQualifiers(Qualifiers Quals) {
+  void setObjectTypeQualifiers(Qualifiers Quals, ExprValueKind Kind) {
     ObjectTypeQualifiers = Quals;
+    ObjectKind = Kind;
     HasObjectTypeQualifiers = true;
   }
 
   /// Set the preferred selector.
   ///
   /// When an Objective-C method declaration result is added, and that
   /// method's selector matches this preferred selector, we give that method
   /// a slight priority boost.
if (!AsNestedNameSpecifier)
     MaybeAddConstructorResults(R);
 }
 
 static void setInBaseClass(ResultBuilder::Result &R) {
   R.Priority += CCD_InBaseClass;
   R.InBaseClass = true;
 }
 
+// Will Object.Candidate() always be called instead of Object.Incumbent()?
+// Precondition: must have the same name, and not be a template.
+static bool overloadDominates(const CXXMethodDecl &Candidate,
+                              const CXXMethodDecl &Incumbent,
+                              const Qualifiers &ObjectQuals,
+                              ExprValueKind ObjectKind) {
+  Candidate.dumpColor();
+  Incumbent.dumpColor();
+  if (Candidate.isVariadic() != Incumbent.isVariadic() ||
+      Candidate.getNumParams() != Incumbent.getNumParams() ||
+      Candidate.getMinRequiredArguments() !=
+          Incumbent.getMinRequiredArguments())
+    return false;
+  for (unsigned I = 0, E = Candidate.getNumParams(); I != E; ++I)
+    if (Candidate.parameters()[I]->getType().getCanonicalType() !=
+        Incumbent.parameters()[I]->getType().getCanonicalType())
+      return false;
+  if (!Candidate.specific_attrs<EnableIfAttr>().empty() ||
+      !Incumbent.specific_attrs<EnableIfAttr>().empty())
+    return false;
+  // At this point, we know calls can't pick one or the other based on
+  // arguments, so one of the two must win. (Or both fail, handled elsewhere).
+  RefQualifierKind CandidateRef = Candidate.getRefQualifier();
+  RefQualifierKind IncumbentRef = Incumbent.getRefQualifier();
+  if (CandidateRef != IncumbentRef) {
+    // If the object kind is LValue/RValue, there's one acceptable ref-qualifier
+    // and it can't be mixed with ref-unqualified overloads (in valid code).
+
+    // For xvalue objects, we prefer the rvalue overload even if we have to
+    // add qualifiers (which is rare, because const&& is rare).
+    if (ObjectKind == clang::VK_XValue)
+      return CandidateRef == RQ_RValue;
+  }
+  // Now the ref qualifiers are the same (or we're in some invalid state).
+  // So make some decision based on the qualifiers.
+  Qualifiers CandidateQual = Candidate.getMethodQualifiers();
+  Qualifiers IncumbentQual = Incumbent.getMethodQualifiers();
+  return IncumbentQual.compatiblyIncludes(CandidateQual) &&
+         !CandidateQual.compatiblyIncludes(IncumbentQual);
+}
+
 void ResultBuilder::AddResult(Result R, DeclContext *CurContext,
                               NamedDecl *Hiding, bool InBaseClass = false) {
   if (R.Kind != Result::RK_Declaration) {
     // For non-declaration results, just add the result.
     Results.push_back(R);
     return;
   }
 
if (const auto *Method = dyn_cast<CXXMethodDecl>(R.Declaration))
         Qualifiers MethodQuals = Method->getMethodQualifiers();
         if (ObjectTypeQualifiers == MethodQuals)
           R.Priority += CCD_ObjectQualifierMatch;
         else if (ObjectTypeQualifiers - MethodQuals) {
           // The method cannot be invoked, because doing so would drop
           // qualifiers.
           return;
         }
+        // Detect cases where a ref-qualified method cannot be invoked.
+        switch (Method->getRefQualifier()) {
+          case RQ_LValue:
+            if (ObjectKind != VK_LValue && !MethodQuals.hasConst())
+              return;
+            break;
+          case RQ_RValue:
+            if (ObjectKind == VK_LValue)
+              return;
+            break;
+          case RQ_None:
+            break;
+        }
+
+        auto &OverloadSet =
+            OverloadMap[std::make_pair(CurContext, Method->getName())];
+        for (const DeclIndexPair& Entry : OverloadSet) {
+          Result &Incumbent = Results[Entry.second];
+          if (overloadDominates(*Method,
+                                *cast<CXXMethodDecl>(Incumbent.Declaration),
+                                ObjectTypeQualifiers, ObjectKind)) {
+            // FIXME: if the overload dominates multiple incumbents then we
+            // should remove all. But two overloads is by far the common case.
+            Incumbent = std::move(R);
+            return;
+          }
+        }
+        OverloadSet.Add(Method, Results.size());
       }
 
   // Insert this result into the set of results.
   Results.push_back(R);
 
   if (!AsNestedNameSpecifier)
     MaybeAddConstructorResults(R);
 }
case PCC_RecoveryInFunction:
     // Unfiltered
     break;
   }
 
   // If we are in a C++ non-static member function, check the qualifiers on
   // the member function to filter/prioritize the results list.
   auto ThisType = getCurrentThisType();
   if (!ThisType.isNull())
-    Results.setObjectTypeQualifiers(ThisType->getPointeeType().getQualifiers());
+    Results.setObjectTypeQualifiers(ThisType->getPointeeType().getQualifiers(),
+                                    VK_LValue);
 
   CodeCompletionDeclConsumer Consumer(Results, CurContext);
   LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
                      CodeCompleter->includeGlobals(),
                      CodeCompleter->loadExternal());
 
   AddOrdinaryNameResults(CompletionContext, S, *this, Results);
   Results.ExitScope();
if (ObjCProtocolDecl *Protocol = dyn_cast<ObjCProtocolDecl>(Container)) {
     for (auto *P : Category->protocols())
       AddObjCProperties(CCContext, P, AllowCategories, AllowNullaryMethods,
                         CurContext, AddedProperties, Results,
                         IsBaseExprStatement, IsClassProperty,
                         /*InOriginalClass*/ false);
   }
 }
 
-static void
-AddRecordMembersCompletionResults(Sema &SemaRef, ResultBuilder &Results,
-                                  Scope *S, QualType BaseType, RecordDecl *RD,
-                                  Optional<FixItHint> AccessOpFixIt) {
+static void AddRecordMembersCompletionResults(
+    Sema &SemaRef, ResultBuilder &Results, Scope *S, QualType BaseType,
+    ExprValueKind BaseKind, RecordDecl *RD, Optional<FixItHint> AccessOpFixIt) {
   // Indicate that we are performing a member access, and the cv-qualifiers
   // for the base object type.
-  Results.setObjectTypeQualifiers(BaseType.getQualifiers());
+  Results.setObjectTypeQualifiers(BaseType.getQualifiers(), BaseKind);
 
   // Access to a C/C++ class, struct, or union.
   Results.allowNestedNameSpecifiers();
   std::vector<FixItHint> FixIts;
   if (AccessOpFixIt)
     FixIts.emplace_back(AccessOpFixIt.getValue());
   CodeCompletionDeclConsumer Consumer(Results, RD, BaseType, std::move(FixIts));
   SemaRef.LookupVisibleDecls(RD, Sema::LookupMemberName, Consumer,
if (!Base)
       return false;
 
     ExprResult ConvertedBase = PerformMemberExprBaseConversion(Base, IsArrow);
     if (ConvertedBase.isInvalid())
       return false;
     Base = ConvertedBase.get();
 
     QualType BaseType = Base->getType();
+    ExprValueKind BaseKind = Base->getValueKind();
 
     if (IsArrow) {
-      if (const PointerType *Ptr = BaseType->getAs<PointerType>())
+      if (const PointerType *Ptr = BaseType->getAs<PointerType>()) {
         BaseType = Ptr->getPointeeType();
-      else if (BaseType->isObjCObjectPointerType())
+        BaseKind = VK_LValue;
+      } else if (BaseType->isObjCObjectPointerType())
         /*Do nothing*/;
       else
         return false;
     }
 
     if (const RecordType *Record = BaseType->getAs<RecordType>()) {
-      AddRecordMembersCompletionResults(*this, Results, S, BaseType,
+      AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,
                                         Record->getDecl(),
                                         std::move(AccessOpFixIt));
     } else if (const auto *TST =
                    BaseType->getAs<TemplateSpecializationType>()) {
       TemplateName TN = TST->getTemplateName();
       if (const auto *TD =
               dyn_cast_or_null<ClassTemplateDecl>(TN.getAsTemplateDecl())) {
         CXXRecordDecl *RD = TD->getTemplatedDecl();
-        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,
-                                          std::move(AccessOpFixIt));
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,
+                                          RD, std::move(AccessOpFixIt));
       }
     } else if (const auto *ICNT = BaseType->getAs<InjectedClassNameType>()) {
       if (auto *RD = ICNT->getDecl())
-        AddRecordMembersCompletionResults(*this, Results, S, BaseType, RD,
-                                          std::move(AccessOpFixIt));
+        AddRecordMembersCompletionResults(*this, Results, S, BaseType, BaseKind,
+                                          RD, std::move(AccessOpFixIt));
     } else if (!IsArrow && BaseType->isObjCObjectPointerType()) {
       // Objective-C property reference.
       AddedPropertiesSet AddedProperties;
 
       if (const ObjCObjectPointerType *ObjCPtr =
               BaseType->getAsObjCInterfacePointerType()) {
         // Add property results based on our interface.
         assert(ObjCPtr && "Non-NULL pointer guaranteed above!");

test/CodeCompletion/member-access.cpp

 // CHECK-CC9: member4 : [#int#]member4 (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: member5 : [#int#]member5
 // CHECK-CC9: memfun1 (InBase) : [#void#][#Base3::#]memfun1(<#float#>) (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: memfun1 (InBase) : [#void#][#Base3::#]memfun1(<#double#>)[# const#] (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: memfun1 (Hidden,InBase) : [#void#]Base2::memfun1(<#int#>) (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: memfun2 (InBase) : [#void#][#Base3::#]memfun2(<#int#>) (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: memfun3 : [#int#]memfun3(<#int#>) (requires fix-it: {181:4-181:5} to "->")
 // CHECK-CC9: operator-> : [#Derived *#]operator->()[# const#]
+
+// These overload sets differ only by return type and this-qualifiers.
+// So for any given callsite, only one is available.
+struct Overloads {
+  int ConstOverload(char) const;
+  double ConstOverload(char);
+
+  int RefOverload(char) &;
+  double RefOverload(char) const&;
+  char RefOverload(char) &&;
+};
+void testLValue(Overloads& Ref) {
+  Ref.
+}
+void testConstLValue(const Overloads& ConstRef) {
+  ConstRef.
+}
+void testRValue() {
+  Overloads().
+}
+void testXValue(Overloads& X) {
+  static_cast<Overloads&&>(X).
+}
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:225:7 %s -o - | FileCheck -check-prefix=CHECK-LVALUE %s \
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \
+// RUN: --implicit-check-not="[#double#]RefOverload(" \
+// RUN: --implicit-check-not="[#char#]RefOverload("
+// CHECK-LVALUE-DAG: [#double#]ConstOverload(
+// CHECK-LVALUE-DAG: [#int#]RefOverload(
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:228:12 %s -o - | FileCheck -check-prefix=CHECK-CONSTLVALUE %s \
+// RUN: --implicit-check-not="[#double#]ConstOverload(" \
+// RUN: --implicit-check-not="[#int#]RefOverload(" \
+// RUN: --implicit-check-not="[#char#]RefOverload("
+// CHECK-CONSTLVALUE: [#int#]ConstOverload(
+// CHECK-CONSTLVALUE: [#double#]RefOverload(
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:231:15 %s -o - | FileCheck -check-prefix=CHECK-PRVALUE %s \
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \
+// RUN: --implicit-check-not="[#int#]RefOverload(" \
+// RUN: --implicit-check-not="[#double#]RefOverload("
+// CHECK-PRVALUE: [#double#]ConstOverload(
+// CHECK-PRVALUE: [#char#]RefOverload(
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:234:31 %s -o - | FileCheck -check-prefix=CHECK-XVALUE %s \
+// RUN: --implicit-check-not="[#int#]ConstOverload(" \
+// RUN: --implicit-check-not="[#int#]RefOverload(" \
+// RUN: --implicit-check-not="[#double#]RefOverload("
+// CHECK-XVALUE: [#double#]ConstOverload(
+// CHECK-XVALUE: [#char#]RefOverload(