-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[Clang] Add diagnostic for why std::is_abstract is false #156199
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
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang Author: Sebastian Proell (sebproell) ChangesAdds onto #141911 Full diff: https://github.com/llvm/llvm-project/pull/156199.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c934fed2c7462..36e7b66ea1fb0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1773,7 +1773,8 @@ def note_unsatisfied_trait
"%TriviallyCopyable{trivially copyable}|"
"%Empty{empty}|"
"%StandardLayout{standard-layout}|"
- "%Final{final}"
+ "%Final{final}|"
+ "%Abstract{abstract}"
"}1">;
def note_unsatisfied_trait_reason
@@ -1818,7 +1819,10 @@ def note_unsatisfied_trait_reason
"%CVVoidType{is a cv void type}|"
"%IncompleteArrayType{is an incomplete array type}|"
"%NotClassOrUnion{is not a class or union type}|"
- "%NotMarkedFinal{is not marked 'final'}"
+ "%NotMarkedFinal{is not marked 'final'}|"
+ "%UnionType{is a union type}|"
+ "%NotStructOrClass{is not a struct or class type}|"
+ "%OverridesAllPureVirtual{overrides all pure virtual functions from base class %1}"
"}0">;
def warn_consteval_if_always_true : Warning<
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index 37552331478f1..7a2b7a67e57c7 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -1966,6 +1966,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) {
.Case("is_standard_layout", TypeTrait::UTT_IsStandardLayout)
.Case("is_constructible", TypeTrait::TT_IsConstructible)
.Case("is_final", TypeTrait::UTT_IsFinal)
+ .Case("is_abstract", TypeTrait::UTT_IsAbstract)
.Default(std::nullopt);
}
@@ -2640,6 +2641,66 @@ static void DiagnoseNonStandardLayoutReason(Sema &SemaRef, SourceLocation Loc,
SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
}
+static void DiagnoseNonAbstractReason(Sema &SemaRef, SourceLocation Loc,
+ const CXXRecordDecl *D) {
+ // If this type has any abstract base classes, their respective virtual
+ // functions must have been overridden.
+ for (const CXXBaseSpecifier &B : D->bases()) {
+ assert(B.getType()->getAsCXXRecordDecl() && "invalid base?");
+ if (B.getType()->getAsCXXRecordDecl()->isAbstract()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::OverridesAllPureVirtual
+ << B.getType() << B.getSourceRange();
+ }
+ }
+}
+
+static void DiagnoseNonAbstractReason(Sema &SemaRef, SourceLocation Loc,
+ QualType T) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
+ << T << diag::TraitName::Abstract;
+
+ if (T->isReferenceType()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::Ref;
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NotStructOrClass;
+ return;
+ }
+
+ if (T->isUnionType()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::UnionType;
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NotStructOrClass;
+ return;
+ }
+
+ if (SemaRef.Context.getAsArrayType(T)) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NotStructOrClass;
+ return;
+ }
+
+ if (T->isFunctionType()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::FunctionType;
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NotStructOrClass;
+ return;
+ }
+
+ if (!T->isStructureOrClassType()) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NotStructOrClass;
+ return;
+ }
+
+ const CXXRecordDecl *D = T->getAsCXXRecordDecl();
+ if (D->hasDefinition())
+ DiagnoseNonAbstractReason(SemaRef, Loc, D);
+}
+
void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
E = E->IgnoreParenImpCasts();
if (E->containsErrors())
@@ -2681,6 +2742,9 @@ void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
DiagnoseIsFinalReason(*this, E->getBeginLoc(), QT); // unsatisfied
break;
}
+ case UTT_IsAbstract:
+ DiagnoseNonAbstractReason(*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 7c6c9ea4dde80..4fb55f85d565d 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
@@ -58,6 +58,13 @@ struct is_final {
template <typename T>
constexpr bool is_final_v = __is_final(T);
+template <typename T>
+struct is_abstract {
+ static constexpr bool value = __is_abstract(T);
+};
+template <typename T>
+constexpr bool is_abstract_v = __is_abstract(T);
+
#endif
#ifdef STD2
@@ -134,6 +141,15 @@ using is_final = __details_is_final<T>;
template <typename T>
constexpr bool is_final_v = __is_final(T);
+template <typename T>
+struct __details_is_abstract {
+ static constexpr bool value = __is_abstract(T);
+};
+template <typename T>
+using is_abstract = __details_is_abstract<T>;
+template <typename T>
+constexpr bool is_abstract_v = __is_abstract(T);
+
#endif
@@ -203,6 +219,13 @@ using is_final = __details_is_final<T>;
template <typename T>
constexpr bool is_final_v = is_final<T>::value;
+template <typename T>
+struct __details_is_abstract : bool_constant<__is_abstract(T)> {};
+template <typename T>
+using is_abstract = __details_is_abstract<T>;
+template <typename T>
+constexpr bool is_abstract_v = is_abstract<T>::value;
+
#endif
}
@@ -299,6 +322,22 @@ static_assert(std::is_final_v<Arr>);
// expected-note@-1 {{'int[3]' is not final}} \
// expected-note@-1 {{because it is not a class or union type}}
+
+static_assert(!std::is_abstract<int>::value);
+
+static_assert(std::is_abstract<int&>::value);
+// expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_abstract<int &>::value'}} \
+// expected-note@-1 {{'int &' is not abstract}} \
+// expected-note@-1 {{because it is a reference type}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+static_assert(std::is_abstract_v<int&>);
+// expected-error@-1 {{static assertion failed due to requirement 'std::is_abstract_v<int &>'}} \
+// expected-note@-1 {{'int &' is not abstract}} \
+// expected-note@-1 {{because it is a reference type}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+
namespace test_namespace {
using namespace std;
static_assert(is_trivially_relocatable<int&>::value);
@@ -376,6 +415,18 @@ namespace test_namespace {
// expected-note@-1 {{'Fn' (aka 'void ()') is not final}} \
// expected-note@-1 {{because it is a function type}} \
// expected-note@-1 {{because it is not a class or union type}}
+
+ static_assert(is_abstract<int&>::value);
+ // expected-error-re@-1 {{static assertion failed due to requirement '{{.*}}is_abstract<int &>::value'}} \
+ // expected-note@-1 {{'int &' is not abstract}} \
+ // expected-note@-1 {{because it is a reference type}} \
+ // expected-note@-1 {{because it is not a struct or class type}}
+
+ static_assert(is_abstract_v<int&>);
+ // expected-error@-1 {{static assertion failed due to requirement 'is_abstract_v<int &>'}} \
+ // expected-note@-1 {{'int &' is not abstract}} \
+ // expected-note@-1 {{because it is a reference type}} \
+ // expected-note@-1 {{because it is not a struct or class type}}
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
index 1619b0b22b85f..dff6ff59df275 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
@@ -872,3 +872,79 @@ namespace is_final_tests {
// expected-note@-1 {{because it is not a class or union type}}
}
+
+namespace is_abstract_tests {
+struct Abstract1 {
+ virtual void fn1() = 0;
+};
+
+struct Abstract2 {
+ virtual void fn2() = 0;
+};
+
+struct NonAbstract
+{
+ virtual void f() {}
+};
+
+// Multiple inheritance reports all abstract base classes that had their pure virtual functions overridden.
+struct Overrides : Abstract1, Abstract2, NonAbstract {
+ void fn1() override {}
+ void fn2() override {}
+};
+
+static_assert(__is_abstract(Overrides));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(is_abstract_tests::Overrides)'}} \
+// expected-note@-1 {{'Overrides' is not abstract}} \
+// expected-note@-1 {{because it overrides all pure virtual functions from base class 'Abstract1'}} \
+// expected-note@-1 {{because it overrides all pure virtual functions from base class 'Abstract2'}} \
+
+// Inheriting over two levels reports the last class only although the source of the pure virtual function
+// is the top-most base.
+struct Derived : Abstract1 {
+};
+
+struct Derived2 : Derived {
+ void fn1() override {}
+};
+
+static_assert(__is_abstract(Derived2));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(is_abstract_tests::Derived2)'}} \
+// expected-note@-1 {{'Derived2' is not abstract}} \
+// expected-note@-1 {{because it overrides all pure virtual functions from base class 'Derived'}} \
+
+
+using I = int;
+static_assert(__is_abstract(I));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int)'}} \
+// expected-note@-1 {{'I' (aka 'int') is not abstract}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+using Fty = void(); // function type
+static_assert(__is_abstract(Fty));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(void ())'}} \
+// expected-note@-1 {{'Fty' (aka 'void ()') is not abstract}} \
+// expected-note@-1 {{because it is a function type}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+using Arr = int[3];
+static_assert(__is_abstract(Arr));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int[3])'}} \
+// expected-note@-1 {{'Arr' (aka 'int[3]') is not abstract}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+using Ref = int&;
+static_assert(__is_abstract(Ref));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int &)'}} \
+// expected-note@-1 {{'Ref' (aka 'int &') is not abstract}} \
+// expected-note@-1 {{because it is a reference type}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+using Ptr = int*;
+static_assert(__is_abstract(Ptr));
+// expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int *)'}} \
+// expected-note@-1 {{'Ptr' (aka 'int *') is not abstract}} \
+// expected-note@-1 {{because it is not a struct or class type}}
+
+
+}
|
|
cc @cor3ntin |
d86aa58 to
ce6fb88
Compare
|
Ping @cor3ntin @erichkeane |
shafik
left a comment
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.
Can we add a more detailed summary, at least outlines the various cases you seek to cover e.g. ref, array, union etc
| using Arr = int[3]; | ||
| static_assert(__is_abstract(Arr)); | ||
| // expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int[3])'}} \ | ||
| // expected-note@-1 {{'Arr' (aka 'int[3]') is not abstract}} \ |
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.
is not abstract kind of feels weak considering we get a better message for functions and references. Same w/ the pointer case below.
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.
True. I was looking at is_final for ideas on what to check for. I added a nicer message for these cases. Would it be useful to extend the other diagnostics that are already implemented in the same way? I could do that in another PR once this one is merged.
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 deal w/ this be done as a follow-up
| return; | ||
| } | ||
|
|
||
| if (T->isUnionType()) { |
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 case does not look covered in the tests. Always make sure you cover all the diagnostics and various branches.
erichkeane
left a comment
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.
1 comment, otherwise the comments that @shafik made are all valid/need to be dealt with, I'm happy when he is.
|
@shafik @erichkeane Thank you for reviewing.
@shafik I hope I understood this correctly and updated the PR description. |
| using Arr = int[3]; | ||
| static_assert(__is_abstract(Arr)); | ||
| // expected-error@-1 {{static assertion failed due to requirement '__is_abstract(int[3])'}} \ | ||
| // expected-note@-1 {{'Arr' (aka 'int[3]') is not abstract}} \ |
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 deal w/ this be done as a follow-up
|
This my first PR in llvm and I think I need an approval for the workflows to run? And I need one of you to merge please. |
|
@sebproell Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Explain why a type is not abstract. Handles arrays, refs, unions, pointers, and functions. If the non-abstract type has abstract base classes, point out that their pure virtual methods must have been overridden. Adds onto llvm#141911
Explain why a type is not abstract. Handles arrays, refs, unions, pointers, and functions. If the non-abstract type has abstract base classes, point out that their pure virtual methods must have been overridden.
Adds onto #141911