[PATCH] D141954: Forbid implicit conversion of constraint expression to bool

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 490187.
erichkeane added a comment.

The changes to the module map file seemed to make things worse! I reverted it for now, hoping this just goes green.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141954/new/

https://reviews.llvm.org/D141954

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaConcept.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp


Index: clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp
===================================================================
--- /dev/null
+++ clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -std=c++20 -x c++ -verify %s
+
+template<typename T> concept C =
+sizeof(T) == 4 && !true;      // requires atomic constraints sizeof(T) == 4 and !true
+
+template<typename T> struct S {
+  constexpr operator bool() const { return true; }
+};
+
+// expected-error at +3{{atomic constraint must be of type 'bool' (found 'S<int>')}}
+// expected-note@#FINST{{while checking constraint satisfaction}}
+// expected-note@#FINST{{in instantiation of function template specialization}}
+template<typename T> requires (S<T>{})
+void f(T);
+void f(int);
+
+// Ensure this applies to operator && as well.
+// expected-error at +3{{atomic constraint must be of type 'bool' (found 'S<int>')}}
+// expected-note@#F2INST{{while checking constraint satisfaction}}
+// expected-note@#F2INST{{in instantiation of function template specialization}}
+template<typename T> requires (S<T>{} && true)
+void f2(T);
+void f2(int);
+
+void g() {
+  f(0); // #FINST
+  f2(0); // #F2INST
+}  
Index: clang/lib/Sema/SemaConcept.cpp
===================================================================
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -329,14 +329,7 @@
           Sema::SFINAETrap Trap(S);
           SubstitutedExpression =
               S.SubstConstraintExpr(const_cast<Expr *>(AtomicExpr), MLTAL);
-          // Substitution might have stripped off a contextual conversion to
-          // bool if this is the operand of an '&&' or '||'. For example, we
-          // might lose an lvalue-to-rvalue conversion here. If so, put it back
-          // before we try to evaluate.
-          if (SubstitutedExpression.isUsable() &&
-              !SubstitutedExpression.isInvalid())
-            SubstitutedExpression =
-                S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
+
           if (SubstitutedExpression.isInvalid() || Trap.hasErrorOccurred()) {
             // C++2a [temp.constr.atomic]p1
             //   ...If substitution results in an invalid type or expression, the
@@ -370,8 +363,24 @@
           }
         }
 
+        // [temp.constr.atomic]p3: To determine if an atomic constraint is
+        // satisfied, the parameter mapping and template arguments are first
+        // substituted into its expression.  If substitution results in an
+        // invalid type or expression, the constraint is not satisfied.
+        // Otherwise, the lvalue-to-rvalue conversion is performed if necessary,
+        // and E shall be a constant expression of type bool.
+        //
+        // Perform the L to R Value conversion if necessary. We do so for all
+        // non-PRValue categories, else we fail to extend the lifetime of
+        // temporaries, and that fails the constant expression check.
+        if (!SubstitutedExpression.get()->isPRValue())
+          SubstitutedExpression = ImplicitCastExpr::Create(
+              S.Context, SubstitutedExpression.get()->getType(),
+              CK_LValueToRValue, SubstitutedExpression.get(),
+              /*BasePath=*/nullptr, VK_PRValue, FPOptionsOverride());
+
         if (!S.CheckConstraintExpression(SubstitutedExpression.get()))
-          return ExprError();
+          return ExprError(); // convert to bool here?
 
         return SubstitutedExpression;
       });
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -767,6 +767,9 @@
   and `P1975R0: <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1975r0.html>`_,
   which allows parenthesized aggregate-initialization.
 
+- Fixed an issue with concept requirement evaluation, where we incorrectly allowed implicit
+  conversions to bool for a requirement.  This fixes `GH54524 <https://github.com/llvm/llvm-project/issues/54524>`_.
+
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141954.490187.patch
Type: text/x-patch
Size: 4184 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230118/d238f95a/attachment.bin>