Skip to content

[Clang] Added explanation why is_trivially_copyable evaluated to false. #142341

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cor3ntin merged 10 commits into from
Jun 7, 2025
Merged

[Clang] Added explanation why is_trivially_copyable evaluated to false. #142341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e423a7
[Clang] Added explanation why a is trivial copyable evaluated to false.
egorshamshura Jun 2, 2025
8edd53d
Fixed check for trivial destructor using Dtr->isTrivial() now, remove…
egorshamshura Jun 2, 2025
65ca243
Fixed for eligible special member functions
egorshamshura Jun 2, 2025
9880d4f
Removed Arc lifetime
egorshamshura Jun 2, 2025
2401565
Changed NR to NTR
egorshamshura Jun 2, 2025
94bec75
Added tests for non-trvial-copyable bases, for special members
egorshamshura Jun 2, 2025
8636714
Removed extra isIneligibleOrNotSelected check
egorshamshura Jun 2, 2025
437992d
Added test for multiple base classes
egorshamshura Jun 3, 2025
7f999b9
Fixed case with destructor and added test, fixed typo
egorshamshura Jun 3, 2025
1836846
Merge branch 'main' into explain-trivial-copyable
egorshamshura Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1764,7 +1764,8 @@ def err_user_defined_msg_constexpr : Error<

// Type traits explanations
def note_unsatisfied_trait : Note<"%0 is not %enum_select<TraitName>{"
"%TriviallyRelocatable{trivially relocatable}"
"%TriviallyRelocatable{trivially relocatable}|"
"%TriviallyCopyable{trivially copyable}"
"}1">;

def note_unsatisfied_trait_reason
Expand All @@ -1776,6 +1777,8 @@ def note_unsatisfied_trait_reason
"%VBase{has a virtual base %1}|"
"%NRBase{has a non-trivially-relocatable base %1}|"
"%NRField{has a non-trivially-relocatable member %1 of type %2}|"
"%NTCBase{has a non-trivially-copyable base %1}|"
Copy link
Collaborator

@erichkeane erichkeane Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum names here are inconsistent... non-trivially-relocatable took only NR whereas this is taking NTC. That said, I VASTLY prefer the T be present. Not in the scope here of course, but a followup tochange those NR to NTR perhaps is valuable.

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed NR to NTR

"%NTCField{has a non-trivially-copyable member %1 of type %2}|"
"%DeletedDtr{has a %select{deleted|user-provided}1 destructor}|"
"%UserProvidedCtr{has a user provided %select{copy|move}1 "
"constructor}|"
Expand Down
97 changes: 97 additions & 0 deletions clang/lib/Sema/SemaTypeTraits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
//===----------------------------------------------------------------------===//

#include "clang/AST/DeclCXX.h"
#include "clang/AST/Type.h"
#include "clang/Basic/DiagnosticParse.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/TypeTraits.h"
#include "clang/Sema/EnterExpressionEvaluationContext.h"
#include "clang/Sema/Initialization.h"
#include "clang/Sema/Lookup.h"
Expand Down Expand Up @@ -1922,6 +1924,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) {
return llvm::StringSwitch<std::optional<TypeTrait>>(Name)
.Case("is_trivially_relocatable",
TypeTrait::UTT_IsCppTriviallyRelocatable)
.Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable)
Copy link
Collaborator

@erichkeane erichkeane Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 isnt the number, but at a certain number of entries here, we're going to want to tablegen/.def file this. Perhaps in TokenKinds.def?

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I did it right, but I couldn't figure out how from TYPE_TRAIT_1 could get a name without the "__" prefix. So I made a TYPE_TRAIT_DIAG for this

Copy link
Collaborator

@erichkeane erichkeane Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, I was suggesting adding a new field to the TYPE_TRAIT_1/TYPE_TRAIT_N/etc variants. It does end up being a little bit of extra work, which is why I was mentioning it more as a followup/future direction here instead.

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I revert changes?

Copy link
Collaborator

@erichkeane erichkeane Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'd suggest going back to the original, unless you want to do all the work for updating the TYPE_TRAIT_N/TYPE_TRAIT_1 (which I'm not requiring here at all!). Just something to think about for a future direction.

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks)

Copy link
Contributor

@cor3ntin cor3ntin Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane yeah, maybe we should move the whole type traits thing to a tablegen file at some point... but it seems involved

.Default(std::nullopt);
}

Expand Down Expand Up @@ -2083,6 +2086,97 @@ static void DiagnoseNonTriviallyRelocatableReason(Sema &SemaRef,
SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
}

static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
SourceLocation Loc,
const CXXRecordDecl *D) {
for (const CXXBaseSpecifier &B : D->bases()) {
assert(B.getType()->getAsCXXRecordDecl() && "invalid base?");
if (B.isVirtual())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::VBase << B.getType()
<< B.getSourceRange();
if (!B.getType().isTriviallyCopyableType(D->getASTContext())) {
Copy link
Collaborator

@shafik shafik Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if statement covered in the tests below?

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test for it

SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::NTCBase << B.getType()
<< B.getSourceRange();
}
}
for (const FieldDecl *Field : D->fields()) {
if (!Field->getType().isTriviallyCopyableType(Field->getASTContext()))
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::NTCField << Field
<< Field->getType() << Field->getSourceRange();
}
if (D->hasDeletedDestructor())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::DeletedDtr << 0
<< D->getDestructor()->getSourceRange();

if (D->isUnion()) {
auto DiagSPM = [&](CXXSpecialMemberKind K, bool Has) {
if (Has)
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::UnionWithUserDeclaredSMF << K;
};
DiagSPM(CXXSpecialMemberKind::CopyConstructor,
D->hasUserDeclaredCopyConstructor());
DiagSPM(CXXSpecialMemberKind::CopyAssignment,
D->hasUserDeclaredCopyAssignment());
DiagSPM(CXXSpecialMemberKind::MoveConstructor,
D->hasUserDeclaredMoveConstructor());
DiagSPM(CXXSpecialMemberKind::MoveAssignment,
D->hasUserDeclaredMoveAssignment());
return;
}
Copy link
Contributor

@cor3ntin cor3ntin Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no special rules for unions for trivially copyable

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
const auto *Decl = cast<CXXConstructorDecl>(
LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
if (Decl && Decl->isUserProvided())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::UserProvidedCtr
<< Decl->isMoveConstructor() << Decl->getSourceRange();
}
if (!D->hasSimpleMoveAssignment() && !D->hasSimpleCopyAssignment()) {
CXXMethodDecl *Decl =
LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/true);
if (Decl && Decl->isUserProvided())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::UserProvidedAssign
<< Decl->isMoveAssignmentOperator() << Decl->getSourceRange();
}
Copy link
Contributor

@cor3ntin cor3ntin Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For trivially copyable, the rule is that all special members that exist must be trivial.
https://eel.is/c++draft/class.prop#1

I think it would be better to iterate over all eligible special member functions, and emit a diagnostic for any one that is not trivial (using D->methods(), FunctionDecl::isIneligibleOrNotSelected, FunctionDecl::getDefaultedFunctionKind, and FunctionDecl::isTrivial

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it, now I'm iterating over all special member functions

CXXDestructorDecl *Dtr = D->getDestructor();
if (Dtr && Dtr->isUserProvided() && !Dtr->isDefaulted())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::DeletedDtr << 1
<< Dtr->getSourceRange();
Copy link
Contributor

@cor3ntin cor3ntin Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to check that the destructor is trivial (Dtr->isTrivial())

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, now using Dtr->isTrivial()

}

static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
SourceLocation Loc, QualType T) {
SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
<< T << diag::TraitName::TriviallyCopyable;

if (T->isReferenceType())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::Ref;

T = T.getNonReferenceType();

if (T.hasNonTrivialObjCLifetime())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::HasArcLifetime;
Copy link
Contributor

@cor3ntin cor3ntin Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arc lifetimes do not impact the ability to copy a type afaik (but they might impact the ability to copy a class containing them, in which case they should have a deleted constructor) @ojhunt

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


const CXXRecordDecl *D = T->getAsCXXRecordDecl();
if (!D || D->isInvalidDecl())
return;

if (D->hasDefinition())
DiagnoseNonTriviallyCopyableReason(SemaRef, Loc, D);

SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
}

void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
E = E->IgnoreParenImpCasts();
if (E->containsErrors())
Expand All @@ -2097,6 +2191,9 @@ void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
case UTT_IsCppTriviallyRelocatable:
DiagnoseNonTriviallyRelocatableReason(*this, E->getBeginLoc(), Args[0]);
break;
case UTT_IsTriviallyCopyable:
DiagnoseNonTriviallyCopyableReason(*this, E->getBeginLoc(), Args[0]);
break;
default:
break;
}
Expand Down
72 changes: 72 additions & 0 deletions clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ struct is_trivially_relocatable {

template <typename T>
constexpr bool is_trivially_relocatable_v = __builtin_is_cpp_trivially_relocatable(T);

template <typename T>
struct is_trivially_copyable {
static constexpr bool value = __is_trivially_copyable(T);
};

template <typename T>
constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
#endif

#ifdef STD2
Expand All @@ -25,6 +33,17 @@ using is_trivially_relocatable = __details_is_trivially_relocatable<T>;

template <typename T>
constexpr bool is_trivially_relocatable_v = __builtin_is_cpp_trivially_relocatable(T);

template <typename T>
struct __details_is_trivially_copyable {
static constexpr bool value = __is_trivially_copyable(T);
};

template <typename T>
using is_trivially_copyable = __details_is_trivially_copyable<T>;

template <typename T>
constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
#endif


Expand All @@ -45,6 +64,15 @@ using is_trivially_relocatable = __details_is_trivially_relocatable<T>;

template <typename T>
constexpr bool is_trivially_relocatable_v = is_trivially_relocatable<T>::value;

template <typename T>
struct __details_is_trivially_copyable : bool_constant<__is_trivially_copyable(T)> {};

template <typename T>
using is_trivially_copyable = __details_is_trivially_copyable<T>;

template <typename T>
constexpr bool is_trivially_copyable_v = is_trivially_copyable<T>::value;
#endif

}
Expand All @@ -60,6 +88,18 @@ static_assert(std::is_trivially_relocatable_v<int&>);
// expected-note@-1 {{'int &' is not trivially relocatable}} \
// expected-note@-1 {{because it is a reference type}}

static_assert(std::is_trivially_copyable<int>::value);

static_assert(std::is_trivially_copyable<int&>::value);
// expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_trivially_copyable<int &>::value'}} \
// expected-note@-1 {{'int &' is not trivially copyable}} \
// expected-note@-1 {{because it is a reference type}}
static_assert(std::is_trivially_copyable_v<int&>);
// expected-error@-1 {{static assertion failed due to requirement 'std::is_trivially_copyable_v<int &>'}} \
// expected-note@-1 {{'int &' is not trivially copyable}} \
// expected-note@-1 {{because it is a reference type}}


namespace test_namespace {
using namespace std;
static_assert(is_trivially_relocatable<int&>::value);
Expand All @@ -70,6 +110,15 @@ namespace test_namespace {
// expected-error@-1 {{static assertion failed due to requirement 'is_trivially_relocatable_v<int &>'}} \
// expected-note@-1 {{'int &' is not trivially relocatable}} \
// expected-note@-1 {{because it is a reference type}}

static_assert(is_trivially_copyable<int&>::value);
// expected-error-re@-1 {{static assertion failed due to requirement '{{.*}}is_trivially_copyable<int &>::value'}} \
// expected-note@-1 {{'int &' is not trivially copyable}} \
// expected-note@-1 {{because it is a reference type}}
static_assert(is_trivially_copyable_v<int&>);
// expected-error@-1 {{static assertion failed due to requirement 'is_trivially_copyable_v<int &>'}} \
// expected-note@-1 {{'int &' is not trivially copyable}} \
// expected-note@-1 {{because it is a reference type}}
}


Expand All @@ -82,6 +131,14 @@ concept C = std::is_trivially_relocatable_v<T>; // #concept2

template <C T> void g(); // #cand2

template <typename T>
requires std::is_trivially_copyable<T>::value void f2(); // #cand3

template <typename T>
concept C2 = std::is_trivially_copyable_v<T>; // #concept4

template <C2 T> void g2(); // #cand4

void test() {
f<int&>();
// expected-error@-1 {{no matching function for call to 'f'}} \
Expand All @@ -97,5 +154,20 @@ void test() {
// expected-note@#concept2 {{because 'std::is_trivially_relocatable_v<int &>' evaluated to false}} \
// expected-note@#concept2 {{'int &' is not trivially relocatable}} \
// expected-note@#concept2 {{because it is a reference type}}

f2<int&>();
// expected-error@-1 {{no matching function for call to 'f2'}} \
// expected-note@#cand3 {{candidate template ignored: constraints not satisfied [with T = int &]}} \
// expected-note-re@#cand3 {{because '{{.*}}is_trivially_copyable<int &>::value' evaluated to false}} \
// expected-note@#cand3 {{'int &' is not trivially copyable}} \
// expected-note@#cand3 {{because it is a reference type}}

g2<int&>();
// expected-error@-1 {{no matching function for call to 'g2'}} \
// expected-note@#cand4 {{candidate template ignored: constraints not satisfied [with T = int &]}} \
// expected-note@#cand4 {{because 'int &' does not satisfy 'C2'}} \
// expected-note@#concept4 {{because 'std::is_trivially_copyable_v<int &>' evaluated to false}} \
// expected-note@#concept4 {{'int &' is not trivially copyable}} \
// expected-note@#concept4 {{because it is a reference type}}
}
}
80 changes: 80 additions & 0 deletions clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,83 @@ static_assert(__builtin_is_cpp_trivially_relocatable(U2));
// expected-note@#tr-U2 {{'U2' defined here}}

}

namespace trivially_copyable {
struct B {
virtual ~B();
};
struct S : virtual B { // #tc-S
S();
int & a;
const int ci;
B & b;
B c;
~S();
};
static_assert(__is_trivially_copyable(S));
// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S)'}} \
// expected-note@-1 {{'S' is not trivially copyable}} \
// expected-note@-1 {{because it has a virtual base 'B'}} \
// expected-note@-1 {{because it has a non-trivially-copyable base 'B'}} \
Copy link
Collaborator

@shafik shafik Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test a case in which there are multiple non-trvial-copyable bases.

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tests for this case

Copy link
Collaborator

@shafik shafik Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the other test case you added but I don't see the test case for multiple base classes. I am guessing maybe you missed a commit.

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that I have added a tests with multiple base classes (structs S5, S6). Or do you need a test with three or more? Or what do you mean by "multiple base classes"?

Copy link
Contributor Author

@egorshamshura egorshamshura Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that I've figured it out. I added one more test.

// expected-note@-1 {{because it has a non-trivially-copyable member 'c' of type 'B'}} \
// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B &'}} \
// expected-note@-1 {{because it has a non-trivially-copyable member 'a' of type 'int &'}} \
// expected-note@-1 {{because it has a user-provided destructor}}
// expected-note@#tc-S {{'S' defined here}}

struct S2 { // #tc-S2
S2(S2&&);
S2& operator=(const S2&);
};
static_assert(__is_trivially_copyable(S2));
// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S2)'}} \
// expected-note@-1 {{'S2' is not trivially copyable}} \
// expected-note@-1 {{because it has a user provided move constructor}} \
// expected-note@-1 {{because it has a user provided copy assignment operator}} \
// expected-note@#tc-S2 {{'S2' defined here}}

struct S3 {
~S3() = delete;
};
static_assert(__is_trivially_copyable(S3));

union U { // #tc-U
U(const U&);
U(U&&);
U& operator=(const U&);
U& operator=(U&&);
};
static_assert(__is_trivially_copyable(U));
// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::U)'}} \
// expected-note@-1 {{'U' is not trivially copyable}} \
// expected-note@-1 {{because it is a union with a user-declared copy constructor}} \
// expected-note@-1 {{because it is a union with a user-declared copy assignment operator}} \
// expected-note@-1 {{because it is a union with a user-declared move constructor}} \
// expected-note@-1 {{because it is a union with a user-declared move assignment operator}}
// expected-note@#tc-U {{'U' defined here}}

struct S4 { // #tc-S4
~S4();
B b;
};
static_assert(__is_trivially_copyable(S4));
// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S4)'}} \
// expected-note@-1 {{'S4' is not trivially copyable}} \
// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B'}} \
// expected-note@-1 {{because it has a user-provided destructor}} \
// expected-note@#tc-S4 {{'S4' defined here}}

union U2 { // #tc-U2
U2(const U2&);
U2(U2&&);
B b;
};
static_assert(__is_trivially_copyable(U2));
// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::U2)'}} \
// expected-note@-1 {{'U2' is not trivially copyable}} \
// expected-note@-1 {{because it is a union with a user-declared copy constructor}} \
// expected-note@-1 {{because it is a union with a user-declared move constructor}} \
// expected-note@-1 {{because it has a deleted destructor}} \
// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B'}} \
// expected-note@#tc-U2 {{'U2' defined here}}
}