[SemaCXX] Don't check base's dtor is accessible

[modocache]

Summary

In https://reviews.llvm.org/D45898?id=157373, @rsmith advises to move a
checkMemberDestructor into InitListChecker::CheckStructUnionTypes,
to check base classes for an accessible destructor when performing
aggregate initialization. However, in so doing, the following C++ code
now fails to compile:

class Base { protected: ~Base() = default; };
struct Derived : Base {};
void foo() { Derived d = {}; }

GCC 8.2 and Clang 7.0.0 both treat this C++ code as valid, whereas
Clang trunk emits the following error diagnostic:

<source>:3:27: error: temporary of type 'Base' has protected destructor
void foo() { Derived d = {}; }
                          ^
<source>:1:25: note: declared protected here
class Base { protected: ~Base() = default; };
                        ^

I think Clang trunk is wrong about the validity of this C++ code: my
understanding of the C++17 standard is that the Base destructor need
only be accessible from within the context of Derived, and not at the
context in which Derived is aggregate initialized within the foo
function.

Add a test for the case above, and modify the destructor access check
within InitListChecker::CheckStructUnionTypes to check only that the
derived class has an accessible destructor.

Test Plan: check-clang

[modocache]

Here's a compiler explorer link demonstrating that GCC 8.2 and Clang 7.0.0 compile the example code, but Clang trunk emits an error: https://godbolt.org/z/l3baI_

I realize the proposed "fix" here isn't likely to be exactly correct, but I thought it could be a good starting point. I'd also like to confirm that my example is, in fact, valid C++ code.

Any and all reviews appreciated!

[rjmccall]

That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make their destructors protected in order to prevent types from being constructed except as base sub-objects, which is still valid here. But at the same time, at a low level, we are directly accessing a protected destructor from a context that is not code actually defined in a subclass.

[ahatanak]

http://wg21.link/p0968r0#2227 says:

The destructor for each element of class type is potentially invoked (15.4 [class.dtor]) from the context where the aggregate initialization occurs. [Note: This provision ensures that destructors can be called for fully-constructed subobjects in case an exception is thrown (18.2 [except.ctor]). —end note]

And '15.4 Destructors' has this sentence:
A program is ill-formed if a destructor that is potentially invoked is deleted or not accessible from the context of the invocation.

I'm not sure what "context" is in the case above (foo or Derived?), but if it's foo, it seems like clang is correct.

[twoh]

I think the context is Derived here. My understanding of http://wg21.link/p0968r0#2227 (in this patch's context) is that when Derived is aggregate initialized, the destructor for each element of Base is potentially invoked as well.

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base. I think the right fix for 1956-1960 is

if (!VerifyOnly) {
  auto *BaseRD = Base.getType()->getAs<RecordType>()->getDecl();
  for (FieldDecl *FD : BaseRD->fields()) {
    QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
    if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
      hadError = true;
      return;
    }
  }
}

[rsmith]

In D53860#1280959, @twoh wrote:

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base.

The code is correct (or at least, following the current standard rule) as-is. The base class subobject itself is an element of the aggregate. Note that aggregate-initialization of Derived does not require Base to be an aggregate at all, so recursing to the fields of Base would be inappropriate.

If this is causing significant problems for real code, we can certainly try to take this back to the C++ committee and request a do-over, but I think it's unlikely the rule would be changed: for aggregate initialization, direct base class subobjects are (by design) treated exactly the same as fields, and the destructors for all such subobjects are potentially invoked by aggregate initialization (because if an exception is thrown after an element is initialized and before initialization of the complete object finishes, the element's destructor will be invoked). You could argue that the context for the access check should be the derived class and not the context in which aggregate initialization is performed. I've checked the minutes, and we did indeed discuss that when resolving C++ DR 2227, and decided that the context for the access check should be the context of the aggregate initialization, not the context of the class definition.

For what it's worth, this is not the only way that the C++17 extensions to aggregate initialization break existing code. There are easy workarounds: make sure that your class is not an aggregate (add a constructor), or don't use list-initialization.

[twoh]

@rsmith I see. Thank you for the clarification!

[modocache]

Yes, thanks @rsmith! And sorry @ahatanak for the trouble of explaining your code here.

The new standards behavior does impact @twoh and I's codebase in a few places, but as you explained we can simply change the source to not use list-initialization.

Thanks all!

lib/Sema/SemaInit.cpp

if (Init) {
       CheckSubElementType(BaseEntity, IList, Base.getType(), Index,
                           StructuredList, StructuredIndex);
       InitializedSomething = true;
     } else if (VerifyOnly) {
       CheckEmptyInitializable(BaseEntity, InitLoc);
     }
 
     if (!VerifyOnly)
-      if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+      if (hasAccessibleDestructor(DeclType, InitLoc, SemaRef)) {
         hadError = true;
         return;
       }
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be

test/SemaCXX/aggregate-initialization.cpp

void test2() {
     auto *t = new S2 {1,2,3,4}; // expected-error {{attempt to use a deleted function}}
   }
 
 #if __cplusplus >= 201703L
   namespace BaseDestructor {
      struct S0 { int f; ~S0() = delete; }; // expected-note {{'~S0' has been explicitly marked deleted here}}
 
     // Check destructor of base class.
-    struct S3 : S0 {};
+    struct S3 : S0 {}; // expected-note {{destructor of 'S3' is implicitly deleted because base class 'ElementDestructor::BaseDestructor::S0' has a deleted destructor}}
 
     void test3() {
       S3 s3 = {1}; // expected-error {{attempt to use a deleted function}}
     }
   }
 #endif
 
   // A's destructor doesn't have to be accessible from the context of C's
   // initialization.
   struct A { friend struct B; private: ~A(); };
   struct B { B(); A a; };
   struct C { B b; };
   C c = { B() };
+
+  // Zylong's destructor doesn't have to be accessible from the context of
+  // Yerbl's initialization.
+  struct Zylong { protected: ~Zylong(); };
+  struct Yerbl : Zylong {};
+  Yerbl y = {};
 }