This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add desugared type to hover
ClosedPublic

Authored by lh123 on Nov 24 2021, 3:21 AM.

Details

Summary

Add desugared type to hover when the desugared type and the pretty-printed type are different.

c++
template<typename T>
struct TestHover {
  using Type = T;
};

int main() {
  TestHover<int>::Type a;
}

variable a

Type: TestHover<int>::Type (aka 'int')

Diff Detail

Event Timeline

lh123 created this revision.Nov 24 2021, 3:21 AM
lh123 requested review of this revision.Nov 24 2021, 3:21 AM
lh123 updated this revision to Diff 389452.Nov 24 2021, 3:32 AM

add testcase

lh123 edited the summary of this revision. (Show Details)Nov 24 2021, 3:33 AM
lh123 updated this revision to Diff 389638.Nov 24 2021, 7:20 PM

format code and add testcase for template.

lh123 added a comment.Nov 24 2021, 7:27 PM

Related discussions: D72498

lh123 edited the summary of this revision. (Show Details)Nov 24 2021, 7:28 PM

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.

clang-tools-extra/clangd/Hover.cpp
692

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
1077

I'm not sure that quoting the aka type when we don't quote the original is appropriate

1132

I think printing two types here is too disruptive to the reading flow, this is a parenthetical already

clang-tools-extra/clangd/Hover.h
35

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

lh123 updated this revision to Diff 389909.Nov 25 2021, 9:51 PM

address comment.

Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2021, 9:51 PM
lh123 marked 4 inline comments as done.Nov 25 2021, 9:52 PM
lh123 updated this revision to Diff 389911.Nov 25 2021, 10:05 PM

fix some bug.

lh123 updated this revision to Diff 389915.Nov 25 2021, 10:25 PM
lh123 retitled this revision from [clangd] Add canonical type to hover to [clangd] Add desugared type to hover.
lh123 edited the summary of this revision. (Show Details)
  1. rename CanType to DesugaredTy.
  2. fix some bug in compare PrintedType.
lh123 updated this revision to Diff 389921.Nov 25 2021, 11:24 PM

handle TypeAliasTemplateDecl.

lh123 updated this revision to Diff 389924.EditedNov 25 2021, 11:29 PM

rename TTD to TAT.

lh123 updated this revision to Diff 389931.Nov 26 2021, 12:28 AM

Move some desugared type to Definition.

lh123 added inline comments.Nov 26 2021, 12:31 AM
clang-tools-extra/clangd/Hover.cpp
692

It seems we can't do this, vscode always display

int64_t
// aka: long long

as

int64_t
    // aka: long long

So I change it to int64_t // aka: long long

lh123 added inline comments.Nov 26 2021, 1:13 AM
clang-tools-extra/clangd/Hover.cpp
169

It seems we lost namespace qualifiers.
It always display std::string as basic_string<char>

lh123 added inline comments.Nov 26 2021, 1:18 AM
clang-tools-extra/clangd/Hover.cpp
169

Should we set FullyQualifiedName printing policy for DesugaredTy.

lh123 added inline comments.Nov 26 2021, 1:25 AM
clang-tools-extra/clangd/Hover.cpp
169

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>.

lh123 updated this revision to Diff 389968.Nov 26 2021, 2:50 AM
lh123 changed the repository for this revision from rZORG LLVM Github Zorg to rG LLVM Github Monorepo.

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

clang-tools-extra/clangd/Hover.cpp
141–142

nit: ASTCtx or Ctx to distinguish from clangd::Context

148

nit: declare a HoverInfo::PrintedType Result and write directly into it instead of these partial variables?

163

prefer printing into the same string rather than copying into a new one

169

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.

169

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)

186

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...

676

HI.Definition = std::move(PType.Type)

676

this formatting of the definition appears twice, pull out a function? string typeAsDefinition(PrintedType) or so

1075–1079

appendCode(llvm::to_string(ReturnType))

(from ScopedPrinter.h)

1254

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?

clang-tools-extra/clangd/unittests/HoverTests.cpp
50–51

you could reduce the noise here by giving PrintedType a constructor from char* or so...

clang/include/clang/AST/ASTDiagnostic.h
38 ↗(On Diff #389968)

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.

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.

As discussed offline I share these concerns, and it's not the first time we're facing them. I am afraid that this might be disturbing for a good fraction of users if we don't polish it around common types.
A couple of suggestions I have (they don't necessarily need to land as part of this patch, but I think we should land some damage control mechanism before next release):

  • Make AKA-printing completely optional, to give users a way out (while probably starting with off by default and experimenting for a while to see the most annoying cases and polishing them as needed)
  • Have a list of user-configurable FQNs that AKA printing would be disabled for (and provide a good set of defaults inside clangd for STL)
clang-tools-extra/clangd/Hover.cpp
1254

i think it's attached to the type of the parameter, not it's name nor the initializer-expr. so I think we should move this closer to the type (i.e. std::string (aka basic_sting<char>) Foo = "", basically llvm::toString(P.Type) ?)

lh123 updated this revision to Diff 390149.Nov 27 2021, 2:58 AM
lh123 marked 13 inline comments as done.

address comment.

lh123 updated this revision to Diff 390153.Nov 27 2021, 4:08 AM

format code.

lh123 updated this revision to Diff 390154.Nov 27 2021, 4:09 AM

format again.

nridge added a subscriber: nridge.Nov 29 2021, 1:25 PM
lh123 updated this revision to Diff 391545.Dec 2 2021, 10:08 PM

rebase to head.

sammccall accepted this revision.Dec 7 2021, 3:49 AM

Thank you! (& Sorry for the delay, have been sick)
LG with the understanding we may have to iterate on behavior/configurability.

As noted below I'd suggest either disabling aka for params, or reverting to the previous version, but I don't feel strongly.

clang-tools-extra/clangd/Hover.cpp
1248

this can just be << *P.Type, skipping the temporary string

1254

i think it's attached to the type of the parameter

This is logically correct but I think it's harder to read. This puts text in the middle of code, in a way that doesn't seem obvious to me: parens mean several things in C++ and it may be hard to recognize this means none of them.

Worst case is we have function types: word(*)() (aka long(*)()) x = nullptr

It also disrupts the reading flow in the case where the aka is *not* needed for understanding.
I think overall the previous version was better, though not great.

I'm tempted to say we should scope down this patch further until we have a better feel for how it behaves, i.e. exclude param types from aka for now. Param types are less obviously useful to disambiguate than result types. (e.g. because in most cases you can hover over the input expression).

This revision is now accepted and ready to land.Dec 7 2021, 3:49 AM

Have a list of user-configurable FQNs that AKA printing would be disabled for (and provide a good set of defaults inside clangd for STL)

BTW this seems hard to implement well without invasive changes to the desugar logic.

Nontrivial case: the list is {std::string}, the type is std::string*.

Pathological case: the list is {std::string}, and the type is pair<std::string*, size_t>. Desugar yields pair<basic_string<char>*, long long>, desired output is pair<std::string*, long long>.
desugarForDiagnostic needs to exclude the first TP from desugaring based on it matching a string, but we don't stringify all the intermediate types.

lh123 added inline comments.Dec 7 2021, 8:49 AM
clang-tools-extra/clangd/Hover.cpp
1254

I'm tempted to say we should scope down this patch further until we have a better feel for how it behaves, i.e. exclude param types from aka for now. Param types are less obviously useful to disambiguate than result types. (e.g. because in most cases you can hover over the input expression).

I‘d like to keep the a.k.a type for parameter. because:

  1. sometime we pass literal (or null pointer) as parameter, but clang doesn't support hover on literal. eg:
using mint = int;
void foo(mint *);
void code() {
    foo(nullptr); // hover over foo, we can get 'mint * (aka int *)'
}
  1. It may be useful when making function calls, although it does not work well when the function is overloaded.(add a.k.a type to signature help?) eg:
using mint = int;
void foo(mint *);
void code() {
    foo(); // hover on foo, we can get 'mint * (aka int *)'
}

I think overall the previous version was better, though not great.

Agree with that, it seems that putting a.k.a in the middle of the code makes the hover look bad based on my recent use

lh123 added inline comments.Dec 7 2021, 9:51 AM
clang-tools-extra/clangd/Hover.cpp
1254

It is also useful when pass function calling as parameters.

struct Test {
  Test(int);
};
using Alias = Test;

int foo();
void bar(Alias);

void code() {
    bar(foo()); // Hover over bar, we can know that 'int' is implicitly converted to 'Test'
}
This revision was automatically updated to reflect the committed changes.
lh123 marked 2 inline comments as done.