-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[Clang] Add detailed notes explaining why is_aggregate evaluates to false #152488
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.
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
base: main
Are you sure you want to change the base?
[Clang] Add detailed notes explaining why is_aggregate evaluates to false #152488
Conversation
@llvm/pr-subscribers-clang Author: Shamshura Egor (egorshamshura) ChangesThis commit is part of #141911 Full diff: https://github.com/llvm/llvm-project/pull/152488.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f903b7f4dacd0..65cb5e809cce0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1772,7 +1772,8 @@ def note_unsatisfied_trait
"%Replaceable{replaceable}|"
"%TriviallyCopyable{trivially copyable}|"
"%Empty{empty}|"
- "%StandardLayout{standard-layout}"
+ "%StandardLayout{standard-layout}|"
+ "%Aggregate{aggregate}"
"}1">;
def note_unsatisfied_trait_reason
@@ -1805,6 +1806,8 @@ def note_unsatisfied_trait_reason
"%DeletedDtr{has a %select{deleted|user-provided}1 destructor}|"
"%UserProvidedCtr{has a user provided %select{copy|move}1 "
"constructor}|"
+ "%UserDeclaredCtr{has a user-declared constructor}|"
+ "%InheritedCtr{has an inherited constructor}|"
"%DeletedCtr{has a deleted %select{copy|move}1 "
"constructor}|"
"%UserProvidedAssign{has a user provided %select{copy|move}1 "
@@ -1815,7 +1818,11 @@ def note_unsatisfied_trait_reason
"%sub{select_special_member_kind}1}|"
"%FunctionType{is a function type}|"
"%CVVoidType{is a cv void type}|"
- "%IncompleteArrayType{is an incomplete array type}"
+ "%IncompleteArrayType{is an incomplete array type}|"
+ "%PrivateDirectDataMember{has a private direct data member}|"
+ "%ProtectedDirectDataMember{has a protected direct data member}|"
+ "%PrivateDirectBase{has a private direct base}|"
+ "%ProtectedDirectBase{has a protected direct base}"
"}0">;
def warn_consteval_if_always_true : Warning<
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index 1d8687e4bf1c1..868de469d91c8 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -1965,6 +1965,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) {
.Case("is_assignable", TypeTrait::BTT_IsAssignable)
.Case("is_empty", TypeTrait::UTT_IsEmpty)
.Case("is_standard_layout", TypeTrait::UTT_IsStandardLayout)
+ .Case("is_aggregate", TypeTrait::UTT_IsAggregate)
.Case("is_constructible", TypeTrait::TT_IsConstructible)
.Default(std::nullopt);
}
@@ -2595,6 +2596,81 @@ static void DiagnoseNonStandardLayoutReason(Sema &SemaRef, SourceLocation Loc,
SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
}
+static void DiagnoseNonAggregateReason(Sema &SemaRef, SourceLocation Loc,
+ const CXXRecordDecl *D) {
+ for (const CXXConstructorDecl *Ctor : D->ctors()) {
+ if (Ctor->isUserProvided())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::UserDeclaredCtr;
+ if (Ctor->isInheritingConstructor())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::InheritedCtr;
+ }
+
+ for (const FieldDecl *Field : D->fields()) {
+ switch (Field->getAccess()) {
+ case AS_private:
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::PrivateDirectDataMember;
+ break;
+ case AS_protected:
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::ProtectedDirectDataMember;
+ break;
+ default:
+ break;
+ }
+ }
+
+ for (const CXXBaseSpecifier &B : D->bases()) {
+ if (B.isVirtual()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::VBase << B.getType()
+ << B.getSourceRange();
+ continue;
+ }
+ switch (B.getAccessSpecifier()) {
+ case AS_private:
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::PrivateDirectBase;
+ break;
+ case AS_protected:
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::ProtectedDirectBase;
+ break;
+ default:
+ break;
+ }
+ }
+
+ for (const CXXMethodDecl *Method : D->methods()) {
+ if (Method->isVirtual()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::VirtualFunction << Method
+ << Method->getSourceRange();
+ }
+ }
+
+ SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
+}
+
+static void DiagnoseNonAggregateReason(Sema &SemaRef, SourceLocation Loc, QualType T) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
+ << T << diag::TraitName::Aggregate;
+
+ if (T->isVoidType())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::CVVoidType;
+
+ T = T.getNonReferenceType();
+ const CXXRecordDecl *D = T->getAsCXXRecordDecl();
+ if (!D || D->isInvalidDecl())
+ return;
+
+ if (D->hasDefinition())
+ DiagnoseNonAggregateReason(SemaRef, Loc, D);
+}
+
void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
E = E->IgnoreParenImpCasts();
if (E->containsErrors())
@@ -2627,6 +2703,9 @@ void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
case TT_IsConstructible:
DiagnoseNonConstructibleReason(*this, E->getBeginLoc(), Args);
break;
+ case UTT_IsAggregate:
+ DiagnoseNonAggregateReason(*this, E->getBeginLoc(), Args[0]);
+ break;
default:
break;
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
index f3ddbbfe15bdc..aaf19ce67ac50 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
@@ -50,6 +50,14 @@ struct is_constructible {
template <typename... Args>
constexpr bool is_constructible_v = __is_constructible(Args...);
+
+template <typename T>
+struct is_aggregate {
+ static constexpr bool value = __is_aggregate(T);
+};
+
+template <typename T>
+constexpr bool is_aggregate_v = __is_aggregate(T);
#endif
#ifdef STD2
@@ -88,7 +96,7 @@ constexpr bool is_assignable_v = __is_assignable(T, U);
template <typename T>
struct __details_is_empty {
- static constexpr bool value = __is_empty(T);
+ static constexpr bool value = __is_empty(T);
};
template <typename T>
using is_empty = __details_is_empty<T>;
@@ -97,9 +105,7 @@ constexpr bool is_empty_v = __is_empty(T);
template <typename T>
struct __details_is_standard_layout {
-static constexpr bool value = __is_standard_layout(T);
-
-
+ static constexpr bool value = __is_standard_layout(T);
};
template <typename T>
using is_standard_layout = __details_is_standard_layout<T>;
@@ -116,6 +122,17 @@ using is_constructible = __details_is_constructible<Args...>;
template <typename... Args>
constexpr bool is_constructible_v = __is_constructible(Args...);
+
+template <typename T>
+struct __details_is_aggregate {
+ static constexpr bool value = __is_aggregate(T);
+};
+
+template <typename T>
+using is_aggregate = __details_is_aggregate<T>;
+
+template <typename T>
+constexpr bool is_aggregate_v = __is_aggregate(T);
#endif
@@ -173,12 +190,20 @@ template <typename... Args>
struct __details_is_constructible : bool_constant<__is_constructible(Args...)> {};
template <typename... Args>
-using is_constructible = __details_is_constructible<Args...>;
+using is_constructible = __details_is_constructible<Args...>;
template <typename... Args>
constexpr bool is_constructible_v = is_constructible<Args...>::value;
-#endif
+template <typename T>
+struct __details_is_aggregate : bool_constant<__is_aggregate(T)> {};
+
+template <typename T>
+using is_aggregate = __details_is_aggregate<T>;
+
+template <typename T>
+constexpr bool is_aggregate_v = is_aggregate<T>::value;
+#endif
}
static_assert(std::is_trivially_relocatable<int>::value);
@@ -248,6 +273,16 @@ static_assert(std::is_constructible_v<void>);
// expected-error@-1 {{static assertion failed due to requirement 'std::is_constructible_v<void>'}} \
// expected-note@-1 {{because it is a cv void type}}
+static_assert(!std::is_aggregate<int>::value);
+
+static_assert(std::is_aggregate<void>::value);
+// expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_aggregate<void>::value'}} \
+// expected-note@-1 {{'void' is not aggregate}} \
+// expected-note@-1 {{because it is a cv void type}}
+static_assert(std::is_aggregate_v<void>);
+// expected-error@-1 {{static assertion failed due to requirement 'std::is_aggregate_v<void>'}} \
+// expected-note@-1 {{'void' is not aggregate}} \
+// expected-note@-1 {{because it is a cv void type}}
namespace test_namespace {
using namespace std;
static_assert(is_trivially_relocatable<int&>::value);
@@ -300,6 +335,15 @@ namespace test_namespace {
static_assert(is_constructible_v<void>);
// expected-error@-1 {{static assertion failed due to requirement 'is_constructible_v<void>'}} \
// expected-note@-1 {{because it is a cv void type}}
+
+ static_assert(std::is_aggregate<void>::value);
+ // expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_aggregate<void>::value'}} \
+ // expected-note@-1 {{'void' is not aggregate}} \
+ // expected-note@-1 {{because it is a cv void type}}
+ static_assert(std::is_aggregate_v<void>);
+ // expected-error@-1 {{static assertion failed due to requirement 'std::is_aggregate_v<void>'}} \
+ // expected-note@-1 {{'void' is not aggregate}} \
+ // expected-note@-1 {{because it is a cv void type}}
}
@@ -337,6 +381,14 @@ concept C3 = std::is_constructible_v<Args...>; // #concept6
template <C3 T> void g3(); // #cand6
+template <typename T>
+requires std::is_aggregate<T>::value void f5(); // #cand9
+
+template <typename T>
+concept C5 = std::is_aggregate_v<T>; // #concept10
+
+template <C5 T> void g5(); // #cand10
+
void test() {
f<int&>();
// expected-error@-1 {{no matching function for call to 'f'}} \
@@ -393,6 +445,21 @@ void test() {
// expected-note@#cand6 {{because 'void' does not satisfy 'C3'}} \
// expected-note@#concept6 {{because 'std::is_constructible_v<void>' evaluated to false}} \
// expected-note@#concept6 {{because it is a cv void type}}
+
+ f5<void>();
+ // expected-error@-1 {{no matching function for call to 'f5'}} \
+ // expected-note@#cand9 {{candidate template ignored: constraints not satisfied [with T = void]}} \
+ // expected-note-re@#cand9 {{because '{{.*}}is_aggregate<void>::value' evaluated to false}} \
+ // expected-note@#cand9 {{'void' is not aggregate}} \
+ // expected-note@#cand9 {{because it is a cv void type}}
+
+ g5<void>();
+ // expected-error@-1 {{no matching function for call to 'g5'}} \
+ // expected-note@#cand10 {{candidate template ignored: constraints not satisfied [with T = void]}} \
+ // expected-note@#cand10 {{because 'void' does not satisfy 'C5'}} \
+ // expected-note@#concept10 {{because 'std::is_aggregate_v<void>' evaluated to false}} \
+ // expected-note@#concept10 {{'void' is not aggregate}} \
+ // expected-note@#concept10 {{because it is a cv void type}}
}
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
index 54806a93ddf80..7612b46994699 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
@@ -829,3 +829,90 @@ static_assert(__is_standard_layout(H)); // no diagnostics
static_assert(__is_standard_layout(I)); // no diagnostics
}
+namespace is_aggregate {
+ struct S1 { // #ag-S1
+ S1(int);
+ };
+
+ static_assert(__is_aggregate(S1));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S1)'}} \
+ // expected-note@-1 {{because it has a user-declared constructor}} \
+ // expected-note@-1 {{'S1' is not aggregate}} \
+ // expected-note@#ag-S1 {{'S1' defined here}}
+
+ struct B2 {
+ B2(int);
+ };
+
+ struct S2 : B2 { // #ag-S2
+ using B2::B2;
+ };
+
+ static_assert(__is_aggregate(S2));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S2)'}} \
+ // expected-note@-1 {{because it has an inherited constructor}} \
+ // expected-note@-1 {{'S2' is not aggregate}} \
+ // expected-note@#ag-S2 {{'S2' defined here}}
+
+ struct S3 { // #ag-S3
+ protected:
+ int x;
+ };
+ static_assert(__is_aggregate(S3));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S3)'}} \
+ // expected-note@-1 {{because it has a protected direct data member}} \
+ // expected-note@-1 {{'S3' is not aggregate}} \
+ // expected-note@#ag-S3 {{'S3' defined here}}
+
+ struct S4 { // #ag-S4
+ private:
+ int x;
+ };
+ static_assert(__is_aggregate(S4));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S4)'}} \
+ // expected-note@-1 {{because it has a private direct data member}} \
+ // expected-note@-1 {{'S4' is not aggregate}} \
+ // expected-note@#ag-S4 {{'S4' defined here}}
+
+ struct B5 {};
+
+ struct S5 : private B5 { // #ag-S5
+ private:
+ int x;
+ };
+ static_assert(__is_aggregate(S5));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S5)'}} \
+ // expected-note@-1 {{because it has a private direct base}} \
+ // expected-note@-1 {{because it has a private direct data member}} \
+ // expected-note@-1 {{'S5' is not aggregate}} \
+ // expected-note@#ag-S5 {{'S5' defined here}}
+
+ struct B6 {};
+
+ struct S6 : protected B6 { // #ag-S6
+ private:
+ int x;
+ };
+ static_assert(__is_aggregate(S6));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S6)'}} \
+ // expected-note@-1 {{because it has a protected direct base}} \
+ // expected-note@-1 {{because it has a private direct data member}} \
+ // expected-note@-1 {{'S6' is not aggregate}} \
+ // expected-note@#ag-S6 {{'S6' defined here}}
+
+ struct B7 {};
+
+ struct S7 : virtual B7 { // #ag-S7
+ virtual void foo();
+
+ private:
+ int x;
+ };
+ static_assert(__is_aggregate(S7));
+ // expected-error@-1 {{static assertion failed due to requirement '__is_aggregate(is_aggregate::S7)'}} \
+ // expected-note@-1 {{because it has a private direct data member}} \
+ // expected-note@-1 {{because it has a virtual function 'foo'}} \
+ // expected-note@-1 {{because it has a virtual base}} \
+ // expected-note@-1 {{'S7' is not aggregate}} \
+ // expected-note@#ag-S7 {{'S7' defined here}}
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
@cor3ntin Hi!! I can't set reviewers, so can you add someone? |
Fixed it. |
I am mostly afk, I will look at this over the weekend. |
Ok, thanks! |
ping 🥺 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the delay. Thanks for the ping.
I think this looks good.
Can you add tests for arrays? (arrays are aggregate https://eel.is/c++draft/dcl.init.aggr#1)
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
bool HasInherited = llvm::any_of(D->decls(), [](auto const *Sub) { | ||
bool Result = false; | ||
if (auto *UD = dyn_cast<UsingDecl>(Sub)) { | ||
Result = llvm::any_of(UD->shadows(), [](auto const &I) { | ||
return isa<ConstructorUsingShadowDecl>(I); | ||
}); | ||
} | ||
return isa<ConstructorUsingShadowDecl>(Sub) || Result; | ||
}); | ||
|
||
if (HasInherited) { | ||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
<< diag::TraitNotSatisfiedReason::InheritedCtr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop above should be sufficient, right? Why do we check for inherited constructors twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed inner loop
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
case AS_private: | ||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
<< diag::TraitNotSatisfiedReason::PrivateDirectBase; | ||
break; | ||
case AS_protected: | ||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
<< diag::TraitNotSatisfiedReason::ProtectedDirectBase; | ||
break; | ||
default: | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could merge these two diagnostics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
case AS_private: | ||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
<< diag::TraitNotSatisfiedReason::PrivateDirectDataMember; | ||
break; | ||
case AS_protected: | ||
SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
<< diag::TraitNotSatisfiedReason::ProtectedDirectDataMember; | ||
break; | ||
default: | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could merge these two diagnostics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
So, I'm not sure if it's really necessary to add tests for arrays, as array is always an aggregate. However I added simple tests |
@cor3ntin ping 🥺 |
} | ||
} | ||
|
||
for (const CXXBaseSpecifier &B : D->bases()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of a nit: I feel like base classes should go prior to fields.
As an actual issue you may need to diagnose bases recursively as the reason for a type being a non-aggregate may not be direct. e.g. I'm not sure what diagnostic would be produced here for something like
class A {};
class B: private A {};
class C: public B {};
require is_aggregate(C)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godbolt.org/z/fsoP3h4on
I don't think that we need to check it recursively.
(1.3)
} | ||
} | ||
|
||
for (const CXXMethodDecl *Method : D->methods()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better noted as the object being polymorphic, rather than enumerating the individual virtual methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a TraitNotSatisfiedReason::Polymorphic
, however, in my opinion, we also need to say why this class is being polymorphic and show every function that is being virtual.
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect a double-note here is even better. First one that says "because it is a polymorphic type", second of: "check out this method here, which is virtual".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect a double-note here is even better. First one that says "because it is a polymorphic type", second of: "check out this method here, which is virtual".
Added note about polymorphic type.
b25c005
to
f1b6862
Compare
This PR is part of #141911