[PATCH] D151785: [clangd] Desugar dependent type aliases for auto type hints

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks, this is a nice improvement.

A couple of general comments:

- We have an existing heuristic here <https://searchfox.org/llvm/rev/3a458256ee22a0e7c31529de42fa6caa263d88fe/clang-tools-extra/clangd/InlayHints.cpp#667-677>, should we move the logic into `maybeDesugar()` so it's all in one place?
- I don't think "dependent type" is the right terminology, "dependent type" means "depends on a template parameter //whose value is not yet known//", for example `vector<T>::value_type` where we don't know the value of `T`. But here we are talking about things like `vector<int>::value_type`. Maybe in place of "dependent type alias" we can say "alias for template parameter"?



================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:198
+bool isDependentOrTrivialSugaredType(QualType QT) {
+  static auto PeelQualifier = [](QualType QT) {
+    // Neither `PointerType` nor `ReferenceType` is considered as sugared
----------------
nit: it sounds like this name (`PeelQualifier`) is using "qualifier" to mean "pointer or reference", which is different from the meaning of the word in "QualType" (where it means "const or volatile")

maybe `PeelWrappers` would be a better name, since we can think of `PointerType` and `ReferenceType` as wrapping an underlying type?


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:201
+    // type. Peel it.
+    QualType Last = QT;
+    for (QT = QT->getPointeeType(); !QT.isNull();
----------------
a possible reformulation:

```
QualType Next;
while (!(Next = QT->getPointeeType()).isNull()) {
  QT = Next;
}
return QT;
```

this seems slightly cleaner to me, but I'll leave it up to you


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:207
+  };
+  for (QualType Desugared =
+           PeelQualifier(QT->getLocallyUnqualifiedSingleStepDesugaredType());
----------------
this loop can also be reformulated to avoid repeating the update step:

```
while (true) {
  QualType Desugared = Peel(...);
  if (Desugared == QT)
    break;
  if (Desugared->getAs<...>)
    return true;
  QT = Desugared;
}
```


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:215
+      return true;
+    if (Desugared->getAs<BuiltinType>())
+      return true;
----------------
The only test case that seems to rely on this `BuiltinType` heuristic is getting `int` instead of `size_type` for `array.size()`.

`size_type` doesn't seem so bad, maybe we can leave this out for now? Or do you have a more compelling motivating case for it?


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:235
+// parameter QT.
+QualType tryDesugar(ASTContext &AST, QualType QT) {
+  if (isDependentOrTrivialSugaredType(QT))
----------------
name suggestion: `maybeDesugar`

(`try` sounds like it might fail and return something that needs to be checked like `optional` or `Expected`)


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:267
     StructuredBindingPolicy = TypeHintPolicy;
     StructuredBindingPolicy.PrintCanonicalTypes = true;
   }
----------------
zyounan wrote:
> `PrintCanonicalTypes` turns on printing default template arguments, which would prevent the patch from printing, e.g., `std::basic_string<char>`, within the default length limitation.
> 
> ```
> std::map<std::string, int> Map;
> 
> for (auto &[Key, Value] : Map) // Key: basic_string<char, char_traits<char>, allocator<char>>, whose length exceeds the default threshold.
> 
> ```
> 
> Is it safe to drop it now? I believe this patch can handle the case the comment mentioned.
Unfortunately, I don't think it works in general. I tried commenting out this line and 
`TypeHints.StructuredBindings_TupleLike` failed.

(Note, this was with the `BuiltinType` heuristic removed. If we keep the `BuilinType` heuristic, a modified version of the testcase (e.g. struct members are changed from `int` to a class type) still fails.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151785