[clang] Allow delayed function instantiation at TU end if initial instantiation fails

[StefanPaulet]

Some eager instantiations may try to instantiate a forward declared templated function before it is defined. This introduces one more attempt to instantiate at the end of the TU.
Resolves #115731

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (StefanPaulet)

Changes

Some eager instantiations may try to instantiate a forward declared templated function before it is defined. This introduces one more attempt to instantiate at the end of the TU.
Resolves #115731

---

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

4 Files Affected:

• (modified) clang/include/clang/Sema/Sema.h (+7-4)
• (modified) clang/lib/Sema/Sema.cpp (+1-1)
• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+8-6)
• (added) clang/test/SemaTemplate/instantiate-function-delayed.cpp (+26)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 6ea6c67447b6f0..ce27260bc78801 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13523,7 +13523,9 @@ class Sema final : public SemaBase {
           S.PendingLocalImplicitInstantiations);
     }
 
-    void perform() { S.PerformPendingInstantiations(/*LocalOnly=*/true); }
+    void perform(bool AtEndOfTU = false) {
+      S.PerformPendingInstantiations(/*LocalOnly=*/true, AtEndOfTU);
+    }
 
     ~LocalEagerInstantiationScope() {
       assert(S.PendingLocalImplicitInstantiations.empty() &&
@@ -13568,10 +13570,10 @@ class Sema final : public SemaBase {
       S.SavedVTableUses.back().swap(S.VTableUses);
     }
 
-    void perform() {
+    void perform(bool AtEndOfTU = false) {
       if (Enabled) {
         S.DefineUsedVTables();
-        S.PerformPendingInstantiations();
+        S.PerformPendingInstantiations(false, AtEndOfTU);
       }
     }
 
@@ -13790,7 +13792,8 @@ class Sema final : public SemaBase {
 
   /// Performs template instantiation for all implicit template
   /// instantiations we have seen until this point.
-  void PerformPendingInstantiations(bool LocalOnly = false);
+  void PerformPendingInstantiations(bool LocalOnly = false,
+                                    bool AtEndOfTU = false);
 
   TemplateParameterList *
   SubstTemplateParams(TemplateParameterList *Params, DeclContext *Owner,
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 942e7ece4283e3..c97a253239df2b 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1138,7 +1138,7 @@ void Sema::ActOnEndOfTranslationUnitFragment(TUFragmentKind Kind) {
 
   {
     llvm::TimeTraceScope TimeScope("PerformPendingInstantiations");
-    PerformPendingInstantiations();
+    PerformPendingInstantiations(false, true);
   }
 
   emitDeferredDiags();
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 10efde7c3fe540..0f9a39062750bc 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5267,9 +5267,9 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
 
   // This class may have local implicit instantiations that need to be
   // instantiation within this scope.
-  LocalInstantiations.perform();
+  LocalInstantiations.perform(AtEndOfTU);
   Scope.Exit();
-  GlobalInstantiations.perform();
+  GlobalInstantiations.perform(AtEndOfTU);
 }
 
 VarTemplateSpecializationDecl *Sema::BuildVarTemplateInstantiation(
@@ -5612,9 +5612,9 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation,
 
       // This variable may have local implicit instantiations that need to be
       // instantiated within this scope.
-      LocalInstantiations.perform();
+      LocalInstantiations.perform(AtEndOfTU);
       Local.Exit();
-      GlobalInstantiations.perform();
+      GlobalInstantiations.perform(AtEndOfTU);
     }
   } else {
     assert(Var->isStaticDataMember() && PatternDecl->isStaticDataMember() &&
@@ -6448,7 +6448,7 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D,
   return D;
 }
 
-void Sema::PerformPendingInstantiations(bool LocalOnly) {
+void Sema::PerformPendingInstantiations(bool LocalOnly, bool AtEndOfTU) {
   std::deque<PendingImplicitInstantiation> delayedPCHInstantiations;
   while (!PendingLocalImplicitInstantiations.empty() ||
          (!LocalOnly && !PendingInstantiations.empty())) {
@@ -6476,9 +6476,11 @@ void Sema::PerformPendingInstantiations(bool LocalOnly) {
             });
       } else {
         InstantiateFunctionDefinition(/*FIXME:*/ Inst.second, Function, true,
-                                      DefinitionRequired, true);
+                                      DefinitionRequired, AtEndOfTU);
         if (Function->isDefined())
           Function->setInstantiationIsPending(false);
+        else if (!AtEndOfTU)
+          LateParsedInstantiations.push_back(Inst);
       }
       // Definition of a PCH-ed template declaration may be available only in the TU.
       if (!LocalOnly && LangOpts.PCHInstantiateTemplates &&
diff --git a/clang/test/SemaTemplate/instantiate-function-delayed.cpp b/clang/test/SemaTemplate/instantiate-function-delayed.cpp
new file mode 100644
index 00000000000000..286248b48603b7
--- /dev/null
+++ b/clang/test/SemaTemplate/instantiate-function-delayed.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+template <typename T>
+auto foo(T const& arg) -> T;
+
+template <typename Fp, typename Vis>
+auto dispatch(Fp fp, Vis vis) {
+  return fp(vis);
+}
+
+auto baz(int v) {
+  auto callable = []<typename Arg>(Arg const& arg) -> int {
+    return foo(arg);
+  };
+  return dispatch(callable, v);
+}
+
+template <typename T>
+auto foo(T const& arg) -> T {
+  return arg;
+}
+
+int main() {
+  return baz(5);
+}
\ No newline at end of file

[StefanPaulet]

I did not remove the AtEndOfTU parameter from Sema::InstantiateVariableDefinition as it breaks some tests related to diagnostics (e.g. SemaCXX/undefined-internal.cpp) , as previously the parameter could have been set to true although the end of the translation unit was not yet reached.

[StefanPaulet]

@erichkeane Sorry to bother, but I was told to ask you about changes in template instantiations

[erichkeane]

I am not particularly comfortable/knowledgable in how this all works, but I don't see a problem here.

My current concern is that we're going to get the same error 2x for the same failed instantiation, and that seems unfortunate.

Also, we need a release note.

[erichkeane]

unrelated formatting change.

[erichkeane]

newline needed at the end of test.

Also, would like to see a codegen test that shows that we are properly generating the right instantiations.

[cor3ntin]

The behavior I would expect is

callable is instantiated. It has a trailing return type so only the declaration of foo is instantiated, and that also has a return type so we do not need the definition. foo is marked referenced / odr-used. That should be enough to force an instantiation of the definition at the end of the TU.

Can you try to figure out what would cause an eager instantiation of foo's definition?

(I share @erichkeane's concerns about duplicated diagnostics)

[StefanPaulet]

The trace that I found was:

• return type for dispatch must be deduced
• dispatch is instantiated
• callable() is instantiated as part of LocalEagerInstantiationScope (calling PerformPendingInstantiations)
• InstantiateFunctionDefinition for callable() is called with Recursive=true
• A GlobalEagerInstantiationScope is created with /*Enabled=*/ Recursive = true
• foo is eagerly instantiated from GlobalInstantiations.perform()

The issue comes from the fact that Sema::PerformPendingInstantiations calls InstantiateFunctionDefinition with both Recursive=true and AtEndOfTU=true. I was not sure about when Recursive should be enabled so I did not touch it. Could you shed some light on its purpose please?

As for the diagnostics issue, I think it can be solved if I only try a second instantiation for implicit instantiations. Diagnostics for implicit instantiations only get emitted at the end of the TU. Also, implicit instantiations seem to be the only ones that cause this issue, as I haven't been able to replicate this behaviour otherwise.

[cor3ntin]

Wait @erichkeane, I think this is just another case of "lambda call operators get instantiated too soon"...
The change is almost certainly not the right solution, but the right solution is... a lot of work...

[StefanPaulet]

Indeed, replacing the lambda with a user declared struct with the same operator() works: https://godbolt.org/z/8GWv6d88K

May I try to see if I find a way to delay the instantiations of lambda call operators?

[erichkeane]

Indeed, replacing the lambda with a user declared struct with the same operator() works: https://godbolt.org/z/8GWv6d88K

May I try to see if I find a way to delay the instantiations of lambda call operators?

This is unfortunately a sizable amount of work. We're not supposed to instantiate into the body of a lambda call operator until it is called (meaning, at all!). This is a fairly large amount of work that is on my 'things to do when I have a ton of time' list. I did the bare basics of it at one point to show another patch author how to 'start', and it is similar to the 'deferred concept instantation' (or delayed?) work that I did about 3 years ago.

You're welcome to try, and that is seemingly the right way forward here, but it is not for the feint of heart.

[LYP951018]

I have a draft which implements the capture rule for deferred lambda instantiation #105953
I've been too busy lately and haven't continued working on this Pull Request.If you're interested,you can take a look at it.

[StefanPaulet]

I have a draft which implements the capture rule for deferred lambda instantiation #105953 I've been too busy lately and haven't continued working on this Pull Request.If you're interested,you can take a look at it.

Will look there, thanks

[erichkeane]

I have the very rough start of what needs to happen here; https://reviews.llvm.org/D138148

[StefanPaulet]

Thanks. Will give it a shot, though it really seems to be an issue above my level