[Sema] Fix friend destructor declarations after D130936

[royjacobson]

Summary

I accidentally broke friend destructor declarations in D130936.

Modify it to skip performing the destructor name check if we have a dependent friend declaration.

[royjacobson]

Don't regress on invalid (non-dependent) friend destructor declarations.

[royjacobson]

Hi @hubert.reinterpretcast, thanks for the quick catch!

I have posted this initial patch, but it still misses the case

template <typename T>
struct E {
  friend T::S::~V();
};

Something goes wrong there that I haven't figured out yet. Do you think I should land this as-is to fix the master?

[hubert.reinterpretcast]

clang/lib/Sema/SemaDecl.cpp

11518–11520

There's nothing we can diagnose about this without an instantiation (because S could be an alias for a class having V as the injected-class-name).

It is, however, true that we don't diagnose this even with problematic instantiations:

struct R { struct V; ~R(); };
struct QQ { using S = R; };

template <class T>
struct A { friend T::S::~V(); };

A<QQ> a;

[hubert.reinterpretcast]

clang/lib/Sema/SemaDecl.cpp

11522

This condition appears to be true even when the friend itself is not dependent. The error message changes for:

struct C;
struct B { ~B(); };
template <typename T>
struct A {
  friend B::~C();
};

(causing the template and non-template cases to generate different messages).

clang/test/SemaCXX/member-class-11.cpp

36–40

Please replace this with the case where there is an instantiation. Also, the prior change to the release notes in https://reviews.llvm.org/D130936 should be adjusted to reflect the new scope of what is fixed.

[royjacobson]

Update to handle some edge cases better.

[royjacobson]

Add another test case.

[royjacobson]

Updated the patch and added more test coverage.

I still don't diagnose the dependent friend case, but I didn't see how to do it easily.

clang/lib/Sema/SemaDecl.cpp

11518–11520

Wow, thanks! That's some edge case I haven't thought about..

clang/test/SemaCXX/member-class-11.cpp

36–40

Updated the test cases accordingly.

I don't think there's things to add in the release notes as this is just fixing breakage from my previous patch, not really diagnosing new cases?

[hubert.reinterpretcast]

This LGTM; thanks!

In D131541#3721383, @royjacobson wrote:

I still don't diagnose the dependent friend case, but I didn't see how to do it easily.

Maybe adding code to TemplateDeclInstantiator::VisitFriendDecl will do the trick.

clang/test/SemaCXX/member-class-11.cpp

36–40

Okay; I guess "invalid destructor names were incorrectly accepted on template classes" uses "on" and not "in" (and can be read to match the current scope of the fix).

clang/lib/Sema/SemaDecl.cpp

bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
 
   // Semantic checking for this function declaration (in isolation).
 
   if (getLangOpts().CPlusPlus) {
     // C++-specific checks.
     if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(NewFD)) {
       CheckConstructor(Constructor);
     } else if (CXXDestructorDecl *Destructor =
                    dyn_cast<CXXDestructorDecl>(NewFD)) {
+      // We check here for invalid destructor names.
+      // If we have a friend destructor declaration that is dependent, we can't
+      // diagnose right away because cases like this are still valid:
+      // template <class T> struct A { friend T::X::~Y(); };
+      // struct B { struct Y { ~Y(); }; using X = Y; };
+      // template struct A<B>;
+      if (NewFD->getFriendObjectKind() == Decl::FriendObjectKind::FOK_None ||
+          !Destructor->getThisType()->isDependentType()) {
         CXXRecordDecl *Record = Destructor->getParent();
         QualType ClassType = Context.getTypeDeclType(Record);
 
         DeclarationName Name = Context.DeclarationNames.getCXXDestructorName(
             Context.getCanonicalType(ClassType));
         if (NewFD->getDeclName() != Name) {
           Diag(NewFD->getLocation(), diag::err_destructor_name);
           NewFD->setInvalidDecl();
           return Redeclaration;
         }
+      }
     } else if (auto *Guide = dyn_cast<CXXDeductionGuideDecl>(NewFD)) {
       if (auto *TD = Guide->getDescribedFunctionTemplate())
         CheckDeductionGuideTemplate(TD);
 
       // A deduction guide is not on the list of entities that can be
       // explicitly specialized.
       if (Guide->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
         Diag(Guide->getBeginLoc(), diag::err_deduction_guide_specialized)

clang/test/SemaCXX/member-class-11.cpp

 
 struct B;
 
 template<class T>
 struct C {
   ~B(); // expected-error {{expected the class name after '~' to name the enclosing class}}
 };
 
+template <typename T>
+struct D {
+  friend T::S::~S();
+private:
+  static constexpr int secret = 42;
+};
+
+template <typename T>
+struct E {
+  friend T::S::~V();
+};
+
+struct BadInstantiation {
+  struct S {
+    struct V {};
+  };
+};
+
+struct GoodInstantiation {
+  struct V {
+    ~V();
+  };
+  using S = V;
+};
+
+// FIXME: We should diagnose this while instantiating.
+E<BadInstantiation> x;
+E<GoodInstantiation> y;
+
+struct Q {
+  struct S { ~S(); };
+};
+
+Q::S::~S() {
+  void foo(int);
+  foo(D<Q>::secret);
+}
+
+struct X {
+  ~X();
+};
+struct Y;
+
+struct Z1 {
+  friend X::~Y(); // expected-error {{expected the class name after '~' to name the enclosing class}}
+};
+
+template <class T>
+struct Z2 {
+  friend X::~Y(); // expected-error {{expected the class name after '~' to name the enclosing class}}
+};
+
 }