-fdelayed-template-parsing: handle cases where a late-parsed function is not a direct member of a template, but rather nested inside a class that's a member of a template (PR19613)

[hans]

Summary

This patch seems to fix the problem. Please take a look.

[hans]

Both Reid and David suggested I add Richard to look at this interesting piece of code :)

[rsmith]

Neither the before code nor the after code looks right...

The outermost lexical level (and possibly others in the future) can have multiple template parameter lists, and you should use getNumTemplateParameterLists/getTemplateParameterList on each level to add all of them (these exist on DeclaratorDecl for FunctionDecls and on TagDecl for the other cases). For instance:

template<typename A> struct S {
  template<typename B> struct T;
};
template<typename A> template<typename B> struct S<A>::T {
  template<typename C, typename D> struct U;
  template<typename C> struct U<C, C> {
    template<typename E> void f() { /* ... */ }
  };
};

[hans]

New patch addressing Richard's feedback.

[rsmith]

include/clang/Sema/Sema.h

4886–4888

Can we fold these three functions together, so we can just say 'reenter the scope of this declaration' and enter all its template parameter lists at once? It's a bit strange that 'ActOnReenterDeclaratorTemplateScope' doesn't actually enter the innermost template scope if the declarator is itself declaring a template.

lib/Parse/ParseTemplate.cpp

1251–1252

This side also might have surrounding template parameter lists, if a partial specialization is defined out of line:

template<typename T> struct S {
  template<typename U> struct X;
  template<typename U> struct X<U*>;
};
template<typename T> template<typename U> struct S<T>::X<U*> {
  void f() {}
};

1254

Why 'record' rather than 'class'? We're pretty consistent in using 'ClassTemplate' (even though we use the unfortunate name 'CXXRecordDecl').

1255–1265

These steps seem backwards: we should reenter the surrounding template scopes for the class template before we reenter the class template itself.

lib/Sema/SemaDeclCXX.cpp

5983–5999

Maybe factor this out into a static helper template rather than essentially duplicating it?

[hans]

Thanks for the review! New patch coming up.

include/clang/Sema/Sema.h

4886–4888

Done. I've folded them into one function.

lib/Parse/ParseTemplate.cpp

1251–1252

Thanks! Added this to the battery of tests.

1254

ActOnReenterRecordTemplateScope has gone away now.

1255–1265

You're right. I've changed the order.

lib/Sema/SemaDeclCXX.cpp

5983–5999

Since I baked the three ActOnReenter*TemplateScope together, I think the code became simpler.

[hans]

New patch version.

[rsmith]

No objection to the current patch other than the dyn_cast<Decl> thing, but some thoughts.

include/clang/Sema/Sema.h

4887–4888

It's unusual to have an out parameter and a void return type. Have you considered returning the number of template parameter lists that were entered?

lib/Parse/ParseTemplate.cpp

1232–1233

(Not really related to your patch, but...) Shouldn't we be resetting TemplateParameterDepth to 0 somewhere in here? If we decide to lazily parse a templated function definition while we're half-way through parsing another template, our depths will get messed up. (IIRC, we've turned off delayed parsing for constexpr functions and functions with deduced return types, perhaps due to this bug...)

1251–1252

No need for the dyn_cast here. Every DeclContext is a Decl.

1258–1266

Could ActOnReenterTemplateScope do this itself? (That is, entering a templated decl could also enter its template, and thus enter all the template parameter lists on the same logical declaration.)

[hans]

include/clang/Sema/Sema.h

4887–4888

Oops, yeah that makes a lot of sense. Done.

lib/Parse/ParseTemplate.cpp

1232–1233

I don't have a deep enough understanding of this to answer, but I do know that we don't have any tests that fail if I reset TemplateParameterDepth to 0 at the end of this function..

1251–1252

Done. I got confused because DeclContext doesn't inherit from Decl, but I guess every subclass of DeclContext does.

1258–1266

I failed some tests when I tried that before, but that seems to have been unrelated because it works now and makes things much nicer.

[hans]

Addressing Richard's comments.

[rsmith]

Thanks, that looks nicer.

lib/Parse/ParseTemplate.cpp

1252–1258

cast, not dyn_cast, please.

1260–1264

Can you push the function template case into ActOnReenterTemplateScope too?

lib/Sema/SemaDeclCXX.cpp

6000–6002

You should handle all TagDecls here, not just CXXRecordDecls. Under the resolution of DR1607, this is valid:

template<typename T> struct A {
  enum class E;
};
template<typename T> enum class A<T>::E {
  n = (true ? 0 : [] {
    struct Q { Q() { /* delay parsed */ } } q;
    return 0;
  }())
};

... and instantiating A<T>::E will instantiate the default constructor of Q (note, we currently reject this because we went for a more conservative tentative fix for issue 1607).

[hans]

lib/Parse/ParseTemplate.cpp

1252–1258

Done.

1260–1264

I can make Actions.ActOnReenterTemplateScope(getCurScope(), FunD); be covered by the loop above if I make sure not to call Actions.PushDeclContext() on it.

I have no idea why we then do an extra Actions.ActOnReenterTemplateScope(getCurScope(), LPT.D); since that should have the same effect. But if I remove that, we start failing tests, so I guess it needs to be there.

lib/Sema/SemaDeclCXX.cpp

6000–6002

"Done."

[hans]

Addressing Richard's comments.

[rsmith]

I'm well into nitpicking mode now; please go ahead and commit after this round.

... and then we can talk about function definitions inside lambdas inside variable templates. I'm sure that'll make your day =)

lib/Parse/ParseTemplate.cpp

1252

Maybe sink this into its only use.

1260–1264

I meant, can you make ActOnReenterTemplateScope "do the right thing" when given the pattern declaration of a function template? That is, call getDescribedFunctionTemplate on it, and handle the template parameter list from the function template too. (That's why you still need this extra line -- LPT.D here is the template, FunD is the pattern within the template.)

lib/Sema/SemaDeclCXX.cpp

5987

Please add a comment saying that it doesn't matter what order we collect the template parameter lists in, so future readers don't worry that they're in the wrong order like I just did =)

5989–5990

Maybe just assert in this case? People shouldn't be passing TemplateDecls in here.

[hans]

lib/Parse/ParseTemplate.cpp

1252

Done.

1260–1264

OK, managed to get this work eventually.

lib/Sema/SemaDeclCXX.cpp

5987

Done.

5989–5990

They do, but as you pointed out offline, we should probably continue with the templated decl in that case, so doing that now.

[hans]

r207822

include/clang/Sema/Sema.h

   void CheckCompletedCXXClass(CXXRecordDecl *Record);
   void ActOnFinishCXXMemberSpecification(Scope* S, SourceLocation RLoc,
                                          Decl *TagDecl,
                                          SourceLocation LBrac,
                                          SourceLocation RBrac,
                                          AttributeList *AttrList);
   void ActOnFinishCXXMemberDecls();
 
   void ActOnReenterCXXMethodParameter(Scope *S, ParmVarDecl *Param);
-  void ActOnReenterTemplateScope(Scope *S, Decl *Template);
-  void ActOnReenterDeclaratorTemplateScope(Scope *S, DeclaratorDecl *D);
+  unsigned ActOnReenterTemplateScope(Scope *S, Decl *Template);
   void ActOnStartDelayedMemberDeclarations(Scope *S, Decl *Record);
   void ActOnStartDelayedCXXMethodDeclaration(Scope *S, Decl *Method);
   void ActOnDelayedCXXMethodParameter(Scope *S, Decl *Param);
   void ActOnFinishDelayedMemberDeclarations(Scope *S, Decl *Record);
   void ActOnFinishDelayedCXXMethodDeclaration(Scope *S, Decl *Method);
   void ActOnFinishDelayedMemberInitializers(Decl *Record);
   void MarkAsLateParsedTemplate(FunctionDecl *FD, Decl *FnD,
                                 CachedTokens &Toks);
   void UnmarkAsLateParsedTemplate(FunctionDecl *FD);

lib/Parse/ParseTemplate.cpp

 
 /// \brief Late parse a C++ function template in Microsoft mode.
 void Parser::ParseLateTemplatedFuncDef(LateParsedTemplate &LPT) {
   if (!LPT.D)
      return;
 
   // Get the FunctionDecl.
   FunctionDecl *FunD = LPT.D->getAsFunction();
   // Track template parameter depth.
   TemplateParameterDepthRAII CurTemplateDepthTracker(TemplateParameterDepth);
 
   // To restore the context after late parsing.
   Sema::ContextRAII GlobalSavedContext(Actions, Actions.CurContext);
 
   SmallVector<ParseScope*, 4> TemplateParamScopeStack;
 
   // Get the list of DeclContexts to reenter.
   SmallVector<DeclContext*, 4> DeclContextsToReenter;
-  DeclContext *DD = FunD->getLexicalParent();
+  DeclContext *DD = FunD;
   while (DD && !DD->isTranslationUnit()) {
     DeclContextsToReenter.push_back(DD);
     DD = DD->getLexicalParent();
   }
 
   // Reenter template scopes from outermost to innermost.
   SmallVectorImpl<DeclContext *>::reverse_iterator II =
       DeclContextsToReenter.rbegin();
   for (; II != DeclContextsToReenter.rend(); ++II) {
-    if (ClassTemplatePartialSpecializationDecl *MD =
-            dyn_cast_or_null<ClassTemplatePartialSpecializationDecl>(*II)) {
-      TemplateParamScopeStack.push_back(
-          new ParseScope(this, Scope::TemplateParamScope));
-      Actions.ActOnReenterTemplateScope(getCurScope(), MD);
-      ++CurTemplateDepthTracker;
-    } else if (CXXRecordDecl *MD = dyn_cast_or_null<CXXRecordDecl>(*II)) {
-      bool IsClassTemplate = MD->getDescribedClassTemplate() != 0;
-      TemplateParamScopeStack.push_back(
-          new ParseScope(this, Scope::TemplateParamScope, 
-                        /*ManageScope*/IsClassTemplate));
-      Actions.ActOnReenterTemplateScope(getCurScope(),
-                                        MD->getDescribedClassTemplate());
-      if (IsClassTemplate) 
-        ++CurTemplateDepthTracker;
-    }
+    Decl *D = cast<Decl>(*II);
+
+    TemplateParamScopeStack.push_back(new ParseScope(this,
+          Scope::TemplateParamScope));
+    unsigned NumParamLists =
+      Actions.ActOnReenterTemplateScope(getCurScope(), D);
+    CurTemplateDepthTracker.addDepth(NumParamLists);
     TemplateParamScopeStack.push_back(new ParseScope(this, Scope::DeclScope));
+    if (*II != FunD)
       Actions.PushDeclContext(Actions.getCurScope(), *II);
   }
-  TemplateParamScopeStack.push_back(
-      new ParseScope(this, Scope::TemplateParamScope));
 
-  DeclaratorDecl *Declarator = dyn_cast<DeclaratorDecl>(FunD);
-  const unsigned DeclaratorNumTemplateParameterLists = 
-      (Declarator ? Declarator->getNumTemplateParameterLists() : 0);
-  if (Declarator && DeclaratorNumTemplateParameterLists != 0) {
-    Actions.ActOnReenterDeclaratorTemplateScope(getCurScope(), Declarator);
-    CurTemplateDepthTracker.addDepth(DeclaratorNumTemplateParameterLists);
-  }
   Actions.ActOnReenterTemplateScope(getCurScope(), LPT.D);
   ++CurTemplateDepthTracker;
 
   assert(!LPT.Toks.empty() && "Empty body!");
 
   // Append the current token at the end of the new token stream so that it
   // doesn't get lost.
   LPT.Toks.push_back(Tok);
   PP.EnterTokenStream(LPT.Toks.data(), LPT.Toks.size(), true, false);
 
   // Consume the previously pushed token.

lib/Sema/SemaDeclCXX.cpp

if (!ClassDecl->hasUserDeclaredDestructor()) {
     // shows up in the right place in the vtable and that we diagnose problems
     // with the implicit exception specification.
     if (ClassDecl->isDynamicClass() ||
         ClassDecl->needsOverloadResolutionForDestructor())
       DeclareImplicitDestructor(ClassDecl);
   }
 }
 
-void Sema::ActOnReenterDeclaratorTemplateScope(Scope *S, DeclaratorDecl *D) {
+unsigned Sema::ActOnReenterTemplateScope(Scope *S, Decl *D) {
   if (!D)
-    return;
+    return 0;
 
-  int NumParamList = D->getNumTemplateParameterLists();
-  for (int i = 0; i < NumParamList; i++) {
-    TemplateParameterList* Params = D->getTemplateParameterList(i);
-    for (TemplateParameterList::iterator Param = Params->begin(),
-                                      ParamEnd = Params->end();
-          Param != ParamEnd; ++Param) {
-      NamedDecl *Named = cast<NamedDecl>(*Param);
-      if (Named->getDeclName()) {
-        S->AddDecl(Named);
-        IdResolver.AddDecl(Named);
-      }
-    }
-  }
+  SmallVector<TemplateParameterList *, 4> ParameterLists;
+
+  if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D))
+    ParameterLists.push_back(TD->getTemplateParameters());
+
+  if (auto *PSD = dyn_cast<ClassTemplatePartialSpecializationDecl>(D))
+    ParameterLists.push_back(PSD->getTemplateParameters());
+
+  if (DeclaratorDecl *DD = dyn_cast<DeclaratorDecl>(D)) {
+    for (unsigned i = 0; i < DD->getNumTemplateParameterLists(); ++i)
+      ParameterLists.push_back(DD->getTemplateParameterList(i));
   }
 
-void Sema::ActOnReenterTemplateScope(Scope *S, Decl *D) {
-  if (!D)
-    return;
-  
-  TemplateParameterList *Params = 0;
-  if (TemplateDecl *Template = dyn_cast<TemplateDecl>(D))
-    Params = Template->getTemplateParameters();
-  else if (ClassTemplatePartialSpecializationDecl *PartialSpec
-           = dyn_cast<ClassTemplatePartialSpecializationDecl>(D))
-    Params = PartialSpec->getTemplateParameters();
-  else
-    return;
+  if (TagDecl *TD = dyn_cast<TagDecl>(D)) {
+    for (unsigned i = 0; i < TD->getNumTemplateParameterLists(); ++i)
+      ParameterLists.push_back(TD->getTemplateParameterList(i));
+    if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(TD)) {
+      if (ClassTemplateDecl *CTD = RD->getDescribedClassTemplate())
+        ParameterLists.push_back(CTD->getTemplateParameters());
+    }
+  }
 
-  for (TemplateParameterList::iterator Param = Params->begin(),
-                                    ParamEnd = Params->end();
-       Param != ParamEnd; ++Param) {
-    NamedDecl *Named = cast<NamedDecl>(*Param);
-    if (Named->getDeclName()) {
-      S->AddDecl(Named);
-      IdResolver.AddDecl(Named);
+  for (TemplateParameterList *Params : ParameterLists) {
+    for (NamedDecl *Param : *Params) {
+      if (Param->getDeclName()) {
+        S->AddDecl(Param);
+        IdResolver.AddDecl(Param);
       }
     }
   }
 
+  return ParameterLists.size();
+}
+
 void Sema::ActOnStartDelayedMemberDeclarations(Scope *S, Decl *RecordD) {
   if (!RecordD) return;
   AdjustDeclIfTemplate(RecordD);
   CXXRecordDecl *Record = cast<CXXRecordDecl>(RecordD);
   PushDeclContext(S, Record);
 }
 
 void Sema::ActOnFinishDelayedMemberDeclarations(Scope *S, Decl *RecordD) {

test/Parser/DelayedTemplateParsing.cpp

 template <typename T>
 constexpr T Fun(T A) { return T(0); }
 
 constexpr int Var = Fun(20);
 }
 
 template <typename T>
 auto invalidTrailingRetType() -> Bogus {} // expected-error {{unknown type name 'Bogus'}}
+
+namespace PR19613 {
+
+struct HeapTypeConfig {
+  static void from_bitset();
+};
+
+template <class Config>
+struct TypeImpl  {
+  struct BitsetType;
+
+  static void Any() {
+    BitsetType::New();
+  }
+};
+
+template<class Config>
+struct TypeImpl<Config>::BitsetType {
+  static void New() {
+    Config::from_bitset();
+  }
+};
+
+static void f() {
+  TypeImpl<HeapTypeConfig>::Any();
+}
+
+template<typename A> struct S {
+  template<typename B> struct T;
+};
+template<typename A> template<typename B> struct S<A>::T {
+  template<typename C, typename D> struct U;
+  template<typename C> struct U<C, C> {
+    template<typename E> static int f() {
+      return sizeof(A) + sizeof(B) + sizeof(C) + sizeof(E);
+    }
+  };
+};
+
+static void g() {
+  S<int>::T<int>::U<int,int>::f<int>();
+}
+
+template<typename T> struct SS {
+  template<typename U> struct X;
+  template<typename U> struct X<U*>;
+};
+template<typename T> template<typename U> struct SS<T>::X<U*> {
+  static int f() {
+    return sizeof(T) + sizeof(U);
+  }
+};
+
+static void h() {
+  SS<int>::X<int*>::f();
+}
+
+}