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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I agree we often show the wrong amount of sugar, and with the thrust of this patch.

Some pareto-improments are possible with a single type, but we can't solve this problem satisfactorily:

- the right amount of sugar can vary for the same code pattern (`vector<T>::iterator` vs `cast_retty<x, y>::ret_type`)
- it can depend on what the reader's context is, which we can't know

However I think desugaring all the way to the canonical type, and showing it whenever the strings are not exactly equal, are too blunt as policies.
A similar situation in clang is deciding when to print "aka" in a diagnostic, and what the AKA type should be. The code controlling that is in `Desugar()` in `clang/lib/AST/ASTDiagnostic.cpp`. It's handled pretty carefully, and in a place that's likely to be well-maintained.
(It's also wrapped in functions that process a whole diagnostic to take into account multiple types that appear, which we can ignore).

I'd suggest we expose that function from ASTDiagnostic.h and use it for this purpose.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:691
   } else {
     HI.Definition = printType(QT, PP);
 
----------------
This seems like a pretty important case to handle.
A wrinkle is that this gets treated as C++ code (e.g. client-side syntax highlighted).

So we might need something like

```
int64_t
// aka
long long
```

or
```
int64_t
// aka: long long
```


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1076
+    if (CanonicalReturnType && CanonicalReturnType != ReturnType)
+      ReturnTypeStr += llvm::formatv(" (aka '{0}')", *CanonicalReturnType);
+    Output.addParagraph().appendText("→ ").appendCode(ReturnTypeStr);
----------------
I'm not sure that quoting the aka type when we don't quote the original is appropriate


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1129
+          CalleeArgInfo->CanonicalType != CalleeArgInfo->Type)
+        ConvertedTypeStr +=
+            llvm::formatv(" aka '{0}'", *CalleeArgInfo->CanonicalType);
----------------
I think printing two types here is too disruptive to the reading flow, this is a parenthetical already


================
Comment at: clang-tools-extra/clangd/Hover.h:35
+    /// CanonicalType
+    llvm::Optional<std::string> CanonicalType;
     /// None for unnamed parameters.
----------------
instead of these ad-hoc pairs, I'd suggest introducing a common struct like:

```
Optional<PrintedType> Type;

struct PrintedType {
  std::string Type;
  Optional<std::string> AKA;
};
```

that way producing function like printType and formatting functions that produce "foo (aka bar)" can more simply be shared between instances of this pattern


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522