(PR46111) Properly handle elaborated types in an implicit deduction guide

[erichkeane]

Summary

As reported in PR46111, implicit instantiation of a deduction guide
causes us to have an elaborated type as the parameter, rather than the
dependent type.

After review and feedback from @rsmith, this patch solves this problem
by wrapping the value in an uninstantiated typedef/type-alias that is
instantiated when required later.

[erichkeane]

I hope that @rsmith pays particular attention here for me, I'm unable to come up with any cases where this isn't sufficient/breaks other things, but I also don't feel like I know deduction guides well enough to claim that I don't break anything :) All the lit tests pass, for what its worth.

[mclow.lists]

I've got nothing to say here.

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

1962–1964

Removing the qualifier seems like it might work out OK in practice, given that we've already resolved the name to a specific declaration, but removing the elaborated type keyword (eg, struct X -> X) seems wrong to me.

If the problem is that expanding the typedef is breaking the invariants of NestedNameSpecifier, I'd be concerned that there are other cases where we're breaking those invariants in addition to this one. For example, do we have the same problem with DependentNameType?

Perhaps we should be special-casing transform of nested name specifiers instead. In your example below, it would seem more correct to transform typename STy::Child -> typename PR46111::template S<T>::Child. But that doesn't work in general; consider typedef struct PR46111::S<T> STy; for example.

One possible option would be to change TransformTypedefType to wrap its resulting type in a TypeofType type. That would give us a consistent transformation pattern: typename STy::Child -> typename __typeof(<substituted typedef>)::Child. But it's a bit hacky. It would be cleaner in some sense for TransformTypedefType to actually produce a new TypedefDecl (as if by instantiating the member typedef), and to produce a new TypedefType referring to the transformed typedef declaration.

Out of the available options, transforming the typedef declaration is, I think, my favored choice so far. (In the absence of a better DeclContext for it, I think we could set its DeclContext to be the CXXDeductionGuideDecl. That would at least have the right template parameters etc. in scope. We'll need to delay parenting it until after we create the deduction guide, perhaps by initially creating it as a child of the TUDecl then reparenting it, like we do for function parameters.)

[erichkeane]

clang/lib/Sema/SemaTemplate.cpp

1962–1964

If the problem is that expanding the typedef is breaking the invariants of NestedNameSpecifier, I'd be concerned that there are other cases where we're breaking those invariants in addition to this one. For example, do we have the same problem with DependentNameType?

I'm not sure, this is the only one I saw with my reproducer. I've tried to get DependentNameType to cause an issue, but I cannot come up with a reproducer that has this issue, which is likely due to my lack of knowledge on deduction guides.

It would be cleaner in some sense for TransformTypedefType to actually produce a new TypedefDecl (as if by instantiating the member typedef), and to produce a new TypedefType referring to the transformed typedef declaration.

I'll look into this. It doesn't SOUND too difficult to deal with the parent management, though I'm having trouble figuring out how to get this to transform, nor which TypeSouceInfo to use to construct it.

Presumably, it should be an anonymous typdef with empty location data, but the rest isn't particularly clear. If you have suggestions, I'd definitely appreciate them, otherwise I'll continue trying to get a functional patch together.

[erichkeane]

@rsmith I think this implements what you've suggested? I'm struggling a little with the template instantiations here, I'm not terribly sure I understand them as well as I'd hope.

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

I can see the parameter itself with the correct type being registered here: https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplate.cpp#L2249

But I don't have a good idea on how I'm supposed to do this with the typedef. Since it is a materialized typedef, it doesn't seem like we have the 'old' decl to register, nor access to the LocalInstantiationScope.

Can you make a suggestion? Am I missing something simple, or did I just not do what you suggested? If thats the case, can you rephrase your thought?

Thanks!
-Erich

[erichkeane]

In D80743#2074121, @erichkeane wrote:

@rsmith I think this implements what you've suggested? I'm struggling a little with the template instantiations here, I'm not terribly sure I understand them as well as I'd hope.

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

I can see the parameter itself with the correct type being registered here: https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplate.cpp#L2249

But I don't have a good idea on how I'm supposed to do this with the typedef. Since it is a materialized typedef, it doesn't seem like we have the 'old' decl to register, nor access to the LocalInstantiationScope.

Can you make a suggestion? Am I missing something simple, or did I just not do what you suggested? If thats the case, can you rephrase your thought?

Thanks!
-Erich

Hmm... so I tried to assign the typedef to the CXXRecordDecl inside the template instead (giving it an immediate parent), but it this error:

llvm-project/clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:149:24: error: type 'int' cannot be used prior to '::' because it has no members
    using U = typename T::type;
                   ^
llvm-project/clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:150:10: note: in instantiation of template class 'look_into_current_instantiation::C<int>' requested here
    C(T, U);
     ^
llvm-project/clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:156:5: note: while substituting deduced template arguments into function template '<deduction guide for C>' [with T = int]
  C c = {1, 2};

Is that the intended error? Or am I causing an instantiation we shouldn't be (and breaking the reason for this TreeTransform-er)?

This is the reproducer, including a comment that makes me think this is the intended behavior:

// We should have a substitution failure in the immediate context of
// deduction when using the C(T, U) constructor (probably; core wording
// unclear).
template<typename T> struct C {
  using U = typename T::type;
  C(T, U);
};

struct R { R(int); typedef R type; };
C(...) -> C<R>;

C c = {1, 2};

[rsmith]

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<typename T> struct A {
  using U = X1<T>;
  A(X2<U>);
};

into something like

template<typename T> A(X2<U>) -> A<T> {
  using U = X1<T>;
};

... 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<typename T> struct Type { using type = typename T::type; };
struct B { using type = int; };
template<typename T = B> struct A {
  using type = Type<T>::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...)

clang/lib/Sema/SemaTemplate.cpp

1967–1968

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).

[erichkeane]

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<typename T> struct A {
  using U = X1<T>;
  A(X2<U>);
};

into something like

template<typename T> A(X2<U>) -> A<T> {
  using U = X1<T>;
};

... 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<typename T> struct Type { using type = typename T::type; };
struct B { using type = int; };
template<typename T = B> struct A {
  using type = Type<T>::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.

clang/lib/Sema/SemaTemplate.cpp

1967–1968

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!

[erichkeane]

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?

[rsmith]

clang/lib/Sema/SemaTemplate.cpp

1964

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.

[erichkeane]

clang/lib/Sema/SemaTemplate.cpp

1964

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!

[ye-luo]

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

clang/lib/Sema/SemaTemplate.cpp

public:
 
   TypeSourceInfo *transform(TypeSourceInfo *TSI) { return TransformType(TSI); }
 
   QualType TransformTypedefType(TypeLocBuilder &TLB, TypedefTypeLoc TL) {
     return TransformType(
         TLB,
         TL.getTypedefNameDecl()->getTypeSourceInfo()->getTypeLoc());
   }
+
+  QualType TransformElaboratedType(TypeLocBuilder &TLB, ElaboratedTypeLoc TL) {
+    return TransformType(TLB, TL.getNamedTypeLoc());
+  }
 };
 
 /// Transform to convert portions of a constructor declaration into the
 /// corresponding deduction guide, per C++1z [over.match.class.deduct]p1.
 struct ConvertConstructorToDeductionGuideTransform {
   ConvertConstructorToDeductionGuideTransform(Sema &S,
                                               ClassTemplateDecl *Template)
       : SemaRef(S), Template(Template) {}
 
   Sema &SemaRef;
   ClassTemplateDecl *Template;
 

clang/test/AST/deduction-guides.cpp

+// RUN: %clang_cc1 -fsyntax-only %s -ast-dump -std=c++17 | FileCheck %s
+
+namespace PR46111 {
+template <typename>
+struct S;
+
+template <typename T>
+struct HasDeductionGuide {
+  typedef PR46111::S<T> STy;
+  HasDeductionGuide(typename STy::Child);
+  };
+
+// This causes deduction guides to be generated for all constructors.
+HasDeductionGuide() ->HasDeductionGuide<int>;
+
+// The parameter to this one shouldn't be an elaborated type.
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit <deduction guide for HasDeductionGuide> 'auto (typename S<T>::Child) -> HasDeductionGuide<T>'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit <deduction guide for HasDeductionGuide> 'auto (HasDeductionGuide<T>) -> HasDeductionGuide<T>'
+// CHECK: CXXDeductionGuideDecl {{.*}} implicit <deduction guide for HasDeductionGuide> 'auto () -> HasDeductionGuide<int>'
+} // namespace PR46111