-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

4887–4889 ↗

(On Diff #8998)

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

1254–1256

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() {}
};

1262–1272

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

1272

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

lib/Sema/SemaDeclCXX.cpp

5983–6000 ↗

(On Diff #8998)

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

4887–4889 ↗

(On Diff #8998)

Done. I've folded them into one function.

lib/Parse/ParseTemplate.cpp

1254–1256

Thanks! Added this to the battery of tests.

1262–1272

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

1272

ActOnReenterRecordTemplateScope has gone away now.

lib/Sema/SemaDeclCXX.cpp

5983–6000 ↗

(On Diff #8998)

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 ↗

(On Diff #9013)

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...)

1273–1274

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

1280–1288

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 ↗

(On Diff #9013)

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..

1273–1274

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

1280–1288

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

1275

cast, not dyn_cast, please.

1287–1289

Can you push the function template case into ActOnReenterTemplateScope too?

lib/Sema/SemaDeclCXX.cpp

6000 ↗

(On Diff #9020)

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

1275

Done.

1287–1289

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 ↗

(On Diff #9020)

"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

1275

Maybe sink this into its only use.

1287–1289

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 ↗

(On Diff #9023)

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 ↗

(On Diff #9023)

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

[hans]

lib/Parse/ParseTemplate.cpp

1275

Done.

1287–1289

OK, managed to get this work eventually.

lib/Sema/SemaDeclCXX.cpp

5987 ↗

(On Diff #9023)

Done.

5989–5990 ↗

(On Diff #9023)

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

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();
   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;
+    } else if (CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(*II)) {
+      ClassTemplateDecl *CTD = RD->getDescribedClassTemplate();
+
+      if (!CTD) {
+        if (CXXRecordDecl *Parent = dyn_cast<CXXRecordDecl>(RD->getParent()))
+          CTD = Parent->getDescribedClassTemplate();
+      }
+
+      bool IsClassTemplate = CTD != 0;
       TemplateParamScopeStack.push_back(
           new ParseScope(this, Scope::TemplateParamScope, 
-                        /*ManageScope*/IsClassTemplate));
-      Actions.ActOnReenterTemplateScope(getCurScope(),
-                                        MD->getDescribedClassTemplate());
-      if (IsClassTemplate) 
+                        /*EnteredScope*/IsClassTemplate));
+
+      if (IsClassTemplate) {
+        Actions.ActOnReenterTemplateScope(getCurScope(), CTD);
         ++CurTemplateDepthTracker;
       }
+    }
     TemplateParamScopeStack.push_back(new ParseScope(this, Scope::DeclScope));
     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);

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();
+}
+
+}