[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 <https://reviews.llvm.org/D80743#2074537>, @rsmith wrote:

> In D80743#2074121 <https://reviews.llvm.org/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.



================
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