[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes #68379

antangelo · 2023-10-06T04:08:58Z

When a nested template is instantiated, the template pattern of the inner class is not copied into the outer class ClassTemplateSpecializationDecl. The specialization contains a ClassTemplateDecl with an empty record that points to the original template pattern instead.

As a result, when looking up the constructors of the inner class, no results are returned. This patch finds the original template pattern and uses that for the lookup instead.

Based on CWG2471 we must also substitute the known outer template arguments when creating deduction guides for the inner class.

Fixes #46200
Fixes #57812

llvmbot · 2023-10-06T04:10:05Z

@llvm/pr-subscribers-clang

Changes

When a nested template is instantiated, the template pattern of the inner class is not copied into the outer class ClassTemplateSpecializationDecl. The specialization contains a ClassTemplateDecl with an empty record that points to the original template pattern instead.

As a result, when looking up the constructors of the inner class, no results are returned. This patch finds the original template pattern and uses that for the lookup instead.

Based on CWG2471 we must also substitute the known outer template arguments when creating deduction guides for the inner class.

Fixes #46200
Fixes #57812

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

2 Files Affected:

(modified) clang/lib/Sema/SemaTemplate.cpp (+21-1)
(added) clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp (+12)

diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 080f005e04402cb..33907bd58b44fc4 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2250,6 +2250,7 @@ struct ConvertConstructorToDeductionGuideTransform {
 
   Sema &SemaRef;
   ClassTemplateDecl *Template;
+  ClassTemplateDecl *NestedPattern = nullptr;
 
   DeclContext *DC = Template->getDeclContext();
   CXXRecordDecl *Primary = Template->getTemplatedDecl();
@@ -2327,6 +2328,8 @@ struct ConvertConstructorToDeductionGuideTransform {
     if (FTD) {
       Args.addOuterTemplateArguments(SubstArgs);
       Args.addOuterRetainedLevel();
+      if (NestedPattern)
+        Args.addOuterRetainedLevels(NestedPattern->getTemplateDepth());
     }
 
     FunctionProtoTypeLoc FPTL = CD->getTypeSourceInfo()->getTypeLoc()
@@ -2438,10 +2441,17 @@ struct ConvertConstructorToDeductionGuideTransform {
     SmallVector<QualType, 4> ParamTypes;
     const FunctionProtoType *T = TL.getTypePtr();
 
+    MultiLevelTemplateArgumentList OuterInstantiationArgs;
+    if (NestedPattern)
+      OuterInstantiationArgs = SemaRef.getTemplateInstantiationArgs(Template);
+
     //    -- The types of the function parameters are those of the constructor.
     for (auto *OldParam : TL.getParams()) {
       ParmVarDecl *NewParam =
           transformFunctionTypeParam(OldParam, Args, MaterializedTypedefs);
+      if (NestedPattern && NewParam)
+        NewParam = transformFunctionTypeParam(NewParam, OuterInstantiationArgs,
+                                              MaterializedTypedefs);
       if (!NewParam)
         return QualType();
       ParamTypes.push_back(NewParam->getType());
@@ -2647,13 +2657,23 @@ void Sema::DeclareImplicitDeductionGuides(TemplateDecl *Template,
   if (BuildingDeductionGuides.isInvalid())
     return;
 
+  // If the template is nested, then we need to use the original
+  // pattern to iterate over the constructors.
+  ClassTemplateDecl *Pattern = Transform.Template;
+  while (Pattern->getInstantiatedFromMemberTemplate()) {
+    if (Pattern->isMemberSpecialization())
+      break;
+    Pattern = Pattern->getInstantiatedFromMemberTemplate();
+    Transform.NestedPattern = Pattern;
+  }
+
   // Convert declared constructors into deduction guide templates.
   // FIXME: Skip constructors for which deduction must necessarily fail (those
   // for which some class template parameter without a default argument never
   // appears in a deduced context).
   llvm::SmallPtrSet<NamedDecl *, 8> ProcessedCtors;
   bool AddedAny = false;
-  for (NamedDecl *D : LookupConstructors(Transform.Primary)) {
+  for (NamedDecl *D : LookupConstructors(Pattern->getTemplatedDecl())) {
     D = D->getUnderlyingDecl();
     if (D->isInvalidDecl() || D->isImplicit())
       continue;
diff --git a/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp b/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
new file mode 100644
index 000000000000000..4915c687cf4c4ef
--- /dev/null
+++ b/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// expected-no-diagnostics
+
+template<class T> struct S {
+    template<class U> struct N {
+        N(T) {}
+        N(T, U) {}
+        template<class V> N(V, U) {}
+    };
+};
+
+S<int>::N x{"a", 1};

erichkeane

This seems reasonable, however it needs a release note.

antangelo · 2023-10-06T19:57:16Z

This seems reasonable, however it needs a release note.

Thanks for the review! I've added a release note to the C++ bugfix section

antangelo · 2023-10-13T19:10:45Z

Friendly ping

deduction guides for nested template classes When a nested template is instantiated, the template pattern of the inner class is not copied into the outer class ClassTemplateSpecializationDecl. The specialization contains a ClassTemplateDecl with an empty record that points to the original template pattern instead. As a result, when looking up the constructors of the inner class, no results are returned. This patch finds the original template pattern and uses that for the lookup instead. Based on CWG2471 we must also substitute the known outer template arguments when creating deduction guides for the inner class. Fixes llvm#46200 Fixes llvm#57812

ilovepi · 2023-10-17T00:48:05Z

I think we may be seeing some issues with this patch, when building Fuchsia.

We're seeing the following error.

FAILED: host_x64/obj/sdk/lib/ld/test/ld-unittests.filter-view-tests.cc.o
../../prebuilt/third_party/clang/custom/bin/clang++ -MD -MF host_x64/obj/sdk/lib/ld/test/ld-unittests.filter-view-tests.cc.o.d -DFUCHSIA_API_LEVEL=15 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -I../.. -Ihost_x64/gen -I../../sdk/lib/ld/include -I../../sdk/lib/stdcompat/include -I../../zircon/system/public -I../../src/lib/elfldltl/include -I../../zircon/system/u...
In file included from ../../sdk/lib/ld/test/filter-view-tests.cc:6:
../../sdk/lib/ld/include/lib/ld/internal/filter-view.h:51:27: error: no member 'filter_iterator' in 'ld::internal::filter_view<std::array<int, 6>, (lambda at ../../sdk/lib/ld/test/filter-view-tests.cc:20:48)>'; it has not yet been instantiated
   51 |     filter_iterator(const filter_iterator&) = default;
      |                           ^
note: while building implicit deduction guide first needed here
../../sdk/lib/ld/test/filter-view-tests.cc:20:31: note: in instantiation of template class 'ld::internal::filter_view<std::array<int, 6>, (lambda at ../../sdk/lib/ld/test/filter-view-tests.cc:20:48)>' requested here
   20 |     ld::internal::filter_view filter_view(arr, [](auto) { return true; });
      |                               ^
../../sdk/lib/ld/include/lib/ld/internal/filter-view.h:40:9: note: not-yet-instantiated member is declared here
   40 |   class filter_iterator : public std::iterator_traits<Iter> {
      |         ^
1 error generated.

But the code in question is defining a constructor for the nested class. You can find the code here: https://cs.opensource.google/fuchsia/fuchsia/+/main:sdk/lib/ld/include/lib/ld/internal/filter-view.h;l=51?q=filter_iterator&ss=fuchsia%2Ffuchsia

We're tracking this in https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=135353

Do you have any idea why it would be flagging this case. I don't see any obvious C++ rules being violated, so I think this is rather surprising.

I've included a reproducer, but I haven't had a chance to reduce it yet, so it's likely to be quite large.
filter-view.zip

antangelo · 2023-10-17T02:09:43Z

I've come up with this minimal reproducer:

template<class A>
struct outer {
    template<class B>
    struct inner {
        inner(B b);
        inner(const inner &x) = default;
    };

    template<class B>
    inner(B b) -> inner<B>;
};

outer<int>::inner i(1);

The error seems to be triggered by the inclusion of both the copy constructor and the explicit deduction guide. I'll revert the patch now until I can come up with a proper fix for it.

…plicit deduction guides for nested template classes (#68379)" This reverts commit dd0fba1. It fails on nested classes that have both an explicit deduction guide and a constructor that has an argument of the same type as the class (i.e. a copy constructor).

antangelo · 2023-10-17T21:46:09Z

When SubstType is called on the const inner & parameter to substitute the outer template args, it eventually calls FindInstantiatedDecl on the inner template pattern. The presence of the explicit deduction guide causes the outer template to already be instantiated at this point.

As a result FindInstantiatedDecl attempts to look for the inner template within the instantiated outer template. This fails because the inner template args are not known (so the inner template cannot have been instantiated at this point).

I think that in this case, if the decl passed into FindInstantiatedDecl is a CXXRecordDecl with a described template, and its context is dependent at the given template arguments' level, FindInstantiatedDecl should return the template pattern. Does that make sense, or is it off the mark?

ilovepi · 2023-10-17T22:16:24Z

I think your analysis and approach sounds correct, but probably worth checking with someone a bit more familiar with all of the C++ nuance here.

antangelo · 2023-10-20T03:43:43Z

I've posted a candidate reland of this PR at #69676 .

antangelo requested review from zygoloid and erichkeane October 6, 2023 04:08

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 6, 2023

erichkeane reviewed Oct 6, 2023

View reviewed changes

erichkeane approved these changes Oct 13, 2023

View reviewed changes

antangelo added 2 commits October 16, 2023 13:01

Add release note

67b8a02

antangelo force-pushed the clang/sema/fix-nested-ctad branch from b48a0c4 to 67b8a02 Compare October 16, 2023 17:55

erichkeane approved these changes Oct 16, 2023

View reviewed changes

antangelo merged commit dd0fba1 into llvm:main Oct 16, 2023

antangelo deleted the clang/sema/fix-nested-ctad branch October 16, 2023 19:17

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes #68379

[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes #68379

Uh oh!

antangelo commented Oct 6, 2023

Uh oh!

llvmbot commented Oct 6, 2023

Uh oh!

erichkeane left a comment

Uh oh!

antangelo commented Oct 6, 2023

Uh oh!

antangelo commented Oct 13, 2023

Uh oh!

ilovepi commented Oct 17, 2023

Uh oh!

antangelo commented Oct 17, 2023

Uh oh!

antangelo commented Oct 17, 2023

Uh oh!

ilovepi commented Oct 17, 2023

Uh oh!

antangelo commented Oct 20, 2023

Uh oh!

Uh oh!

[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes #68379

[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes #68379

Uh oh!

Conversation

antangelo commented Oct 6, 2023

Uh oh!

llvmbot commented Oct 6, 2023

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

antangelo commented Oct 6, 2023

Uh oh!

antangelo commented Oct 13, 2023

Uh oh!

ilovepi commented Oct 17, 2023

Uh oh!

antangelo commented Oct 17, 2023

Uh oh!

antangelo commented Oct 17, 2023

Uh oh!

ilovepi commented Oct 17, 2023

Uh oh!

antangelo commented Oct 20, 2023

Uh oh!

Uh oh!