Skip to content

Conversation

egorshamshura
Copy link
Contributor

@egorshamshura egorshamshura commented Aug 7, 2025

This PR is part of #141911

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 7, 2025

@llvm/pr-subscribers-clang

Author: Shamshura Egor (egorshamshura)

Changes

This commit is part of #141911


Full diff: https://github.com/llvm/llvm-project/pull/152488.diff

4 Files Affected:

  • (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+9-2)
  • (modified) clang/lib/Sema/SemaTypeTraits.cpp (+79)
  • (modified) clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp (+73-6)
  • (modified) clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp (+87)
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}}
+}

Copy link

github-actions bot commented Aug 7, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@egorshamshura
Copy link
Contributor Author

egorshamshura commented Aug 7, 2025

@cor3ntin Hi!!
Can you help me, please? I've got a question about inherited constructors. Why Ctor->isInheritingConstructor() doesn't recognize inherited constructors? I think you can look on a test with S2 struct. (It should fail I guess)

I can't set reviewers, so can you add someone?

@egorshamshura
Copy link
Contributor Author

Fixed it.

@cor3ntin
Copy link
Contributor

cor3ntin commented Aug 8, 2025

I am mostly afk, I will look at this over the weekend.
maybe @AaronBallman can help

@egorshamshura
Copy link
Contributor Author

I am mostly afk, I will look at this over the weekend. maybe @AaronBallman can help

Ok, thanks!

@egorshamshura
Copy link
Contributor Author

I am mostly afk, I will look at this over the weekend.
maybe @AaronBallman can help

ping 🥺

Copy link
Contributor

@cor3ntin cor3ntin left a 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)

Comment on lines 2611 to 2624
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;
}
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed inner loop

Comment on lines 2649 to 2658
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;
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 2628 to 2638
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;
}
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@egorshamshura egorshamshura requested a review from cor3ntin August 18, 2025 21:02
@egorshamshura
Copy link
Contributor Author

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

@egorshamshura
Copy link
Contributor Author

@cor3ntin ping 🥺

@cor3ntin cor3ntin requested a review from erichkeane August 23, 2025 08:37
}
}

for (const CXXBaseSpecifier &B : D->bases()) {
Copy link
Contributor

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)

Copy link
Contributor Author

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()) {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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".

Copy link
Contributor Author

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.

@egorshamshura egorshamshura force-pushed the explain-is-aggregate_evaluated_false branch from b25c005 to f1b6862 Compare August 25, 2025 12:02
@egorshamshura egorshamshura requested a review from ojhunt August 25, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants