Fix overloaded static functions in SemaCodeComplete

[yvvan]

Summary

https://bugs.llvm.org/show_bug.cgi?id=33904
Happens when static function is accessed via the class variable. That leads to incorrect overloads number because the variable is considered as the first argument.

struct Bar {

static void foo(); static void foo(int);

};

int main() {

Bar b;
b.foo(/*complete here*/); // did not work before
Bar::foo(/*complete here*/); // worked fine

}

[bkramer]

include/clang/Sema/Sema.h

2681 ↗

(On Diff #109961)

Shouldn't this be called with the new argument somehere? Otherwise it'll always be false.

[yvvan]

Yes, I missed to include one file in this diff where it's used

[yvvan]

Thought that I've just found a regression in my change.

But it's some unrelated behavior in fact - constructor work fine also

[yvvan]

Please check that one

[bkramer]

include/clang/Sema/Sema.h

2681 ↗

(On Diff #109980)

I'd prefer something like "FirstArgumentIsBase"

lib/Sema/SemaOverload.cpp

5871 ↗

(On Diff #109980)

Unnecessary whitespace change.

6339 ↗

(On Diff #109980)

Add a comment that the first argument is the base.

6341 ↗

(On Diff #109980)

clang-format

6342 ↗

(On Diff #109980)

assert that FD is a static method.

[nik]

lib/Sema/SemaOverload.cpp

6342 ↗

(On Diff #109980)

Just stumbled here because I was looking into https://bugs.llvm.org/show_bug.cgi?id=34207
Some observations

• Slicing the first argument unconditionally in this branch (without the if at 6340) fixes the issue.
• The current revision/version of this change does not fix the issue.

In case the crash is adapted with this patch, fine. Otherwise I'll wait until this one got in and look again at it.

[yvvan]

lib/Sema/SemaOverload.cpp

6342 ↗

(On Diff #109980)

"Slicing the first argument unconditionally in this branch" is not valid because constructors are affected (at least them) where this argument is needed

[yvvan]

Review comments + solved https://bugs.llvm.org/show_bug.cgi?id=34207 issue.
I've also added the extra test for that issue

[yvvan]

ping

[yvvan]

one more ping :)

[yvvan]

ping 3

[yvvan]

one more ping...

[bkramer]

This looks good. Sorry for the long wait, do you have commit access?

[yvvan]

@bkramer Not yet, it would be good to have one though :)

[cameron314]

cfe/trunk/lib/Sema/SemaOverload.cpp

6365

This breaks normal (non-static) method overload resolution, since the this argument is now passed to AddMethodCandidate which is not expecting it. It always thinks too many arguments are passed to methods with no parameters; most other calls to AddMethodCandidate in SemaOverload.cpp don't pass the implicit this.

[cameron314]

cfe/trunk/lib/Sema/SemaOverload.cpp

6396

The same slice that was added above needs to be done here for templated static methods that are accessed via a non-static object.

[yvvan]

cfe/trunk/lib/Sema/SemaOverload.cpp

6365

This code is correct. It is sliced at line 6361 - only in the case when the args size is greater than 0.

6396

It might be the case. You can add that :)
I did not check template cases in details.

[cameron314]

cfe/trunk/lib/Sema/SemaOverload.cpp

6365

Hmm, you're right, I didn't see that. That line was missing after a rebase on our side, my fault for not properly cross-referencing the diffs. Sorry for the false alarm.

But shouldn't the slice only be done now when Args.size() > 0 && (!Args[0] || (FirstArgumentIsBase && isa<CXXMethodDecl>(FD) && !isa<CXXConstructorDecl>(FD)))?

6396

It is the case, I have parameter completion tests that fail without this :-)
I'll commit in a few days.

[yvvan]

cfe/trunk/lib/Sema/SemaOverload.cpp

6365

there's already a check for this case that it's a CXXMethodDecl, it's not static. That means that we always need to slice (except those rare cases when we don't have any args, and in fact I don't know why these cases exist)

[nik]

Ping

[nik]

Huch, seems already submitted. Ignore :>

cfe/trunk/include/clang/Sema/Sema.h

void AddOverloadCandidate(FunctionDecl *Function,
                             bool PartialOverloading = false,
                             bool AllowExplicit = false,
                             ConversionSequenceList EarlyConversions = None);
   void AddFunctionCandidates(const UnresolvedSetImpl &Functions,
                       ArrayRef<Expr *> Args,
                       OverloadCandidateSet &CandidateSet,
                       TemplateArgumentListInfo *ExplicitTemplateArgs = nullptr,
                       bool SuppressUserConversions = false,
-                      bool PartialOverloading = false);
+                      bool PartialOverloading = false,
+                      bool FirstArgumentIsBase = false);
   void AddMethodCandidate(DeclAccessPair FoundDecl,
                           QualType ObjectType,
                           Expr::Classification ObjectClassification,
                           ArrayRef<Expr *> Args,
                           OverloadCandidateSet& CandidateSet,
                           bool SuppressUserConversion = false);
   void AddMethodCandidate(CXXMethodDecl *Method,
                           DeclAccessPair FoundDecl,

cfe/trunk/lib/Sema/SemaCodeComplete.cpp

else if (auto UME = dyn_cast<UnresolvedMemberExpr>(NakedFn)) {
     }
 
     // Add the base as first argument (use a nullptr if the base is implicit).
     SmallVector<Expr *, 12> ArgExprs(
         1, UME->isImplicitAccess() ? nullptr : UME->getBase());
     ArgExprs.append(Args.begin(), Args.end());
     UnresolvedSet<8> Decls;
     Decls.append(UME->decls_begin(), UME->decls_end());
+    const bool FirstArgumentIsBase = !UME->isImplicitAccess() && UME->getBase();
     AddFunctionCandidates(Decls, ArgExprs, CandidateSet, TemplateArgs,
                           /*SuppressUsedConversions=*/false,
-                          /*PartialOverloading=*/true);
+                          /*PartialOverloading=*/true,
+                          FirstArgumentIsBase);
   } else {
     FunctionDecl *FD = nullptr;
     if (auto MCE = dyn_cast<MemberExpr>(NakedFn))
       FD = dyn_cast<FunctionDecl>(MCE->getMemberDecl());
     else if (auto DRE = dyn_cast<DeclRefExpr>(NakedFn))
       FD = dyn_cast<FunctionDecl>(DRE->getDecl());
     if (FD) { // We check whether it's a resolved function declaration.
       if (!getLangOpts().CPlusPlus ||

cfe/trunk/lib/Sema/SemaOverload.cpp

 
 /// \brief Add all of the function declarations in the given function set to
 /// the overload candidate set.
 void Sema::AddFunctionCandidates(const UnresolvedSetImpl &Fns,
                                  ArrayRef<Expr *> Args,
                                  OverloadCandidateSet& CandidateSet,
                                  TemplateArgumentListInfo *ExplicitTemplateArgs,
                                  bool SuppressUserConversions,
-                                 bool PartialOverloading) {
+                                 bool PartialOverloading,
+                                 bool FirstArgumentIsBase) {
   for (UnresolvedSetIterator F = Fns.begin(), E = Fns.end(); F != E; ++F) {
     NamedDecl *D = F.getDecl()->getUnderlyingDecl();
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+      ArrayRef<Expr *> FunctionArgs = Args;
       if (isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->isStatic()) {
         QualType ObjectType;
         Expr::Classification ObjectClassification;
+        if (Args.size() > 0) {
           if (Expr *E = Args[0]) {
             // Use the explit base to restrict the lookup:
             ObjectType = E->getType();
             ObjectClassification = E->Classify(Context);
           } // .. else there is an implit base.
+          FunctionArgs = Args.slice(1);
+        }
         AddMethodCandidate(cast<CXXMethodDecl>(FD), F.getPair(),
                            cast<CXXMethodDecl>(FD)->getParent(), ObjectType,
-                           ObjectClassification, Args.slice(1), CandidateSet,
+                           ObjectClassification, FunctionArgs, CandidateSet,
                            SuppressUserConversions, PartialOverloading);
       } else {
-        AddOverloadCandidate(FD, F.getPair(), Args, CandidateSet,
+        // Slice the first argument (which is the base) when we access
+        // static method as non-static
+        if (Args.size() > 0 && (!Args[0] || (FirstArgumentIsBase && isa<CXXMethodDecl>(FD) &&
+                                             !isa<CXXConstructorDecl>(FD)))) {
+          assert(cast<CXXMethodDecl>(FD)->isStatic());
+          FunctionArgs = Args.slice(1);
+        }
+        AddOverloadCandidate(FD, F.getPair(), FunctionArgs, CandidateSet,
                              SuppressUserConversions, PartialOverloading);
       }
     } else {
       FunctionTemplateDecl *FunTmpl = cast<FunctionTemplateDecl>(D);
       if (isa<CXXMethodDecl>(FunTmpl->getTemplatedDecl()) &&
           !cast<CXXMethodDecl>(FunTmpl->getTemplatedDecl())->isStatic()) {
         QualType ObjectType;
         Expr::Classification ObjectClassification;
         if (Expr *E = Args[0]) {
           // Use the explit base to restrict the lookup:
           ObjectType = E->getType();
           ObjectClassification = E->Classify(Context);
         } // .. else there is an implit base.
         AddMethodTemplateCandidate(
             FunTmpl, F.getPair(),
             cast<CXXRecordDecl>(FunTmpl->getDeclContext()),
             ExplicitTemplateArgs, ObjectType, ObjectClassification,
             Args.slice(1), CandidateSet, SuppressUserConversions,
             PartialOverloading);
       } else {
         AddTemplateOverloadCandidate(FunTmpl, F.getPair(),
                                      ExplicitTemplateArgs, Args,
                                      CandidateSet, SuppressUserConversions,
                                      PartialOverloading);
       }
     }
   }
 }
 

cfe/trunk/test/Index/complete-call.cpp

int main() {
   foo_6<int>(42,);
   foo_7(42,);
   s.foo_1(42,);
   s.foo_2(42,);
   s.foo_6<int>(42,);
   s.foo_7(42,);
 }
 
+struct Bar {
+  static void foo_1();
+  void foo_1(float);
+  static void foo_1(int);
+};
+
+void test() {
+  Bar::foo_1();
+  Bar b;
+  b.foo_1();
+}
+
+struct Bar2 : public Bar {
+  Bar2() {
+    Bar::foo_1();
+  }
+};
+
 // RUN: c-index-test -code-completion-at=%s:47:9 %s | FileCheck -check-prefix=CHECK-CC1 %s
 // CHECK-CC1: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{RightParen )} (1)
 // CHECK-CC1: Completion contexts:
 // CHECK-CC1-NEXT: Any type
 // CHECK-CC1-NEXT: Any value
 // CHECK-CC1-NEXT: Enum tag
 // CHECK-CC1-NEXT: Union tag
 // CHECK-CC1-NEXT: Struct tag

 // CHECK-CC59-NEXT: Any type
 // CHECK-CC59-NEXT: Any value
 // CHECK-CC59-NEXT: Enum tag
 // CHECK-CC59-NEXT: Union tag
 // CHECK-CC59-NEXT: Struct tag
 // CHECK-CC59-NEXT: Class name
 // CHECK-CC59-NEXT: Nested name specifier
 // CHECK-CC59-NEXT: Objective-C interface
+
+// RUN: c-index-test -code-completion-at=%s:104:14 %s | FileCheck -check-prefix=CHECK-CC60 %s
+// CHECK-CC60: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{RightParen )} (1)
+// CHECK-CC60: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter float}{RightParen )} (1)
+// CHECK-CC60: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter int}{RightParen )} (1)
+// CHECK-CC60: Completion contexts:
+// CHECK-CC60-NEXT: Any type
+// CHECK-CC60-NEXT: Any value
+// CHECK-CC60-NEXT: Enum tag
+// CHECK-CC60-NEXT: Union tag
+// CHECK-CC60-NEXT: Struct tag
+// CHECK-CC60-NEXT: Class name
+// CHECK-CC60-NEXT: Nested name specifier
+// CHECK-CC60-NEXT: Objective-C interface
+
+// RUN: c-index-test -code-completion-at=%s:106:11 %s | FileCheck -check-prefix=CHECK-CC61 %s
+// CHECK-CC61: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{RightParen )} (1)
+// CHECK-CC61: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter float}{RightParen )} (1)
+// CHECK-CC61: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter int}{RightParen )} (1)
+// CHECK-CC61: Completion contexts:
+// CHECK-CC61-NEXT: Any type
+// CHECK-CC61-NEXT: Any value
+// CHECK-CC61-NEXT: Enum tag
+// CHECK-CC61-NEXT: Union tag
+// CHECK-CC61-NEXT: Struct tag
+// CHECK-CC61-NEXT: Class name
+// CHECK-CC61-NEXT: Nested name specifier
+// CHECK-CC61-NEXT: Objective-C interface
+
+// RUN: c-index-test -code-completion-at=%s:111:16 %s | FileCheck -check-prefix=CHECK-CC62 %s
+// CHECK-CC62: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{RightParen )} (1)
+// CHECK-CC62: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter float}{RightParen )} (1)
+// CHECK-CC62: OverloadCandidate:{ResultType void}{Text foo_1}{LeftParen (}{CurrentParameter int}{RightParen )} (1)
+// CHECK-CC62: Completion contexts:
+// CHECK-CC62-NEXT: Any type
+// CHECK-CC62-NEXT: Any value
+// CHECK-CC62-NEXT: Enum tag
+// CHECK-CC62-NEXT: Union tag
+// CHECK-CC62-NEXT: Struct tag
+// CHECK-CC62-NEXT: Class name
+// CHECK-CC62-NEXT: Nested name specifier
+// CHECK-CC62-NEXT: Objective-C interface
+