Add an option to fully qualify classes and structs.

[jblespiau]

Summary

Generally speaking, using TypePrinter is the most flexible mechanism for
printing types. It makes sense to have this feature just for this
reason.

However, this is also done in another context:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20200518/321225.html

In a nutshell, getFullyQualifiedName is bugged and prints
::tensorfn::Nested< ::std::variant<Tensor, DeviceTensor> >
instead of:
::tensorfn::Nested< ::std::variant<::tensorflow::Tensor,::tensorfn::DeviceTensor> >

I was not able to fix this function, and intend to add this feature and
then start to remove the use of the other function (in CLIF first).

Longer term, we should delete QualTypeNames.cpp, and prefer
TypePrinter.

Things that are not that clear:

• should more types than classes and struct be globally qualified?

[sammccall]

This should really have a test - NamedDeclPrinterTest.cpp seems like the right place.

clang/lib/AST/TypePrinter.cpp

326

structure-or-class-type isn't quite the right check:

• enums and typedefs for instance should also be qualified.
• you're looking through sugar to the underlying type, which may not be what you want

It's going to be hard to get this right from here... I suspect it's better to fix where FullyQualifiedName is checked in DeclPrinter.
This ultimately calls through to NamedDecl::printNestedNameSpecifier, which is probably the right place to respect this option.
(I don't totally understand what's meant to happen if SuppressScope is false but FullyQualiifedName is also false, that calls through to NestedNameSpecifier::print which you may want to also fix, or not)

[jblespiau]

I did spend a few hours, just building and finding how to run tests :(

I have a few additional questions, as I do not really understand what happen. In my initial idea, I wanted to modify the way QualType are printed, not really declarations, but I feel the logic is duplicated somewhere, and that both NamedDecl and Type are implementing the logic.

clang/lib/AST/TypePrinter.cpp

326

There are several elements I do not understand (as you will see, I am completely lost).

1. I am trying to modify QualType::getAsString, and you suggest changing NamecDecl. I do not understand the relationship between Qualtype and NameDecl: I understand declaration within the context of C++:

https://www.dummies.com/programming/cpp/expressions-and-declarations-in-c-programming/
Thus, a Declaration will be made of several parts, some of which will be Type. Thus, for me, the change should not be in NameDecl but in Type, to also make sure the change is there both when we print a NameDecl and a type.. If we modify NameDecl, then, when printing a type through Type::getAsString, we won't get the global "::" specifier.
I have understood NamedDecl::printQualifiedName calls NamedDecl::printNestedNameSpecifier which calls the Type::print function, see
https://github.com/llvm-mirror/clang/blob/master/lib/AST/Decl.cpp#L1630

> From that, I still understand I should modify how types are printed, not NamedDecl. I may completely be incorrect, and I am only looking to understand.

Thus, I feel my change is correcly placed, but we can change it to be:

if (!Policy.SuppressScope && Policy.GlobalScopeQualifiedName &&
    (T->isStructureOrClassType() || T->isEnumeralType() ||
     isa<TypedefType>(T))) {
  OS << "::";
}

Other remarks:

2. As documented:

/// When true, print the fully qualified name of function declarations.
/// This is the opposite of SuppressScope and thus overrules it.
unsigned FullyQualifiedName : 1;

FullyQualifiedName is only controlling the fully-qualification of functions.
https://github.com/llvm/llvm-project/blob/a2a3e9f0a6e91103a0d1fa73086dbdf109c48f69/clang/lib/AST/DeclPrinter.cpp#L625

So I do not think we have to follow this.

I think it is SuppressScope which controls whether it is fully qualified or not,:
https://github.com/llvm/llvm-project/blob/a2a3e9f0a6e91103a0d1fa73086dbdf109c48f69/clang/lib/AST/DeclPrinter.cpp#L629

If Policy.SuppressScope is False, then I understand it's fully qualified.

3. "you're looking through sugar to the underlying type, which may not be what you want": I do not understand "sugar" in that context. I have read this name in the code in clang, but still, I do not understand it. I only know https://en.wikipedia.org/wiki/Syntactic_sugar, which does not seem to apply here (Obviously, I am not a native English speaker).

For the testing:

Building and running

After > 90minutes of building with:
cmake -DLLVM_ENABLE_PROJECTS=clang -G "Unix Makefiles" ../llvm
make check-clang

It took me quite a while to find how to execute a single test file:

~/llvm-project/build/tools/clang/unittests/AST]
└──╼ make -j12 ASTTests && ./ASTTests --gtest_filter=NamedDeclPrinter*

did the job.

• NamedDeclPrinterTest.cpp feels strange for the tests, as what I am modifying is not NamedDecl, but Qualtype::getAsString. But given there is no TypeTest.cpp, maybe it's best location.

I must admit that I am struggling a lot to understand both the execution flow and the code itself :(

[jblespiau]

clang/lib/AST/TypePrinter.cpp

326

(I had issues with the formatting in Markdown. The big bold was not intended to be big and bold^^ Sorry).

[sammccall]

In D80800#2065643, @jblespiau wrote:

I did spend a few hours, just building and finding how to run tests :(

Sorry about that :-(
For what it's worth, building with Ninja should be significantly faster by default: https://llvm.org/docs/GettingStarted.html
But yes, building LLVM is very slow unless you have a powerful machine.

I have a few additional questions, as I do not really understand what happen. In my initial idea, I wanted to modify the way QualType are printed, not really declarations, but I feel the logic is duplicated somewhere, and that both NamedDecl and Type are implementing the logic.

Yes, I think you're right, sorry - I assumed these were sharing code. (This is why we need tests!)
Both cases need to be modified: PrintingPolicy is used for printing both Types and Decls, and the new option would need to apply to both cases.

The idea that the namespace prefix should be "sunk" down to where we know more about the structure of the type seems valid though: if you look at where the site you've found prints the scope, it calls printRecordBefore -> printTag -> AppendScope, the latter is probably the right place to modify for Type.

clang/lib/AST/TypePrinter.cpp

326

I must admit that I am struggling a lot to understand both the execution flow and the code itself :(

FWIW I'm with you on this one, I find this code hard to follow and it's certainly not well tested.
Much of this is old and original authors aren't active on this part of the code anymore. So it's frustratingly hard to add new features to, and a lot of my concern here is that we don't make a complicated area of the code even worse.

I do not understand the relationship between Qualtype and NameDecl

Well, there's a few relationships, that explains why these print functions aren't *totally* separable:

• for types that are declared, such as classes, there's both a Decl and a Type. Clang usually chooses to put most of the information in the Decl, so a RecordType is mostly a thin wrapper around a CXXRecordDecl. When you print a RecordType, it inspects the underlying CXXRecordDecl to get the name etc. (But it doesn't call print on the decl, contrary to what I thought).
• decls can contain types, like the CXXRecordDecl vector<X> contains the type X (which should be printed using Type::print)
• maybe types can directly contain decls, but I can't think of any

I have understood NamedDecl::printQualifiedName calls NamedDecl::printNestedNameSpecifier which calls the Type::print function, see

Maybe I'm missing something, but I don't think printNestedNameSpecifier calls Type::print at all. Where does that happen?
I thint printNestedNameSpecifier has to be modified to handle your new option, in addition to TypePrinter::printScope.

Thus, I feel my change is correcly placed

I don't think it's reasonable to have the code printing the :: so far removed from the code that prints the rest of the scope.

if (!Policy.SuppressScope && Policy.GlobalScopeQualifiedName && (T->isStructureOrClassType() || T->isEnumeralType() || isa<TypedefType>(T)))

Please don't try to guess all the cases if you can avoid it - we'll miss some in review and end up with bugs.
An example off the top of my head: what if T is an unknown type in a dependent scope? If the dependent scope is a type variable (X::typename Foo) you don't want the colons, but if it's a dependent class template instantiation (ns::Foo<X>::typename Bar) then you do...

I do not understand "sugar" in that context. I have read this name in the code in clang, but still, I do not understand it. I only know https://en.wikipedia.org/wiki/Syntactic_sugar, which does not seem to apply here

That's indeed the reference, "sugar" is used to refer to types that are not canonical: https://clang.llvm.org/docs/InternalsManual.html#canonicaltype

For instance, a TypedefType (ns::IntTy) might be sugar for an underlying BuiltinType (int). You'd want to print the :: or not depending on the form you're printing, which is probably the outer sugar type, but functions like isStructureOrClassType will give you an answer based on the underlying type.

This is one of the reasons I'd suggest hooking into locations that are already printing scopes, rather than trying to add new codepaths that do this - a lot of these bits are very subtle if you're not familiar with the AST.

NamedDeclPrinterTest.cpp feels strange for the tests, as what I am modifying is not NamedDecl, but Qualtype::getAsString

Yeah, that was based on my assumption they were sharing code, but in fact we'll need to modify and test both.

But given there is no TypeTest.cpp, maybe it's best location.

It's a bit of a hack, but you could test something like ::ns::Foo<::ns::Bar> in that file which will (I'm fairly sure!) exercise the TypePrinter for Bar - adding a new unittest for TypePrinter seems like a heavy lift for someone new to the codebase.

clang/include/clang/AST/PrettyPrinter.h

PrintingPolicy(const LangOptions &LO)
         SuppressTemplateArgsInCXXConstructors(false), Bool(LO.Bool),
         Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
         UseVoidForZeroParams(!LO.CPlusPlus),
         SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
         PolishForDeclaration(false), Half(LO.Half),
         MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
         MSVCFormatting(false), ConstantsAsWritten(false),
         SuppressImplicitBase(false), FullyQualifiedName(false),
-        PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true) {}
+        GlobalScopeQualifiedName(false), PrintCanonicalTypes(false),
+        PrintInjectedClassNameWithArguments(true) {}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
   /// construct). This should not be used if a real LangOptions object is
   /// available.
   void adjustForCPlusPlus() {
     SuppressTagKeyword = true;
     Bool = true;
struct PrintingPolicy {
 
   /// When true, don't print the implicit 'self' or 'this' expressions.
   unsigned SuppressImplicitBase : 1;
 
   /// When true, print the fully qualified name of function declarations.
   /// This is the opposite of SuppressScope and thus overrules it.
   unsigned FullyQualifiedName : 1;
 
+  // When true, class and struct types (to be expanded if needed) will be
+  // prepended with "::"
+  // Note it also requires `FullyQualifiedName` to also be set to true, as it
+  // does not make sense to prepend "::" to a non fully-qualified name.
+  // This targets generated code.
+  unsigned GlobalScopeQualifiedName : 1;
+
   /// Whether to print types as written or canonically.
   unsigned PrintCanonicalTypes : 1;
 
   /// Whether to print an InjectedClassNameType with template arguments or as
   /// written. When a template argument is unnamed, printing it results in
   /// invalid C++ code.
   unsigned PrintInjectedClassNameWithArguments : 1;
 
   /// Callbacks to use to allow the behavior of printing to be customized.
   const PrintingCallbacks *Callbacks = nullptr;
 };
 
 } // end namespace clang
 
 #endif

clang/lib/AST/TypePrinter.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/ExceptionSpecificationType.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
+#include <iostream>
 #include <string>
 
 using namespace clang;
 
 namespace {
 
   /// RAII object that enables printing of the ARC __strong lifetime
   /// qualifier.
void TypePrinter::printBefore(const Type *T,Qualifiers Quals, raw_ostream &OS) {
 
   bool hasAfterQuals = false;
   if (!CanPrefixQualifiers && !Quals.empty()) {
     hasAfterQuals = !Quals.isEmptyWhenPrinted(Policy);
     if (hasAfterQuals)
       HasEmptyPlaceHolder = false;
   }
 
+  if (Policy.FullyQualifiedName && Policy.GlobalScopeQualifiedName &&
+      T->isStructureOrClassType()) {
+    OS << "::";
+  }
   switch (T->getTypeClass()) {
 #define ABSTRACT_TYPE(CLASS, PARENT)
 #define TYPE(CLASS, PARENT) case Type::CLASS: \
     print##CLASS##Before(cast<CLASS##Type>(T), OS); \
     break;
 #include "clang/AST/TypeNodes.inc"
   }