[clangd] Fix type printing in the presence of qualifiers

[adamcz]

Summary

When printing QualType with qualifiers like "const", or pointing to an
elaborated type, we would print garbage like:

std::const std::vector<int>&

with the initial std:: being calculated correctly, but inserted in the
wrong place and the second std:: not removed (due to elaborated type).

This affected, among others, ExtractFunction and ExpandAuto tweaks.

This change introduces a new callback to PrintingPolicy, which allows us
to influence the printing of namespace qualifiers. In the future, the
same callback can be used to improve handling of "using namespace"
directives as well.

Fixes:

https://github.com/clangd/clangd/issues/640 (ExtractFunction)
https://github.com/clangd/clangd/issues/264 (ExpandAuto)
First point of https://github.com/clangd/clangd/issues/524

[kadircet]

clang/include/clang/AST/PrettyPrinter.h

49

oh wow, i didn't know about these callbacks at all.

this looks pretty need, but i wonder if we should make it more generic like "shouldPrintScope" (or isScopeVisible) and pass not only namespacedecls but the declcontext directly to enable short circuiting of type scopes as well?

also it might be nice to note in comments that any parent scope won't be visited once this callback returns true (or false in the case of shouldPrintScope).

[adamcz]

Addressed review comment

[adamcz]

Thanks!

[kadircet]

Thanks, LGTM!

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

68

can you also test for shortening? e.g.

void ns::Func() { aut^o x = new ns::Class::Nested{}; }

[adamcz]

final review comments

[kadircet]

clang/include/clang/AST/PrettyPrinter.h

51

nit: s/NS/DC/

[adamcz]

s/NS/DC

clang-tools-extra/clangd/AST.cpp

SymbolID getSymbolID(const llvm::StringRef MacroName, const MacroInfo *MI,
   if (MI == nullptr)
     return {};
   llvm::SmallString<128> USR;
   if (index::generateUSRForMacro(MacroName, MI->getDefinitionLoc(), SM, USR))
     return {};
   return SymbolID(USR);
 }
 
-// FIXME: This should be handled while printing underlying decls instead.
 std::string printType(const QualType QT, const DeclContext &CurContext) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
-  auto Decls =
-      explicitReferenceTargets(DynTypedNode::create(QT), DeclRelation::Alias);
-  if (!Decls.empty())
-    OS << getQualification(CurContext.getParentASTContext(), &CurContext,
-                           Decls.front(),
-                           /*VisibleNamespaces=*/llvm::ArrayRef<std::string>{});
   PrintingPolicy PP(CurContext.getParentASTContext().getPrintingPolicy());
-  PP.SuppressScope = true;
   PP.SuppressTagKeyword = true;
+  PP.SuppressUnwrittenScope = true;
+
+  class PrintCB : public PrintingCallbacks {
+  public:
+    PrintCB(const DeclContext *CurContext) : CurContext(CurContext) {}
+    virtual ~PrintCB() {}
+    virtual bool isScopeVisible(const DeclContext *DC) const {
+      return DC->Encloses(CurContext);
+    }
+
+  private:
+    const DeclContext *CurContext;
+  };
+  PrintCB PCB(&CurContext);
+  PP.Callbacks = &PCB;
+
   QT.print(OS, PP);
   return OS.str();
 }
 
 QualType declaredType(const TypeDecl *D) {
   if (const auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D))
     if (const auto *TSI = CTSD->getTypeAsWritten())
       return TSI->getType();

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

EXPECT_EQ(apply("au^to x = inl_ns::Visible();"),
             "Visible x = inl_ns::Visible();");
   // local class
   EXPECT_EQ(apply("namespace x { void y() { struct S{}; ^auto z = S(); } }"),
             "namespace x { void y() { struct S{}; S z = S(); } }");
   // replace array types
   EXPECT_EQ(apply(R"cpp(au^to x = "test";)cpp"),
             R"cpp(const char * x = "test";)cpp");
 
+  EXPECT_EQ(apply("ns::Class * foo() { au^to c = foo(); }"),
+            "ns::Class * foo() { ns::Class * c = foo(); }");
+  EXPECT_EQ(apply("void ns::Func() { au^to x = new ns::Class::Nested{}; }"),
+            "void ns::Func() { Class::Nested * x = new ns::Class::Nested{}; }");
+
   EXPECT_UNAVAILABLE("dec^ltype(au^to) x = 10;");
   // expanding types in structured bindings is syntactically invalid.
   EXPECT_UNAVAILABLE("const ^auto &[x,y] = (int[]){1,2};");
 
   // FIXME: Auto-completion in a template requires disabling delayed template
   // parsing.
   ExtraArgs.push_back("-fno-delayed-template-parsing");
   // unknown types in a template should not be replaced
   EXPECT_THAT(apply("template <typename T> void x() { ^auto y = T::z(); }"),
               StartsWith("fail: Could not deduce type for 'auto' type"));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/tweaks/ExtractFunctionTests.cpp

 void extracted(const int &c) {
 while(c) {}
 }
 void f(const int c) {
   extracted(c);
 })cpp";
   EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
 
+  // Check const qualifier with namespace
+  std::string ConstNamespaceCheckInput = R"cpp(
+namespace X { struct Y { int z; }; }
+int f(const X::Y &y) {
+  [[return y.z + y.z;]]
+})cpp";
+  std::string ConstNamespaceCheckOutput = R"cpp(
+namespace X { struct Y { int z; }; }
+int extracted(const X::Y &y) {
+return y.z + y.z;
+}
+int f(const X::Y &y) {
+  return extracted(y);
+})cpp";
+  EXPECT_EQ(apply(ConstNamespaceCheckInput), ConstNamespaceCheckOutput);
+
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
   // We don't extract from methods for now since they may involve multi-file
   // edits
   std::string MethodFailInput = R"cpp(
     class T {
       void f() {

clang/include/clang/AST/PrettyPrinter.h

 #ifndef LLVM_CLANG_AST_PRETTYPRINTER_H
 #define LLVM_CLANG_AST_PRETTYPRINTER_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 
 namespace clang {
 
+class DeclContext;
 class LangOptions;
 class SourceManager;
 class Stmt;
 class TagDecl;
 
 class PrinterHelper {
 public:
   virtual ~PrinterHelper();
   virtual bool handledStmt(Stmt* E, raw_ostream& OS) = 0;
 };
 
 /// Callbacks to use to customize the behavior of the pretty-printer.
 class PrintingCallbacks {
 protected:
   ~PrintingCallbacks() = default;
 
 public:
   /// Remap a path to a form suitable for printing.
   virtual std::string remapPath(StringRef Path) const {
     return std::string(Path);
   }
+
+  /// When printing type to be inserted into code in specific context, this
+  /// callback can be used to avoid printing the redundant part of the
+  /// qualifier. For example, when inserting code inside namespace foo, we
+  /// should print bar::SomeType instead of foo::bar::SomeType.
+  /// To do this, shouldPrintScope should return true on "foo" NamespaceDecl.
+  /// The printing stops at the first isScopeVisible() == true, so there will
+  /// be no calls with outer scopes.
+  virtual bool isScopeVisible(const DeclContext *DC) const { return false; }
 };
 
 /// Describes how types, statements, expressions, and declarations should be
 /// printed.
 ///
 /// This type is intended to be small and suitable for passing by value.
 /// It is very frequently copied.
 struct PrintingPolicy {

clang/lib/AST/TypePrinter.cpp

void TypePrinter::AppendScope(DeclContext *DC, raw_ostream &OS,
   if (DC->isTranslationUnit())
     return;
 
   // FIXME: Consider replacing this with NamedDecl::printNestedNameSpecifier,
   // which can also print names for function and method scopes.
   if (DC->isFunctionOrMethod())
     return;
 
+  if (Policy.Callbacks && Policy.Callbacks->isScopeVisible(DC))
+    return;
+
   if (const auto *NS = dyn_cast<NamespaceDecl>(DC)) {
     if (Policy.SuppressUnwrittenScope && NS->isAnonymousNamespace())
       return AppendScope(DC->getParent(), OS, NameInScope);
 
     // Only suppress an inline namespace if the name has the same lookup
     // results in the enclosing namespace.
     if (Policy.SuppressInlineNamespace && NS->isInline() && NameInScope &&
         DC->getParent()->lookup(NameInScope).size() ==