[Sema] Use the instantiated name of destructors in FindInstantiatedDecl and RebuildMemberExpr

[ahatanak]

Summary

This fixes PR30361.

clang was failing to compile the test case because it was passing "~C1<T>" instead of "~C1" to FindInstantiatedDecl and RebuildMemberExpr.

[ahatanak]

ping

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4850

Do we need to do this for conversion function names too? (Eg, operator C1<T>*)

[ahatanak]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4850

I added a definition of "operator C1<T>*" and called it in C1::foo1. I didn't see any crashes or asserts.

Calls to a destructor and a conversion function create different AST nodes (the former creates a MemberExpr and the latter creates a CXXDependentScopeMemberExpr) and are transformed by different functions of TreeTransform.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4850

Please also check in that testcase.

lib/Sema/TreeTransform.h

2124

I don't see any reason why this should be specific to destructors. But this is the wrong place for this change; by the time we reach Rebuild* we should have already transformed everything that we need to transform.

Perhaps the problem is instead...

11965–11967

... that we fail to transform DestroyedType here. Can you try fixing this bug and see if that removes the need for your other changes?

[ahatanak]

Added a call to operator C1<T>* in test case.

[ahatanak]

(I'm replying to the comment near TreeTransform.h:11967 because phab doesn't let me hit save)

It looks like RebuildCXXPseudoDestructorExpr isn't even called because a MemberExpr is created instead of a CXXPseudoDestructorExpr.

lib/Sema/TreeTransform.h

2124

If we want to do the transformation before entering RebuildMemberExpr, I think it's also possible to do it in TreeTransform<Derived>::TransformMemberExpr.

[ahatanak]

Rebase. Pass the destructor's DeclarationNameInfo to the call to RebuildMemberExpr rather than getting it inside RebuildMemberExpr.

[ahatanak]

Rebase and ping.

[ahatanak]

ping

[ahatanak]

Call TransformDeclarationNameInfo unconditionally to transform NameInfo rather than doing so only for destructors. I think this is a cleaner fix.

[rsmith]

Looks good, other than error recovery.

lib/Sema/SemaTemplateInstantiateDecl.cpp

4851

You should deal with the possibility of SubstDeclarationNameInfo failing (which will happen if substitution into the name creates an invalid type). If you get a null name back from the Subst call, just return null.

lib/Sema/TreeTransform.h

8716–8717

Likewise here, you should return ExprError() if the transformed name is null (please also add an assert that the name is not null before the transform, as in that case a null transformed name would not indicate an error has been diagnosed).

[ahatanak]

Add code for error recovery.

lib/Sema/TreeTransform.h

8716–8717

I discovered that the name can be null when the member expression is a member of an anonymous union and therefore an assert would cause a few regression tests to fail. For example, the following code would assert:

struct T0 {
  union {
    void *m0;
  };
};
template <typename T>
struct T1 : public T0 {
  void f0() {
    m0 = 0;
  }
};

struct A : public T0 { };

void f1(T1<A> *S) { S->f0(); }

[ahatanak]

Richard, does the updated patch look OK?

lib/Sema/SemaTemplateInstantiateDecl.cpp

if (CXXRecordDecl *Spec = dyn_cast<CXXRecordDecl>(ParentDC)) {
           return nullptr;
 
         ParentDC = Tag->getDecl();
       }
     }
 
     NamedDecl *Result = nullptr;
     if (D->getDeclName()) {
-      DeclContext::lookup_result Found = ParentDC->lookup(D->getDeclName());
+      DeclarationName Name = D->getDeclName();
+      if (auto *DD = dyn_cast<CXXDestructorDecl>(D))
+        Name =
+            SubstDeclarationNameInfo(DD->getNameInfo(), TemplateArgs).getName();
+      DeclContext::lookup_result Found = ParentDC->lookup(Name);
       Result = findInstantiationOf(Context, D, Found.begin(), Found.end());
     } else {
       // Since we don't have a name for the entity we're looking for,
       // our only option is to walk through all of the declarations to
       // find that name. This will occur in a few cases:
       //
       //   - anonymous struct/union within a template
       //   - unnamed class/struct/union/enum within a template

lib/Sema/TreeTransform.h

ExprResult RebuildMemberExpr(Expr *Base, SourceLocation OpLoc,
                                SourceLocation TemplateKWLoc,
                                const DeclarationNameInfo &MemberNameInfo,
                                ValueDecl *Member,
                                NamedDecl *FoundDecl,
                         const TemplateArgumentListInfo *ExplicitTemplateArgs,
                                NamedDecl *FirstQualifierInScope) {
     ExprResult BaseResult = getSema().PerformMemberExprBaseConversion(Base,
                                                                       isArrow);
+    DeclarationNameInfo DNI = MemberNameInfo;
+
+    if (isa<CXXDestructorDecl>(FoundDecl))
+      DNI = getDerived().TransformDeclarationNameInfo(MemberNameInfo);
+
     if (!Member->getDeclName()) {
       // We have a reference to an unnamed field.  This is always the
       // base of an anonymous struct/union member access, i.e. the
       // field is always of record type.
       assert(!QualifierLoc && "Can't have an unnamed field with a qualifier!");
       assert(Member->getType()->isRecordType() &&
              "unnamed member not of record type?");
 
       BaseResult =
         getSema().PerformObjectMemberConversion(BaseResult.get(),
                                                 QualifierLoc.getNestedNameSpecifier(),
                                                 FoundDecl, Member);
       if (BaseResult.isInvalid())
         return ExprError();
       Base = BaseResult.get();
       ExprValueKind VK = isArrow ? VK_LValue : Base->getValueKind();
       MemberExpr *ME = new (getSema().Context)
-          MemberExpr(Base, isArrow, OpLoc, Member, MemberNameInfo,
+          MemberExpr(Base, isArrow, OpLoc, Member, DNI,
                      cast<FieldDecl>(Member)->getType(), VK, OK_Ordinary);
       return ME;
     }
 
     CXXScopeSpec SS;
     SS.Adopt(QualifierLoc);
 
     Base = BaseResult.get();
     QualType BaseType = Base->getType();
 
     // FIXME: this involves duplicating earlier analysis in a lot of
     // cases; we should avoid this when possible.
-    LookupResult R(getSema(), MemberNameInfo, Sema::LookupMemberName);
+    LookupResult R(getSema(), DNI, Sema::LookupMemberName);
     R.addDecl(FoundDecl);
     R.resolveKind();
 
     return getSema().BuildMemberReferenceExpr(Base, BaseType, OpLoc, isArrow,
                                               SS, TemplateKWLoc,
                                               FirstQualifierInScope,
                                               R, ExplicitTemplateArgs,
                                               /*S*/nullptr);
TreeTransform<Derived>::TransformMemberExpr(MemberExpr *E) {
   SourceLocation FakeOperatorLoc =
       SemaRef.getLocForEndOfToken(E->getBase()->getSourceRange().getEnd());
 
   // FIXME: to do this check properly, we will need to preserve the
   // first-qualifier-in-scope here, just in case we had a dependent
   // base (and therefore couldn't do the check) and a
   // nested-name-qualifier (and therefore could do the lookup).
   NamedDecl *FirstQualifierInScope = nullptr;
 
   return getDerived().RebuildMemberExpr(Base.get(), FakeOperatorLoc,
                                         E->isArrow(),
                                         QualifierLoc,
                                         TemplateKWLoc,
                                         E->getMemberNameInfo(),
                                         Member,
                                         FoundDecl,
                                         (E->hasExplicitTemplateArgs()
                                            ? &TransArgs : nullptr),
if (Base->isTypeDependent() || Destroyed.getIdentifier() ||
        !BaseType->getAs<PointerType>()->getPointeeType()
                                               ->template getAs<RecordType>())){
     // This pseudo-destructor expression is still a pseudo-destructor.
     return SemaRef.BuildPseudoDestructorExpr(
         Base, OperatorLoc, isArrow ? tok::arrow : tok::period, SS, ScopeType,
         CCLoc, TildeLoc, Destroyed);
   }
 
   TypeSourceInfo *DestroyedType = Destroyed.getTypeSourceInfo();
   DeclarationName Name(SemaRef.Context.DeclarationNames.getCXXDestructorName(
                  SemaRef.Context.getCanonicalType(DestroyedType->getType())));
   DeclarationNameInfo NameInfo(Name, Destroyed.getLocation());
   NameInfo.setNamedTypeInfo(DestroyedType);
 
   // The scope type is now known to be a valid nested name specifier
   // component. Tack it on to the end of the nested name specifier.
   if (ScopeType) {
     if (!ScopeType->getType()->getAs<TagType>()) {
       getSema().Diag(ScopeType->getTypeLoc().getBeginLoc(),

test/SemaCXX/destructor.cpp

 
 class Invalid {
     ~Invalid();
     UnknownType xx; // expected-error{{unknown type name}}
 };
 
 // The constructor definition should not have errors
 Invalid::~Invalid() {}
+
+namespace PR30361 {
+template <typename T>
+struct C1 {
+  ~C1() {}
+  operator C1<T>* () { return nullptr; }
+  void foo1();
+};
+
+template<typename T>
+void C1<T>::foo1() {
+  C1::operator C1<T>*();
+  C1::~C1();
+}
+
+void foo1() {
+  C1<int> x;
+  x.foo1();
+}
+}