[Clang] Adjust triviality computation in QualType::isTrivialType to C++20 cases.

[royjacobson]

Summary

Up to C++20, hasDefaultConstructor and !hasNonTrivialDefaultConstructor together implied
hasTrivialDefaultConstructor. In C++20, a constructor might be ineligible and can set
hasDefaultConstructor without setting hasNonTrivialDefaultConstructor.

Fix this by querying hasTrivialDefaultConstructor instead of hasDefaultConstructor.

I'd like to backport this to Clang 16.
I only change isTrivialType and in a way that should only affect code that uses
constrained constructors, so I think this is relatively safe to backport.

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

[aaron.ballman]

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

[royjacobson]

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

[aaron.ballman]

In D143891#4122668, @royjacobson wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that __is_trivial and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.

[shafik]

In D143891#4122731, @aaron.ballman wrote:

In D143891#4122668, @royjacobson wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that __is_trivial and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.

I think this makes sense.

clang/lib/AST/Type.cpp

2509

Will you follow-up this change by updating CXXRecordDecl::isTrivial as well or is that a more difficult issue?

[royjacobson]

clang/lib/AST/Type.cpp

2509

I want to, but I need to check the call sites to make sure it won't change ABI or something. And I wanted a fix that I can safely backport :)

[erichkeane]

In D143891#4122731, @aaron.ballman wrote:

In D143891#4122668, @royjacobson wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that __is_trivial and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.

Avoiding hte rest of the discussion, because I cannot even:
ABI Breaks like this are typically OK, BUT we need to update the ClangABIVersion stuff, which includes only implementing this in the newest version.

[erichkeane]

In D143891#4125954, @erichkeane wrote:

In D143891#4122731, @aaron.ballman wrote:

In D143891#4122668, @royjacobson wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that __is_trivial and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.

Avoiding hte rest of the discussion, because I cannot even:
ABI Breaks like this are typically OK, BUT we need to update the ClangABIVersion stuff, which includes only implementing this in the newest version.

Aaron points out offline that this is Concepts specific, and can only be run into with a concept. I didn't realize on first read the 'ineligible' was 'disabled with a constraint'. If so, we don't really make any guarantees about ABI compat with Concepts yet, so the ClangABIVersion stuff is likely unnecessary.

[hubert.reinterpretcast]

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

In my opinion, this is an argument for not fixing in a fix release of a specific version of Clang--not an argument for additional option control in new versions.
Instantiations change all the time from changes to constant expression evaluation, overload resolution, partial ordering rules for templates, etc.
As for object layout, I believe the language and the ABI rules for triviality diverged quite some time ago.

I know that the evaluation for this specific case was that we don't need to apply ABI versioning control for this because it only affects code using Concepts, but I think we will eventually need to (re)discover when ABI versioning control is the appropriate tool.

I propose an action item: Someone needed to review past application of the ABI versioning options to see if we can extract and document workable criteria (perhaps in https://clang.llvm.org/docs/InternalsManual.html).

[erichkeane]

In D143891#4129214, @hubert.reinterpretcast wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

In my opinion, this is an argument for not fixing in a fix release of a specific version of Clang--not an argument for additional option control in new versions.
Instantiations change all the time from changes to constant expression evaluation, overload resolution, partial ordering rules for templates, etc.
As for object layout, I believe the language and the ABI rules for triviality diverged quite some time ago.

I know that the evaluation for this specific case was that we don't need to apply ABI versioning control for this because it only affects code using Concepts, but I think we will eventually need to (re)discover when ABI versioning control is the appropriate tool.

I propose an action item: Someone needed to review past application of the ABI versioning options to see if we can extract and document workable criteria (perhaps in https://clang.llvm.org/docs/InternalsManual.html).

Before doing that amount of work, I'd see if we could convince @rsmith to just share what his rule for this was, I think it was quite reasonable the few times he enforced it, and he can likely better state it at least. Even if he were to share here, I'm sure one of us would be willing to clean up his statement and put it in the Internals Manual.

[royjacobson]

It's an interesting discussion, I just want to remind that this change is for triviality rules from P0848 (conditionally trivial member functions) which is only going to ship in Clang 16 anyway.

[cor3ntin]

This makes sense to me as is. The check ClassDecl->hasTrivialDefaultConstructor() && !ClassDecl->hasNonTrivialDefaultConstructor() can only be different from status quo in C++20.
Changing isTrivial would be a lot more involved change that should not be in scope of this PR

[aaron.ballman]

LGTM but we should add a release note for the change.

[cor3ntin]

In D143891#4144161, @aaron.ballman wrote:

LGTM but we should add a release note for the change.

This fixes P0848R3, which is a clang 16 feature.
If we backport this we don't need a changelog entry.

clang/docs/ReleaseNotes.rst

 
 C++ Specific Potentially Breaking Changes
 -----------------------------------------
 - Clang won't search for coroutine_traits in std::experimental namespace any more.
   Clang will only search for std::coroutine_traits for coroutines then.
 
 ABI Changes in This Version
 ---------------------------
+- ``__is_trivial`` has changed for a small category of classes with constrained default constructors (`#60697 <https://github.com/llvm/llvm-project/issues/60697>`_).
+  *FIXME: Remove this note if we've backported this change to the Clang 16 branch.*
 
 What's New in Clang |release|?
 ==============================
 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 

clang/lib/AST/Type.cpp

bool QualType::isTrivialType(const ASTContext &Context) const {
   //   cv-qualified versions of these types are collectively called trivial
   //   types.
 
   // As an extension, Clang treats vector types as Scalar types.
   if (CanonicalType->isScalarType() || CanonicalType->isVectorType())
     return true;
   if (const auto *RT = CanonicalType->getAs<RecordType>()) {
     if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
-      // C++11 [class]p6:
-      //   A trivial class is a class that has a default constructor,
-      //   has no non-trivial default constructors, and is trivially
-      //   copyable.
-      return ClassDecl->hasDefaultConstructor() &&
+      // C++20 [class]p6:
+      //   A trivial class is a class that is trivially copyable, and
+      //     has one or more eligible default constructors such that each is
+      //     trivial.
+      // FIXME: We should merge this definition of triviality into
+      // CXXRecordDecl::isTrivial. Currently it computes the wrong thing.
+      return ClassDecl->hasTrivialDefaultConstructor() &&
              !ClassDecl->hasNonTrivialDefaultConstructor() &&
              ClassDecl->isTriviallyCopyable();
     }
 
     return true;
   }
 
   // No other types can match.

clang/test/SemaCXX/constrained-special-member-functions.cpp

 static_assert(!__is_trivial(C), "");
 static_assert(__is_trivial(D), "");
 static_assert(__is_trivially_constructible(A), "");
 static_assert(__is_trivially_constructible(B), "");
 static_assert(__is_trivially_constructible(C), "");
 static_assert(__is_trivially_constructible(D), "");
 
 }
+
+namespace GH60697 {
+
+template <class T>
+struct X {
+    X() requires false = default;
+};
+static_assert(!__is_trivial(X<int>));
+
+template <class T>
+struct S {
+    S() requires(__is_trivially_constructible(T)) = default;
+
+    S() requires(!__is_trivially_constructible(T) &&
+                  __is_constructible(T)) {}
+
+    T t;
+};
+
+struct D {
+    D(int i) : i(i) {}
+    int i;
+};
+static_assert(!__is_trivially_constructible(D));
+static_assert(!__is_constructible(D));
+static_assert(!__is_trivial(D));
+
+static_assert(!__is_trivially_constructible(S<D>));
+static_assert(!__is_constructible(S<D>));
+
+static_assert(__is_trivial(S<int>));
+static_assert(!__is_trivial(S<D>));
+
+}