[PATCH] D98068: Exampt asserts for recursive lambdas about LocalInstantiationScope

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I think this is the wrong solution; I think the assertion is correct and is observing a problem elsewhere. Imagine if we had something like:

  template<typename T>
  void f() {
    auto a = [] (auto recurse, auto x) {
      auto b = [] (auto) {
        refer to local decl in a
      };
      if constexpr (recurse)
        a(false_type(), b)
      else
        x(0)
    }
    a(true_type(), a)
  }

We would then have a local instantiation scope stack that looks like:

  f<int>
  f<int>::a::operator()<true_type, f<int>::a>
  f<int>::a::operator()<false_type, f<int>::a::operator()<true_type, f<int>::a>::b>
  f<int>::a::operator()<true_type, f<int>::a>::operator()::b::operator()<int>

... so the reference from the `true_type` version of `b` to the local decl in `a` would find the local from the enclosing `false_type` class, which is wrong. That's the kind of bug that I think this assertion is defending against, and it looks like we have that bug here. (As it happens, this will never actually cause problems, because all references from b to enclosing locals already got substituted when instantiating the `operator()` function template, but that's actually kind of the point -- the local instantiation scope stack is wrong.)

I think the real problem is that the local instantiation scope for the instantiation of a generic lambda's call operator from the instantiated call operator template should never be merged with its parent. Merging the contexts cannot be necessary  -- we can have left the parent instantiation before we instantiate a specialization of the call operator template, so there cannot be any references from the call operator template to enclosing locals that require substitution. I think the bug is in `InstantiateFunctionDefinition`, somewhere around line 4863:

  // Introduce a new scope where local variable instantiations will be
  // recorded, unless we're actually a member function within a local
  // class, in which case we need to merge our results with the parent
  // scope (of the enclosing function).
  bool MergeWithParentScope = false;
  if (CXXRecordDecl *Rec = dyn_cast<CXXRecordDecl>(Function->getDeclContext()))
    MergeWithParentScope = Rec->isLocalClass();
  
  LocalInstantiationScope Scope(*this, MergeWithParentScope);

While we do need the enclosing local instantiation scope when we're instantiating a non-template member of a local class, or when we're instantiating the definition of a member function template as a member function template (that is, when instantiating a generic lambda to form a generic lambda), we do not need the enclosing local instantiation scope when re-substituting into the result of such a local substitution. That is, we do not want a local instantiation scope if we're instantiating a function template specialization, because the template we're instantiating already had references to locals properly substituted. I'd try a change like this to the above code:

  -    MergeWithParentScope = Rec->isLocalClass();
  +    MergeWithParentScope = Rec->isLocalClass() && !Function->isFunctionTemplateSpecialization();


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98068/new/

https://reviews.llvm.org/D98068