[clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

[nridge]

Summary

Fixes PR50774.

[dblaikie]

This'll need a test case & does the change pass all existing tests?
Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (and you can shorten the bug reference to "PR50774" rather than the whole bugs.llvm.org URL)

[nridge]

Thanks for having a look!

In D104619#2831953, @dblaikie wrote:

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

& does the change pass all existing tests?

I mainly pushed the patch to Phabricator in this WIP form because I was hoping that would trigger some sort of CI run that would tell me that :) (I tried running ninja check-clang locally, but that was looking to take a very long time (like 2+ hours) to run locally.)

It's not clear to me if that actually happened? I see something about "pre-merge checks" in the revision metadata (and they're green), but it's not clear to me what test suites it actually ran.

[dblaikie]

In D104619#2831956, @nridge wrote:

Thanks for having a look!

In D104619#2831953, @dblaikie wrote:

This'll need a test case

Definitely. Do you have a suggestion for what test suite that should go into? I had a quick look but couldn't find anything that obviously exercised TypePrinter.

One way you could find some places this might be tested would be to introduce a deliberate break in the code somehow - like changing this parameter to be "true" instead of false - and then running the tests to see what fails. But I think the problem is likely that the tests all want the existing behavior/never tweak the Policy in this context.

So likely what you'd need would be a unit test of some kind (check around in clang/unittests to see if there are other AST pretty printer tests, for instance).

& does the change pass all existing tests?

I mainly pushed the patch to Phabricator in this WIP form because I was hoping that would trigger some sort of CI run that would tell me that :) (I tried running ninja check-clang locally, but that was looking to take a very long time (like 2+ hours) to run locally.)

Hmm - what sort of machine config do you have? What build configuration were you building/running with?

It's not clear to me if that actually happened? I see something about "pre-merge checks" in the revision metadata (and they're green), but it's not clear to me what test suites it actually ran.

Yeah, it looks like some tests ran: https://buildkite.com/llvm-project/premerge-checks/builds/44109 (if you follow from the "Build 157317: pre-merge checks" under the patch description, then copy/paste the URL from somewhere in there you'll get to that build info

[nridge]

add test

[nridge]

In D104619#2831966, @dblaikie wrote:

So likely what you'd need would be a unit test of some kind (check around in clang/unittests to see if there are other AST pretty printer tests, for instance).

Thanks for the suggestion! I did find existing pretty printer tests and was able to write a test for this fix by doing something similar.

I added you as reviewer for the patch, hope that's ok.

[nridge]

fix comment at top of file

[dblaikie]

The test code seems a bit overly general/overengineered for the use case, perhaps? (for instance it has unused parameters like the "Args" parameter - that neither caller passes in a value for, it has accessors (GetPRinted/getNumFoundTypes) when all the code fits on a (largeish) screen & so maybe having public members might be fine for that purpose - maybe all those are boilerplate AST matcher stuff, I guess, which is OK? but wouldn't mind it a bit simpler given the simplicity of the test

& perhaps this could be tested without the full power of AST matchers? Though perhaps that is the easiest way to locate the desired AST node.

clang/unittests/AST/TypePrinterTest.cpp

27–28

this is probably unnecessary/redundant - an llvm::function_ref itself can already be empty/none/boolean tested, which should be adequate without wrapping it in the extra layer of Optional?

But also, since there's only two callers and they both pass a non-empty functor - seems fine not to even test for empty and always apply the functor.

Alternatively, the caller could pass in a PrintingPolicy, instead of mutating the one here? Might be simpler?

[nridge]

I based the infrastructure closely on that in DeclPrinterTest.cpp and StmtPrinterTest.cpp. I figured the general framework could come in useful to people adding new type printing tests in the future.

I'm happy to trim it down a bit (e.g. remove unused methods and such), but in terms of the general approach, I feel like there's value in being consistent with the other printer tests?

[dblaikie]

In D104619#2846255, @nridge wrote:

I based the infrastructure closely on that in DeclPrinterTest.cpp and StmtPrinterTest.cpp. I figured the general framework could come in useful to people adding new type printing tests in the future.

I'm happy to trim it down a bit (e.g. remove unused methods and such), but in terms of the general approach, I feel like there's value in being consistent with the other printer tests?

Ah, sorry, had meant to ask if it'd be inspired by something else - if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

[nridge]

In D104619#2846258, @dblaikie wrote:

if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

That had occurred to me as well. I'm happy to try!

Would it perhaps make sense to do that in a follow-up patch?

[dblaikie]

In D104619#2846259, @nridge wrote:

In D104619#2846258, @dblaikie wrote:

if that's the case, maybe there's room to refactor the commonality - avoiding the near(if it is near) duplication and justifying the complexity/more general design a bit more, by having more uses to generalize over?

That had occurred to me as well. I'm happy to try!

Would it perhaps make sense to do that in a follow-up patch?

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

Alternatively: Types are decls, perhaps this test should go in the existing DeclPrinterTest?

[nridge]

In D104619#2846262, @dblaikie wrote:

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

Sounds good, will give that a try.

Alternatively: Types are decls, perhaps this test should go in the existing DeclPrinterTest?

Types often have corresponding decls (not always, e.g. BuiltinType), but they're distinct hierarchies (e.g. RecordType and RecordDecl can be thought of as "corresponding", but they're not related by inheritance, instead deriving from Type and Decl respectively). DeclPrinterTest exercises Decl::print(), while we want TypePrinterTest to exercise Type::print(), so I think a distinct file makes sense.

[nridge]

Rebase on top of refactor patch

[nridge]

In D104619#2846262, @dblaikie wrote:

If there's already some duplication, perhaps a pre-patch to generalize that functionality, then using that functionality in this patch?

I posted the patch to generalize the AST printing test infrastructure at https://reviews.llvm.org/D105457.

This patch now depends on that one.

[nridge]

Rebase on top of refactor patch changes

[nridge]

@dblaikie now that the dependent patch has been merged and (presumably) stuck, do you think we can proceed with this one?

[dblaikie]

Sure, sounds good, thanks!

clang/lib/AST/TypePrinter.cpp

void TypePrinter::printTemplateId(const TemplateSpecializationType *T,
   const TemplateParameterList *TPL = TD ? TD->getTemplateParameters() : nullptr;
   printTemplateArgumentList(OS, T->template_arguments(), Policy, TPL);
   spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printTemplateSpecializationBefore(
                                             const TemplateSpecializationType *T,
                                             raw_ostream &OS) {
-  printTemplateId(T, OS, false);
+  printTemplateId(T, OS, Policy.FullyQualifiedName);
 }
 
 void TypePrinter::printTemplateSpecializationAfter(
                                             const TemplateSpecializationType *T,
                                             raw_ostream &OS) {}
 
 void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
                                                raw_ostream &OS) {

clang/unittests/AST/CMakeLists.txt

add_clang_unittest(ASTTests
   EvaluateAsRValueTest.cpp
   ExternalASTSourceTest.cpp
   NamedDeclPrinterTest.cpp
   RecursiveASTVisitorTest.cpp
   SizelessTypesTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp
+  TypePrinterTest.cpp
   )
 
 clang_target_link_libraries(ASTTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend

clang/unittests/AST/TypePrinterTest.cpp

+//===- unittests/AST/StmtPrinterTest.cpp --- Statement printer tests ------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains tests for Stmt::printPretty() and related methods.
+//
+// Search this file for WRONG to see test cases that are producing something
+// completely wrong, invalid C++ or just misleading.
+//
+// These tests have a coding convention:
+// * statements to be printed should be contained within a function named 'A'
+//   unless it should have some special name (e.g., 'operator+');
+// * additional helper declarations are 'Z', 'Y', 'X' and so on.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ASTPrint.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallString.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ast_matchers;
+using namespace tooling;
+
+namespace {
+
+using PolicyAdjusterType =
+    Optional<llvm::function_ref<void(PrintingPolicy &Policy)>>;
+
+static void PrintType(raw_ostream &Out, const ASTContext *Context, QualType T,
+                      PolicyAdjusterType PolicyAdjuster) {
+  assert(!T.isNull() && "Expected non-null Type");
+  PrintingPolicy Policy = Context->getPrintingPolicy();
+  if (PolicyAdjuster)
+    (*PolicyAdjuster)(Policy);
+  T.print(Out, Policy);
+}
+
+class PrintMatch : public MatchFinder::MatchCallback {
+  SmallString<1024> Printed;
+  unsigned NumFoundTypes;
+  PolicyAdjusterType PolicyAdjuster;
+
+public:
+  explicit PrintMatch(PolicyAdjusterType PolicyAdjuster)
+      : NumFoundTypes(0), PolicyAdjuster(PolicyAdjuster) {}
+
+  void run(const MatchFinder::MatchResult &Result) override {
+    const QualType *T = Result.Nodes.getNodeAs<QualType>("id");
+    if (!T)
+      return;
+    NumFoundTypes++;
+    if (NumFoundTypes > 1)
+      return;
+
+    llvm::raw_svector_ostream Out(Printed);
+    PrintType(Out, Result.Context, *T, PolicyAdjuster);
+  }
+
+  StringRef getPrinted() const { return Printed; }
+
+  unsigned getNumFoundTypes() const { return NumFoundTypes; }
+};
+
+::testing::AssertionResult
+PrintedTypeMatches(StringRef Code, const std::vector<std::string> &Args,
+                   const DeclarationMatcher &NodeMatch,
+                   StringRef ExpectedPrinted,
+                   PolicyAdjusterType PolicyAdjuster) {
+  PrintMatch Printer(PolicyAdjuster);
+  MatchFinder Finder;
+  Finder.addMatcher(NodeMatch, &Printer);
+  std::unique_ptr<FrontendActionFactory> Factory =
+      newFrontendActionFactory(&Finder);
+
+  if (!runToolOnCodeWithArgs(Factory->create(), Code, Args))
+    return testing::AssertionFailure()
+           << "Parsing error in \"" << Code.str() << "\"";
+
+  if (Printer.getNumFoundTypes() == 0)
+    return testing::AssertionFailure() << "Matcher didn't find any types";
+
+  if (Printer.getNumFoundTypes() > 1)
+    return testing::AssertionFailure() << "Matcher should match only one type "
+                                          "(found "
+                                       << Printer.getNumFoundTypes() << ")";
+
+  if (Printer.getPrinted() != ExpectedPrinted)
+    return ::testing::AssertionFailure()
+           << "Expected \"" << ExpectedPrinted.str()
+           << "\", "
+              "got \""
+           << Printer.getPrinted().str() << "\"";
+
+  return ::testing::AssertionSuccess();
+}
+
+} // unnamed namespace
+
+TEST(TypePrinter, TemplateId) {
+  std::string Code = R"cpp(
+    namespace N {
+      template <typename> struct Type {};
+      
+      template <typename T>
+      void Foo(const Type<T> &Param);
+    }
+  )cpp";
+  auto Matcher = parmVarDecl(hasType(qualType().bind("id")));
+
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "const Type<T> &",
+                                 PolicyAdjusterType([](PrintingPolicy &Policy) {
+                                   Policy.FullyQualifiedName = false;
+                                 })));
+
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "const N::Type<T> &",
+                                 PolicyAdjusterType([](PrintingPolicy &Policy) {
+                                   Policy.FullyQualifiedName = true;
+                                 })));
+}
 No newline at end of file