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

3899–3901

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 ↗

(On Diff #55356)

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

822

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?

include/clang/Sema/Sema.h

   /// ActOnCXXExitDeclInitializer - Invoked after we are finished parsing an
   /// initializer for the declaration 'Dcl'.
   void ActOnCXXExitDeclInitializer(Scope *S, Decl *Dcl);
 
   /// \brief Create a new lambda closure type.
   CXXRecordDecl *createLambdaClosureType(SourceRange IntroducerRange,
                                          TypeSourceInfo *Info,
                                          bool KnownDependent, 
-                                         LambdaCaptureDefault CaptureDefault);
+                                         LambdaCaptureDefault CaptureDefault,
+                                         DeclContext *DC = nullptr);
 
   /// \brief Start the definition of a lambda expression.
   CXXMethodDecl *startLambdaDefinition(CXXRecordDecl *Class,
                                        SourceRange IntroducerRange,
                                        TypeSourceInfo *MethodType,
                                        SourceLocation EndLoc,
                                        ArrayRef<ParmVarDecl *> Params, 
                                        bool IsConstexprSpecified);

   StmtResult SubstStmt(Stmt *S,
                        const MultiLevelTemplateArgumentList &TemplateArgs);
 
   Decl *SubstDecl(Decl *D, DeclContext *Owner,
                   const MultiLevelTemplateArgumentList &TemplateArgs);
 
   ExprResult SubstInitializer(Expr *E,
                        const MultiLevelTemplateArgumentList &TemplateArgs,
-                       bool CXXDirectInit);
+                       bool CXXDirectInit, VarDecl *VarInit = nullptr);
 
   bool
   SubstBaseSpecifiers(CXXRecordDecl *Instantiation,
                       CXXRecordDecl *Pattern,
                       const MultiLevelTemplateArgumentList &TemplateArgs);
 
   bool
   InstantiateClass(SourceLocation PointOfInstantiation,

lib/Sema/SemaLambda.cpp

LSI->GLTemplateParameterList = TemplateParameterList::Create(
         RAngleLoc);
   }
   return LSI->GLTemplateParameterList;
 }
 
 CXXRecordDecl *Sema::createLambdaClosureType(SourceRange IntroducerRange,
                                              TypeSourceInfo *Info,
                                              bool KnownDependent, 
-                                             LambdaCaptureDefault CaptureDefault) {
-  DeclContext *DC = CurContext;
+                                             LambdaCaptureDefault CaptureDefault,
+                                             DeclContext *DC) {
+  if (!DC) {
+    DC = CurContext;
     while (!(DC->isFunctionOrMethod() || DC->isRecord() || DC->isFileContext()))
       DC = DC->getParent();
+  }
+
   bool IsGenericLambda = getGenericLambdaTemplateParameterList(getCurLambda(),
                                                                *this);  
   // Start constructing the lambda class.
   CXXRecordDecl *Class = CXXRecordDecl::CreateLambda(Context, DC, Info,
                                                      IntroducerRange.getBegin(),
                                                      KnownDependent, 
                                                      IsGenericLambda, 
                                                      CaptureDefault);
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())
       KnownDependent = true;
 
   // Determine the signature of the call operator.
   TypeSourceInfo *MethodTyInfo;
   bool ExplicitParams = true;
   bool ExplicitResultType = true;
   bool ContainsUnexpandedParameterPack = false;
   SourceLocation EndLoc;

lib/Sema/SemaTemplateInstantiate.cpp

class TemplateInstantiator : public TreeTransform<TemplateInstantiator> {
     DeclarationName Entity;
 
   public:
     typedef TreeTransform<TemplateInstantiator> inherited;
 
     TemplateInstantiator(Sema &SemaRef,
                          const MultiLevelTemplateArgumentList &TemplateArgs,
                          SourceLocation Loc,
-                         DeclarationName Entity)
-      : inherited(SemaRef), TemplateArgs(TemplateArgs), Loc(Loc),
+                         DeclarationName Entity, VarDecl *VarInit = nullptr)
+      : inherited(SemaRef, VarInit), TemplateArgs(TemplateArgs), Loc(Loc),
         Entity(Entity) { }
 
     /// \brief Determine whether the given type \p T has already been
     /// transformed.
     ///
     /// For the purposes of template instantiation, a type has already been
     /// transformed if it is NULL or if it is not dependent.
     bool AlreadyTransformed(QualType T);
Sema::SubstExpr(Expr *E, const MultiLevelTemplateArgumentList &TemplateArgs) {
   TemplateInstantiator Instantiator(*this, TemplateArgs,
                                     SourceLocation(),
                                     DeclarationName());
   return Instantiator.TransformExpr(E);
 }
 
 ExprResult Sema::SubstInitializer(Expr *Init,
                           const MultiLevelTemplateArgumentList &TemplateArgs,
-                          bool CXXDirectInit) {
+                          bool CXXDirectInit, VarDecl *VarInit) {
   TemplateInstantiator Instantiator(*this, TemplateArgs,
                                     SourceLocation(),
-                                    DeclarationName());
+                                    DeclarationName(), VarInit);
   return Instantiator.TransformInitializer(Init, CXXDirectInit);
 }
 
 bool Sema::SubstExprs(ArrayRef<Expr *> Exprs, bool IsCall,
                       const MultiLevelTemplateArgumentList &TemplateArgs,
                       SmallVectorImpl<Expr *> &Outputs) {
   if (Exprs.empty())
     return false;

lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateVariableInitializer(
 
   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,
-                         OldVar->getInitStyle() == VarDecl::CallInit);
+                         OldVar->getInitStyle() == VarDecl::CallInit, Var);
     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.

lib/Sema/TreeTransform.h

 protected:
   Sema &SemaRef;
 
   /// \brief The set of local declarations that have been transformed, for
   /// cases where we are forced to build new declarations within the transformer
   /// rather than in the subclass (e.g., lambda closure types).
   llvm::DenseMap<Decl *, Decl *> TransformedLocalDecls;
 
+  /// If not null, this is the variable being initialized.
+  VarDecl *VarInit;
+
 public:
   /// \brief Initializes a new tree transformer.
-  TreeTransform(Sema &SemaRef) : SemaRef(SemaRef) { }
+  TreeTransform(Sema &SemaRef, VarDecl *Var = nullptr)
+      : SemaRef(SemaRef), VarInit(Var) {}
 
   /// \brief Retrieves a reference to the derived class.
   Derived &getDerived() { return static_cast<Derived&>(*this); }
 
   /// \brief Retrieves a reference to the derived class.
   const Derived &getDerived() const {
     return static_cast<const Derived&>(*this);
   }
NewCallOpTSI = NewCallOpTLBuilder.getTypeSourceInfo(getSema().Context,
                                                         NewCallOpType);
   }
 
   LambdaScopeInfo *LSI = getSema().PushLambdaScope();
   Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
   LSI->GLTemplateParameterList = TPL;
 
   // Create the local class that will describe the lambda.
+  // If the new lambda is used to initialize a variable template specialization,
+  // use the old lambda's DeclContext.
+  DeclContext *DC = nullptr;
+
+  if (VarInit && isa<VarTemplateSpecializationDecl>(VarInit))
+    DC = E->getLambdaClass()->getParent();
+
   CXXRecordDecl *Class
     = getSema().createLambdaClosureType(E->getIntroducerRange(),
                                         NewCallOpTSI,
                                         /*KnownDependent=*/false,
-                                        E->getCaptureDefault());
+                                        E->getCaptureDefault(), DC);
   getDerived().transformedLocalDecl(E->getLambdaClass(), Class);
 
   // Build the call operator.
   CXXMethodDecl *NewCallOperator = getSema().startLambdaDefinition(
       Class, E->getIntroducerRange(), NewCallOpTSI,
       E->getCallOperator()->getLocEnd(),
       NewCallOpTSI->getTypeLoc().castAs<FunctionProtoTypeLoc>().getParams(),
       E->getCallOperator()->isConstexpr());