[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Ye Luo via Phabricator via cfe-commits]

ye-luo added a comment.

This patch caused severe regression in Clang 11.
https://bugs.llvm.org/show_bug.cgi?id=48177


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG82a21229da36: (PR46111) Properly handle elaborated types in 
an implicit  deduction guide (authored by erichkeane).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D80743?vs=268822=270368#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/deduction-guides.cpp

Index: clang/test/AST/deduction-guides.cpp
===
--- /dev/null
+++ clang/test/AST/deduction-guides.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only %s -ast-dump -std=c++17 | FileCheck %s
+
+namespace PR46111 {
+template 
+struct S;
+
+template 
+struct HasDeductionGuide {
+  typedef PR46111::S STy;
+  HasDeductionGuide(typename STy::Child);
+};
+
+// This causes deduction guides to be generated for all constructors.
+HasDeductionGuide()->HasDeductionGuide;
+
+template 
+struct HasDeductionGuideTypeAlias {
+  using STy = PR46111::S;
+  HasDeductionGuideTypeAlias(typename STy::Child);
+};
+
+// This causes deduction guides to be generated for all constructors.
+HasDeductionGuideTypeAlias()->HasDeductionGuideTypeAlias;
+
+// The parameter to this one shouldn't be an elaborated type.
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (typename STy::Child) -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (HasDeductionGuide) -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}}  'auto () -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (typename STy::Child) -> HasDeductionGuideTypeAlias'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (HasDeductionGuideTypeAlias) -> HasDeductionGuideTypeAlias'
+// CHECK: CXXDeductionGuideDecl {{.*}}  'auto () -> HasDeductionGuideTypeAlias'
+} // namespace PR46111
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5702,6 +5702,9 @@
 bool NeedInstantiate = false;
 if (CXXRecordDecl *RD = dyn_cast(D))
   NeedInstantiate = RD->isLocalClass();
+else if (isa(D) &&
+ isa(D->getDeclContext()))
+  NeedInstantiate = true;
 else
   NeedInstantiate = isa(D);
 if (NeedInstantiate) {
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -3579,6 +3579,12 @@
   if (isa(D))
 return nullptr;
 
+  // Materialized typedefs/type alias for implicit deduction guides may require
+  // instantiation.
+  if (isa(D) &&
+  isa(D->getDeclContext()))
+return nullptr;
+
   // If we didn't find the decl, then we either have a sema bug, or we have a
   // forward reference to a label declaration.  Return null to indicate that
   // we have an uninstantiated label.
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1947,16 +1947,46 @@
 /// constructor to a deduction guide.
 class ExtractTypeForDeductionGuide
   : public TreeTransform {
+  llvm::SmallVectorImpl 
+
 public:
   typedef TreeTransform Base;
-  ExtractTypeForDeductionGuide(Sema ) : Base(SemaRef) {}
+  ExtractTypeForDeductionGuide(
+  Sema ,
+  llvm::SmallVectorImpl )
+  : Base(SemaRef), MaterializedTypedefs(MaterializedTypedefs) {}
 
   TypeSourceInfo *transform(TypeSourceInfo *TSI) { return TransformType(TSI); }
 
   QualType TransformTypedefType(TypeLocBuilder , TypedefTypeLoc TL) {
-return TransformType(
-TLB,
-TL.getTypedefNameDecl()->getTypeSourceInfo()->getTypeLoc());
+ASTContext  = SemaRef.getASTContext();
+TypedefNameDecl *OrigDecl = TL.getTypedefNameDecl();
+TypeLocBuilder InnerTLB;
+QualType Transformed =
+TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
+TypeSourceInfo *TSI =
+TransformType(InnerTLB.getTypeSourceInfo(Context, Transformed));
+
+TypedefNameDecl *Decl = nullptr;
+
+if (isa(OrigDecl))
+  Decl = TypeAliasDecl::Create(
+  Context, Context.getTranslationUnitDecl(), OrigDecl->getBeginLoc(),
+  OrigDecl->getLocation(), OrigDecl->getIdentifier(), TSI);
+else {
+  assert(isa(OrigDecl) && "Not a Type alias or typedef");
+  Decl = TypedefDecl::Create(
+  Context, Context.getTranslationUnitDecl(), OrigDecl->getBeginLoc(),
+  OrigDecl->getLocation(), OrigDecl->getIdentifier(), TSI);
+}
+
+

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:1967
+TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
+TypeSourceInfo *TSI = Context.getTrivialTypeSourceInfo(Transformed);
+

rsmith wrote:
> Retaining the location information here would be good too. (You already have 
> that location info in `InnerTLB`.) I think there might even be a convenience 
> `TypeSourceInfo` -> `TypeSourceInfo` transform you can invoke here.
I think I see what you mean, There is a TypeSourceInfo 
*TransformType(TypeSourceInfo*) that I can use to do the transform, PLUS doing 
InnerTLB.getTypeSourceInfo (instead of ASTContext) seems to keep all the 
required info.

I'll do that before I commit this in the morning.

Thanks again!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Sema/SemaTemplate.cpp:1967
+TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
+TypeSourceInfo *TSI = Context.getTrivialTypeSourceInfo(Transformed);
+

Retaining the location information here would be good too. (You already have 
that location info in `InnerTLB`.) I think there might even be a convenience 
`TypeSourceInfo` -> `TypeSourceInfo` transform you can invoke here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Ping @rsmith

I've done as you suggested, though I'm not sure its necessary for you to do a 
thorough review since you approve of the approach.  Would it be alright for 
someone like @rjmccall or others to do the final pedantic checks?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added a comment.

In D80743#2074537 , @rsmith wrote:

> In D80743#2074121 , @erichkeane 
> wrote:
>
> > @rsmith I think this implements what you've suggested?
>
>
> Yes, thanks.
>
> > This seems to 'work' for a small subset of works, but it doesn't properly 
> > register the typedef to the LocalInstantiationScope, so the normal template 
> > instantiation (like here 
> > https://github.com/llvm/llvm-project/blob/master/clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp#L156)
> >  ends up hitting the 'findInstantiationOf' assert here: 
> > https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplateInstantiate.cpp#L3564
>
> Hm. Yes, that'd be a problem. To fix that, we'll need to transform the 
> typedef declarations as part of transforming the deduction guide. 
> Roughly-speaking, we've transformed
>
>   template struct A {
> using U = X1;
> A(X2);
>   };
>
>
> into something like
>
>   template A(X2) -> A {
> using U = X1;
>   };
>
>
> ... and the problem is that we need to substitute into the new `using U =` 
> typedef before we form a use of `U` when producing the substituted type of 
> the alias declaration.
>
> There are a couple of ways we could approach this:
>
> 1. when we start transforming a deduction guide, we can walk its 
> `TypedefDecl` children, and instantiate those into new typedef declarations 
> immediately, so that later references to them work
> 2. when we reach the reference to the typedef declaration (in 
> `FindInstantiatedDecl`, when we see a declaration whose decl context is a 
> deduction guide), instantiate the declaration then
>
>   These are distinguishable by an example such as:
>
>   ``` template struct Type { using type = typename T::type; }; 
> struct B { using type = int; }; template struct A { using 
> type = Type::type; A(T, typename T::type, type); // #1 A(...); // #2 }; A 
> a(1, 2, 3); ```
>
>   For which option 1 would result in a hard error when substituting T=int 
> into the injected typedef for #1, but option 2 would accept, because 
> substitution stops at the 'typename T::type' without ever reaching the 
> typedef.
>
>   For that reason I think option 2 is the way to go. We already have some 
> logic for this in `FindInstantiatedDecl`; search there for `NeedInstantiate` 
> and try extending it from local classes and enums to also cover typedefs 
> whose decl context is a deduction guide. (You'll need a matching change in 
> `findInstantiationOf` to return `nullptr` in that case. This code doesn't 
> seem very well factored...)


Patch incoming!  option 2 ended up being pretty easy to implement (perhaps 
embarassingly so on my part after your explaination, I can't help but think I 
should have put that together), and it results in it passing all the tests.  
Thanks so much for your patience.




Comment at: clang/lib/Sema/SemaTemplate.cpp:1969-1970
+SemaRef.getASTContext(),
+SemaRef.getASTContext().getTranslationUnitDecl(), SourceLocation(),
+SourceLocation(), TL.getTypedefNameDecl()->getIdentifier(), TSI);
+MaterializedTypedefs.push_back(Decl);

rsmith wrote:
> I think it would be a good idea to retain the source location from the 
> original typedef (and to produce a `TypeAliasDecl` if that's what we started 
> with); if any diagnostics end up referring to this typedef, we will want them 
> to point to the one in the class template (and describe it as a "typedef 
> declaration" or "alias declaration" as appropriate).
Ah, right!  I had a TODO in my notebook for the source locations, but didn't 
think about TypeAliasDecl, though I should have.

Looks like it shouldbe easy enough, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Properly handle elaborated types in an implicit deduction guide

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 268822.
erichkeane retitled this revision from "(PR46111) Desugar Elaborated types in 
Deduction Guides." to "(PR46111) Properly handle elaborated types in an 
implicit  deduction guide".
erichkeane edited the summary of this revision.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/deduction-guides.cpp

Index: clang/test/AST/deduction-guides.cpp
===
--- /dev/null
+++ clang/test/AST/deduction-guides.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only %s -ast-dump -std=c++17 | FileCheck %s
+
+namespace PR46111 {
+template 
+struct S;
+
+template 
+struct HasDeductionGuide {
+  typedef PR46111::S STy;
+  HasDeductionGuide(typename STy::Child);
+};
+
+// This causes deduction guides to be generated for all constructors.
+HasDeductionGuide()->HasDeductionGuide;
+
+template 
+struct HasDeductionGuideTypeAlias {
+  using STy = PR46111::S;
+  HasDeductionGuideTypeAlias(typename STy::Child);
+};
+
+// This causes deduction guides to be generated for all constructors.
+HasDeductionGuideTypeAlias()->HasDeductionGuideTypeAlias;
+
+// The parameter to this one shouldn't be an elaborated type.
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (typename STy::Child) -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (HasDeductionGuide) -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}}  'auto () -> HasDeductionGuide'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (typename STy::Child) -> HasDeductionGuideTypeAlias'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit  'auto (HasDeductionGuideTypeAlias) -> HasDeductionGuideTypeAlias'
+// CHECK: CXXDeductionGuideDecl {{.*}}  'auto () -> HasDeductionGuideTypeAlias'
+} // namespace PR46111
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5701,6 +5701,9 @@
 bool NeedInstantiate = false;
 if (CXXRecordDecl *RD = dyn_cast(D))
   NeedInstantiate = RD->isLocalClass();
+else if (isa(D) &&
+ isa(D->getDeclContext()))
+  NeedInstantiate = true;
 else
   NeedInstantiate = isa(D);
 if (NeedInstantiate) {
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -3558,6 +3558,12 @@
   if (isa(D))
 return nullptr;
 
+  // Materialized typedefs/type alias for implicit deduction guides may require
+  // instantiation.
+  if (isa(D) &&
+  isa(D->getDeclContext()))
+return nullptr;
+
   // If we didn't find the decl, then we either have a sema bug, or we have a
   // forward reference to a label declaration.  Return null to indicate that
   // we have an uninstantiated label.
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1947,16 +1947,45 @@
 /// constructor to a deduction guide.
 class ExtractTypeForDeductionGuide
   : public TreeTransform {
+  llvm::SmallVectorImpl 
+
 public:
   typedef TreeTransform Base;
-  ExtractTypeForDeductionGuide(Sema ) : Base(SemaRef) {}
+  ExtractTypeForDeductionGuide(
+  Sema ,
+  llvm::SmallVectorImpl )
+  : Base(SemaRef), MaterializedTypedefs(MaterializedTypedefs) {}
 
   TypeSourceInfo *transform(TypeSourceInfo *TSI) { return TransformType(TSI); }
 
   QualType TransformTypedefType(TypeLocBuilder , TypedefTypeLoc TL) {
-return TransformType(
-TLB,
-TL.getTypedefNameDecl()->getTypeSourceInfo()->getTypeLoc());
+ASTContext  = SemaRef.getASTContext();
+TypedefNameDecl *OrigDecl = TL.getTypedefNameDecl();
+TypeLocBuilder InnerTLB;
+QualType Transformed =
+TransformType(InnerTLB, OrigDecl->getTypeSourceInfo()->getTypeLoc());
+TypeSourceInfo *TSI = Context.getTrivialTypeSourceInfo(Transformed);
+
+TypedefNameDecl *Decl = nullptr;
+
+if (isa(OrigDecl))
+  Decl = TypeAliasDecl::Create(
+  Context, Context.getTranslationUnitDecl(), OrigDecl->getBeginLoc(),
+  OrigDecl->getLocation(), OrigDecl->getIdentifier(), TSI);
+else {
+  assert(isa(OrigDecl) && "Not a Type alias or typedef");
+  Decl = TypedefDecl::Create(
+  Context, Context.getTranslationUnitDecl(), OrigDecl->getBeginLoc(),
+  OrigDecl->getLocation(), OrigDecl->getIdentifier(), TSI);
+}
+
+MaterializedTypedefs.push_back(Decl);
+
+QualType TDTy = Context.getTypedefType(Decl);
+TypedefTypeLoc TypedefTL =