Forbid implicit conversion of constraint expression to bool

[erichkeane]

Summary

As reported in https://github.com/llvm/llvm-project/issues/54524, and
later in https://github.com/llvm/llvm-project/issues/60038, we were not
properly implmenting temp.constr.atomic P3. This patch stops implicitly
converting constraints to bool, and ensures the Rvalue conversion takes
place as needed.

[erichkeane]

@ldionne @Mordante The libcxx failure isn't clear what it is complaining about, but I thought this fix might end up breaking a libcxx test, since it would be easy to fall into the trap of having an implicit conversion here.

Any chance you could prod at that test a little and see whose fault it is?

ALSO: NOTE TO SELF: need release note!

[philnik]

In D141954#4060212, @erichkeane wrote:

@ldionne @Mordante The libcxx failure isn't clear what it is complaining about, but I thought this fix might end up breaking a libcxx test, since it would be easy to fall into the trap of having an implicit conversion here.

Any chance you could prod at that test a little and see whose fault it is?

ALSO: NOTE TO SELF: need release note!

I think the problem is that __type_traits/common_reference.h doesn't re-export __type_traits/common_type.h in the modulemap. It should be enough to add export common_type to module common_reference (libcxx/include/module.modulemap.in:1399). See module is_arithmetic for an example. This is just a guess though, this stuff can be very weird.

[erichkeane]

In D141954#4060244, @philnik wrote:

In D141954#4060212, @erichkeane wrote:

@ldionne @Mordante The libcxx failure isn't clear what it is complaining about, but I thought this fix might end up breaking a libcxx test, since it would be easy to fall into the trap of having an implicit conversion here.

Any chance you could prod at that test a little and see whose fault it is?

ALSO: NOTE TO SELF: need release note!

I think the problem is that __type_traits/common_reference.h doesn't re-export __type_traits/common_type.h in the modulemap. It should be enough to add export common_type to module common_reference (libcxx/include/module.modulemap.in:1399). See module is_arithmetic for an example. This is just a guess though, this stuff can be very weird.

Oh man, I hope the others know what you're talking about :) I don't really know how I caused this though :/

[shafik]

clang/lib/Sema/SemaConcept.cpp

379

minor fix

[philnik]

In D141954#4060246, @erichkeane wrote:

In D141954#4060244, @philnik wrote:

In D141954#4060212, @erichkeane wrote:

@ldionne @Mordante The libcxx failure isn't clear what it is complaining about, but I thought this fix might end up breaking a libcxx test, since it would be easy to fall into the trap of having an implicit conversion here.

Any chance you could prod at that test a little and see whose fault it is?

ALSO: NOTE TO SELF: need release note!

I think the problem is that __type_traits/common_reference.h doesn't re-export __type_traits/common_type.h in the modulemap. It should be enough to add export common_type to module common_reference (libcxx/include/module.modulemap.in:1399). See module is_arithmetic for an example. This is just a guess though, this stuff can be very weird.

Oh man, I hope the others know what you're talking about :) I don't really know how I caused this though :/

I've applied your patch locally and this diff fixes the issue:

diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 5d4cf53aa334..30209b353d6b 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -747,7 +747,10 @@ module std [system] {
       module assignable                 { private header "__concepts/assignable.h" }
       module boolean_testable           { private header "__concepts/boolean_testable.h" }
       module class_or_enum              { private header "__concepts/class_or_enum.h" }
-      module common_reference_with      { private header "__concepts/common_reference_with.h" }
+      module common_reference_with      {
+        private header "__concepts/common_reference_with.h"
+        export type_traits.common_reference
+      }
       module common_with                { private header "__concepts/common_with.h" }
       module constructible              { private header "__concepts/constructible.h" }
       module convertible_to             { private header "__concepts/convertible_to.h" }

I don't know why your patch changes the behaviour of clang here, but I wouldn't worry too much about it. There is a lot of weirdness around modules and dependent type names resulting in pretty much useless error messages. I hope the work on C++ modules reduces this weirdness, but I don't really know how that stuff works.

[erichkeane]

In D141954#4060647, @philnik wrote:

In D141954#4060246, @erichkeane wrote:

In D141954#4060244, @philnik wrote:

In D141954#4060212, @erichkeane wrote:

@ldionne @Mordante The libcxx failure isn't clear what it is complaining about, but I thought this fix might end up breaking a libcxx test, since it would be easy to fall into the trap of having an implicit conversion here.

Any chance you could prod at that test a little and see whose fault it is?

ALSO: NOTE TO SELF: need release note!

I think the problem is that __type_traits/common_reference.h doesn't re-export __type_traits/common_type.h in the modulemap. It should be enough to add export common_type to module common_reference (libcxx/include/module.modulemap.in:1399). See module is_arithmetic for an example. This is just a guess though, this stuff can be very weird.

Oh man, I hope the others know what you're talking about :) I don't really know how I caused this though :/

I've applied your patch locally and this diff fixes the issue:

diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 5d4cf53aa334..30209b353d6b 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -747,7 +747,10 @@ module std [system] {
       module assignable                 { private header "__concepts/assignable.h" }
       module boolean_testable           { private header "__concepts/boolean_testable.h" }
       module class_or_enum              { private header "__concepts/class_or_enum.h" }
-      module common_reference_with      { private header "__concepts/common_reference_with.h" }
+      module common_reference_with      {
+        private header "__concepts/common_reference_with.h"
+        export type_traits.common_reference
+      }
       module common_with                { private header "__concepts/common_with.h" }
       module constructible              { private header "__concepts/constructible.h" }
       module convertible_to             { private header "__concepts/convertible_to.h" }

I don't know why your patch changes the behaviour of clang here, but I wouldn't worry too much about it. There is a lot of weirdness around modules and dependent type names resulting in pretty much useless error messages. I hope the work on C++ modules reduces this weirdness, but I don't really know how that stuff works.

Thanks! I'll apply that patch here and see if it makes the bot happy.

[erichkeane]

Add module map fix, add nit from Shafik.

[erichkeane]

Also, I forgot I added the release notes in that last patch :)

[erichkeane]

Speaking of: I guess the libcxx failure is NOT caused by me? https://reviews.llvm.org/D141992

I see this on a different review, despite this not being committed.

[philnik]

In D141954#4061962, @erichkeane wrote:

Speaking of: I guess the libcxx failure is NOT caused by me? https://reviews.llvm.org/D141992

I see this on a different review, despite this not being committed.

That's not great. Seems like some recently landed patch broke this. Do you mind committing the libc++-part of this patch to get the Clang CI green again? (Assuming the libc++ CI is green)

[erichkeane]

In D141954#4062029, @philnik wrote:

In D141954#4061962, @erichkeane wrote:

Speaking of: I guess the libcxx failure is NOT caused by me? https://reviews.llvm.org/D141992

I see this on a different review, despite this not being committed.

That's not great. Seems like some recently landed patch broke this. Do you mind committing the libc++-part of this patch to get the Clang CI green again? (Assuming the libc++ CI is green)

If this passes, I might just do the module-map patch as a separate commit to fix the CI, else the libcxx history is going to be awful strange :) I'm probably a bit from having this review approved anyway, as it is still 'night time' for most of my reviewers :)

[erichkeane]

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

[Mordante]

I see the current main pass for the module build in libc++. The error message seems different, from the message for missing parts of the module map. I'll dig a bit further to see whether I can find what's wrong.

[Mordante]

I just tested locally with the latest nightly build

Debian clang version 16.0.0 (++20230116071901+2563ad8ef1a8-1~exp1~20230116072002.532)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

and that passes the test that fails in the CI.

Can you test whether the issue occurs for you in HEAD without your patch.

@ldionne would it make sense to test the Clang CI in the scheduled build too, that would make pin-pointing a possible regression easier.

[erichkeane]

In D141954#4062669, @Mordante wrote:

I just tested locally with the latest nightly build

Debian clang version 16.0.0 (++20230116071901+2563ad8ef1a8-1~exp1~20230116072002.532)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

and that passes the test that fails in the CI.

Can you test whether the issue occurs for you in HEAD without your patch.

@ldionne would it make sense to test the Clang CI in the scheduled build too, that would make pin-pointing a possible regression easier.

I don't have the libcxx build/tests set up in my environment anymore (and it was a bit of a hassle the last time, which was my motivation for asking @ldionne for pre-commit on clang patches :D ), so I don't really have the ability to run that test.

[tahonermann]

clang/lib/Sema/SemaConcept.cpp

377–380

ImplicitCastExpr::Create() has the following assertion. I wonder if we need to guard against this here.

clang/lib/AST/Expr.cpp:
2073   assert((Kind != CK_LValueToRValue ||
2074           !(T->isNullPtrType() || T->getAsCXXRecordDecl())) &&
2075          "invalid type for lvalue-to-rvalue conversion");

Or perhaps it would be better to call Sema::DefaultLvalueConversion() here?

383

This comment appears to be unnecessary.

clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp

3–4

I'm not sure what this is intending to exercise. C isn't used elsewhere; this just appears to validate that a well-formed concept declaration is accepted (even if it is one that will always evaluate as false). I suggest modifying it to drop == 4 && !true and then validate that a diagnostic is issued (which might require a use of the concept).

28

These tests look good.

I think we should also exercise nested requirements to ensure that behavior is consistent:

template<typename T> requires requires {
  requires S<T>{};
}
void f3(T);
void f3(int);

Perhaps also add some examples where && does not denote a conjunction of atomic constraints. For example:

template<typename T> requires (bool(1 && 2))
void f4(T);
void f4(int);

libcxx/include/module.modulemap.in

750–753

I agree with this being done in a separate commit.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

377–380

I don't think we want all the baggage that DefaultLvalueConversion gets us, including diagnostics/etc. That assert IS concerning though... I presume we can come up with something to hit that.

It DOES look like that DefeaultLValue conversion replaces the nullptrtypes with a NullToPointer cast, so perhaps I should do that, and doesn't do anything with a record type.

383

Woops! I thought that wasn't mine :D

clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp

3–4

This is exactly out of the standard, which is why I copied it. however, a test for the diagnostic version seems like a good idea.

[erichkeane]

Added tests suggested by Tom, also moved the cast creation after checking of the return type, so that we end up not hitting hte assert that Tom came up with. Added 2 tests that show the two conditions (that DID repro as a crash).

[tahonermann]

clang/lib/Sema/SemaConcept.cpp

377–380

I'm surprised the additional checks for nullptr aren't tripping that assert; it looks to me like it should. Any ideas?

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

377–380

I moved the creation of this AFTER 'CheckConstraintExpression', which will fail unless the type is 'bool' already. SO the expression here cannot be a nullptr or struct type. Before I moved it and wrote the new tests (NullTy and StructTy), it actually did hit the assert you mentioned.

[erichkeane]

Fix clang-format, hopefully this saves me from the bots ;)

[tahonermann]

Looks good to me.

clang/lib/Sema/SemaConcept.cpp

377–380

Ah, perfect!

clang/docs/ReleaseNotes.rst

 - Implemented CWG2635 as a Defect Report, which prohibits structured bindings from being constrained.
 - Correctly handle access-checks in requires expression. Fixes `GH53364 <https://github.com/llvm/llvm-project/issues/53364>`_,
   `GH53334 <https://github.com/llvm/llvm-project/issues/53334>`_.
 
 - Implemented `P0960R3: <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html>`_
   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
 ^^^^^^^^^^^^^^^^^^^^^
 
 - Support label at end of compound statement (`P2324 <https://wg21.link/p2324r2>`_).
 - Implemented `P1169R4: static operator() <https://wg21.link/P1169R4>`_ and `P2589R1: static operator[] <https://wg21.link/P2589R1>`_.
 - Implemented "char8_t Compatibility and Portability Fix" (`P2513R3 <https://wg21.link/P2513R3>`_).
   This change was applied to C++20 as a Defect Report.
 - Implemented "Permitting static constexpr variables in constexpr functions" (`P2647R1 <https://wg21.link/P2647R1>_`).

clang/lib/Sema/SemaConcept.cpp

return calculateConstraintSatisfaction(
           }
 
           SatisfactionStackRAII StackRAII(S, ID);
 
           // We do not want error diagnostics escaping here.
           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
             //   constraint is not satisfied.
             if (!Trap.hasErrorOccurred())
               // A non-SFINAE error has occurred as a result of this
               // substitution.
               return ExprError();
return calculateConstraintSatisfaction(
                 AtomicExpr,
                 new (S.Context) ConstraintSatisfaction::SubstitutionDiagnostic{
                         SubstDiag.first, StringRef(Mem, MessageSize)});
             Satisfaction.IsSatisfied = false;
             return ExprEmpty();
           }
         }
 
+        // [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;
       });
 }
 
 static bool CheckConstraintSatisfaction(
     Sema &S, const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
     llvm::SmallVectorImpl<Expr *> &Converted,

clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp

+// 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@+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@+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
+}  

libcxx/include/module.modulemap.in

module concepts {
     header "concepts"
     export *
 
     module __concepts {
       module arithmetic                 { private header "__concepts/arithmetic.h" }
       module assignable                 { private header "__concepts/assignable.h" }
       module boolean_testable           { private header "__concepts/boolean_testable.h" }
       module class_or_enum              { private header "__concepts/class_or_enum.h" }
-      module common_reference_with      { private header "__concepts/common_reference_with.h" }
+      module common_reference_with      { 
+        private header "__concepts/common_reference_with.h" 
+        export type_traits.common_reference
+      }
       module common_with                { private header "__concepts/common_with.h" }
       module constructible              { private header "__concepts/constructible.h" }
       module convertible_to             { private header "__concepts/convertible_to.h" }
       module copyable                   { private header "__concepts/copyable.h" }
       module derived_from               { private header "__concepts/derived_from.h" }
       module destructible               { private header "__concepts/destructible.h" }
       module different_from             { private header "__concepts/different_from.h" }
       module equality_comparable        { private header "__concepts/equality_comparable.h" }