[Sema] Pass CombineWithOuterScope = true to constructor of LocalInstantiationScope

[ahatanak]

Summary

This fixes PR28795.

https://llvm.org/bugs/show_bug.cgi?id=28795

Sema wasn't replacing DependentScopeDeclRefExpr with DeclRefExpr during template instantiation of the default argument of the lambda function because it failed to find the instantiated enum "foo" in LocalInstantiationScope. This patch fixes the bug by allowing it to search the outer scope (function func).

[sepavloff]

test/SemaTemplate/default-expr-arguments-3.cpp

20

Please add the following test to the patch:

template<typename T> void f1() {
  enum class foo { a, b };
  struct S {
    int g1(foo n = foo::a);
  };
}

template void f1<int>();

It must also compile correctly.

[ahatanak]

OK, I'll add the test.

When I apply this patch and compile your test, I see DependentScopeDeclRefExpr gets converted to DeclRefExpr when the function is instantiated. This doesn't happen without this patch, but somehow clang still manages to emit the same code.

Also, Erik Pilkington pointed out that clang still crashes when it compiles the following code. I'm trying to figure out why that happens.

// Template variable case:
enum class foo { a, b };
template <class T> auto a = [](foo f = foo::a) { return f; }();

auto foo1() {
  return a<int>;
}

// Template struct case:
template <class T> struct func {
  void bar() {
    enum class foo { a, b };
    [](foo f = foo::a) { return f; }();
  }
};

template struct func<int>;

[ahatanak]

In each of these cases, "foo::a" is a non-dependent type, so it seems that clang shouldn't use DependentScopeDeclRefExpr or CXXDependentScopeMemberExpr in the AST when the template definitions are parsed.

[ahatanak]

Handle the case in which the enum is declared inside a member function and add test cases.

The variable template test case still crashes. It looks like it crashes whenever the lambda expression takes a default argument:

template <class T> auto a = [](int f = 7) { return f; }();

auto foo1() {

return a<int>;

}

[ahatanak]

Fix Sema::getTemplateInstantiationArgs to return the template instantiation args when variable templates are being instantiated.

[sepavloff]

include/clang/Sema/Sema.h

6619 ↗

(On Diff #68078)

I would propose to use something like 'getVarTemplateArguments' to make the name more similar to other methods.

lib/Sema/SemaTemplateInstantiate.cpp

186–198

The code does not seem to be the best solution. It assumes that if instantiation of a variable is requested, its arguments define all parameters needed to instantiate code. It is not so for the code:

template<typename T2>
struct C1 {
  template<typename T>
  static int v1;
};

template<typename T2>
template<typename T>
int C1<T2>::v1 = [](int a = T2(1)) { return a; }();

int main(int argc, char *argv[]) {
  (void) C1<long*>::v1<int>;
  return 0;
}

This problem is caused by different nature of variable templates. When instantiating a function or a class all instantiated entities go to the contexts defined by the class or the function respectively. A variable does not represent a declaration context and its initializer and all declarations created during the instantiation go to enclosing context. So logic in getTemplateInstantiationArgs does not work.
May be we should extend the logic of getTemplateInstantiationArgs so that it supports variable template and still remains recursive?

[ahatanak]

I agree that extending the logic of getTemplateInstantiationArgs seems like a better approach. I changed Sema::getTemplateInstantiationArgs to search for the template arguments twice, first for the initializer's template arguments and then for the VarTemplateSpecializationDecl. This should enable handling cases where a variable template is declared inside a class/struct.

I wonder whether VarTemplateSpecializationDecl can be made a subclass of DeclContext and have it contain the lambda class of the initializer. I think that will simplify the logic of getTemplateInstantiationArgs.

[sepavloff]

IIUC, the problem is observed because Sema::getTemplateInstantiationArgs does not handle the case of variable templates properly. Classes and functions are declaration contexts and implementation of the aforementioned function (and probably others) relies on this fact. Variable does not represents a context, and this causes errors like this. We cannot make VarTemplateSpecializationDecl a subclass of DeclContext because the latter not only serves as a host for other declarations but also supports name lookup. None is pertinent to the case of variable specialization.

I think, logic of getTemplateInstantiationArgs should be changed. The new implementation could inspect current instantiation (Sema::ActiveTemplateInstantiations) to check if it is an instantiation of a variable template. This could eliminate need of VarTemplateSpec and VarTemplateSpecializationRAII. Such solution looks more flexible as variable initializer may contain references to other variable instantiations, so single value of VarTemplateSpec is not sufficient to track instantiations.

[ahatanak]

Do you think we can transform the Exprs for the uninstantiated default arguments in TreeTransform<Derived>::TransformLambdaExpr? I guess it would be much simpler than trying to make getTemplateInstantiationArgs return the correct template arguments.

[ahatanak]

• Transform uninstantiated default arguments Exprs in TreeTransform<Derived>::TransformLambdaExpr.
• In default-expr-arguments-3.cpp, check that there are no crashes or diagnostics emitted instead of printing the AST.

[ahatanak]

Rebase and ping.

[sepavloff]

Looks good. Thank you!

lib/Sema/SemaTemplateInstantiate.cpp

while (!Ctx->isFileContext()) {
 
     Ctx = Ctx->getParent();
     RelativeToPrimary = false;
   }
 
   return Result;
 }
 
 bool Sema::ActiveTemplateInstantiation::isInstantiationRecord() const {
   switch (Kind) {
   case TemplateInstantiation:
   case ExceptionSpecInstantiation:
   case DefaultTemplateArgumentInstantiation:
   case DefaultFunctionArgumentInstantiation:
   case ExplicitTemplateArgumentSubstitution:
   case DeducedTemplateArgumentSubstitution:
   case PriorTemplateArgumentSubstitution:
     return true;
 
   case DefaultTemplateArgumentChecking:
     return false;
   }
 
   llvm_unreachable("Invalid InstantiationKind!");
 }
 
 Sema::InstantiatingTemplate::InstantiatingTemplate(
     Sema &SemaRef, ActiveTemplateInstantiation::InstantiationKind Kind,
     SourceLocation PointOfInstantiation, SourceRange InstantiationRange,
if (OldParm->hasUninstantiatedDefaultArg()) {
     NewParm->setUnparsedDefaultArg();
     UnparsedDefaultArgInstantiations[OldParm].push_back(NewParm);
   } else if (Expr *Arg = OldParm->getDefaultArg()) {
     FunctionDecl *OwningFunc = cast<FunctionDecl>(OldParm->getDeclContext());
     if (OwningFunc->isLexicallyWithinFunctionOrMethod()) {
       // Instantiate default arguments for methods of local classes (DR1484)
       // and non-defining declarations.
       Sema::ContextRAII SavedContext(*this, OwningFunc);
-      LocalInstantiationScope Local(*this);
+      LocalInstantiationScope Local(*this, true);
       ExprResult NewArg = SubstExpr(Arg, TemplateArgs);
       if (NewArg.isUsable()) {
         // It would be nice if we still had this.
         SourceLocation EqualLoc = NewArg.get()->getLocStart();
         SetParamDefaultArgument(NewParm, NewArg.get(), EqualLoc);
       }
     } else {
       // FIXME: if we non-lazily instantiated non-dependent default args for

test/SemaTemplate/default-expr-arguments-3.cpp

+// RUN: %clang_cc1 -std=c++14 -ast-dump %s 2>&1 | FileCheck %s
+
+namespace PR28795 {
+  // CHECK: FunctionDecl{{.*}}func 'void (void)'
+  // CHECK:  LambdaExpr
+  // CHECK:   CXXMethodDecl{{.*}}operator() 'enum foo (enum foo) const' inline
+  // CHECK:    ParmVarDecl{{.*}}f 'enum foo' cinit
+  // CHECK:     DeclRefExpr{{.*}}'enum foo' EnumConstant{{.*}}'a' 'enum foo'
+
+  template<typename T>
+  void func() {
+    enum class foo { a, b };
+    auto bar = [](foo f = foo::a) { return f; };
+    bar();
+  }
+
+  void foo() {
+    func<int>();
+  }
+}