[clang] Fix consteval operators in template contexts

[Fznamznon]

Summary

Clang used to reject consteval operators if they're used inside a
template due to TreeTransform putting two different DeclRefExpr
expressions for the same reference of the same operator declaration into
ReferenceToConsteval set.
It seems there was an attempt to not rebuild the whole operator that
never succeeded, so this patch just removes this attempt and
problemating referencing of a DeclRefExpr that always ended up
discarded.

Fixes https://github.com/llvm/llvm-project/issues/62886

[cor3ntin]

clang/lib/Sema/TreeTransform.h

11965–11968

I don't understand why we would not need to transform the callee. do we do that elsewhere?

[Fznamznon]

clang/lib/Sema/TreeTransform.h

11965–11968

For example, the code above for call and subscript operators never transforms the callee.
This TransFormExpr call seems to be a no-op for overloaded operator call case, and the result never ends up in the resulting ast.

[erichkeane]

Like @cor3ntin I'm concerned about removing the transform call. I'm just as concerned that it caused no regressions...

If we have to transform the arguments, then this can be a dependent thing, which means the callee should be possible to be dependent, right? Thus needs to be transformed somewhere?

[Fznamznon]

In D151553#4380667, @erichkeane wrote:

Like @cor3ntin I'm concerned about removing the transform call. I'm just as concerned that it caused no regressions...

If we have to transform the arguments, then this can be a dependent thing, which means the callee should be possible to be dependent, right? Thus needs to be transformed somewhere?

Oh, I didn't get @cor3ntin 's question in the first place! The question was where the callee instantiated, right? The callee is instantiated when the whole operator call expr is rebuilt in Sema::RebuildCXXOperatorCallExpr. From what I see, Sema::RebuildCXXOperatorCallExpr doesn't really care which callee is passed there. It checks which operation it represents and performs lookup depending on the arguments. This triggers instantiation.

Please see the test I'm adding. There is several cases where the operator callee is dependent. They work fine with the change. I also see the instantiated function decl in the AST, just the same as before the change.

[rsmith]

clang/lib/Sema/TreeTransform.h

11965–11968

To me it looks like we need to transform the callee in order to transform the declarations in the UnresolvedLookupExpr, if the operator has such functions. For example, in:

namespace A {
  template<typename T> T f(T t) {
    T operator+(T, T);
    return t + t;
  }
}
namespace B {
  struct X {};
}
void g(B::X x) { A::f(x); }

... we need to transform the UnresolvedLookupExpr for the t + t to map from the operator+ declaration in the A::f template to the instantiated operator+ declaration in A::f<B::X>.

But I think this patch is going in the right direction. We don't *actually* want to transform the callee; all we want to do is to transform the function or list of functions contained in the callee to form an UnresolvedSet of candidates found during the template parse. Building and then throwing away a DeclRefExpr or UnresolvedLookupExpr denoting that function or set of functions is both a waste of time and (as demonstrated by the bug being addressed here) problematic.

Instead of calling TransformExpr on the callee, we should be calling TransformOverloadExprDecls if the callee is an UnresolvedLookupExpr or TransformDecl if it's a DeclRefExpr, and I think RebuildCXXOperatorCallExpr should be changed to take an UnresolvedSet and a RequiresADL flag, and it looks like we can then remove the OrigCallee parameter, if we pass in some extra source locations.

[Fznamznon]

clang/lib/Sema/TreeTransform.h

11965–11968

Thank you for the feedback @rsmith , I appreciate it very much.
I tried the described approach, and it fixes the original bug I was trying to resolve as well as makes the mentioned example work correctly whereas my original change broke it. Apparently, we don't have test like that in check-clang set. I'm going to polish the resulting code a bit and put up to review tomorrow.

[Fznamznon]

Transform the callee, add a test

[Fznamznon]

Rebase

[Fznamznon]

libcxx CI seems to be failing for other patches as well.

[cor3ntin]

clang/lib/Sema/TreeTransform.h

12002

Can Functions ever held more than one element here?

[Fznamznon]

clang/lib/Sema/TreeTransform.h

12002

Good catch! I don't think that more than one element can happen here.

[Fznamznon]

Use 1-element UnresolvedSet, rebase

[cor3ntin]

clang/lib/Sema/TreeTransform.h

11988–12014

I don't think this is useful, it is only used L11999 but not in L11994, and in either cases you can use ULE->requiresADL()

12015

here i think RequiresADL is always false

15214–15219

Maybe it would be worth investigating that further before merging the PR? Although the pr is a clear improvement so I'll let you decide what to do!

[Fznamznon]

clang/lib/Sema/TreeTransform.h

15214–15219

I've noticed that it is likely a dead code, so I didn't want to pass more parameters to RebuildCXXOperatorCallExpr in order to support dead code, but didn't feel confident enough to remove it. So I left this FIXME.
RebuildCXXOperatorCallExpr is only called by TransformCXXOperatorCallExpr and TransformCXXFoldExpr. When subscript expression comes to TransformCXXOperatorCallExpr, it never falls down to RebuildCXXOperatorCallExpr after c1512250960bd247519a9f959ad4af202402dcc4 , and I don't think that it is possible to have subscript expression as a part of a fold expression. So, for me, it seems the whole if and FIXME actually can be removed. WDYT?

[Fznamznon]

Remove RequiresADL

[cor3ntin]

clang/lib/Sema/TreeTransform.h

15214–15219

I think you are right, It's seems like something I should have done as part of rGc1512250960bd247519a9f959ad4af202402dcc4.

It think you should remove it. (if we break something we can put it back with tests!)

[Fznamznon]

Remove dead code

[cor3ntin]

I think this looks good!
Thanks

clang/docs/ReleaseNotes.rst

 - Fix crash when passing a braced initializer list to a parentehsized aggregate
   initialization expression.
   (`#63008 <https://github.com/llvm/llvm-project/issues/63008>`_).
 - Reject increment of bool value in unevaluated contexts after C++17.
   (`#47517 <https://github.com/llvm/llvm-project/issues/47517>`_).
 - Fix assertion and quality of diagnostic messages in a for loop
   containing multiple declarations and a range specifier
   (`#63010 <https://github.com/llvm/llvm-project/issues/63010>`_).
+- Fix rejects-valid when consteval operator appears inside of a template.
+  (`#62886 <https://github.com/llvm/llvm-project/issues/62886>`_).
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/TreeTransform.h

   ///
   /// By default, performs semantic analysis to build the new expression.
   /// The semantic analysis provides the behavior of template instantiation,
   /// copying with transformations that turn what looks like an overloaded
   /// operator call into a use of a builtin operator, performing
   /// argument-dependent lookup, etc. Subclasses may override this routine to
   /// provide different behavior.
   ExprResult RebuildCXXOperatorCallExpr(OverloadedOperatorKind Op,
-                                              SourceLocation OpLoc,
-                                              Expr *Callee,
-                                              Expr *First,
-                                              Expr *Second);
+                                        SourceLocation CalleeLoc,
+                                        bool RequiresADL,
+                                        const UnresolvedSetImpl &Functions,
+                                        Expr *First, Expr *Second);
 
   /// Build a new C++ "named" cast expression, such as static_cast or
   /// reinterpret_cast.
   ///
   /// By default, this routine dispatches to one of the more-specific routines
   /// for a particular named case, e.g., RebuildCXXStaticCastExpr().
   /// Subclasses may override this routine to provide different behavior.
   ExprResult RebuildCXXNamedCastExpr(SourceLocation OpLoc,
#include "clang/Basic/OperatorKinds.def"
   case OO_Conditional:
     llvm_unreachable("conditional operator is not actually overloadable");
 
   case OO_None:
   case NUM_OVERLOADED_OPERATORS:
     llvm_unreachable("not an overloaded operator?");
   }
 
-  ExprResult Callee = getDerived().TransformExpr(E->getCallee());
-  if (Callee.isInvalid())
-    return ExprError();
-
   ExprResult First;
   if (E->getOperator() == OO_Amp)
     First = getDerived().TransformAddressOfOperand(E->getArg(0));
   else
     First = getDerived().TransformExpr(E->getArg(0));
   if (First.isInvalid())
     return ExprError();
 
   ExprResult Second;
   if (E->getNumArgs() == 2) {
     Second =
         getDerived().TransformInitializer(E->getArg(1), /*NotCopyInit=*/false);
     if (Second.isInvalid())
       return ExprError();
   }
 
-  if (!getDerived().AlwaysRebuild() &&
-      Callee.get() == E->getCallee() &&
-      First.get() == E->getArg(0) &&
-      (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
-    return SemaRef.MaybeBindToTemporary(E);
-
   Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   FPOptionsOverride NewOverrides(E->getFPFeatures());
   getSema().CurFPFeatures =
       NewOverrides.applyOverrides(getSema().getLangOpts());
   getSema().FpPragmaStack.CurrentValue = NewOverrides;
 
-  return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
-                                                 E->getOperatorLoc(),
-                                                 Callee.get(),
-                                                 First.get(),
-                                                 Second.get());
+  Expr *Callee = E->getCallee();
+  if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(Callee)) {
+    LookupResult R(SemaRef, ULE->getName(), ULE->getNameLoc(),
+                   Sema::LookupOrdinaryName);
+    if (getDerived().TransformOverloadExprDecls(ULE, ULE->requiresADL(), R))
+      return ExprError();
+
+    return getDerived().RebuildCXXOperatorCallExpr(
+        E->getOperator(), E->getOperatorLoc(), Callee->getBeginLoc(),
+        ULE->requiresADL(), R.asUnresolvedSet(), First.get(), Second.get());
+  }
+
+  UnresolvedSet<1> Functions;
+  if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(Callee))
+    Callee = ICE->getSubExprAsWritten();
+  NamedDecl *DR = cast<DeclRefExpr>(Callee)->getDecl();
+  ValueDecl *VD = cast_or_null<ValueDecl>(
+      getDerived().TransformDecl(DR->getLocation(), DR));
+  if (!VD)
+    return ExprError();
+
+  if (!isa<CXXMethodDecl>(VD))
+    Functions.addDecl(VD);
+
+  return getDerived().RebuildCXXOperatorCallExpr(
+      E->getOperator(), E->getOperatorLoc(), Callee->getBeginLoc(),
+      /*RequiresADL=*/false, Functions, First.get(), Second.get());
 }
 
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformCXXMemberCallExpr(CXXMemberCallExpr *E) {
   return getDerived().TransformCallExpr(E);
 }
 
 template <typename Derived>
if (Out.get()->containsUnexpandedParameterPack()) {
           Callee, E->getBeginLoc(), LeftFold ? Result.get() : Out.get(),
           E->getOperator(), E->getEllipsisLoc(),
           LeftFold ? Out.get() : Result.get(), E->getEndLoc(),
           OrigNumExpansions);
     } else if (Result.isUsable()) {
       // We've got down to a single element; build a binary operator.
       Expr *LHS = LeftFold ? Result.get() : Out.get();
       Expr *RHS = LeftFold ? Out.get() : Result.get();
-      if (Callee)
+      if (Callee) {
+        UnresolvedSet<16> Functions;
+        Functions.append(Callee->decls_begin(), Callee->decls_end());
         Result = getDerived().RebuildCXXOperatorCallExpr(
             BinaryOperator::getOverloadedOperator(E->getOperator()),
-            E->getEllipsisLoc(), Callee, LHS, RHS);
-      else
+            E->getEllipsisLoc(), Callee->getBeginLoc(), Callee->requiresADL(),
+            Functions, LHS, RHS);
+      } else {
         Result = getDerived().RebuildBinaryOperator(E->getEllipsisLoc(),
                                                     E->getOperator(), LHS, RHS);
+      }
     } else
       Result = Out;
 
     if (Result.isInvalid())
       return true;
   }
 
   // If we're retaining an expansion for a left fold, it is the outermost
TreeTransform<Derived>::RebuildTemplateName(CXXScopeSpec &SS,
   Name.setOperatorFunctionId(NameLoc, Operator, SymbolLocations);
   Sema::TemplateTy Template;
   getSema().ActOnTemplateName(
       /*Scope=*/nullptr, SS, TemplateKWLoc, Name, ParsedType::make(ObjectType),
       /*EnteringContext=*/false, Template, AllowInjectedClassName);
   return Template.get();
 }
 
-template<typename Derived>
-ExprResult
-TreeTransform<Derived>::RebuildCXXOperatorCallExpr(OverloadedOperatorKind Op,
-                                                   SourceLocation OpLoc,
-                                                   Expr *OrigCallee,
-                                                   Expr *First,
-                                                   Expr *Second) {
-  Expr *Callee = OrigCallee->IgnoreParenCasts();
+template <typename Derived>
+ExprResult TreeTransform<Derived>::RebuildCXXOperatorCallExpr(
+    OverloadedOperatorKind Op, SourceLocation OpLoc, SourceLocation CalleeLoc,
+    bool RequiresADL, const UnresolvedSetImpl &Functions, Expr *First,
   bool isPostIncDec = Second && (Op == OO_PlusPlus || Op == OO_MinusMinus);
 
   if (First->getObjectKind() == OK_ObjCProperty) {
     BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
     if (BinaryOperator::isAssignmentOp(Opc))
       return SemaRef.checkPseudoObjectAssignment(/*Scope=*/nullptr, OpLoc, Opc,
                                                  First, Second);
     ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
     if (Result.isInvalid())
       return ExprError();
     First = Result.get();
   }
 
   if (Second && Second->getObjectKind() == OK_ObjCProperty) {
     ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
     if (Result.isInvalid())
       return ExprError();
     Second = Result.get();
   }
 
   // Determine whether this should be a builtin operation.
   if (Op == OO_Subscript) {
     if (!First->getType()->isOverloadableType() &&
         !Second->getType()->isOverloadableType())
-      return getSema().CreateBuiltinArraySubscriptExpr(
-          First, Callee->getBeginLoc(), Second, OpLoc);
+      return getSema().CreateBuiltinArraySubscriptExpr(First, CalleeLoc, Second,
+                                                       OpLoc);
   } else if (Op == OO_Arrow) {
     // It is possible that the type refers to a RecoveryExpr created earlier
     // in the tree transformation.
     if (First->getType()->isDependentType())
       return ExprError();
     // -> is never a builtin operation.
     return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
if (!First->getType()->isOverloadableType() &&
         = SemaRef.CreateBuiltinBinOp(OpLoc, Opc, First, Second);
       if (Result.isInvalid())
         return ExprError();
 
       return Result;
     }
   }
 
-  // Compute the transformed set of functions (and function templates) to be
-  // used during overload resolution.
-  UnresolvedSet<16> Functions;
-  bool RequiresADL;
-
-  if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(Callee)) {
-    Functions.append(ULE->decls_begin(), ULE->decls_end());
-    // If the overload could not be resolved in the template definition
-    // (because we had a dependent argument), ADL is performed as part of
-    // template instantiation.
-    RequiresADL = ULE->requiresADL();
-  } else {
-    // If we've resolved this to a particular non-member function, just call
-    // that function. If we resolved it to a member function,
-    // CreateOverloaded* will find that function for us.
-    NamedDecl *ND = cast<DeclRefExpr>(Callee)->getDecl();
-    if (!isa<CXXMethodDecl>(ND))
-      Functions.addDecl(ND);
-    RequiresADL = false;
-  }
-
   // Add any functions found via argument-dependent lookup.
   Expr *Args[2] = { First, Second };
   unsigned NumArgs = 1 + (Second != nullptr);
 
   // Create the overloaded operator invocation for unary operators.
   if (NumArgs == 1 || isPostIncDec) {
     UnaryOperatorKind Opc
       = UnaryOperator::getOverloadedOpcode(Op, isPostIncDec);
     return SemaRef.CreateOverloadedUnaryOp(OpLoc, Opc, Functions, First,
                                            RequiresADL);
   }
 
-  if (Op == OO_Subscript) {
-    SourceLocation LBrace;
-    SourceLocation RBrace;
-
-    if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Callee)) {
-      DeclarationNameLoc NameLoc = DRE->getNameInfo().getInfo();
-      LBrace = NameLoc.getCXXOperatorNameBeginLoc();
-      RBrace = NameLoc.getCXXOperatorNameEndLoc();
-    } else {
-      LBrace = Callee->getBeginLoc();
-      RBrace = OpLoc;
-    }
-
-    return SemaRef.CreateOverloadedArraySubscriptExpr(LBrace, RBrace,
-                                                      First, Second);
-  }
+  // FIXME: The following code for subscript expression is either never executed
+  // or not tested by check-clang.
+  if (Op == OO_Subscript)
+    return SemaRef.CreateOverloadedArraySubscriptExpr(CalleeLoc, OpLoc, First,
+                                                      Second);
 
   // Create the overloaded operator invocation for binary operators.
   BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
   ExprResult Result = SemaRef.CreateOverloadedBinOp(
       OpLoc, Opc, Functions, Args[0], Args[1], RequiresADL);
   if (Result.isInvalid())
     return ExprError();
 
   return Result;

clang/test/SemaCXX/consteval-operators.cpp

+// RUN: %clang_cc1 -std=c++2a -emit-llvm-only -Wno-unused-value %s -verify
+
+// expected-no-diagnostics
+
+struct A {
+  consteval A operator+() { return {}; }
+};
+consteval A operator~(A) { return {}; }
+consteval A operator+(A, A) { return {}; }
+
+template <class> void f() {
+  A a;
+  A b = ~a;
+  A c = a + a;
+  A d = +a;
+}
+template void f<int>();
+
+template <class T> void foo() {
+  T a;
+  T b = ~a;
+  T c = a + a;
+  T d = +a;
+}
+
+template void foo<A>();
+
+template <typename DataT> struct B { DataT D; };
+
+template <typename DataT>
+consteval B<DataT> operator+(B<DataT> lhs, B<DataT> rhs) {
+  return B<DataT>{lhs.D + rhs.D};
+}
+
+template <class T> consteval T template_add(T a, T b) { return a + b; }
+
+consteval B<int> non_template_add(B<int> a, B<int> b) { return a + b; }
+
+void bar() {
+  constexpr B<int> a{};
+  constexpr B<int> b{};
+  auto constexpr c = a + b;
+}
+
+static_assert((template_add(B<int>{7}, B<int>{3})).D == 10);
+static_assert((non_template_add(B<int>{7}, B<int>{3})).D == 10);

clang/test/SemaCXX/overloaded-operator.cpp

#define idB ((void()), B())
   float &operator!(B);
   float &operator+=(B, B);
   float &operator<<(B, B);
   float &operator++(B);
   float &operator++(B, int);
   float &operator->*(B, B);
   template void f<int>();
 }
+
+namespace test {
+namespace A {
+template<typename T> T f(T t) {
+  T operator+(T, T);
+  return t + t;
+}
+}
+namespace B {
+  struct X {};
+}
+void g(B::X x) { A::f(x); }
+}