PR45000: Let Sema::SubstParmVarDecl handle default args of lambdas in initializers

[aaronpuchert]

Summary

We extend the behavior for local functions and methods of local classes
to lambdas in variable initializers. The initializer is not a separate
scope, but we treat it as such.

We also remove the (faulty) instantiation of default arguments in
TreeTransform::TransformLambdaExpr, because it doesn't do proper
initialization, and if it did, we would do it twice (and thus also emit
eventual errors twice).

[aaronpuchert]

@rsmith, should I put a testcase under test/SemaTemplate/, perhaps as instantiate-lambda.cpp?

[aaronpuchert]

In D76038#1918571, @aaronpuchert wrote:

@rsmith, should I put a testcase under test/SemaTemplate/, perhaps as instantiate-lambda.cpp?

I'll probably put it into test/SemaCXX/vartemplate-lambda.cpp like D23096 that added the loop.

[aaronpuchert]

Seems the same issue in Sema::SubstParmVarDecl was fixed in rGdc40b618cf397df7369406b3f61e91ccb57fb9f6.

[mstorsjo]

Ping - any update on this one? Are you waiting on @rsmith for input on where to place a testcase?

[aaronpuchert]

In D76038#1978667, @mstorsjo wrote:

Ping - any update on this one? Are you waiting on @rsmith for input on where to place a testcase?

No, I just didn't get around to writing the test yet, thanks for reminding me.

Although input from @rsmith would be appreciated. There are two ways to write the test: rely on the assertion to catch the type mismatch or inspect an AST dump and look for an ImplicitCastExpr. I'm tending towards the latter option, because it doesn't rely on assertions. (Which aren't always on.)

[aaronpuchert]

I tested a bit more and there is a problem: some errors now appear twice, and I think they shouldn't.

template <typename T>
void f(int x = [](T x = nullptr) -> int { return x; }());

void g() { f<int>(); }

The error is expected, but it should only appear once:

<stdin>:2:23: error: cannot initialize a parameter of type 'int' with an rvalue of type 'nullptr_t'
  void f(int x = [](T x = nullptr) -> int { return x; }());
                      ^   ~~~~~~~

[aaronpuchert]

Add test case, loosely based on that in the bug report. By choosing a conversion that should error out we can detect the issue in a pure frontend test.

However, we would expect the error only once. Further investigation is needed.

[rsmith]

clang/lib/Sema/TreeTransform.h

12161–12162

This logic shouldn't even be here in the first place.. this should all be handled by SubstParmVarDecl in SemaTemplateInstantiate. It's wrong for TreeTransform to be assuming that TransformFunctionProtoType will have built new function parameters -- it doesn't do so by default, so in the non-template-instantiation case, this is clobbering the default argument information on the original lambda. And in the template instantiation case, this is clobbering the default argument information (including initialization) that SubstParmVarDecl already did.

Please try deleting this loop entirely. If there are still problems (and I think there are: I don't think we handle delayed instantiation for default argument in generic lambdas properly), we should address them by changing how we do default argument instantiation in SubstParmVarDecl. In particular, where that function calls SetParamDefaultArgument after calling SubstExpr, we should presumably instead be calling setUninstantiatedDefaultArg on the parameter if it's a parameter of a generic lambda (we still need to instantiate the default argument with the template arguments of the actual lambda call operator).

[rsmith]

clang/lib/Sema/TreeTransform.h

12161–12162

Hmm, what I wrote above isn't entirely true -- TreeTransform does create new parameters by default. But nonetheless, it's the responsibility of TransformFunctionTypeParam to set up the default argument, not the responsibility of this code, so we shouldn't be partially overwriting its results here. (Perhaps we should move the substitution into default arguments from SubstParmVarDecl into TreeTransform::TransformFunctionTypeParam, though I think that can be done separately from this bugfix.)

[aaronpuchert]

clang/lib/Sema/TreeTransform.h

12161–12162

Thanks for your hints! I was already somewhat suspicious of that loop. Removing it works indeed pretty well, but I'm running into an issue with variable templates. Default arguments are usually only instantiated when needed by a call, which happens in Sema::CheckCXXDefaultArgExpr. Now in the following code, the MultiLevelTemplateArgumentList returned from Sema::getTemplateInstantiationArgs is empty instead of being [int]:

template <typename T>
int k = [](T x = 0.0) -> int { return x; }();

template int k<int>;

Looking at getTemplateInstantiationArgs, that's not very surprising: if we are a DeclContext, we go through the contexts and collect all template arguments from those, but a variable template is not a context. The variable template case is handled right at the beginning, but only if the declaration itself is a variable template.

Meaning that deep inside the initializer of a variable template, we don't get its template parameters.

[aaronpuchert]

clang/lib/Sema/TreeTransform.h

12161–12162

Or maybe default arguments need to instantiated immediately similar to the situation from DR1484? It says "Within a template declaration ...", which would seem to apply here. There is no enclosing scope though, so I'm not sure we can call this a local class.

So I guess we need to instantiate in more cases in SubstParmVarDecl, as you indicated.

[aaronpuchert]

Let's have a look at an example. Is this allowed?

template<typename T, int... args>
int x = [](T = T()){ return 0; }(args...);

template int x<int>; // Uses default argument int() = 0.

struct S { S(int); };
template int x<S, 0>; // Default argument not well-formed, but not needed.

Currently we don't allow it, and neither does GCC.

[aaronpuchert]

Remove loop in TreeTransform::TransformLambdaExpr and make sure Sema::SubstParmVarDecl handles the situation.

[aaronpuchert]

This seems to be in line with our current behavior, so I hope it's right:

struct S { S(int); };

template<typename T>
auto l = [](T x = T()) { return x; };

template<typename T>
struct Fun {
    T operator()(T x = T()) const { return x; }
};

void f() {
    l<int>();   // Ok.
    l<S>(S(0)); // Error at "T()": no matching constructor for initialization of 'S'
    Fun<S> f;
    f(S(0));    // Ok.
}

[aaronpuchert]

Adding @ahatanak and @sepavloff since I'm effectively reverting D23096 now.

[aaronpuchert]

Slightly adapt test results: there is an additional error now.

[rsmith]

The responses to the various testcases you posted look OK to me. The language rules aren't clear, and we can revisit this if they get clarified in a different direction, but treating lambdas in variable templates the same as function definitions seems reasonable for now.

In D76038#1991426, @aaronpuchert wrote:

Adding @ahatanak and @sepavloff since I'm effectively reverting D23096 now.

This looks reasonable to me, but maybe ping @ahatanak (on email / IRC / wherever) to make sure this isn't breaking something we don't have a test for.

[ahatanak]

The test I have passes after applying this patch.

[zahiraam]

This change is generating this crash discussed here: https://bugs.llvm.org/show_bug.cgi?id=49834
@aaronpuchert do you have any fix for it?

[aaronpuchert]

In D76038#3219500, @zahiraam wrote:

This change is generating this crash discussed here: https://bugs.llvm.org/show_bug.cgi?id=49834

Now migrated to https://github.com/llvm/llvm-project/issues/49178.

@aaronpuchert do you have any fix for it?

No, everything I know is in that bug report. I was hoping @rsmith would reply with an idea, but it's quite complicated.

clang/include/clang/AST/DeclBase.h

public:
   /// isDefinedOutsideFunctionOrMethod - This predicate returns true if this
   /// scoped decl is defined outside the current function or method.  This is
   /// roughly global variables and functions, but also handles enums (which
   /// could be defined inside or outside a function etc).
   bool isDefinedOutsideFunctionOrMethod() const {
     return getParentFunctionOrMethod() == nullptr;
   }
 
-  /// Returns true if this declaration lexically is inside a function.
-  /// It recognizes non-defining declarations as well as members of local
-  /// classes:
+  /// Returns true if this declaration is lexically inside a function or inside
+  /// a variable initializer. It recognizes non-defining declarations as well
+  /// as members of local classes:
   /// \code
   ///     void foo() { void bar(); }
   ///     void foo2() { class ABC { void bar(); }; }
+  ///     inline int x = [](){ return 0; };
   /// \endcode
-  bool isLexicallyWithinFunctionOrMethod() const;
+  bool isInLocalScope() const;
 
   /// If this decl is defined inside a function/method/block it returns
   /// the corresponding DeclContext, otherwise it returns null.
   const DeclContext *getParentFunctionOrMethod() const;
   DeclContext *getParentFunctionOrMethod() {
     return const_cast<DeclContext*>(
                     const_cast<const Decl*>(this)->getParentFunctionOrMethod());
   }

clang/lib/AST/DeclBase.cpp

void Decl::setDeclContextsImpl(DeclContext *SemaDC, DeclContext *LexicalDC,
   } else {
     auto *MDC = new (Ctx) Decl::MultipleDC();
     MDC->SemanticDC = SemaDC;
     MDC->LexicalDC = LexicalDC;
     DeclCtx = MDC;
   }
 }
 
-bool Decl::isLexicallyWithinFunctionOrMethod() const {
+bool Decl::isInLocalScope() const {
   const DeclContext *LDC = getLexicalDeclContext();
   while (true) {
     if (LDC->isFunctionOrMethod())
       return true;
     if (!isa<TagDecl>(LDC))
       return false;
+    if (const auto *CRD = dyn_cast<CXXRecordDecl>(LDC))
+      if (CRD->isLambda())
+        return true;
     LDC = LDC->getLexicalParent();
   }
   return false;
 }
 
 bool Decl::isInAnonymousNamespace() const {
   for (const DeclContext *DC = getDeclContext(); DC; DC = DC->getParent()) {
     if (const auto *ND = dyn_cast<NamespaceDecl>(DC))

clang/lib/Sema/SemaTemplateInstantiate.cpp

ParmVarDecl *Sema::SubstParmVarDecl(ParmVarDecl *OldParm,
   if (OldParm->hasUninstantiatedDefaultArg()) {
     Expr *Arg = OldParm->getUninstantiatedDefaultArg();
     NewParm->setUninstantiatedDefaultArg(Arg);
   } else if (OldParm->hasUnparsedDefaultArg()) {
     NewParm->setUnparsedDefaultArg();
     UnparsedDefaultArgInstantiations[OldParm].push_back(NewParm);
   } else if (Expr *Arg = OldParm->getDefaultArg()) {
     FunctionDecl *OwningFunc = cast<FunctionDecl>(OldParm->getDeclContext());
-    if (OwningFunc->isLexicallyWithinFunctionOrMethod()) {
+    if (OwningFunc->isInLocalScope()) {
       // Instantiate default arguments for methods of local classes (DR1484)
       // and non-defining declarations.
       Sema::ContextRAII SavedContext(*this, OwningFunc);
       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()->getBeginLoc();

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (Proto->hasExceptionSpec() || Proto->getNoReturnAttr()) {
     // DR1330: In C++11, defer instantiation of a non-trivial
     // exception specification.
     // DR1484: Local classes and their members are instantiated along with the
     // containing function.
     if (SemaRef.getLangOpts().CPlusPlus11 &&
         EPI.ExceptionSpec.Type != EST_None &&
         EPI.ExceptionSpec.Type != EST_DynamicNone &&
         EPI.ExceptionSpec.Type != EST_BasicNoexcept &&
-        !Tmpl->isLexicallyWithinFunctionOrMethod()) {
+        !Tmpl->isInLocalScope()) {
       FunctionDecl *ExceptionSpecTemplate = Tmpl;
       if (EPI.ExceptionSpec.Type == EST_Uninstantiated)
         ExceptionSpecTemplate = EPI.ExceptionSpec.SourceTemplate;
       ExceptionSpecificationType NewEST = EST_Uninstantiated;
       if (EPI.ExceptionSpec.Type == EST_Unevaluated)
         NewEST = EST_Unevaluated;
 
       // Mark the function has having an uninstantiated exception specification.

clang/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
   CXXMethodDecl *NewCallOperator = getSema().startLambdaDefinition(
       Class, E->getIntroducerRange(), NewCallOpTSI,
       E->getCallOperator()->getEndLoc(),
       NewCallOpTSI->getTypeLoc().castAs<FunctionProtoTypeLoc>().getParams(),
       E->getCallOperator()->getConstexprKind(),
       NewTrailingRequiresClause.get());
 
   LSI->CallOperator = NewCallOperator;
 
-  for (unsigned I = 0, NumParams = NewCallOperator->getNumParams();
-       I != NumParams; ++I) {
-    auto *P = NewCallOperator->getParamDecl(I);
-    if (P->hasUninstantiatedDefaultArg()) {
-      EnterExpressionEvaluationContext Eval(
-          getSema(),
-          Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed, P);
-      ExprResult R = getDerived().TransformExpr(
-          E->getCallOperator()->getParamDecl(I)->getDefaultArg());
-      P->setDefaultArg(R.get());
-    }
-  }
-
   getDerived().transformAttrs(E->getCallOperator(), NewCallOperator);
   getDerived().transformedLocalDecl(E->getCallOperator(), {NewCallOperator});
 
   // Number the lambda for linkage purposes if necessary.
   getSema().handleLambdaNumbering(Class, NewCallOperator, Mangling);
 
   // Introduce the context of the call operator.
   Sema::ContextRAII SavedContext(getSema(), NewCallOperator,
                                  /*NewThisContext*/false);

clang/test/SemaCXX/vartemplate-lambda.cpp

 // RUN: %clang_cc1 -std=c++14 -fsyntax-only -verify %s
 
 template <class> auto fn0 = [] {};
 template <typename> void foo0() { fn0<char>(); }
 
 template<typename T> auto fn1 = [](auto a) { return a + T(1); };
-template<typename T> auto v1 = [](int a = T(1)) { return a; }();
+template<typename T> auto v1 = [](int a = T()) { return a; }();
+// expected-error@-1{{cannot initialize a parameter of type 'int' with an rvalue of type 'int *'}}
+// expected-error@-2{{no matching function for call}}
+// expected-note@-3{{passing argument to parameter 'a' here}}
+// expected-note@-4{{candidate function not viable}}
+// expected-note@-5{{conversion candidate of type 'int (*)(int)'}}
 
 struct S {
   template<class T>
   static constexpr T t = [](int f = T(7)){return f;}(); // expected-error{{constexpr variable 't<int>' must be initialized by a constant expression}} expected-note{{cannot be used in a constant expression}}
 };
 
 template <typename X>
 int foo2() {
   X a = 0x61;
   fn1<char>(a);
   (void)v1<int>;
+  (void)v1<int *>; // expected-note{{in instantiation of variable template specialization 'v1' requested here}}
   (void)S::t<int>; // expected-note{{in instantiation of static data member 'S::t<int>' requested here}}
   return 0;
 }
 
 template<class C>
 int foo3() {
   C::m1(); // expected-error{{type 'long long' cannot be used prior to '::' because it has no members}}
   return 1;

clang/test/SemaTemplate/instantiate-local-class.cpp

template<typename T> void f() {
     struct S {
       union {
         int i = 0;
       };
     };
   }
   void g() { f<int>(); }
 }
+
+namespace PR45000 {
+  template <typename T>
+  void f(int x = [](T x = nullptr) -> int { return x; }());
+  // expected-error@-1 {{cannot initialize a parameter of type 'int' with an rvalue of type 'nullptr_t'}}
+  // expected-note@-2 {{passing argument to parameter 'x' here}}
+  // expected-error@-3 {{no matching function for call}}
+  // expected-note@-4 {{candidate function not viable: requires single argument 'x', but no arguments were provided}}
+  // expected-note@-5 {{conversion candidate of type 'auto (*)(int) -> int'}}
+
+  void g() { f<int>(); }
+  // expected-note@-1 {{in instantiation of default function argument expression for 'f<int>' required here}}
+}