Check a class doesn't have a dependent type before iterating over its base classes

[ahatanak]

Summary

This fixes a crash when a class defined in a method of a templated class inherits from a class that is forward-declared.

[ahatanak]

I'm not sure why this shouldn't be caught in Sema::CheckBaseSpecifier. But there is a check for the definition of the class before findCircularInheritance is called, so I'm guessing there is a reason the code shouldn't be rejected here.

[rsmith]

We should reject a definition of a class that has a non-dependent base class that's an incomplete type. We need non-dependent bases to be complete in order to be able to do name lookup into them when processing the template definition. (The C++ standard permits us to reject such cases but does not require it, because "a hypothetical instantiation of [the templated class] immediately following its definition would be ill-formed due to a construct that does not depend on a template parameter".)

Then there's a question of error recovery and AST invariants: should we permit a base specifier to name a non-dependent type that's not a complete class type? If so, we'll need to make sure that all code that iterates over base classes checks for this condition (I bet there are more cases than the two that you found). But tooling use cases probably do want to know that such base specifiers were written. So perhaps we should allow such base classes but mark the class definition as invalid if we find them -- in which case we would want something like this patch as part of the error recovery.

[ahatanak]

I think the non-dependent base class you mention in this case is B0, but it seems that the type of B0 in the AST is dependent as isDependentType returns true. Should we fix this? Or does isDependentType mean something different?

[ahatanak]

TagDecl::isDependentType() is returning true.

[jkorous]

In D78760#2003118, @rsmith wrote:

If so, we'll need to make sure that all code that iterates over base classes checks for this condition (I bet there are more cases than the two that you found).

Just a potentially related thought: I was recently trying to understand under what circumstances should I expect CXXBaseSpecifier::getType() to return QualType instance for which isNull() could be true. I gave up and don't make any assumption.
Would the suggested change affect this?

[rsmith]

In D78760#2005874, @ahatanak wrote:

TagDecl::isDependentType() is returning true.

Oh, sorry, we do already do the thing I was suggesting. But in your testcase the issue is that the base class B0 is dependent but is also resolved during template instantiation to a (dependent) class type. We should handle this by skipping dependent base classes, not by skipping incomplete base classes. For example, consider this testcase (which currently asserts):

struct A {};
template<typename T> struct X {
  struct B : A {};
  struct C : A, B {};
};

here, we can resolve the B base of C to the member C of the current instantiation, but we should not traverse to B's base class list when considering the base classes of C, because we do not know that that definition of B will be used by any particular instantiation of C. For example:

template<> struct X<int>::B {};
X<int>::C c; // has only one `A` base class

[ahatanak]

Does the updated patch look okay?

[ahatanak]

ping

[ahatanak]

ping

[ahatanak]

Rebase and ping.

clang/lib/AST/DeclCXX.cpp

 /// Determine whether a class has a repeated base class. This is intended for
 /// use when determining if a class is standard-layout, so makes no attempt to
 /// handle virtual bases.
 static bool hasRepeatedBaseClass(const CXXRecordDecl *StartRD) {
   llvm::SmallPtrSet<const CXXRecordDecl*, 8> SeenBaseTypes;
   SmallVector<const CXXRecordDecl*, 8> WorkList = {StartRD};
   while (!WorkList.empty()) {
     const CXXRecordDecl *RD = WorkList.pop_back_val();
+    if (!RD->hasDefinition())
+      continue;
     for (const CXXBaseSpecifier &BaseSpec : RD->bases()) {
       if (const CXXRecordDecl *B = BaseSpec.getType()->getAsCXXRecordDecl()) {
         if (!SeenBaseTypes.insert(B).second)
           return true;
         WorkList.push_back(B);
       }
     }
   }

clang/lib/Sema/SemaDeclCXX.cpp

 NoteIndirectBases(ASTContext &Context, IndirectBaseSet &Set,
                   const QualType &Type)
 {
   // Even though the incoming type is a base, it might not be
   // a class -- it could be a template parm, for instance.
   if (auto Rec = Type->getAs<RecordType>()) {
     auto Decl = Rec->getAsCXXRecordDecl();
 
+    if (!Decl->hasDefinition())
+      return;
+
     // Iterate over its bases.
     for (const auto &BaseSpec : Decl->bases()) {
       QualType Base = Context.getCanonicalType(BaseSpec.getType())
         .getUnqualifiedType();
       if (Set.insert(Base).second)
         // If we've not already seen it, recurse.
         NoteIndirectBases(Context, Set, Base);
     }

clang/test/SemaCXX/class.cpp

 void f() {
     S::c; // expected-error {{invalid use of non-static data member}}
 }
 }
 
 struct PR9989 { 
   static int const PR9989_Member = sizeof PR9989_Member; 
 };
+
+namespace forward_declaration {
+struct B1 {};
+
+template <class T>
+struct S0 {
+  void m0() {
+    struct B0; // expected-note {{member is declared here}}
+
+    struct D : B0, B1 { // expected-error {{implicit instantiation}} expected-note {{in instantiation of}}
+    };
+  }
+};
+
+void test() {
+  S0<int>().m0(); // expected-note {{in instantiation of}}
+}
+} // namespace forward_declaration