[clang] [Clang] Follow up to previous lambda-in-concept patch (PR #210381)

[Erich Keane via cfe-commits]

================
@@ -3761,13 +3761,17 @@ Decl *TemplateDeclInstantiator::VisitNonTypeTemplateParmDecl(
   if (IsExpandedParameterPack)
     Param = NonTypeTemplateParmDecl::Create(
         SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
-        D->getDepth() - TemplateArgs.getNumSubstitutedLevels(),
+        D->getDepth() - (TemplateArgs.retainInnerDepths()
+                             ? 0
+                             : TemplateArgs.getNumSubstitutedLevels()),
         D->getPosition(), D->getIdentifier(), T, TSI,
         ExpandedParameterPackTypes, ExpandedParameterPackTypesAsWritten);
   else
     Param = NonTypeTemplateParmDecl::Create(
         SemaRef.Context, Owner, D->getInnerLocStart(), D->getLocation(),
-        D->getDepth() - TemplateArgs.getNumSubstitutedLevels(),
+        D->getDepth() - (TemplateArgs.retainInnerDepths()
+                             ? 0
+                             : TemplateArgs.getNumSubstitutedLevels()),
----------------
erichkeane wrote:

> That's true, but the depth preservation didn't come up in that discussion, which would have steered us sooner to the conclusion that both instantiation modes have issues here.
> 
> I have been thinking more about this problem.
> 
> I think even depth preservation is the wrong thing here, that's not exactly what parameter mapping substitution asks for.
> 
> What happens is that we are reparenting the lambda to the top level concept, which is at depth 0, so all these lambdas should end up at the same template depth, ie 1.
> 
> So for example, a lambda nested as a constant template parameter type of another lambda should still have one level reduced, which is not what the patch is doing, which preserves all levels.
> 
> Do we have test cases for that?

Hmm... this is an interesting thought.  Re-writing the depth of all our AST nodes when we re-parent might be a troublesome amount of work though, right?  Wouldn't the depth changes end up being pretty intrusive?

I THINK the 'outer retained levels' was supposed to deal with stuff like this though, the idea was (at least as far as I could tell?) figure out the adjustment needed for parameter mapping, though I'm not sure it was ever effectively implemented for concepts/lambda combinations.

At the moment, I agree that the above adjustment seems a bit 'hacky', but I'm not sure we're actually in a position to be doing this correctly without a sizable change.  I might suggest we move ahead with this and consider how we want to handle the 'moving' of an AST node.

https://github.com/llvm/llvm-project/pull/210381