[clang-tools-extra] 5cdb906 - [clangd] Unify printing policy for type hints

[Younan Zhang via cfe-commits]

Author: Younan Zhang
Date: 2023-06-13T21:31:10+08:00
New Revision: 5cdb906f1e4093600f99aab8660e1536514a57e8

URL: https://github.com/llvm/llvm-project/commit/5cdb906f1e4093600f99aab8660e1536514a57e8
DIFF: https://github.com/llvm/llvm-project/commit/5cdb906f1e4093600f99aab8660e1536514a57e8.diff

LOG: [clangd] Unify printing policy for type hints

(This patch addresses the comment from https://reviews.llvm.org/D151785#4402460.)

Previously, we used a special printing policy that enabled `PrintCanonicalTypes`
to print type hints for structure bindings. This was intended to
eliminate type aliases like `tuple_element::type`. However, this also
caused TypePrinter to print default template arguments, which could
result in losing the ability to see types like `std::basic_string<char>`
if the fully expanded template-id exceeded the default inlay hint threshold.

Simply getting the canonical type at the call site could help us get rid of
the side effect.

This also merges overloaded `addTypeHint` into one function without
`PrintingPolicy`.

Reviewed By: nridge

Differential Revision: https://reviews.llvm.org/D152520

Added: 
    

Modified: 
    clang-tools-extra/clangd/InlayHints.cpp
    clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index f81b51550ac9b..416a01f4737bc 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -258,8 +258,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
         Cfg(Cfg), RestrictRange(std::move(RestrictRange)),
         MainFileID(AST.getSourceManager().getMainFileID()),
         Resolver(AST.getHeuristicResolver()),
-        TypeHintPolicy(this->AST.getPrintingPolicy()),
-        StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
+        TypeHintPolicy(this->AST.getPrintingPolicy()) {
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
@@ -269,14 +268,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
     TypeHintPolicy.AnonymousTagLocations =
         false; // do not print lambda locations
 
-    // For structured bindings, print canonical types. This is important because
-    // for bindings that use the tuple_element protocol, the non-canonical types
-    // would be "tuple_element<I, A>::type".
-    // For "auto", we often prefer sugared types.
     // Not setting PrintCanonicalTypes for "auto" allows
     // SuppressDefaultTemplateArgs (set by default) to have an effect.
-    StructuredBindingPolicy = TypeHintPolicy;
-    StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
 
   bool VisitTypeLoc(TypeLoc TL) {
@@ -358,8 +351,12 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
     // but show hints for the individual bindings.
     if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
       for (auto *Binding : DD->bindings()) {
-        addTypeHint(Binding->getLocation(), Binding->getType(), /*Prefix=*/": ",
-                    StructuredBindingPolicy);
+        // For structured bindings, print canonical types. This is important
+        // because for bindings that use the tuple_element protocol, the
+        // non-canonical types would be "tuple_element<I, A>::type".
+        if (auto Type = Binding->getType(); !Type.isNull())
+          addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
+                      /*Prefix=*/": ");
       }
       return true;
     }
@@ -724,22 +721,17 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
   }
 
   void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
-    addTypeHint(R, T, Prefix, TypeHintPolicy);
-  }
-
-  void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
-                   const PrintingPolicy &Policy) {
     if (!Cfg.InlayHints.DeducedTypes || T.isNull())
       return;
 
     // The sugared type is more useful in some cases, and the canonical
     // type in other cases.
     auto Desugared = maybeDesugar(AST, T);
-    std::string TypeName = Desugared.getAsString(Policy);
+    std::string TypeName = Desugared.getAsString(TypeHintPolicy);
     if (T != Desugared && !shouldPrintTypeHint(TypeName)) {
       // If the desugared type is too long to display, fallback to the sugared
       // type.
-      TypeName = T.getAsString(Policy);
+      TypeName = T.getAsString(TypeHintPolicy);
     }
     if (shouldPrintTypeHint(TypeName))
       addInlayHint(R, HintSide::Right, InlayHintKind::Type, Prefix, TypeName,
@@ -764,14 +756,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
-  // We want to suppress default template arguments, but otherwise print
-  // canonical types. Unfortunately, they're conflicting policies so we can't
-  // have both. For regular types, suppressing template arguments is more
-  // important, whereas printing canonical types is crucial for structured
-  // bindings, so we use two separate policies. (See the constructor where
-  // the policies are initialized for more details.)
   PrintingPolicy TypeHintPolicy;
-  PrintingPolicy StructuredBindingPolicy;
 };
 
 } // namespace

diff  --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 2a4e3ca88973b..59ec9aa4776da 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -1347,8 +1347,11 @@ TEST(TypeHints, DefaultTemplateArgs) {
     struct A {};
     A<float> foo();
     auto $var[[var]] = foo();
+    A<float> bar[1];
+    auto [$binding[[value]]] = bar;
   )cpp",
-                  ExpectedHint{": A<float>", "var"});
+                  ExpectedHint{": A<float>", "var"},
+                  ExpectedHint{": A<float>", "binding"});
 }
 
 TEST(TypeHints, Deduplication) {