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

150–152

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

816–817

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/Scope.h

private:
 
   /// \brief Used to determine if errors occurred in this scope.
   DiagnosticErrorTrap ErrorTrap;
 
   /// A lattice consisting of undefined, a single NRVO candidate variable in
   /// this scope, or over-defined. The bit is true when over-defined.
   llvm::PointerIntPair<VarDecl *, 1, bool> NRVO;
 
+  void setFlags(Scope *Parent, unsigned F);
+
 public:
   Scope(Scope *Parent, unsigned ScopeFlags, DiagnosticsEngine &Diag)
     : ErrorTrap(Diag) {
     Init(Parent, ScopeFlags);
   }
 
   /// getFlags - Return the flags for this scope.
   ///
   unsigned getFlags() const { return Flags; }
-  void setFlags(unsigned F) { Flags = F; }
+  void setFlags(unsigned F) { setFlags(getParent(), F); }
 
   /// isBlockScope - Return true if this scope correspond to a closure.
   bool isBlockScope() const { return Flags & BlockScope; }
 
   /// getParent - Return the scope that this is nested in.
   ///
   const Scope *getParent() const { return AnyParent; }
   Scope *getParent() { return AnyParent; }

lib/Parse/ParseTemplate.cpp

if (!TemplateParams.empty()) {
           return nullptr;
         }
       }
     } else {
       LastParamListWasEmpty = true;
     }
   } while (Tok.isOneOf(tok::kw_export, tok::kw_template));
 
+  unsigned NewFlags = getCurScope()->getFlags() & ~Scope::TemplateParamScope;
+  ParseScopeFlags TemplateScopeFlags(this, NewFlags, isSpecialization);
+
   // Parse the actual template declaration.
   return ParseSingleDeclarationAfterTemplate(Context,
                                              ParsedTemplateInfo(&ParamLists,
                                                              isSpecialization,
                                                          LastParamListWasEmpty),
                                              ParsingTemplateParams,
                                              DeclEnd, AS, AccessAttrs);
 }

lib/Sema/Scope.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Sema/Scope.h"
 #include "clang/AST/Decl.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 
-void Scope::Init(Scope *parent, unsigned flags) {
+void Scope::setFlags(Scope *parent, unsigned flags) {
   AnyParent = parent;
   Flags = flags;
 
   if (parent && !(flags & FnScope)) {
     BreakParent    = parent->BreakParent;
     ContinueParent = parent->ContinueParent;
   } else {
     // Control scopes do not contain the contents of nested function scopes for
else if ((flags & ClassScope) && getParent()->isClassScope())
       ; // Nested class scopes aren't ambiguous.
     else if ((flags & ClassScope) && getParent()->getFlags() == DeclScope)
       ; // Classes inside of namespaces aren't ambiguous.
     else if ((flags & EnumScope))
       ; // Don't increment for enum scopes.
     else
       incrementMSManglingNumber();
   }
+}
+
+void Scope::Init(Scope *parent, unsigned flags) {
+  setFlags(parent, flags);
 
   DeclsInScope.clear();
   UsingDirectives.clear();
   Entity = nullptr;
   ErrorTrap.reset();
   NRVO.setPointerAndInt(nullptr, 0);
 }
 

lib/Sema/SemaLambda.cpp

void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
   // Determine if we're within a context where we know that the lambda will
   // be dependent, because there are template parameters in scope.
   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;
+  if (CurScope->getTemplateParamParent())
 
   // Determine the signature of the call operator.
   TypeSourceInfo *MethodTyInfo;
   bool ExplicitParams = true;
   bool ExplicitResultType = true;
   bool ContainsUnexpandedParameterPack = false;
   SourceLocation EndLoc;
   SmallVector<ParmVarDecl *, 8> Params;

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/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> {
+#if __cplusplus <= 199711
+    typename B<int>::X x; // expected-error {{'typename' occurs outside of a template}}
+#else
     typename B<int>::X x;
+#endif
   };
 }
 
 namespace dr184 { // dr184: yes
   template<typename T = float> struct B {};
 
   template<template<typename TT = float> class T> struct A {
     void f();

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