[Clang] Avoid transforming lambdas when rebuilding immediate expressions

[zyn0217]

When rebuilding immediate invocations inside RemoveNestedImmediateInvocation(), we employed a TreeTransform to exercise the traversal. The transformation has a side effect that, for template specialization types, their default template arguments are substituted separately, and if any lambdas are present, they will be transformed into distinct types than those used to instantiate the templates right before the consteval handling.

This resulted in B::func() getting redundantly instantiated for the case in question. Since we're also in an immediate evaluation context, the body of foo() would also get instantiated, so we end up with a spurious friend redefinition error.

Like what we have done in ComplexRemove, this patch also avoids the lambda's transformation in TemplateInstantiator if we know we're rebuilding immediate calls. In addition, this patch also consolidates the default argument substitution logic in CheckTemplateArgumentList().

Fixes #107175

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

When rebuilding immediate invocations inside RemoveNestedImmediateInvocation(), we employed a TreeTransform to exercise the traversal. The transformation has a side effect that, for template specialization types, their default template arguments are substituted separately, and if any lambdas are present, they will be transformed into distinct types than those used to instantiate the templates right before the consteval handling.

This resulted in B::func() getting redundantly instantiated for the case in question. Since we're also in an immediate evaluation context, the body of foo() would also get instantiated, so we end up with a spurious friend redefinition error.

Like what we have done in ComplexRemove, this patch also avoids the lambda's transformation in TemplateInstantiator if we know we're rebuilding immediate calls. In addition, this patch also consolidates the default argument substitution logic in CheckTemplateArgumentList().

Fixes #107175

---

Full diff: https://github.com/llvm/llvm-project/pull/108693.diff

6 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2-2)
• (modified) clang/lib/Sema/SemaExpr.cpp (+2-2)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+18-37)
• (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+4)
• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+7-3)
• (modified) clang/test/SemaCXX/cxx2a-consteval.cpp (+24)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 79b154ef1aef5e..022c10e11c545a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -387,8 +387,8 @@ Bug Fixes to C++ Support
 - A follow-up fix was added for (#GH61460), as the previous fix was not entirely correct. (#GH86361)
 - Fixed a crash in the typo correction of an invalid CTAD guide. (#GH107887)
 - Fixed a crash when clang tries to subtitute parameter pack while retaining the parameter
-  pack. #GH63819, #GH107560
-
+  pack. (#GH63819), (#GH107560)
+- Avoided a redundant friend declaration instantiation under the certain ``consteval`` contexts. (#GH107175)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8f3e15cc9a9bb7..2a22b05afe1c34 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -17521,7 +17521,7 @@ static void RemoveNestedImmediateInvocation(
         else
           break;
       }
-      /// ConstantExpr are the first layer of implicit node to be removed so if
+      /// ConstantExprs are the first layer of implicit node to be removed so if
       /// Init isn't a ConstantExpr, no ConstantExpr will be skipped.
       if (auto *CE = dyn_cast<ConstantExpr>(Init);
           CE && CE->isImmediateInvocation())
@@ -17534,7 +17534,7 @@ static void RemoveNestedImmediateInvocation(
     }
     ExprResult TransformLambdaExpr(LambdaExpr *E) {
       // Do not rebuild lambdas to avoid creating a new type.
-      // Lambdas have already been processed inside their eval context.
+      // Lambdas have already been processed inside their eval contexts.
       return E;
     }
     bool AlwaysRebuild() { return false; }
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index e5ea02a919f4eb..b052afede2cd67 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -5508,50 +5508,31 @@ bool Sema::CheckTemplateArgumentList(
     }
 
     // Check whether we have a default argument.
-    TemplateArgumentLoc Arg;
+    bool HasDefaultArg;
 
     // Retrieve the default template argument from the template
     // parameter. For each kind of template parameter, we substitute the
     // template arguments provided thus far and any "outer" template arguments
     // (when the template parameter was part of a nested template) into
     // the default argument.
-    if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(*Param)) {
-      if (!hasReachableDefaultArgument(TTP))
-        return diagnoseMissingArgument(*this, TemplateLoc, Template, TTP,
+    TemplateArgumentLoc Arg = SubstDefaultTemplateArgumentIfAvailable(
+        Template, TemplateLoc, RAngleLoc, *Param, SugaredConverted,
+        CanonicalConverted, HasDefaultArg);
+
+    if (Arg.getArgument().isNull()) {
+      if (!HasDefaultArg) {
+        if (TemplateTypeParmDecl *TTP = dyn_cast<TemplateTypeParmDecl>(*Param))
+          return diagnoseMissingArgument(*this, TemplateLoc, Template, TTP,
+                                         NewArgs);
+        if (NonTypeTemplateParmDecl *NTTP =
+                dyn_cast<NonTypeTemplateParmDecl>(*Param))
+          return diagnoseMissingArgument(*this, TemplateLoc, Template, NTTP,
+                                         NewArgs);
+        return diagnoseMissingArgument(*this, TemplateLoc, Template,
+                                       cast<TemplateTemplateParmDecl>(*Param),
                                        NewArgs);
-
-      if (SubstDefaultTemplateArgument(*this, Template, TemplateLoc, RAngleLoc,
-                                       TTP, SugaredConverted,
-                                       CanonicalConverted, Arg))
-        return true;
-    } else if (NonTypeTemplateParmDecl *NTTP
-                 = dyn_cast<NonTypeTemplateParmDecl>(*Param)) {
-      if (!hasReachableDefaultArgument(NTTP))
-        return diagnoseMissingArgument(*this, TemplateLoc, Template, NTTP,
-                                       NewArgs);
-
-      if (SubstDefaultTemplateArgument(*this, Template, TemplateLoc, RAngleLoc,
-                                       NTTP, SugaredConverted,
-                                       CanonicalConverted, Arg))
-        return true;
-    } else {
-      TemplateTemplateParmDecl *TempParm
-        = cast<TemplateTemplateParmDecl>(*Param);
-
-      if (!hasReachableDefaultArgument(TempParm))
-        return diagnoseMissingArgument(*this, TemplateLoc, Template, TempParm,
-                                       NewArgs);
-
-      NestedNameSpecifierLoc QualifierLoc;
-      TemplateName Name = SubstDefaultTemplateArgument(
-          *this, Template, TemplateLoc, RAngleLoc, TempParm, SugaredConverted,
-          CanonicalConverted, QualifierLoc);
-      if (Name.isNull())
-        return true;
-
-      Arg = TemplateArgumentLoc(
-          Context, TemplateArgument(Name), QualifierLoc,
-          TempParm->getDefaultArgument().getTemplateNameLoc());
+      }
+      return true;
     }
 
     // Introduce an instantiation record that describes where we are using
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index c42cc250bb904a..55f38743e2768e 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1673,6 +1673,10 @@ namespace {
     }
 
     ExprResult TransformLambdaExpr(LambdaExpr *E) {
+      // Do not rebuild lambdas to avoid creating a new type.
+      // Lambdas have already been processed inside their eval contexts.
+      if (SemaRef.RebuildingImmediateInvocation)
+        return E;
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       Sema::ConstraintEvalRAII<TemplateInstantiator> RAII(*this);
 
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index bb311e38409280..c3cb9d5d8c2c3d 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -6293,9 +6293,13 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D,
 
           if (!SubstRecord) {
             // T can be a dependent TemplateSpecializationType when performing a
-            // substitution for building a deduction guide.
-            assert(CodeSynthesisContexts.back().Kind ==
-                   CodeSynthesisContext::BuildingDeductionGuides);
+            // substitution for building a deduction guide or for template
+            // argument deduction in the process of rebuilding immediate
+            // expressions. (Because the default argument that involves a lambda
+            // is untransformed and thus could be dependent at this point.)
+            assert(SemaRef.RebuildingImmediateInvocation ||
+                   CodeSynthesisContexts.back().Kind ==
+                       CodeSynthesisContext::BuildingDeductionGuides);
             // Return a nullptr as a sentinel value, we handle it properly in
             // the TemplateInstantiator::TransformInjectedClassNameType
             // override, which we transform it to a TemplateSpecializationType.
diff --git a/clang/test/SemaCXX/cxx2a-consteval.cpp b/clang/test/SemaCXX/cxx2a-consteval.cpp
index 81923617f637e8..ae331055c52b2e 100644
--- a/clang/test/SemaCXX/cxx2a-consteval.cpp
+++ b/clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1248,3 +1248,27 @@ void test() {
 }
 
 }
+
+// Test that we don't redundantly instantiate the friend declaration in
+// RemoveNestedImmediateInvocation(). Otherwise, we would end up with spurious
+// redefinition errors.
+namespace GH107175 {
+
+consteval void consteval_func() {}
+
+template <auto> struct define_f {
+  friend void foo() {}
+};
+
+template <auto = [] {}> struct A {};
+
+struct B {
+  template <auto T> consteval void func() { (void)define_f<T>{}; }
+};
+
+int main() {
+  B{}.func<A{}>();
+  consteval_func();
+}
+
+} // namespace GH107175

[cor3ntin]

I think this makes sense
I added a couple extra pair of eyes to the review so please wait a bit before merging.

In the future, it would be better to push the unrelated parts of the pr in separate NFC commits, for ease of review. Thanks!

[erichkeane]

I was going to comment on the amount of NFC changes here, but as Corentin already approved, I think we're OK for this for now. LGTM as well.

[zyn0217]

In the future, it would be better to push the unrelated parts of the pr in separate NFC commits, for ease of review.

Oops, I thought that was not too large, so I refactored that along the way of the call... anyways, I'll be careful next time :)