[PATCH] D123127: [AST] Add a new TemplateName for templates found via a using declaration.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, I think this is just about ready to go.
Only substantive issue is I think we should address the QualifiedTemplate/UsingTemplate interaction in a more principled way.



================
Comment at: clang/include/clang/AST/PropertiesBase.td:625
+let Class = PropertyTypeCase<TemplateName, "UsingTemplate"> in {
+  def : Property<"usingShadowDecl", UsingShadowDeclRef> {
+    let Read = [{ node.getAsUsingTemplateName()->getFoundDecl() }];
----------------
rename these properties too?


================
Comment at: clang/include/clang/AST/TemplateName.h:446
   /// that this qualified name refers to.
   TemplateDecl *Template;
 
----------------
hokein wrote:
> sammccall wrote:
> > It seems really unfortunate that this is a `TemplateDecl*` instead of a `TemplateName`.
> > 
> > for:
> > ```
> > template <int> class x;
> > namespace a { using ::x; }
> > a::x<0>;
> > ```
> > we want `a::x` to include both a QualifiedTemplateName (modelling the a:: qualifier) and a UsingTemplateName (modelling the UsingShadowDecl that was found).
> > 
> > I'd guess we can change `Template` to be a `TemplateName` internally, and add `getTemplate()` while keeping the existing `get[Template]Decl` APIs on top of `TemplateName::getAsTemplateDecl()`.
> > I suppose this could be a separate change (though it'd be nice to know whether it's going to work...)
> > 
> > Of course if that changed there's a natural followon to take the qualifier out of DependentTemplateName and turn it into a QualifiedTemplateName wrapping a DependentTemplateName. (Definitely out of scope for this patch, not sure if it's reasonable to expect you to do it at all)
> > we want a::x to include both a QualifiedTemplateName (modelling the a:: qualifier) and a UsingTemplateName (modelling the UsingShadowDecl that was found).
> 
> > I'd guess we can change Template to be a TemplateName internally, and add getTemplate() while keeping the existing get[Template]Decl APIs on top of TemplateName::getAsTemplateDecl().
> > I suppose this could be a separate change (though it'd be nice to know whether it's going to work...)
> 
> Agree, I think this is the right solution, but would require more work (a followup patch).
> 
> There are two options, 1) leave the qualified template name out, and support it in a followup patch, 2) workaround it -- we wrap the qualified template name with a using template name, though it doesn't quite match our mental model, I think it is probably ok. (I added a FIXME in `SemaTemplate.cpp`).
> 
> Let me know what you think, also happy to switch to 1).
I don't like 2), it adds code in order to introduce an incorrect "inside-out" AST in a surprising way that will lead to subtle bugs, and we'll need to undo this later. To me this seems probably worse than dropping one of the sugar nodes and never fixing it, which is already pretty bad.

I think we should do 1) instead - I think it's actually simpler to land the QualifiedTemplate-holds-TemplateName patch *first*, but either order seems fine.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11026
           Context.hasSameTemplateName(SpecifiedName, GuidedTemplate);
-      if (SpecifiedName.getKind() == TemplateName::Template && TemplateMatches)
+      if ((SpecifiedName.getKind() == TemplateName::Template ||
+           SpecifiedName.getKind() == TemplateName::UsingTemplate) &&
----------------
hokein wrote:
> sammccall wrote:
> > I don't think it can be valid to find the template name via a usingshadowdecl, because the deduction guide must be declared in the same scope as the template. (err_deduction_guide_wrong_scope).
> > 
> > Is this to prevent an unneccesary second diagnostic? (And if so, I wonder whether we should also include QualifiedTemplate, maybe as a separate change). It may deserve a comment
> yeah, it prevents a bogus diagnostic introduced by this patch.
> 
> ```
>  namespace N {
>     template<typename T> struct NamedNS1 {}; // expected-note {{here}}
>   }
>   using N::NamedNS1;
>   NamedNS1(int) -> NamedNS1<int>; // expected-error {{deduction guide must be declared in the same scope as template}}
> ```
> 
> With this patch without the change here, we emit an extra diagnostic `deduced type 'NamedNS1<int>' of deduction guide is not written as a specialization of template 'NamedNS1'`. 
> 
> And this part of code might have another issue where it emits a wrong `err_deduction_guide_defines_function` for a correct code, (this is something I plan to take a look but not in this patch) .
That makes sense, though it seems to me inconsistent to suppress the secondary diagnostic for UsingTemplate but emit it for QualifiedTemplate - it's equally bogus/valid in both cases.

Can we either suppress both, or suppress neither for now and suppress them in a subsequent patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127