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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D123127#3429589 <https://reviews.llvm.org/D123127#3429589>, @sammccall wrote:

> This looks pretty good!
> The tests in clang are sadly indirect.

This is mostly due to the fact that `TemplateName` is not dumped. This patch extend the `NodeDumper` to print a `using` indicator, I think it is relatively obvious in the AST.

> I think adding support to clangd's FindTarget would be a small change and would allow a fairly direct test, but maybe it will affect a bunch of existing tests or possibly have a blast radius. Up to you.

Adding support in clangd `FindTarget` is my next step, it would be a followup patch. I feel like the FindTarget tests are more related to the application logic of the new `TemplateName`, and are less direct compared with the tests in this patch.



================
Comment at: clang/include/clang/AST/TemplateName.h:446
   /// that this qualified name refers to.
   TemplateDecl *Template;
 
----------------
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).


================
Comment at: clang/lib/AST/ASTContext.cpp:6176
   }
+  case TemplateName::UsingTemplate:
+    return getCanonicalTemplateName(
----------------
sammccall wrote:
> you could also handle this in the QualifiedTemplate/Template case, since the decl is guaranteed to be present.
yeah, but I think the current version is clearer.


================
Comment at: clang/lib/AST/TemplateName.cpp:273
+  } else if (auto *U = getAsUsingTemplateName()) {
+    U->getUnderlying().print(OS, Policy);
   } else {
----------------
sammccall wrote:
> Why is this correct, rather than potentially printing the wrong sugar?
> 
> If the reasoning is that that the underlying TemplateName has the same name and can't have any unwanted sugar, that seems subtle and deserves a comment.
> 
> I think just printing the unqualified name directly would be clearer.
> 
good point, you're right, this will print duplicated nested specifiers. Change it to print unqualified name only, and added a test.


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


================
Comment at: clang/lib/Tooling/CMakeLists.txt:62
         # Skip this in debug mode because parsing AST.h is too slow
-        --skip-processing=${skip_expensive_processing}
+        --skip-processing=1
         -I ${LLVM_BINARY_DIR}/lib/clang/${CLANG_VERSION}/include
----------------
sammccall wrote:
> revert?
> 
> (though yes, this is annoying)
oops, an accident change, reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123127