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

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

I'd prefer something like "FirstArgumentIsBase"

lib/Sema/SemaOverload.cpp

5871

Unnecessary whitespace change.

6339

Add a comment that the first argument is the base.

6341

clang-format

6342

assert that FD is a static method.

[nik]

lib/Sema/SemaOverload.cpp

6342

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

"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 ↗

(On Diff #120374)

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 ↗

(On Diff #120374)

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 ↗

(On Diff #120374)

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

6396 ↗

(On Diff #120374)

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 ↗

(On Diff #120374)

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 ↗

(On Diff #120374)

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 ↗

(On Diff #120374)

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 :>

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 ExtraFirstArgument = false);
   void AddMethodCandidate(DeclAccessPair FoundDecl,
                           QualType ObjectType,
                           Expr::Classification ObjectClassification,
                           ArrayRef<Expr *> Args,
                           OverloadCandidateSet& CandidateSet,
                           bool SuppressUserConversion = false);
   void AddMethodCandidate(CXXMethodDecl *Method,
                           DeclAccessPair FoundDecl,

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 extraArgument = !UME->isImplicitAccess() && UME->getBase();
     AddFunctionCandidates(Decls, ArgExprs, CandidateSet, TemplateArgs,
                           /*SuppressUsedConversions=*/false,
-                          /*PartialOverloading=*/true);
+                          /*PartialOverloading=*/true,
+                          extraArgument);
   } 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 ||

lib/Sema/SemaOverload.cpp

if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Function)) {
     if (!isa<CXXConstructorDecl>(Method)) {
       // If we get here, it's because we're calling a member function
       // that is named without a member access expression (e.g.,
       // "this->f") that was either written explicitly or created
       // implicitly. This can happen with a qualified call to a member
       // function, e.g., X::f(). We use an empty type for the implied
       // object argument (C++ [over.call.func]p3), and the acting context
       // is irrelevant.
+
       AddMethodCandidate(Method, FoundDecl, Method->getParent(), QualType(),
                          Expr::Classification::makeSimpleLValue(), Args,
                          CandidateSet, SuppressUserConversions,
                          PartialOverloading, EarlyConversions);
       return;
     }
     // We treat a constructor like a non-member function, since its object
     // argument doesn't participate in overload resolution.

 
 /// \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 ExtraFirstArgument) {
   for (UnresolvedSetIterator F = Fns.begin(), E = Fns.end(); F != E; ++F) {
     NamedDecl *D = F.getDecl()->getUnderlyingDecl();
     if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
       if (isa<CXXMethodDecl>(FD) && !cast<CXXMethodDecl>(FD)->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.
         AddMethodCandidate(cast<CXXMethodDecl>(FD), F.getPair(),
                            cast<CXXMethodDecl>(FD)->getParent(), ObjectType,
                            ObjectClassification, Args.slice(1), CandidateSet,
                            SuppressUserConversions, PartialOverloading);
       } else {
+        // Slice the first argument when we access static method as non-static
+        if (Args.size() > 0 && ExtraFirstArgument && isa<CXXMethodDecl>(FD)
+                && !isa<CXXConstructorDecl>(FD)) {
+          Args = Args.slice(1);
+        }
         AddOverloadCandidate(FD, F.getPair(), Args, CandidateSet,
                              SuppressUserConversions, PartialOverloading);
       }
     } else {
       FunctionTemplateDecl *FunTmpl = cast<FunctionTemplateDecl>(D);
       if (isa<CXXMethodDecl>(FunTmpl->getTemplatedDecl()) &&
           !cast<CXXMethodDecl>(FunTmpl->getTemplatedDecl())->isStatic()) {
         QualType ObjectType;

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();
+  static void foo_1(int);
+};
+
+void test() {
+  Bar::foo_1();
+  Bar b;
+  b.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:103: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 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:105: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 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