[clangd] Type hints for variables with 'auto' type

[nridge]

test formatting fix

[nridge]

Here's a first draft of an implementation of type hints. It provides hints for variable declarations that use auto (possibly with const, &, etc.) as the type.

There are additional places in which type hints could be useful, such as:

• Functions with deduced return type (C++14)
• Structured bindings (either for the aggregate, or for the individual bindings)
• decltype(expr) in any context
• probably others I'm not thinking of

but I think it makes sense to pursue those in subsequent patches.

The patch also doesn't currently make an attempt to omit the type hint in cases where the type is obvious from the initializer. There's probably some low-hanging fruit to be had there:

• = Foo(...) or = (Foo) (...)
• = static_cast<Foo>(...)
• It might be nice to handle things like make_unique<Foo>(...) or dyn_cast<Foo>(...), though I'm not sure if there's a general heuristic that would apply to things like this as a category.

Client-side patch for experimentation: https://github.com/clangd/vscode-clangd/pull/188

[sammccall]

Sorry *again* about the delayed response, will explain off-list.

This looks great. There's a substantive question about auto &x⟦: int &⟧ = 42 vs auto⟦int⟧ &x = 42. But I don't think this should block landing this unless you want to.

clang-tools-extra/clangd/InlayHints.cpp

33

For lambda values, the overwhelmingly common case is auto X = <lambda-expr>.

A few reasons we may want to not show these at all (and so omit type hints for lambdas as a first approximation):

• the fact that it's a lambda is pretty obvious
• lambdas very often have complex introducers (captures/params) that a hint will displace/add noise to
• we may want to add type hints to captures-with-initializers, which are very much like auto-typed variables, and the fewer hints we have to cram on a line the better

(fine to consider this stuff later, just wanted to mention it)

76

FWIW I think hinting the individual bindings will be more useful and also more consistent with the other hints you have here.

81

why this check vs checking whether AT is deduced?
(thus fixing the dependent T testcase)
At first I assumed this was the usual "AutoType in AST doesn't store the deduced type" problem, but shouldn't it work properly if we're starting at the variable type?

82

This places the hint on the variable rather than the auto. And fittingly (but subtly), it prints type rather than just the placeholder type (e.g. const auto& x = 42 => const int&, not int)

This decision is worth a comment, and probably a little discussion :-)

The alternative of annotating auto has advantages:

• the deduced auto type is the most obvious question when reading the code
• that auto deduces to a particular type is the "correct" model, and may lead to better language understanding. Extreme unrealistic example: in auto x = foo(), *y = bar(); I'd strongly prefer one hint.
• the hint will often be shorter, which is an important consideration since we're reflowing column-limited text
• the annotated code more closely resembles code that does not use auto

And annotating the variable has advantages too:

• the type of the variable is the most directly important fact
• generalizes better to cases where auto is not present (lambda captures, individual structured bindings)
• i suspect simpler implementation

My intuition is to expect annotating auto to be a little nicer, mostly because it will come closer to making code read the same way whether it uses auto or not. I'm not certain though. Interested if you have strong feelings about this!

To save a round-trip: a likely conclusion is "maybe annotating auto is better, but it's not clear and/or it's a bunch of work". That's 100% fine, but we should just leave a comment whether this is something decided against vs desirable but deferred vs unsure.

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

445

consider = [b(1+1)](int a) { ... } and we can consider adding type hints to b later

487

It'd be useful to have cases showing how we handle "bulky" typenames, e.g. ns::OuterClass::Template<int> which I think we render as Template<int>.

489

not sure if you meant dynamic_cast or dyn_cast here, both are important but distinct!

489

maybe mention the other low-hanging fruit cases from the patch description too

490

The use of a __reserved name might be a good hint this is a structural type whose name is not a meaningful hint. (I guess maybe that's already what you meant!)

[nridge]

Address review comments

clang-tools-extra/clangd/InlayHints.cpp

33

There is some value in showing (lambda) as a hint: it tells you the initializer is not an IIFE (immediately-invoked function expression, i.e. auto var = [](...){ ... }(); -- note the parentheses at the end). For multi-line lambdas, this information is not otherwise present on the first line of the declaration.

Granted, how useful this is depends on the code style, i.e. whether IIFEs are common or even allowed.

I'm going to keep this as-is for now, but I'm definitely open to revise this based on usage experience.

76

Makes sense, I've updated the comment here and in the testcase to reflect this direction.

81

Not sure if I'm understanding the suggestion correctly, but if I make the condition AT->isDeduced(), I get an unwanted auto hint for var1 in TypeHints.DependentType as well (and moreover, the hint for var2 is also auto instead of the desired T).

82

Very good points here.

The argument I find most convincing for annotating the auto is the space savings, which are significant especially in the presence of const.

On the other hand, the arguments I find most convincing for the current approach are:

• the generalization to lambda captures and structured bindings, as you mention
• the familiarity to people coming from other languages like Typescript or Rust which have let var = expr; and who get a hint after the var

(Not sure if you view that second one as a valid consideration, but thought I'd mention it :))

For now, I've kept the current approach and documented it, but I'm definitely happy to reconsider based on usage experience / user feedback.

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

445

Good idea, and... turns out the existing code was already adding the hint for the init-capture!

487

Added a case like this to NoNamespaces (and renamed that test case NoQualifiers).

489

I meant llvm::dyn_cast. It's not obvious to me what sort of heuristic would allow us to recognize things like that -- if you have ideas please feel free to mention.

Syntactic casts and built-in cast operators are indeed low-hanging fruit, I added mention of this in a FIXME here.

490

I didn't really have a heuristic mind, though omitting the hint for __reserved names could be reasonable.

If we consider a case like:

std::vector<Foo> vec;
auto it2 = vec.erase(it1);

the hint is __normal_iterator<Foo *, vector<Foo>>. Ideally, I'd want just Foo *, but I guess we don't get to have that, so omitting it might be the next best thing.

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

81

and moreover, the hint for var2 is also auto instead of the desired T

I've looked into this a bit further, and it looks to me like the T isn't even stored in the AST. The type is just recorded as auto (and that's what e.g. hover shows too).

I suspect the auto is only filled in after instantiation (i.e. once the initializer expression has a concrete type).

clang-tools-extra/clangd/InlayHints.cpp

 namespace clang {
 namespace clangd {
 
 class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
 public:
   InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
       : Results(Results), AST(AST.getASTContext()),
         MainFileID(AST.getSourceManager().getMainFileID()),
-        Resolver(AST.getHeuristicResolver()) {
+        Resolver(AST.getHeuristicResolver()),
+        TypeHintPolicy(this->AST.getPrintingPolicy()) {
     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
   }
 
   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() ||
else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CalleeDecls[0]))
       Callee = FTD->getTemplatedDecl();
     if (!Callee)
       return true;
 
     processCall(E->getRParenLoc(), Callee, {E->getArgs(), E->getNumArgs()});
     return true;
   }
 
+  bool VisitVarDecl(VarDecl *D) {
+    // Structured binding - not handled yet.
+    if (isa<DecompositionDecl>(D))
+      return true;
+
+    if (auto *AT = D->getType()->getContainedAutoType()) {
+      if (!D->getType()->isDependentType()) {
+        addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
+                     ": " + D->getType().getAsString(TypeHintPolicy));
+      }
+    }
+    return true;
+  }
+
   // FIXME: Handle RecoveryExpr to try to hint some invalid calls.
 
 private:
   using NameVec = SmallVector<StringRef, 8>;
 
   // The purpose of Anchor is to deal with macros. It should be the call's
   // opening or closing parenthesis or brace. (Always using the opening would
   // make more sense but CallExpr only exposes the closing.) We heuristically
Results.push_back(InlayHint{
         Kind, Label.str()});
   }
 
   std::vector<InlayHint> &Results;
   ASTContext &AST;
   FileID MainFileID;
   StringRef MainFileBuf;
   const HeuristicResolver *Resolver;
+  PrintingPolicy TypeHintPolicy;
 };
 
 std::vector<InlayHint> inlayHints(ParsedAST &AST) {
   std::vector<InlayHint> Results;
   InlayHintVisitor Visitor(Results, AST);
   Visitor.TraverseAST(AST.getASTContext());
   return Results;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/Protocol.h

 /// A set of predefined hint kinds.
 enum class InlayHintKind {
   /// The hint corresponds to parameter information.
   /// An example of a parameter hint is a hint in this position:
   ///    func(^arg);
   /// which shows the name of the corresponding parameter.
   ParameterHint,
 
+  /// The hint corresponds to information about a deduced type.
+  /// An example of a type hint is a hint in this position:
+  ///    auto var ^ = expr;
+  /// which shows the deduced type of the variable.
+  TypeHint,
+
   /// Other ideas for hints that are not currently implemented:
   ///
-  /// * Type hints, showing deduced types.
   /// * Chaining hints, showing the types of intermediate expressions
   ///   in a chain of function calls.
   /// * Hints indicating implicit conversions or implicit constructor calls.
 };
 llvm::json::Value toJSON(InlayHintKind);
 
 /// An annotation to be displayed inline next to a range of source code.
 struct InlayHint {

clang-tools-extra/clangd/Protocol.cpp

bool fromJSON(const llvm::json::Value &Params, InlayHintsParams &R,
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("textDocument", R.textDocument);
 }
 
 llvm::json::Value toJSON(InlayHintKind K) {
   switch (K) {
   case InlayHintKind::ParameterHint:
     return "parameter";
+  case InlayHintKind::TypeHint:
+    return "type";
   }
   llvm_unreachable("Unknown clang.clangd.InlayHintKind");
 }
 
 llvm::json::Value toJSON(const InlayHint &H) {
   return llvm::json::Object{
       {"range", H.range}, {"kind", H.kind}, {"label", H.label}};
 }

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

 #include "Protocol.h"
 #include "TestTU.h"
 #include "XRefs.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
+
+std::ostream &operator<<(std::ostream &Stream, const InlayHint &Hint) {
+  return Stream << Hint.label;
+}
+
 namespace {
 
 using ::testing::UnorderedElementsAre;
 
-std::vector<InlayHint> parameterHints(ParsedAST &AST) {
+std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind) {
   std::vector<InlayHint> Result;
   for (auto &Hint : inlayHints(AST)) {
-    if (Hint.kind == InlayHintKind::ParameterHint)
+    if (Hint.kind == Kind)
       Result.push_back(Hint);
   }
   return Result;
 }
 
 struct ExpectedHint {
   std::string Label;
   std::string RangeName;
+
+  friend std::ostream &operator<<(std::ostream &Stream,
+                                  const ExpectedHint &Hint) {
+    return Stream << Hint.RangeName << ": " << Hint.Label;
+  }
 };
 
 MATCHER_P2(HintMatcher, Expected, Code, "") {
   return arg.label == Expected.Label &&
          arg.range == Code.range(Expected.RangeName);
 }
 
 template <typename... ExpectedHints>
-void assertParameterHints(llvm::StringRef AnnotatedSource,
-                          ExpectedHints... Expected) {
+void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
   Annotations Source(AnnotatedSource);
   TestTU TU = TestTU::withCode(Source.code());
-  TU.ExtraArgs.push_back("-std=c++11");
+  TU.ExtraArgs.push_back("-std=c++14");
   auto AST = TU.build();
 
-  EXPECT_THAT(parameterHints(AST),
+  EXPECT_THAT(hintsOfKind(AST, Kind),
               UnorderedElementsAre(HintMatcher(Expected, Source)...));
 }
 
+template <typename... ExpectedHints>
+void assertParameterHints(llvm::StringRef AnnotatedSource,
+                          ExpectedHints... Expected) {
+  assertHints(InlayHintKind::ParameterHint, AnnotatedSource, Expected...);
+}
+
+template <typename... ExpectedHints>
+void assertTypeHints(llvm::StringRef AnnotatedSource,
+                     ExpectedHints... Expected) {
+  assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
+}
+
 TEST(ParameterHints, Smoke) {
   assertParameterHints(R"cpp(
     void foo(int param);
     void bar() {
       foo($param[[42]]);
     }
   )cpp",
                        ExpectedHint{"param: ", "param"});
void bar() {
       // FIXME: Ideally we'd want to omit this.
       s.setTimeoutMillis($timeout_millis[[120]]);
     }
   )cpp",
                        ExpectedHint{"timeoutMillis: ", "timeoutMillis"},
                        ExpectedHint{"timeout_millis: ", "timeout_millis"});
 }
 
+TEST(TypeHints, Smoke) {
+  assertTypeHints(R"cpp(
+    auto $waldo[[waldo]] = 42;
+  )cpp",
+                  ExpectedHint{": int", "waldo"});
+}
+
+TEST(TypeHints, Decorations) {
+  assertTypeHints(R"cpp(
+    int x = 42;
+    auto* $var1[[var1]] = &x;
+    auto&& $var2[[var2]] = x;
+    const auto& $var3[[var3]] = x;
+  )cpp",
+                  ExpectedHint{": int *", "var1"},
+                  ExpectedHint{": int &", "var2"},
+                  ExpectedHint{": const int &", "var3"});
+}
+
+TEST(TypeHints, DecltypeAuto) {
+  assertTypeHints(R"cpp(
+    int x = 42;
+    int& y = x;
+    decltype(auto) $z[[z]] = y;
+  )cpp",
+                  ExpectedHint{": int &", "z"});
+}
+
+TEST(TypeHints, NoNamespaces) {
+  assertTypeHints(R"cpp(
+    namespace A {
+      namespace B {
+        struct S {};
+        S foo();
+        auto $x[[x]] = foo();
+      }
+    }
+  )cpp",
+                  ExpectedHint{": S", "x"});
+}
+
+TEST(TypeHints, Lambda) {
+  // Do not print something overly verbose like the lambda's location.
+  assertTypeHints(R"cpp(
+    auto $L[[L]] = [](int a, int b) { return a + b; };
+  )cpp",
+                  ExpectedHint{": (lambda)", "L"});
+}
+
+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
+  assertTypeHints(R"cpp(
+    struct Point {
+      int x;
+      int y;
+    };
+    Point foo();
+    auto [x, y] = foo();
+  )cpp");
+}
+
+TEST(TypeHints, ReturnTypeDeduction) {
+  // FIXME: Not handled yet.
+  // This test is currently here mostly because a naive implementation
+  // might have us print something not super helpful like the function type.
+  assertTypeHints(R"cpp(
+    auto func(int x) {
+      return x + 1;
+    }
+  )cpp");
+}
+
+TEST(TypeHint, DependentType) {
+  assertTypeHints(R"cpp(
+    template <typename T>
+    void foo(T arg) {
+      // The hint would just be "auto" and we can't do any better.
+      auto var1 = arg.method();
+      // FIXME: It would be nice to show "T" as the hint.
+      auto var2 = arg;
+    }
+  )cpp");
+}
+
+// Annoyances:
+//  - auto x = dyn_cast<LongTypeName>(y);
+//  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
+
 } // namespace
 } // namespace clangd
 } // namespace clang
 No newline at end of file