Fix a rejects-valid with consteval on overloaded operators

[aaron.ballman]

Summary

It seems that Clang 11 regressed functionality that was working in Clang 10 regarding calling a few overloaded operators in an immediate context. Specifically, we were not checking for immediate invocations of array subscripting and the arrow operators, but we properly handle the other overloaded operators.

This fixes the two problematic operators and adds some test coverage to show they're equivalent to calling the operator directly. This addresses PR50779.

[erichkeane]

This seems like two places this was missed in: https://reviews.llvm.org/D63960 however I see a another place grepping around we might have missed too? Can you find a way to write tests for that?

Also, perhaps 1 more cleanup?

clang/lib/Sema/SemaOverload.cpp

13800

Wonder if as a 'while we're here' we should make this the same as the others? Does CheckForImmediateInvocation handle an invalid result without the extra check like we have here?

14290

Was this one missed too?

[aaron.ballman]

clang/lib/Sema/SemaOverload.cpp

13800

CheckForImmediateInvocation handles invalid results (I checked), so we could remove the isInvalid() lines below if we wanted (that's really an NFC cleanup).

14290

I couldn't devise a test case that was failing with member function call expressions, so I left this one alone. We have a bunch of existing test coverage for calling a consteval member function, so I'm assuming this is correct, but if someone finds a test case that fails here, it's easy enough to fix.

[rsmith]

Looks good to me.

clang/lib/Sema/SemaOverload.cpp

14290

This code is only reachable for a call through a pointer-to-member. We don't need to worry about consteval member function pointers because they can't escape constant-evaluated contexts anyway. Eg, (p->*&Class::consteval_fn)() is ill-formed outside of a constant-evaluated context -- we should make sure we have a test for that.

[erichkeane]

clang/lib/Sema/SemaOverload.cpp

14290

I can't come up with one either, I think we're fine for now.

[aaron.ballman]

clang/lib/Sema/SemaOverload.cpp

14290

struct test {
  consteval int f() const { return 12; }
};

constexpr test t;
int main() {
  constexpr int i = (t.*&test::f)();
}

@rsmith -- would you expect us to accept or reject this? GCC accepts, MSVC rejects, Clang currently rejects. This is different from your test case (because this is in a constant evaluated context), which we do already reject with a decent message: https://godbolt.org/z/3nv4bco9M

[aaron.ballman]

clang/lib/Sema/SemaOverload.cpp

14290

Thinking about this a bit more, I think that code should be accepted.

struct test {
  consteval int f() const { return 12; }
};

constexpr test t;
int main() {
  constexpr int i = t.f(); // If this works
  constexpr int j = (t.*&test::f)(); // This should also work
}

However, when I make the obvious changes in this patch to support it, we stop getting the diagnostic outside of a constant evaluated context. So my plan is to land the small fixes we know are correct and are happy with, and we can debate this case more later.

[aaron.ballman]

Updated the patch to add new test cases.

[aaron.ballman]

I landed the non-pmf changes in b9941de0bfac4bad93e11dff26396e34a53e3891

clang/lib/Sema/SemaOverload.cpp

           CheckArgAlignment(OpLoc, FnDecl, "'this'", ThisType,
                             ThisTypeFromDecl);
         }
 
         checkCall(FnDecl, nullptr, ImplicitThis, ArgsArray,
                   isa<CXXMethodDecl>(FnDecl), OpLoc, TheCall->getSourceRange(),
                   VariadicDoesNotApply);
 
         ExprResult R = MaybeBindToTemporary(TheCall);
         if (R.isInvalid())
           return ExprError();
 
         R = CheckForImmediateInvocation(R, FnDecl);
         if (R.isInvalid())
           return ExprError();
 
         // For a rewritten candidate, we've already reversed the arguments

             CurFPFeatureOverrides());
         if (CheckCallReturnType(FnDecl->getReturnType(), LLoc, TheCall, FnDecl))
           return ExprError();
 
         if (CheckFunctionCall(Method, TheCall,
                               Method->getType()->castAs<FunctionProtoType>()))
           return ExprError();
 
-        return MaybeBindToTemporary(TheCall);
+        return CheckForImmediateInvocation(MaybeBindToTemporary(TheCall),
+                                           FnDecl);
       } else {
         // We matched a built-in operator. Convert the arguments, then
         // break out so that we will build the appropriate built-in
         // operator node.
         ExprResult ArgsRes0 = PerformImplicitConversion(
             Args[0], Best->BuiltinParamTypes[0], Best->Conversions[0],
             AA_Passing, CCK_ForBuiltinOverloadedOp);
         if (ArgsRes0.isInvalid())

       return ExprError();
 
     if (ConvertArgumentsForCall(call, op, nullptr, proto, Args, RParenLoc))
       return ExprError();
 
     if (CheckOtherCall(call, proto))
       return ExprError();
 
     return MaybeBindToTemporary(call);
   }
 
   // We only try to build a recovery expr at this level if we can preserve
   // the return type, otherwise we return ExprError() and let the caller
   // recover.
   auto BuildRecoveryExpr = [&](QualType Type) {
     if (!AllowRecovery)
       return ExprError();

 
   if (CheckCallReturnType(Method->getReturnType(), OpLoc, TheCall, Method))
     return ExprError();
 
   if (CheckFunctionCall(Method, TheCall,
                         Method->getType()->castAs<FunctionProtoType>()))
     return ExprError();
 
-  return MaybeBindToTemporary(TheCall);
+  return CheckForImmediateInvocation(MaybeBindToTemporary(TheCall), Method);
 }
 
 /// BuildLiteralOperatorCall - Build a UserDefinedLiteral by creating a call to
 /// a literal operator described by the provided lookup results.
 ExprResult Sema::BuildLiteralOperatorCall(LookupResult &R,
                                           DeclarationNameInfo &SuffixInfo,
                                           ArrayRef<Expr*> Args,
                                           SourceLocation LitEndLoc,

clang/test/SemaCXX/cxx2a-consteval.cpp

 
 auto e = g(f()); // expected-error {{is not a constant expression}}
                  // expected-note@-1 {{non-constexpr function 'f' cannot be used in a constant expression}}
 
 using T = decltype(g(f()));
 static_assert(is_same<long, T>::value);
 
 } // namespace unevaluated
+
+namespace PR50779 {
+struct derp {
+  int b = 0;
+};
+
+constexpr derp d;
+
+struct test {
+  consteval int operator[](int i) const { return {}; }
+  consteval const derp * operator->() const { return &d; }
+  consteval int f() const { return 12; } // expected-note 2{{declared here}}
+};
+
+constexpr test a;
+
+// We previously rejected both of these overloaded operators as taking the
+// address of a consteval function outside of an immediate context, but we
+// accepted direct calls to the overloaded operator. Now we show that we accept
+// both forms.
+constexpr int s = a.operator[](1);
+constexpr int t = a[1];
+constexpr int u = a.operator->()->b;
+constexpr int v = a->b;
+// FIXME: I believe this case should work, but we currently reject.
+constexpr int w = (a.*&test::f)(); // expected-error {{cannot take address of consteval function 'f' outside of an immediate invocation}}
+constexpr int x = a.f();
+
+// Show that we reject when not in an immediate context.
+int w2 = (a.*&test::f)(); // expected-error {{cannot take address of consteval function 'f' outside of an immediate invocation}}
+}