[PATCH] D114522: [clangd] Add desugared type to hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty good in terms of behavior, some impl comments but we should land this.

My main remaining concern is just that this will trigger too often when there's no confusion over types, and clutter the display.
Some common cases I'm worried about:

- std::string -> basic_string<char>. Maybe we should special-case this to hide the aka?
- `int64_t` vs `long long` et al. I have no idea what to do about this.

@kadircet hopefully has thoughts on what hover should look like



================
Comment at: clang-tools-extra/clangd/Hover.cpp:142
 
-std::string printType(QualType QT, const PrintingPolicy &PP) {
+HoverInfo::PrintedType printType(QualType QT, ASTContext &Context,
+                                 const PrintingPolicy &PP) {
----------------
nit: ASTCtx or Ctx to distinguish from clangd::Context


================
Comment at: clang-tools-extra/clangd/Hover.cpp:149
     QT = QT->getAs<DecltypeType>()->getUnderlyingType();
   std::string Result;
   llvm::raw_string_ostream OS(Result);
----------------
nit: declare a `HoverInfo::PrintedType Result` and write directly into it instead of these partial variables?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:160
   OS.flush();
-  QT.print(OS, PP);
-  return Result;
+  std::string Type = Result + QT.getAsString(PP);
+  llvm::Optional<std::string> AKA;
----------------
prefer printing into the same string rather than copying into a new one


================
Comment at: clang-tools-extra/clangd/Hover.cpp:166
+    if (ShouldAKA)
+      AKA = Result + DesugaredTy.getAsString(PP);
+  }
----------------
The e.g. "class " prefix isn't needed in the a.k.a, so no need to copy strings for that.

(and in fact I'm not sure it's possible to hit an AKA when this string is nonempty)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:189
 
-std::string printType(const TemplateTemplateParmDecl *TTP,
-                      const PrintingPolicy &PP) {
-  std::string Res;
-  llvm::raw_string_ostream OS(Res);
-  OS << "template <";
-  llvm::StringRef Sep = "";
+HoverInfo::PrintedType printType(const TemplateTemplateParmDecl *TTP,
+                                 const PrintingPolicy &PP) {
----------------
How strongly do you feel about handling this case?

This code is much more surprising than the previous version and maybe we can find a simpler way to express it, but all of this only matters if we have a template-template-paramater with a non-type-template-parameter with nontrivial sugar which obscures meaning. 

That seems pretty unlikely to ever come up, and if it does I'm not sure repeating the whole TTP is a great experience.

I'd suggest just dropping the possibility of an AKA type here until someone runs into a case where it's a real issue...


================
Comment at: clang-tools-extra/clangd/Hover.cpp:689
+  auto PType = printType(PrettyThisType, ASTCtx, PP);
+  HI.Definition = llvm::formatv("{0}", PType.Type);
+  if (PType.AKA)
----------------
HI.Definition = std::move(PType.Type)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:689
+  auto PType = printType(PrettyThisType, ASTCtx, PP);
+  HI.Definition = llvm::formatv("{0}", PType.Type);
+  if (PType.AKA)
----------------
sammccall wrote:
> HI.Definition = std::move(PType.Type)
this formatting of the definition appears twice, pull out a function? `string typeAsDefinition(PrintedType)` or so


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1093
     // - `int param2 = 5`
-    Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
+    std::string Buffer;
+    llvm::raw_string_ostream OS(Buffer);
----------------
appendCode(llvm::to_string(ReturnType))

(from ScopedPrinter.h)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1271
     OS << " = " << *P.Default;
+  if (P.Type && P.Type->AKA)
+    OS << llvm::formatv(" (aka {0})", *P.Type->AKA);
----------------
Hmm, this seems to render as:

```std::string Foo = "" (aka basic_string<char>)```

it's not *completely* clear what the aka refers to.
I don't have a better solution, though.
@kadircet any opinions here?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:166
+    if (ShouldAKA)
+      AKA = Result + DesugaredTy.getAsString(PP);
+  }
----------------
lh123 wrote:
> lh123 wrote:
> > lh123 wrote:
> > > It seems we lost `namespace qualifiers`.
> > > It always display `std::string` as `basic_string<char>`
> > Should we set `FullyQualifiedName` printing policy  for `DesugaredTy`.
> Sometimes we need to distinguish certain types with the same name based on the `fully qualified name`.
> eg.
> ```
> namespace custom {
> template <typename T> struct vector {};
> } // namespace custom
> 
> void code() {
>   custom::vector<size_t> a;
>   std::vector<size_t> b;
> }
> ```
> Currently, both `a` and `b` show `vector<unsigned long>`.
I'd lean against setting FullyQualifiedName for the AKA type. Clang doesn't for diagnostics (I see `'std::string' aka 'basic_string<char>'`).
It is more specific and adds verbosity. It should be very rare that it is the only way to find things out, e.g. in your example the *original* types would be different as they include the namespace as spelled in the code.



================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:48
          HI.Definition = "void foo()";
-         HI.ReturnType = "void";
-         HI.Type = "void ()";
+         HI.ReturnType = {"void", llvm::None};
+         HI.Type = {"void ()", llvm::None};
----------------
you could reduce the noise here by giving PrintedType a constructor from char* or so...


================
Comment at: clang/include/clang/AST/ASTDiagnostic.h:38
+  /// whenever we remove significant sugar from the type.
+  QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA);
 }  // end namespace clang
----------------
when exposing this, we should probably give it a more specific name like `desugarForDiagnostic`, so it's not confused with the various QT->desugar() or getCanonicalType() type functions.

While renaming, it should start with a lowercase d.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522