[PATCH] D102148: [clangd] Type hints for variables with 'auto' type

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:33
+    TypeHintPolicy.SuppressScope = true; // keep type names short
+    TypeHintPolicy.AnonymousTagLocations =
+        false; // do not print lambda locations
----------------
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)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:76
+  bool VisitVarDecl(VarDecl *D) {
+    // Structured binding - not handled yet.
+    if (isa<DecompositionDecl>(D))
----------------
FWIW I think hinting the individual bindings will be more useful and also more consistent with the other hints you have here.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:81
+    if (auto *AT = D->getType()->getContainedAutoType()) {
+      if (!D->getType()->isDependentType()) {
+        addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
----------------
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?


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:82
+      if (!D->getType()->isDependentType()) {
+        addInlayHint(D->getLocation(), InlayHintKind::TypeHint,
+                     ": " + D->getType().getAsString(TypeHintPolicy));
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:445
+  assertTypeHints(R"cpp(
+    auto $L[[L]] = [](int a, int b) { return a + b; };
+  )cpp",
----------------
consider `= [b(1+1)](int a) { ... }` and we can consider adding type hints to b later


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:487
+}
+
+// Annoyances:
----------------
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>`.


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:489
+// Annoyances:
+//  - auto x = dyn_cast<LongTypeName>(y);
+//  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
----------------
not sure if you meant dynamic_cast or dyn_cast here, both are important but distinct!


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:489
+// Annoyances:
+//  - auto x = dyn_cast<LongTypeName>(y);
+//  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
----------------
sammccall wrote:
> not sure if you meant dynamic_cast or dyn_cast here, both are important but distinct!
maybe mention the other low-hanging fruit cases from the patch description too


================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:490
+//  - auto x = dyn_cast<LongTypeName>(y);
+//  - stdlib algos return unwieldy __normal_iterator<X*, ...> type
+
----------------
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!)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102148/new/

https://reviews.llvm.org/D102148