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
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 4 commits
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
88 changes: 88 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,88 @@ 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();

for (const CXXMethodDecl *Method : D->methods()) {
if (Method->isIneligibleOrNotSelected() || Method->isTrivial() ||
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.

Are all three conditions in the if statement covered by the test?

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.

Added coverage for two conditions, removed Method->isIneligibleOrNotSelected(). Frankly speaking, I could not create a case when Method->isIneligibleOrNotSelected() is true and Method->isTrivial() || !Method->isUserProvided() is not

!Method->isUserProvided()) {
continue;
}
auto SpecialMemberKind =
SemaRef.getDefaultedFunctionKind(Method).asSpecialMember();
switch (SpecialMemberKind) {
case CXXSpecialMemberKind::CopyConstructor:
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.

If every single case label covered in the tests?

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 tests for every single case

case CXXSpecialMemberKind::MoveConstructor:
case CXXSpecialMemberKind::CopyAssignment:
case CXXSpecialMemberKind::MoveAssignment: {
bool IsAssignment =
SpecialMemberKind == CXXSpecialMemberKind::CopyAssignment ||
SpecialMemberKind == CXXSpecialMemberKind::MoveAssignment;
bool IsMove =
SpecialMemberKind == CXXSpecialMemberKind::MoveConstructor ||
SpecialMemberKind == CXXSpecialMemberKind::MoveAssignment;

SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< (IsAssignment ? diag::TraitNotSatisfiedReason::UserProvidedAssign
: diag::TraitNotSatisfiedReason::UserProvidedCtr)
<< IsMove << Method->getSourceRange();
break;
}
default: {
break;
}
}
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.

Suggested change
default: {
break;
}
}
default:
break;
}

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.

Fixed

}
CXXDestructorDecl *Dtr = D->getDestructor();
if (Dtr && !Dtr->isTrivial())
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
<< diag::TraitNotSatisfiedReason::DeletedDtr << 1
<< Dtr->getSourceRange();
}
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.

Can you move that to the previous place where we talked about the destructor?
We should say either "has a deleted destructor" or "has a non-trivial destructor" (but not both)

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.

Oh, fixed it, decided to write "has a deleted destructor". Is it good now?


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;

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 +2182,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}}
}
}
51 changes: 51 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,54 @@ 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));

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}}
}