[PATCH] D104617: [clangd] Type hints for structured bindings
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa15adbcddd07: [clangd] Type hints for structured bindings
(authored by nridge).
Changed prior to commit:
https://reviews.llvm.org/D104617?vs=356418=356419#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104617/new/
https://reviews.llvm.org/D104617
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -461,19 +461,70 @@
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
+ struct tuple_size {};
+ template <>
+ struct tuple_size {
+constexpr static unsigned value = 2;
+ };
+ template
+ struct tuple_element {};
+ template
+ struct tuple_element {
+using type = int;
+ };
+}
+template
+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");
}
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -32,6 +32,14 @@
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::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::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) {
@@ -76,9 +84,8 @@
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(),
+"-> ");
}
}
@@ -86,10 +93,14 @@
}
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(D))
+// Do not show hints for the aggregate in a structured binding,
+// but show hints for the individual bindings.
+if (auto *DD = dyn_cast(D)) {
+ for (auto *Binding : DD->bindings()) {
+addTypeHint(Binding->getLocation(), Binding->getType(), ": ");
+ }
return true;
+}
if (D->getType()->getContainedAutoType()) {
if (!D->getType()->isDependentType()) {
@@ -98,8 +109,7 @@
// (e.g. for `const auto& x = 42`, print `const int&`).
// Alternatively, we could place the hint on
[PATCH] D104617: [clangd] Type hints for structured bindings
nridge updated this revision to Diff 356418.
nridge marked 2 inline comments as done.
nridge added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104617/new/
https://reviews.llvm.org/D104617
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -461,19 +461,70 @@
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
+ struct tuple_size {};
+ template <>
+ struct tuple_size {
+constexpr static unsigned value = 2;
+ };
+ template
+ struct tuple_element {};
+ template
+ struct tuple_element {
+using type = int;
+ };
+}
+template
+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");
}
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -32,6 +32,14 @@
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::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::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) {
@@ -76,9 +84,8 @@
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(),
+"-> ");
}
}
@@ -86,10 +93,14 @@
}
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(D))
+// Do not show hints for the aggregate in a structured binding,
+// but show hints for the individual bindings.
+if (auto *DD = dyn_cast(D)) {
+ for (auto *Binding : DD->bindings()) {
+addTypeHint(Binding->getLocation(), Binding->getType(), ": ");
+ }
return true;
+}
if (auto *AT = D->getType()->getContainedAutoType()) {
if (!D->getType()->isDependentType()) {
@@ -98,8 +109,7 @@
// (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,
-
[PATCH] D104617: [clangd] Type hints for structured bindings
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Cool!
Comment at: clang-tools-extra/clangd/InlayHints.cpp:40
+// types would be "tuple_element::type".
+TypeHintPolicy.PrintCanonicalTypes = true;
}
nridge wrote:
> 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
> struct SomeLongType {};
>
> using ShortType = SomeLongType;
>
> 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](https://searchfox.org/llvm/rev/e497b12a69604b6d691312a30f6b86da4f18f7f8/clang/include/clang/AST/Type.h#4953)),
> so setting `PrintCanonicalTypes=true` doesn't actually regress cases like
> this (they were already printing the canonical type).
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.
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:466
+ // Hint individual bindings, not the aggregate.
assertTypeHints(R"cpp(
+// 1. Struct with public fields.
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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104617/new/
https://reviews.llvm.org/D104617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D104617: [clangd] Type hints for structured bindings
nridge added inline comments.
Comment at: clang-tools-extra/clangd/InlayHints.cpp:40
+// types would be "tuple_element::type".
+TypeHintPolicy.PrintCanonicalTypes = true;
}
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
struct SomeLongType {};
using ShortType = SomeLongType;
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](https://searchfox.org/llvm/rev/e497b12a69604b6d691312a30f6b86da4f18f7f8/clang/include/clang/AST/Type.h#4953)),
so setting `PrintCanonicalTypes=true` doesn't actually regress cases like this
(they were already printing the canonical type).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104617/new/
https://reviews.llvm.org/D104617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D104617: [clangd] Type hints for structured bindings
nridge created this revision.
nridge added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
nridge requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Hints are shown for the individual bindings, not the aggregate.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D104617
Files:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Index: clang-tools-extra/clangd/unittests/InlayHintTests.cpp
===
--- clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -462,19 +462,57 @@
}
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.
+ // Hint individual bindings, not the aggregate.
assertTypeHints(R"cpp(
+// 1. Struct with public fields.
struct Point {
int x;
int y;
};
Point foo();
-auto [x, y] = foo();
- )cpp");
+auto [$x1[[x1]], $y1[[y1]]] = foo();
+
+// 2. Array
+int arr[2];
+auto [$x2[[x2]], $y2[[y2]]] = arr;
+
+// 3. Tuple-like type.
+struct IntPair {
+ int a;
+ int b;
+};
+namespace std {
+ template
+ struct tuple_size {};
+ template <>
+ struct tuple_size {
+constexpr static unsigned value = 2;
+ };
+ template
+ struct tuple_element {};
+ template
+ struct tuple_element {
+using type = int;
+ };
+}
+template
+int get(const IntPair& p) {
+ if constexpr (I == 0) {
+return p.a;
+ } else if constexpr (I == 1) {
+return p.b;
+ }
+}
+IntPair bar();
+auto [$x3[[x3]], $y3[[y3]]] = bar();
+
+// 4. No initializer (ill-formed).
+// Do not show useless "NULL TYPE" hint.
+auto [x4, y4]; /*error-ok*/
+ )cpp",
+ ExpectedHint{": int", "x1"}, ExpectedHint{": int", "y1"},
+ ExpectedHint{": int", "x2"}, ExpectedHint{": int", "y2"},
+ ExpectedHint{": int", "x3"}, ExpectedHint{": int", "y3"});
}
TEST(TypeHints, ReturnTypeDeduction) {
Index: clang-tools-extra/clangd/InlayHints.cpp
===
--- clang-tools-extra/clangd/InlayHints.cpp
+++ clang-tools-extra/clangd/InlayHints.cpp
@@ -32,6 +32,12 @@
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::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::type".
+TypeHintPolicy.PrintCanonicalTypes = true;
}
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
@@ -76,9 +82,8 @@
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(),
+"-> ");
}
}
@@ -86,10 +91,14 @@
}
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(D))
+// Do not show hints for the aggregate in a structured binding,
+// but show hints for the individual bindings.
+if (auto *DD = dyn_cast(D)) {
+ for (auto *Binding : DD->bindings()) {
+addTypeHint(Binding->getLocation(), Binding->getType(), ": ");
+ }
return true;
+}
if (auto *AT = D->getType()->getContainedAutoType()) {
if (!D->getType()->isDependentType()) {
@@ -98,8 +107,7 @@
// (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;
@@ -311,6 +319,15 @@
Kind, Label.str()});
}
+ void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
+// Do not print useless "NULL TYPE" hint.
