[clangd] Type hints for structured bindings

[nridge]

Summary

Hints are shown for the individual bindings, not the aggregate.

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

40

While playing around with this, it did occur to me that in some cases it's more helpful to print sugared types than canonical types, for example in a case like this:

template <typename, typename, typename>
struct SomeLongType {};

using ShortType = SomeLongType<int, float, double>;

ShortType func();

auto x = func();  // would prefer "ShortType" as hint

However, it turns out that AutoType doesn't retain the sugared type to begin with (there's a FIXME about that here), so setting PrintCanonicalTypes=true doesn't actually regress cases like this (they were already printing the canonical type).

[sammccall]

Cool!

clang-tools-extra/clangd/InlayHints.cpp

40

This makes sense - can you add a comment with a short version of this?
// Often we'd prefer sugar types for auto, but the AST doesn't retain them anyway.

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

468–469

since we have the handy assertTypeHints function, can we split this into several separate tests?
(Or one table-driven test so at least the test code + corresponding assertions are next to each other)

[nridge]

Address review comments

clang-tools-extra/clangd/InlayHints.cpp

InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
     bool Invalid = false;
     llvm::StringRef Buf =
         AST.getSourceManager().getBufferData(MainFileID, &Invalid);
     MainFileBuf = Invalid ? StringRef{} : Buf;
 
     TypeHintPolicy.SuppressScope = true; // keep type names short
     TypeHintPolicy.AnonymousTagLocations =
         false; // do not print lambda locations
+    // Print canonical types. Otherwise, SuppressScope would result in
+    // things like "metafunction<args>::type" being shorted to just "type",
+    // which is useless. This is particularly important for structured
+    // bindings that use the tuple_element protocol, where the non-canonical
+    // types would be "tuple_element<I, A>::type".
+    // Note, for "auto", we would often prefer sugared types, but the AST
+    // doesn't currently retain them in DeducedType anyways.
+    TypeHintPolicy.PrintCanonicalTypes = true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     // Weed out constructor calls that don't look like a function call with
     // an argument list, by checking the validity of getParenOrBraceRange().
     // Also weed out std::initializer_list constructors as there are no names
     // for the individual arguments.
     if (!E->getParenOrBraceRange().isValid() ||
bool VisitCallExpr(CallExpr *E) {
     processCall(E->getRParenLoc(), Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *D) {
     if (auto *AT = D->getReturnType()->getContainedAutoType()) {
       QualType Deduced = AT->getDeducedType();
       if (!Deduced.isNull()) {
-        addInlayHint(D->getFunctionTypeLoc().getRParenLoc(),
-                     InlayHintKind::TypeHint,
-                     "-> " + D->getReturnType().getAsString(TypeHintPolicy));
+        addTypeHint(D->getFunctionTypeLoc().getRParenLoc(), D->getReturnType(),
+                    "-> ");
       }
     }
 
     return true;
   }
 
   bool VisitVarDecl(VarDecl *D) {
-    // Do not show hints for the aggregate in a structured binding.
-    // In the future, we may show hints for the individual bindings.
-    if (isa<DecompositionDecl>(D))
+    // Do not show hints for the aggregate in a structured binding,
+    // but show hints for the individual bindings.
+    if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
+      for (auto *Binding : DD->bindings()) {
+        addTypeHint(Binding->getLocation(), Binding->getType(), ": ");
+      }
       return true;
+    }
 
     if (D->getType()->getContainedAutoType()) {
       if (!D->getType()->isDependentType()) {
         // Our current approach is to place the hint on the variable
         // and accordingly print the full type
         // (e.g. for `const auto& x = 42`, print `const int&`).
         // Alternatively, we could place the hint on the `auto`
         // (and then just print the type deduced for the `auto`).
-        addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
-                     ": " + D->getType().getAsString(TypeHintPolicy));
+        addTypeHint(D->getLocation(), D->getType(), ": ");
       }
     }
     return true;
   }
 
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
if (!FileRange)
       return;
     Results.push_back(InlayHint{
         Range{
             sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
             sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())},
         Kind, Label.str()});
   }
 
+  void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+    // Do not print useless "NULL TYPE" hint.
+    if (!T.getTypePtrOrNull())
+      return;
+
+    addInlayHint(R, InlayHintKind::TypeHint,
+                 std::string(Prefix) + T.getAsString(TypeHintPolicy));
+  }
+
   std::vector<InlayHint> &Results;
   ASTContext &AST;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
   PrintingPolicy TypeHintPolicy;
 };
 

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

void f() {
         return a + cap + init; 
       };
     }
   )cpp",
                   ExpectedHint{": (lambda)", "L"},
                   ExpectedHint{": int", "init"});
 }
 
-TEST(TypeHints, StructuredBindings) {
-  // FIXME: Not handled yet.
-  // To handle it, we could print:
-  //  - the aggregate type next to the 'auto', or
-  //  - the individual types inside the brackets
-  // The latter is probably more useful.
+// Structured bindings tests.
+// Note, we hint the individual bindings, not the aggregate.
+
+TEST(TypeHints, StructuredBindings_PublicStruct) {
   assertTypeHints(R"cpp(
+    // Struct with public fields.
     struct Point {
       int x;
       int y;
     };
     Point foo();
-    auto [x, y] = foo();
+    auto [$x[[x]], $y[[y]]] = foo();
+  )cpp",
+                  ExpectedHint{": int", "x"}, ExpectedHint{": int", "y"});
+}
+
+TEST(TypeHints, StructuredBindings_Array) {
+  assertTypeHints(R"cpp(
+    int arr[2];
+    auto [$x[[x]], $y[[y]]] = arr;
+  )cpp",
+                  ExpectedHint{": int", "x"}, ExpectedHint{": int", "y"});
+}
+
+TEST(TypeHints, StructuredBindings_TupleLike) {
+  assertTypeHints(R"cpp(
+    // Tuple-like type.
+    struct IntPair {
+      int a;
+      int b;
+    };
+    namespace std {
+      template <typename T>
+      struct tuple_size {};
+      template <>
+      struct tuple_size<IntPair> {
+        constexpr static unsigned value = 2;
+      };
+      template <unsigned I, typename T>
+      struct tuple_element {};
+      template <unsigned I>
+      struct tuple_element<I, IntPair> {
+        using type = int;
+      };
+    }
+    template <unsigned I>
+    int get(const IntPair& p) {
+      if constexpr (I == 0) {
+        return p.a;
+      } else if constexpr (I == 1) {
+        return p.b;
+      }
+    }
+    IntPair bar();
+    auto [$x[[x]], $y[[y]]] = bar();
+  )cpp",
+                  ExpectedHint{": int", "x"}, ExpectedHint{": int", "y"});
+}
+
+TEST(TypeHints, StructuredBindings_NoInitializer) {
+  assertTypeHints(R"cpp(
+    // No initializer (ill-formed).
+    // Do not show useless "NULL TYPE" hint.    
+    auto [x, y];  /*error-ok*/
   )cpp");
 }
 
 TEST(TypeHints, ReturnTypeDeduction) {
   assertTypeHints(
       R"cpp(
     auto f1(int x$ret1a[[)]];  // Hint forward declaration too
     auto f1(int x$ret1b[[)]] { return x + 1; }

 //  - auto x = llvm::dyn_cast<LongTypeName>(y);  // and similar
 //  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
 //    (For this one, perhaps we should omit type hints that start
 //     with a double underscore.)
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 No newline at end of file