Implement Type Hierarchy for LSP

[zhengger]

How can I revover this function? Thanks!
My IDE:
Name: Visual Studio Code
Version: 1.45.1
Commit: 5763d909d5f12fe19f215cbfdd29a91c0fa9208a
Date: 2020-05-14T08:33:47.663Z
Electron: 7.2.4
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Darwin x64 18.7.0

My Flutter and Dart SDK :3.11.0

[DanTup]

It seems like the Dart extension may not have activated correctly (there should be a Dart or Flutter version number in the status bar when it has).

Can you check Help -> Toggle Developer Tools and see if there are any errors listed about the extension failing to activate?

Thanks!

[zhengger]

@DanTup
Hi Dan,
Thank you.
I think the extension should have been activated cause all works well except the "showTypeHierarch" function.
How can I check the errors in the Developer Tools. Here are two screenshot I took just:

[DanTup]

Ah, ok - in the original screenshot it didn't show Flutter/Dart versions in the status bar, but your latest one does. Are you using the LSP preview mode ("dart.previewLsp": true in your VS Code user settings)?

[zhengger]

Ah, ok - in the original screenshot it didn't show Flutter/Dart versions in the status bar, but your latest one does. Are you using the LSP preview mode ("dart.previewLsp": true in your VS Code user settings)?

Hi Dan, @DanTup
I have tried both cases. The 'dart.showTypeHierarchy' error happened always.
Case 1: "dart.previewLsp": false:

Case 2: "dart.previewLsp": true:

[zhengger]

Update:
After I removed all costumed entries related with Dart/Flutter in settings.json file of VS CODE, the Hierarchy function works now. So I'll close this post now.
Thanks Dan.

[DanTup]

I think this is a bug that it doesn't work for LSP (eg. when you have "dart.previewLsp": true in settings). Since the goal is to move everything to LSP, this will need fixing.

[DanTup]

@bwilkerson since I can't add screenshots to Gerrit, here's how this looks with https://dart-review.googlesource.com/c/sdk/+/264002.

I may look at adding the type arguments to the class names on the nodes (I think it'd seem more natural to see List<T> instead of List even if we don't include the concrete type) but I'll do that in another change (I may need to change how we locate the target when it comes back from the client, since we currently use the name).

[bwilkerson]

I like the way that looks.

Actual type arguments might be informative. We should be able to just drop everything after the first < in order to get back the name to use to locate the target.

[DanTup]

Actual type arguments might be informative.

By "actual" do you mean those actually used in the declaration of the "parent" node (eg. List<String> and not just List<T>)? I originally thought that might be odd (because the node represents the class and not some concrete type), although now it occurs to me that the relationship label means the node isn't "generic" anymore and maybe it does make sense. I'll take a look.

We should be able to just drop everything after the first < in order to get back the name to use to locate the target.

Yeah, that could work - although there are a few places we've talked about "element references" in the past and I wonder if they're a better solution here. I have a somewhat-abandoned CL that made it easier to get to/from elements<->strings and I wonder if it's worth extracting and finishing that to use in places like this (and inlay hints tooltips that I need to work on).

[bwilkerson]

By "actual" I meant the arguments from the type. For example, in

class A<S> {}
class B extends A<int> {}

the actual type arguments for A are <int>. But for

class A<S> {}
class B<T> extends A<T> {}

the actual type arguments for A are <T> (the type parameter from B).

I have a somewhat-abandoned CL that made it easier to get to/from elements<->strings ...

I don't remember that CL exactly; does it make use of ElementLocation?

[DanTup]

I don't remember that CL exactly; does it make use of ElementLocation?

You may not have seen it - it was related to trying to fix potential inconsistencies in diagnostic context messages (it requires some input, though I don't know if the benefit it provides is worth the apparent complexity it's adding) - https://dart-review.googlesource.com/c/sdk/+/240920

It uses the Reference class that is used in summaries. It looks like ElementLocation may be similar though, so I'm not sure if one would fit better than the other. There are a few places (this one, possible Call Hierarchy, type Inlay Hints) where we show something related to a type/member to the user and later want to get back to that element and be forgiving if files changed (and name seems like the thing the user would expect us to be using to locate it).

[bwilkerson]

If you're storing the Reference in the summary, then that's obviously the right class to use.

But for this purpose ElementLocation has the advantage that it's a public class. On the other hand, I can't find anywhere to convert an ElementLocation back into an element, so it looks like the support is incomplete at this point.

[DanTup]

On the other hand, I can't find anywhere to convert an ElementLocation back into an element, so it looks like the support is incomplete at this point.

I'll have a look and see if this is something I can add so I can use this here and in a few other places, thanks!

I've opened #4217 to track including the type arguments and will close this one as the basic functionality is implemented (dart-lang/sdk@6f8d1e4 - shipping in the SDK rather than Dart-Code).