Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include type arguments in Type Hierarchy #4217

Closed
DanTup opened this issue Oct 14, 2022 · 8 comments
Closed

Include type arguments in Type Hierarchy #4217

DanTup opened this issue Oct 14, 2022 · 8 comments
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Milestone

Comments

@DanTup
Copy link
Member

DanTup commented Oct 14, 2022

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

When browsing the hierarchy from A, we see a node for B but it doesn't include <String> (or <T>).

@DanTup DanTup added is enhancement in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server labels Oct 14, 2022
@DanTup DanTup added this to the v3.52.0 milestone Oct 14, 2022
@dupuchba
Copy link

Is there anyway I can help ? I've worked quite a lot with the analyzer API

@DanTup
Copy link
Member Author

DanTup commented Oct 19, 2022

Right now this is blocked on me finishing https://dart-review.googlesource.com/c/sdk/+/264300 (which is blocked on another change I'm working on). I'm hoping to get them done fairly soon, though.

@DanTup
Copy link
Member Author

DanTup commented Oct 20, 2022

@bwilkerson with the ability to round-trip element locations, I had a look at this. Including the type arguments for the "next level" works fine because from an element when we get its supertypes/subtypes we have the type arguments (in an InterfaceType).

However, since we then only round-trip an element reference, those type args are lost in subsequent hops:

Screenshot 2022-10-20 at 14 57 34

I guess it turns out that really I wanted to tround-trip the InterfaceType rather than the Element. Is there anything existing that might help with this, or do you have any suggestions for what might be a reasonable way? (I may still send what I've got so far in the meantime since I think this behaviour is probably still an improvement over not showing any type args at all).

@bwilkerson
Copy link

To the best of my knowledge, we don't have any support for serializing and deserializing instances of InterfaceType (outside the summary file format, but I'm skeptical that we want to try to send binary data back and forth to the client).

The easiest way I can think of off-hand would be to keep the ElementLocation of the original element, then some indication of the "path" from that to the type we're trying to access. If the list of supertypes is stable then the path could just be the indexes into the lists of children.

I'm not sure whether it's worth the effort, so I'm happy to go either way with it.

But it does seem to me that it's better to be consistent, because I think it could be misleading not to be. As a result, I'd say we should either have all or none. That is, if we don't use an InterfaceType at every level of the hierarchy then we shouldn't use it at any level (and if we're not displaying actual types, then we should probably just drop the type arguments from the display).

@DanTup
Copy link
Member Author

DanTup commented Oct 20, 2022

If the list of supertypes is stable then the path could just be the indexes into the lists of children.

Interesting idea. I don't think we can guarantee they're stable (the type hierarchy nodes are long-lived on the client) but we could compare the new type we arrive at with the element location and ensure they still match up. If they don't, we could fall back to just showing the elements type parameters (as in the example above), but I think that will be an edge case (there's a Refresh button the user can use to re-sync the type hierarchy if they make changes that are likely to have affected its navigation, so having it sometimes go awry after changes is not unexpected).

I'll give it a go and see how it works.

@dupuchba
Copy link

ultra reactive ! I've been sick all day, I am feeling better and I'll take a look tomorrow

@DanTup
Copy link
Member Author

DanTup commented Oct 25, 2022

@dupuchba hope you're feeling better!

I was able to make this work properly using Brian's suggestion above. Types are now shown (and correctly flow across supertypes):

Screenshot 2022-10-25 at 13 09 17

Your example from #4226 looks like this:

Screenshot 2022-10-25 at 14 02 20

And in the case where the type args are available:

Screenshot 2022-10-25 at 14 03 29

Change hasn't landed yet, it's up for review at https://dart-review.googlesource.com/c/sdk/+/264981. This is an SDK change so once landed, it will show up in a future Dart/Flutter SDK release (and not a VS Code extension release).

@DanTup DanTup added the relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available label Oct 25, 2022
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Oct 25, 2022
Fixes Dart-Code/Dart-Code#4217.

Change-Id: I8b8dec4ad25a9eb4a4f80dd036e8a9b61bb012d2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/264981
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
@DanTup
Copy link
Member Author

DanTup commented Oct 31, 2022

This changed landed in dart-lang/sdk@27ba8fc. It's part of the SDK so will show up in a future Dart/Flutter SDK release.

@DanTup DanTup closed this as completed Oct 31, 2022
@DanTup DanTup modified the milestones: v3.52.0, Next SDK Release Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is enhancement relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Projects
None yet
Development

No branches or pull requests

3 participants