[Clang][P1061] Fix template arguments in local classes

[ricejasonf]

In the development of P1061 (Structured Bindings Introduce a Patch), I found this bug in the template instantiation of a
local class. The issue is caused by the instantiation of the original template and not the partially instantiated template. In
the example (sans the fix) the instantiation uses the first template parameter from the previous instantiation and not the current one so the error hits an assertion when it is expecting an NTTP. If they were both types then it might gladly accept the type from the wrong template which is kind of scary.

In the test, the reference to i is substituted with a placeholder AST object that represents the resolved value when instantiating g. However, since the old template is used, the instantiation sees an AST object that only contains the template argument index in the context of instantiating the lambda which has a type template parameter (ie auto).

I question if we should use getTemplateInstantiationPattern at all here. Other errors involving local classes in nested templates could also be caused by the misuse of this function (because it gets the uninstantiated template).

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jason Rice (ricejasonf)

Changes

In the development of P1061 (Structured Bindings Introduce a Patch), I found this bug in the template instantiation of a
local class. The issue is caused by the instantiation of the original template and not the partially instantiated template. In
the example (sans the fix) the instantiation uses the first template parameter from the previous instantiation and not the current one so the error hits an assertion when it is expecting an NTTP. If they were both types then it might gladly accept the type from the wrong template which is kind of scary.

In the test, the reference to i is substituted with a placeholder AST object that represents the resolved value when instantiating g. However, since the old template is used, the instantiation sees an AST object that only contains the template argument index in the context of instantiating the lambda which has a type template parameter (ie auto).

I question if we should use getTemplateInstantiationPattern at all here. Other errors involving local classes in nested templates could also be caused by the misuse of this function (because it gets the uninstantiated template).

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+5-1)
• (added) clang/test/SemaCXX/local-class-template-param-crash.cpp (+14)

diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fb0f38df62a744..baa5ff35295349 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -4433,8 +4433,12 @@ Sema::InstantiateClassMembers(SourceLocation PointOfInstantiation,
       // No need to instantiate in-class initializers during explicit
       // instantiation.
       if (Field->hasInClassInitializer() && TSK == TSK_ImplicitInstantiation) {
+        // Handle local classes which could have substituted template params.
         CXXRecordDecl *ClassPattern =
-            Instantiation->getTemplateInstantiationPattern();
+            Instantiation->isLocalClass()
+                ? Instantiation->getInstantiatedFromMemberClass()
+                : Instantiation->getTemplateInstantiationPattern();
+
         DeclContext::lookup_result Lookup =
             ClassPattern->lookup(Field->getDeclName());
         FieldDecl *Pattern = Lookup.find_first<FieldDecl>();
diff --git a/clang/test/SemaCXX/local-class-template-param-crash.cpp b/clang/test/SemaCXX/local-class-template-param-crash.cpp
new file mode 100644
index 00000000000000..ffa8590eaf77d8
--- /dev/null
+++ b/clang/test/SemaCXX/local-class-template-param-crash.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+// expected-no-diagnostics
+
+template <int i>
+int g() {
+  return [] (auto) -> int {
+    struct L {
+      int m = i;
+    };
+    return 0;
+  } (42);
+}
+
+int v = g<1>();

[zyn0217]

I feel like this is some fallout of not decoupling instantiating lambda body from the lambda transformation. If we had been able to avoid the body transform when transforming the lambda for the call expression, we could also have avoided instantiating an offending intermediate L, right? @erichkeane

[cor3ntin]

That seems about right.
But the fix is probably fine? Maybe we can add a FIXME in the comment for now.

[zyn0217]

Yeah, probably also worth an assertion for which we’re within a lambda.

[cor3ntin]

@ricejasonf Can you add a release note?
Sorry for the lack of updates - but feel free to ping us weekly in the future

[cor3ntin]

@ricejasonf ping :)

[mizvekov]

The main point of getTemplateInstantiationPattern is to figure out the instantiation pattern as the name suggests, it would be better to fix the problem within it.

[ricejasonf]

The comments for that states

  /// Retrieve the record declaration from which this record could be
  /// instantiated. Returns null if this class is not a template instantiation.
  const CXXRecordDecl *getTemplateInstantiationPattern() const;

So the "pattern" as I read it, is not an instantiation itself but the template (or partial specialization). In the case of the field in struct L we need the intermediate instantiated struct to get the instantiated field.

[mizvekov]

Yes, the pattern is the corpus of the template, the parts which needs to be have template parameters replaced in order to produce an instantiation.

I think what I said still stands.

[ricejasonf]

I verified that it is still broken on trunk (on godbolt). This is now rebased on main with a shiny new release note. Please merge it if you think it is the right fix (and the tests finish). @cor3ntin