This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by erichkeane on May 28 2020, 10:30 AM.

Details

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.

Diff Detail

Event Timeline

erichkeane created this revision.May 28 2020, 10:30 AM

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.

I've got nothing to say here.

rsmith added inline comments.Jun 3 2020, 11:39 AM
clang/lib/Sema/SemaTemplate.cpp
1992–1994

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 marked an inline comment as done.Jun 3 2020, 1:09 PM
erichkeane added inline comments.
clang/lib/Sema/SemaTemplate.cpp
1992–1994

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.

@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

@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 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
1970–1971

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 marked an inline comment as done.Jun 5 2020, 8:46 AM

@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
1970–1971

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 updated this revision to Diff 268822.Jun 5 2020, 8:48 AM
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. (Show Details)

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 accepted this revision.Jun 11 2020, 5:13 PM
rsmith added inline comments.
clang/lib/Sema/SemaTemplate.cpp
1967

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.

This revision is now accepted and ready to land.Jun 11 2020, 5:13 PM
erichkeane marked an inline comment as done.Jun 11 2020, 7:00 PM
erichkeane added inline comments.
clang/lib/Sema/SemaTemplate.cpp
1967

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!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2020, 5:54 AM
ye-luo added a subscriber: ye-luo.Nov 27 2020, 1:20 PM

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