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

antangelo · 2023-10-20T03:41:17Z

Reland of dd0fba1

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.

Changes from the last iteration:

The outer retained levels from the outer template are always added to the MultiLevelTemplateArgumentList for rewriting FunctionTemplateDecl arguments, even if there is no FTD and the arguments are empty.
When building implicit deduction guides, the template pattern underlying decl is pushed as the current context. This resolves the issue where FindInstantiatedDecl is unable to find the inner template class.
Tests are updated to cover the failing case, and to assert that the type is correct after argument deduction in the implicit case.

…plicit deduction guides for nested template classes (llvm#68379)" 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.

llvmbot · 2023-10-20T03:42:55Z

@llvm/pr-subscribers-clang

Author: None (antangelo)

Changes

Reland of dd0fba1

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.

Changes from the last iteration:

The outer retained levels from the outer template are always added to the MultiLevelTemplateArgumentList for rewriting FunctionTemplateDecl arguments, even if there is no FTD and the arguments are empty.
When building implicit deduction guides, the template pattern underlying decl is pushed as the current context. This resolves the issue where FindInstantiatedDecl is unable to find the inner template class.
Tests are updated to cover the failing case, and to assert that the type is correct after argument deduction in the implicit case.

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+5)
(modified) clang/lib/Sema/SemaTemplate.cpp (+25-1)
(modified) clang/test/SemaTemplate/nested-deduction-guides.cpp (+5)
(added) clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp (+15)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc8caf9221b9d29..53a9507fc26613a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -536,6 +536,11 @@ Bug Fixes to C++ Support
   declaration definition. Fixes:
   (`#61763 <https://github.com/llvm/llvm-project/issues/61763>`_)
 
+- Fix a bug where implicit deduction guides are not correctly generated for nested template
+  classes. Fixes:
+  (`#46200 <https://github.com/llvm/llvm-project/issues/46200>`_)
+  (`#57812 <https://github.com/llvm/llvm-project/issues/57812>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index f0197f7c102a857..c2477ec0063e418 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2253,6 +2253,7 @@ struct ConvertConstructorToDeductionGuideTransform {
 
   Sema &SemaRef;
   ClassTemplateDecl *Template;
+  ClassTemplateDecl *NestedPattern = nullptr;
 
   DeclContext *DC = Template->getDeclContext();
   CXXRecordDecl *Primary = Template->getTemplatedDecl();
@@ -2332,6 +2333,9 @@ struct ConvertConstructorToDeductionGuideTransform {
       Args.addOuterRetainedLevel();
     }
 
+    if (NestedPattern)
+      Args.addOuterRetainedLevels(NestedPattern->getTemplateDepth());
+
     FunctionProtoTypeLoc FPTL = CD->getTypeSourceInfo()->getTypeLoc()
                                    .getAsAdjusted<FunctionProtoTypeLoc>();
     assert(FPTL && "no prototype for constructor declaration");
@@ -2441,10 +2445,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());
@@ -2650,13 +2661,24 @@ 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).
+  ContextRAII SavedContext(*this, Pattern->getTemplatedDecl());
   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;
@@ -2702,6 +2724,8 @@ void Sema::DeclareImplicitDeductionGuides(TemplateDecl *Template,
           Transform.buildSimpleDeductionGuide(Transform.DeducedType))
           ->getTemplatedDecl())
       ->setDeductionCandidateKind(DeductionCandidate::Copy);
+
+  SavedContext.pop();
 }
 
 /// Diagnose the presence of a default template argument on a
diff --git a/clang/test/SemaTemplate/nested-deduction-guides.cpp b/clang/test/SemaTemplate/nested-deduction-guides.cpp
index 2c5dda456a138a8..38410b93ead3b97 100644
--- a/clang/test/SemaTemplate/nested-deduction-guides.cpp
+++ b/clang/test/SemaTemplate/nested-deduction-guides.cpp
@@ -4,10 +4,15 @@
 template<typename T> struct A {
   template<typename U> struct B {
     B(...);
+    B(const B &) = default;
   };
   template<typename U> B(U) -> B<U>;
 };
 A<void>::B b = 123;
+A<void>::B copy = b;
 
 using T = decltype(b);
 using T = A<void>::B<int>;
+
+using Copy = decltype(copy);
+using Copy = A<void>::B<int>;
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..10b70f9c8c4f357
--- /dev/null
+++ b/clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp
@@ -0,0 +1,15 @@
+// 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};
+
+using T = decltype(x);
+using T = S<int>::N<int>;

aeubanks · 2023-11-01T16:21:22Z

this breaks

$ cat /tmp/a2.ii
template <bool> struct enable_if;
template <class> struct A {

  template <class... B> struct C {
    C(B...);
    template <class A, enable_if<A::valueint> = 0>
    C(A);
  };

  template <class... B>
  C(B...) -> C<B...>;
};

int New;

struct S : A<int> {
  void f() { C{New}; }
};

$ clang -fsyntax-only -std=c++20 /tmp/a2.ii
/tmp/a2.ii:17:14: error: no matching constructor for initialization of 'C<>' (aka 'A<int>::C<>')                                                                                                                                                                                          
   17 |   void f() { C{New}; }                                                                                                                                                                                                                                                            
      |              ^~~~~~                                                                                                                                                                                                                                                               
/tmp/a2.ii:4:32: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const C<>' for 1st argument                                                                                                                                   
    4 |   template <class... B> struct C {                                                                                                                                                                                                                                                
      |                                ^                                                                                                                                                                                                                                                  
/tmp/a2.ii:4:32: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'C<>' for 1st argument                                                                                                                                         
    4 |   template <class... B> struct C {
      |                                ^
/tmp/a2.ii:7:5: note: candidate template ignored: substitution failure [with A = int]: type 'int' cannot be used prior to '::' because it has no members
    6 |     template <class A, enable_if<A::valueint> = 0>
      |                                  ~
    7 |     C(A);
      |     ^
/tmp/a2.ii:5:5: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
    5 |     C(B...);
      |     ^
1 error generated.

is that intentional?

aeubanks · 2023-11-01T21:47:42Z

and if not, could we revert while investigating?

…aring implicit deduction guides for nested template classes" (#69676)" This reverts commit f418319. Failing test case: #69676 (comment)

antangelo · 2023-11-01T22:11:50Z

Apologies for the delay, I haven't been able to look into this until now. The patch has been reverted in 801c78d

aeubanks · 2023-11-01T22:38:36Z

thanks! I appreciate the quick response

kadircet · 2023-11-02T15:39:48Z

thanks for the revert! I also encountered certain crashes bisecting back to this patch, with a similar reproducer:

template <class T>
struct Module {
  template <class... Bazs>
  struct Baz {
    template <class X, int Y = 0>
    explicit Baz(X);
  };
  Baz(int) -> Baz<int, char>;
};

struct Bar {};

void foo() { Module<Bar>::Baz x{2}; }

antangelo · 2023-11-22T05:40:28Z

Thank you for the reproducers! I have posted a candidate reland that addresses both of these issues here: #73087

…aring implicit deduction guides for nested template classes" (#69676)" This reverts commit f418319. Rebased on-to ChromeOS llvm-r516547-1. Originally the upstream commit was 801c78d Failing test case: llvm/llvm-project#69676 (comment) patch.cherry: true patch.metadata.original_sha: 801c78d patch.platforms: chromiumos patch.version_range.from: 516547 patch.version_range.until: 517370 Change-Id: I9cbc4621ba83389e7c524dbc52a1c7a0111b6e51

antangelo requested review from zygoloid and erichkeane October 20, 2023 03:41

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

antangelo mentioned this pull request Oct 20, 2023

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

Merged

erichkeane approved these changes Oct 20, 2023

View reviewed changes

antangelo merged commit f418319 into llvm:main Oct 24, 2023

antangelo deleted the clang/sema/fix-nested-ctad-reland branch October 24, 2023 18:07

antangelo added a commit that referenced this pull request Nov 1, 2023

Revert "Reland "[clang][Sema] Use original template pattern when decl…

801c78d

…aring implicit deduction guides for nested template classes" (#69676)" This reverts commit f418319. Failing test case: #69676 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

antangelo commented Oct 20, 2023

Uh oh!

llvmbot commented Oct 20, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

antangelo commented Nov 1, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

kadircet commented Nov 2, 2023

Uh oh!

antangelo commented Nov 22, 2023

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

antangelo commented Oct 20, 2023

Uh oh!

llvmbot commented Oct 20, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

antangelo commented Nov 1, 2023

Uh oh!

aeubanks commented Nov 1, 2023

Uh oh!

kadircet commented Nov 2, 2023

Uh oh!

antangelo commented Nov 22, 2023

Uh oh!

Uh oh!