PR45589: Properly decompose overloaded && and || operators in

zygoloid · zygoloid · commit 6c29073efb0c · 2020-05-12T13:45:45.000-07:00
constraint expressions.

We create overloaded `&amp;&amp;` and `||` operators to hold the possible
unqualified lookup results (if any) when the operands are dependent. We
could avoid building these in some cases (we will never use the stored
lookup results, and it would be better to not store them or perform the
lookups), but in the general case we will probably still need to handle
overloaded operators even with that optimization.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -6394,15 +6394,10 @@ class Sema final {
   /// A diagnostic is emitted if it is not, false is returned, and
   /// PossibleNonPrimary will be set to true if the failure might be due to a
   /// non-primary expression being used as an atomic constraint.
-  bool CheckConstraintExpression(Expr *CE, Token NextToken = Token(),
+  bool CheckConstraintExpression(const Expr *CE, Token NextToken = Token(),
                                  bool *PossibleNonPrimary = nullptr,
                                  bool IsTrailingRequiresClause = false);
 
-  /// Check whether the given type-dependent expression will be the name of a
-  /// function or another callable function-like entity (e.g. a function
-  // template or overload set) for any substitution.
-  bool IsDependentFunctionNameExpr(Expr *E);
-
 private:
   /// Caches pairs of template-like decls whose associated constraints were
   /// checked for subsumption and whether or not the first's constraints did in
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
@@ -28,21 +28,47 @@
 using namespace clang;
 using namespace sema;
 
-bool
-Sema::CheckConstraintExpression(Expr *ConstraintExpression, Token NextToken,
-                                bool *PossibleNonPrimary,
-                                bool IsTrailingRequiresClause) {
+namespace {
+class LogicalBinOp {
+  OverloadedOperatorKind Op = OO_None;
+  const Expr *LHS = nullptr;
+  const Expr *RHS = nullptr;
+
+public:
+  LogicalBinOp(const Expr *E) {
+    if (auto *BO = dyn_cast<BinaryOperator>(E)) {
+      Op = BinaryOperator::getOverloadedOperator(BO->getOpcode());
+      LHS = BO->getLHS();
+      RHS = BO->getRHS();
+    } else if (auto *OO = dyn_cast<CXXOperatorCallExpr>(E)) {
+      Op = OO->getOperator();
+      LHS = OO->getArg(0);
+      RHS = OO->getArg(1);
+    }
+  }
+
+  bool isAnd() const { return Op == OO_AmpAmp; }
+  bool isOr() const { return Op == OO_PipePipe; }
+  explicit operator bool() const { return isAnd() || isOr(); }
+
+  const Expr *getLHS() const { return LHS; }
+  const Expr *getRHS() const { return RHS; }
+};
+}
+
+bool Sema::CheckConstraintExpression(const Expr *ConstraintExpression,
+                                     Token NextToken, bool *PossibleNonPrimary,
+                                     bool IsTrailingRequiresClause) {
   // C++2a [temp.constr.atomic]p1
   // ..E shall be a constant expression of type bool.
 
   ConstraintExpression = ConstraintExpression->IgnoreParenImpCasts();
 
-  if (auto *BinOp = dyn_cast<BinaryOperator>(ConstraintExpression)) {
-    if (BinOp->getOpcode() == BO_LAnd || BinOp->getOpcode() == BO_LOr)
-      return CheckConstraintExpression(BinOp->getLHS(), NextToken,
-                                       PossibleNonPrimary) &&
-             CheckConstraintExpression(BinOp->getRHS(), NextToken,
-                                       PossibleNonPrimary);
+  if (LogicalBinOp BO = ConstraintExpression) {
+    return CheckConstraintExpression(BO.getLHS(), NextToken,
+                                     PossibleNonPrimary) &&
+           CheckConstraintExpression(BO.getRHS(), NextToken,
+                                     PossibleNonPrimary);
   } else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpression))
     return CheckConstraintExpression(C->getSubExpr(), NextToken,
                                      PossibleNonPrimary);
@@ -60,7 +86,7 @@ Sema::CheckConstraintExpression(Expr *ConstraintExpression, Token NextToken,
           (NextToken.is(tok::l_paren) &&
            (IsTrailingRequiresClause ||
             (Type->isDependentType() &&
-             IsDependentFunctionNameExpr(ConstraintExpression)) ||
+             isa<UnresolvedLookupExpr>(ConstraintExpression)) ||
             Type->isFunctionType() ||
             Type->isSpecificBuiltinType(BuiltinType::Overload))) ||
           // We have the following case:
@@ -99,39 +125,37 @@ calculateConstraintSatisfaction(Sema &S, const Expr *ConstraintExpr,
                                 AtomicEvaluator &&Evaluator) {
   ConstraintExpr = ConstraintExpr->IgnoreParenImpCasts();
 
-  if (auto *BO = dyn_cast<BinaryOperator>(ConstraintExpr)) {
-    if (BO->getOpcode() == BO_LAnd || BO->getOpcode() == BO_LOr) {
-      if (calculateConstraintSatisfaction(S, BO->getLHS(), Satisfaction,
-                                          Evaluator))
-        return true;
+  if (LogicalBinOp BO = ConstraintExpr) {
+    if (calculateConstraintSatisfaction(S, BO.getLHS(), Satisfaction,
+                                        Evaluator))
+      return true;
 
-      bool IsLHSSatisfied = Satisfaction.IsSatisfied;
+    bool IsLHSSatisfied = Satisfaction.IsSatisfied;
 
-      if (BO->getOpcode() == BO_LOr && IsLHSSatisfied)
-        // [temp.constr.op] p3
-        //    A disjunction is a constraint taking two operands. To determine if
-        //    a disjunction is satisfied, the satisfaction of the first operand
-        //    is checked. If that is satisfied, the disjunction is satisfied.
-        //    Otherwise, the disjunction is satisfied if and only if the second
-        //    operand is satisfied.
-        return false;
+    if (BO.isOr() && IsLHSSatisfied)
+      // [temp.constr.op] p3
+      //    A disjunction is a constraint taking two operands. To determine if
+      //    a disjunction is satisfied, the satisfaction of the first operand
+      //    is checked. If that is satisfied, the disjunction is satisfied.
+      //    Otherwise, the disjunction is satisfied if and only if the second
+      //    operand is satisfied.
+      return false;
 
-      if (BO->getOpcode() == BO_LAnd && !IsLHSSatisfied)
-        // [temp.constr.op] p2
-        //    A conjunction is a constraint taking two operands. To determine if
-        //    a conjunction is satisfied, the satisfaction of the first operand
-        //    is checked. If that is not satisfied, the conjunction is not
-        //    satisfied. Otherwise, the conjunction is satisfied if and only if
-        //    the second operand is satisfied.
-        return false;
+    if (BO.isAnd() && !IsLHSSatisfied)
+      // [temp.constr.op] p2
+      //    A conjunction is a constraint taking two operands. To determine if
+      //    a conjunction is satisfied, the satisfaction of the first operand
+      //    is checked. If that is not satisfied, the conjunction is not
+      //    satisfied. Otherwise, the conjunction is satisfied if and only if
+      //    the second operand is satisfied.
+      return false;
 
-      return calculateConstraintSatisfaction(S, BO->getRHS(), Satisfaction,
-          std::forward<AtomicEvaluator>(Evaluator));
-    }
-  }
-  else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr))
+    return calculateConstraintSatisfaction(
+        S, BO.getRHS(), Satisfaction, std::forward<AtomicEvaluator>(Evaluator));
+  } else if (auto *C = dyn_cast<ExprWithCleanups>(ConstraintExpr)) {
     return calculateConstraintSatisfaction(S, C->getSubExpr(), Satisfaction,
         std::forward<AtomicEvaluator>(Evaluator));
+  }
 
   // An atomic constraint expression
   ExprResult SubstitutedAtomicExpr = Evaluator(ConstraintExpr);
@@ -725,19 +749,16 @@ NormalizedConstraint::fromConstraintExpr(Sema &S, NamedDecl *D, const Expr *E) {
   // - The normal form of an expression (E) is the normal form of E.
   // [...]
   E = E->IgnoreParenImpCasts();
-  if (auto *BO = dyn_cast<const BinaryOperator>(E)) {
-    if (BO->getOpcode() == BO_LAnd || BO->getOpcode() == BO_LOr) {
-      auto LHS = fromConstraintExpr(S, D, BO->getLHS());
-      if (!LHS)
-        return None;
-      auto RHS = fromConstraintExpr(S, D, BO->getRHS());
-      if (!RHS)
-        return None;
+  if (LogicalBinOp BO = E) {
+    auto LHS = fromConstraintExpr(S, D, BO.getLHS());
+    if (!LHS)
+      return None;
+    auto RHS = fromConstraintExpr(S, D, BO.getRHS());
+    if (!RHS)
+      return None;
 
-      return NormalizedConstraint(
-          S.Context, std::move(*LHS), std::move(*RHS),
-          BO->getOpcode() == BO_LAnd ? CCK_Conjunction : CCK_Disjunction);
-    }
+    return NormalizedConstraint(S.Context, std::move(*LHS), std::move(*RHS),
+                                BO.isAnd() ? CCK_Conjunction : CCK_Disjunction);
   } else if (auto *CSE = dyn_cast<const ConceptSpecializationExpr>(E)) {
     const NormalizedConstraint *SubNF;
     {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
@@ -18945,11 +18945,6 @@ ExprResult Sema::ActOnObjCAvailabilityCheckExpr(
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
 }
 
-bool Sema::IsDependentFunctionNameExpr(Expr *E) {
-  assert(E->isTypeDependent());
-  return isa<UnresolvedLookupExpr>(E);
-}
-
 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                     ArrayRef<Expr *> SubExprs, QualType T) {
   // FIXME: enable it for C++, RecoveryExpr is type-dependent to suppress
diff --git a/clang/test/SemaTemplate/constraints.cpp b/clang/test/SemaTemplate/constraints.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify %s -DDEPENDENT_OR
+
+#ifdef DEPENDENT_OR
+// This causes the || below to be a CXXOperatorCallExpr not a BinaryOperator.
+struct A {}; bool operator||(A, A);
+#endif
+
+namespace PR45589 {
+  template<typename T> struct X { static constexpr bool value = T::value; }; // expected-error {{cannot be used prior to '::'}}
+  struct False { static constexpr bool value = false; };
+  struct True { static constexpr bool value = true; };
+
+  template<typename T> concept C = true;
+
+  template<bool B, typename T> constexpr int test = 0;
+  template<bool B, typename T> requires C<T> constexpr int test<B, T> = 1;
+  template<bool B, typename T> requires (B && C<T>) || (X<T>::value && C<T>) constexpr int test<B, T> = 2; // expected-error {{non-constant expression}} expected-note {{subexpression}} expected-note {{instantiation of}} expected-note {{while substituting}}
+  static_assert(test<true, False> == 2);
+  static_assert(test<true, True> == 2);
+  static_assert(test<true, char> == 2); // satisfaction of second term of || not considered
+  static_assert(test<false, False> == 1);
+  static_assert(test<false, True> == 2); // constraints are partially ordered
+  // FIXME: These diagnostics are excessive.
+  static_assert(test<false, char> == 1); // expected-note 2{{while}} expected-note 2{{during}}
+}
