Fix for PR27015 (variable template initialized with a generic lambda expression)

[ahatanak]

Summary

I'm sending a WIP patch which fixes PR27015 to get an early feedback from the community.

This patch attempts to fix a crash which happens when a variable template is initialized with a generic lambda expression. Please see the example in the email I sent to cfe-dev:

http://lists.llvm.org/pipermail/cfe-dev/2016-April/048391.html

This patch makes changes to ensure the instantiated lambda class (which is the lambda class for fn<char> in the example) gets the right parent DeclContex (which is Decl::TranslationUnit in the example). After applying this patch, clang no longer crash compiling the example program. However, it still crashes when it compiles the following code:

$ cat test0.cpp
template <class> auto fn = [] {};
template <typename> void fn1() { fn<char>; }

$ clang -std=c++14 -c -o /dev/null test0.cpp

Assertion failed: (isDependentContext() && "cannot iterate dependent diagnostics of non-dependent context"), function ddiags, file include/clang/AST/DependentDiagnostic.h, line 176.
...
10 clang-3.8 0x000000010b4afd18 clang::Sema::PerformDependentDiagnostics(clang::DeclContext const*, clang::MultiLevelTemplateArgumentList const&) + 40
11 clang-3.8 0x000000010b46ee1b (anonymous namespace)::TemplateInstantiator::transformedLocalDecl(clang::Decl*, clang::Decl*) + 251

It fails when DeclContext::ddiags() is called on the lambda class of the old lambda expression because it's not a dependent context.

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3909–3917

Instead of the other changes, switch CurContext to the context of the variable template here:

Sema::ContextRAII SwitchContext(*this, Var->getDeclContext());

[ahatanak]

Address Richard's review comments and add a test case.

The test case currently asserts when fn0 is instantiated. This doesn't happen if I give the template parameter a name.

template <class N> auto fn0 = [] {};

instead of

template <class> auto fn0 = [] {};

I can prevent the assert if I make changes in Sema::ActOnTypeParameter to call S->AddDecl(Param) regardless of whether Param has a name. I'm not sure this is the right fix at the moment.

[ahatanak]

I'm looking for a way to avoid the assert in Sema::PerformDependentDiagnostics that is fired when a template parameter doesn't have a name.

In order to avoid the assert, CXXRecordDecl::isDependentLambda() should return true for the old lambda class, and in order to do that, somehow I have to set KnownDependent to true in Sema::ActOnStartOfLambdaDefinition when the lambda is used to initialize a variable template.

I can think of two ways to fix this:

1. Make changes in Sema::ActOnTypeParameter to call S->AddDecl(Param) regardless of whether Param has a name. This looks harmless to me because the list of Scope's decls is used just to see if a declaration belongs to a certain scope.

2. Add a field to Scope that indicates whether a variable template is being parsed.

Does this sound like a good idea or are there other ways to do what I'm trying to do?

[ahatanak]

Made a couple of changes to distinguish between an explicit specialization ('template<>') and a template declaration which doesn't have a named template parameter (see the variable template fn0 in test case vartemplate-lambda.cpp for an example).

• In Parser::ParseTemplateDeclarationOrSpecialization, clear the TemplateParamScope bit of the scope's flag if it's seen only explicit specializations.

• In Sema::ActOnStartOfLambdaDefinition, instead of checking whether the decl list is empty, check the TemplateParamScope bit of the template scope to see if it's in a "real" template declaration scope. I initially tried just removing the decls_empty() check, but found out that it asserted when the following code in test/CodeGenCXX/mangle-lambdas.cpp was compiled:

template<> double StaticMembers<double>::z = []{return 42; }();

Also, I had to change the check in test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp. Because the dependent bit of the lambda class is now set to true, Sema::CheckTemplateArgument returns early (near line 4876) before printing the last two of the three diagnostic messages:

1. error: lambda expression in an unevaluated operand
2. error: non-type template argument is not a constant expression
3. note: non-literal type 'const lambdas::(lambda at p1.cpp:2:21)' cannot be used in a constant expression

I'm not sure whether the lambda in p1.cpp should be considered dependent, but I don't think we've been doing the check correctly. If we add another template parameter in front of "I", clang prints only the first message:

template<int J, int I = ([] { return 5; }())>
int f();

[rsmith]

lib/Parse/ParseTemplate.cpp

151–153

Use ParseScopeFlags to change the flags here rather than calling setFlags directly. Please also use getFlags() & ~Scope::TemplateParamScope rather than ^ to make it more obvious that you're clearing the flag not just flipping it.

lib/Sema/SemaLambda.cpp

818

This should not be necessary. It looks like Scope::setFlags fails to update TemplateParamParent (etc) if the relevant flag changes. Instead of adding this if, try changing the body of Scope::setFlags to call Init(AnyParent, F);.

[ahatanak]

Use ParseScopeFlags to clear the TemplateParamScope rather than calling setFlags directly.

I tried calling Init(AnyParent, F) is Scope::setFlags, but it caused a lot of test cases to fail, which I'm currently investigating.

[ahatanak]

If I try calling Init(AnyParent, F) in Scope::setFlags, clang fails to compile the following code because it cannot find the definition of struct "foo":

void foo3(void) 
  struct foo {
    int a, f;
  };
  char *np = nullptr;
  int *ip = &(((struct foo *)np)->f);
  *ip = 0;
}

I can fix this test if I add a boolean flag to Scope::Init, which tells the function to return early before DeclsInScope and other fields are cleared.

All the other tests pass too except for test/CXX/drs/dr1xx.cpp:

namespace dr183 { // dr183: sup 382
  template<typename T> struct A {};
  template<typename T> struct B {
    typedef int X;
  };
  template<> struct A<int> {
    typename B<int>::X x;
  };
}

clang used to compile this code without any warnings or errors, but it now issues the following warning if -std=c++98 is on the command line:

dr1xx.cpp:7:5: warning: 'typename' occurs outside of a template [-Wc++11-extensions]

typename B<int>::X x;
^~~~~~~~~

1 warning generated.

Is clang correct to issue the warning in this case?

[ahatanak]

• Defined a private overload of Scope::setFlags which is called from Scope::setFlags and Scope::Init.
• Stop checking TmplScope->isTemplateParamScope() in Sema::ActOnStartOfLambdaDefinition.
• Fix test case test/CXX/drs/dr1xx.cpp to check an error message (not a warning) is printed when it's compiled with -std=c++98.

[rsmith]

LGTM, thanks!

[ahatanak]

Thanks for the review. I committed the patch in r267956 and r267975.

Do you think I should make setFlags(unsigned F) return early if F == Flags?

lib/Parse/ParseTemplate.cpp

Parser::ParseTemplateDeclarationOrSpecialization(unsigned Context,
                                                  SourceLocation &DeclEnd,
                                                  AccessSpecifier AS,
                                                  AttributeList *AccessAttrs) {
   assert(Tok.isOneOf(tok::kw_export, tok::kw_template) &&
          "Token does not start a template declaration.");
 
   // Enter template-parameter scope.
   ParseScope TemplateParmScope(this, Scope::TemplateParamScope);
+  Scope *TemplateParmScopePtr = getCurScope();
 
   // Tell the action that names should be checked in the context of
   // the declaration to come.
   ParsingDeclRAIIObject
     ParsingTemplateParams(*this, ParsingDeclRAIIObject::NoParent);
 
   // Parse multiple levels of template headers within this template
   // parameter scope, e.g.,
if (!TemplateParams.empty()) {
           return nullptr;
         }
       }
     } else {
       LastParamListWasEmpty = true;
     }
   } while (Tok.isOneOf(tok::kw_export, tok::kw_template));
 
+  if (isSpecialization)
+    TemplateParmScopePtr->setFlags(TemplateParmScopePtr->getFlags() ^
+                                   Scope::TemplateParamScope);
+
   // Parse the actual template declaration.
   return ParseSingleDeclarationAfterTemplate(Context,
                                              ParsedTemplateInfo(&ParamLists,
                                                              isSpecialization,
                                                          LastParamListWasEmpty),
                                              ParsingTemplateParams,
                                              DeclEnd, AS, AccessAttrs);
 }

lib/Sema/SemaLambda.cpp

void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
   bool KnownDependent = false;
   LambdaScopeInfo *const LSI = getCurLambda();
   assert(LSI && "LambdaScopeInfo should be on stack!");
 
   // The lambda-expression's closure type might be dependent even if its
   // semantic context isn't, if it appears within a default argument of a
   // function template.
   if (Scope *TmplScope = CurScope->getTemplateParamParent())
-    if (!TmplScope->decl_empty())
+    if (TmplScope->isTemplateParamScope())
       KnownDependent = true;
 
   // Determine the signature of the call operator.
   TypeSourceInfo *MethodTyInfo;
   bool ExplicitParams = true;
   bool ExplicitResultType = true;
   bool ContainsUnexpandedParameterPack = false;
   SourceLocation EndLoc;

lib/Sema/SemaTemplateInstantiateDecl.cpp

if (Var->getAnyInitializer())
     return;
 
   if (OldVar->getInit()) {
     if (Var->isStaticDataMember() && !OldVar->isOutOfLine())
       PushExpressionEvaluationContext(Sema::ConstantEvaluated, OldVar);
     else
       PushExpressionEvaluationContext(Sema::PotentiallyEvaluated, OldVar);
 
     // Instantiate the initializer.
-    ExprResult Init =
-        SubstInitializer(OldVar->getInit(), TemplateArgs,
+    ExprResult Init;
+
+    {
+      ContextRAII SwitchContext(*this, Var->getDeclContext());
+      Init = SubstInitializer(OldVar->getInit(), TemplateArgs,
                               OldVar->getInitStyle() == VarDecl::CallInit);
+    }
+
     if (!Init.isInvalid()) {
       bool TypeMayContainAuto = true;
       Expr *InitExpr = Init.get();
 
       if (Var->hasAttr<DLLImportAttr>() &&
           (!InitExpr ||
            !InitExpr->isConstantInitializer(getASTContext(), false))) {
         // Do not dynamically initialize dllimport variables.

test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp

#else
   // expected-note@-5 {{read of non-constexpr variable 'iptr' is not allowed in a constant expression}}
 #endif
 }
 #endif // CPP11ONLY
 
 namespace default_args {
 #ifdef CPP11ONLY
 namespace lambdas {
-template<int I = ([] { return 5; }())> //expected-error 2{{constant expression}} expected-note{{constant expression}}
+template<int I = ([] { return 5; }())> //expected-error {{constant expression}}
 int f();
 }
 #endif // CPP11ONLY
 
 }

test/SemaCXX/vartemplate-lambda.cpp

+// RUN: %clang_cc1 -std=c++14 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template <class> auto fn0 = [] {};
+template <typename> void foo0() { fn0<char>(); }
+
+template<typename T> auto fn1 = [](auto a) { return a + T(1); };
+
+template <typename X>
+int foo2() {
+  X a = 0x61;
+  fn1<char>(a);
+  return 0;
+}
+
+int main() {
+  foo2<int>();
+}