[analysis_server] `HoverInformation.containingClassDescription` usage

[kallentu]

Brian and I have been discussing adding a Declared in: line to the hovers for dot shorthands in this CL: https://dart-review.googlesource.com/c/sdk/+/431944

We noticed that HoverInformation.containingClassDescription contains the information we would want, and it seems to be populated here when we find an ExecutableElement with the given node.

However, we don't seem to use it in the handler to add to the hover content, even though we compute the information.

Is there a historical reason for this? Was it too noisy?
Would it be reasonable add it to the hover now?
@DanTup do you have any insight on this?

cc. @bwilkerson @DanTup

[DanTup]

I did some searching and the only info about this field I could find (between Dart-Code's legacy protocol implementation and LSP) was in this issue here:

Dart-Code/Dart-Code#555

There was some discussion about how containingClassDescription was being combined with other parts of the field, and I then decided to remove it (Dart-Code/Dart-Code#555 (comment) / Dart-Code/Dart-Code@92e8c16) because it was resulting in duplication. The LSP implementation was likely based on the remaining code here at the time it was written.

I've no objection to adding it back, but if we do this for all hovers, we should ensure the original concern there doesn't come back (eg. we don't end up showing something like Future.Future.then) - although if you're just adding a new line like "Declared in:" then I don't think that would be a concern anyway.

[bwilkerson]

Adding a new line would be much less confusing.

We should see what IntelliJ is doing with that field. If it isn't being used, or if it's resulting in duplicate information in all non-dot-shorthand cases, then the right solution is to only populate the field when there's value in having it be shown.

[DanTup]

In Android Studio I see it in hovers here:

Which I guess comes from https://github.com/JetBrains/intellij-plugins/blob/39cfead50e7a673a961f08590f70cef273b8a469/Dart/src/com/jetbrains/lang/dart/ide/documentation/DartDocUtil.java#L174-L179

It's on a separate line so I don't think it has any confusion like was noted in the Dart-Code issue above (which I think was because we were putting it in front of elementDescription).

Here's the tooltip for Future.then() which was used as an example in the Dart-Code issue.

So I went back and checked Dart-Code, and I think that perhaps I made a mistake in my original comment. I think when I saw "Future" duplicated, one was actually the class name and one was the return type. But joining containingClassDescription and elementDescription produced odd results because it was {class} {return type} {member name}.

So, my suggestion is that we add a new line to the LSP hovers for "Declared in" as suggested above, and we ensure it's populated for dot shorthands. It does mean an extra line in the LSP hovers, but as shown in the last image above, we're not currently showing that information (the instances of Future are the return type, not the containing class).

[kallentu]

So, my suggestion is that we add a new line to the LSP hovers for "Declared in" as suggested above, and we ensure it's populated for dot shorthands.

I'll add it as a new line in the CL that was in progress. Thanks for both your input.

[DanTup]

Thanks! I saw this today and I wonder if we should merge it into the same line as the library name because the lines are quite tall in VS Code:

For example we could show (although I'm not sure about italics versus code):

Declared in Foo in package:foo/foo.dart

And if we don't show the type, we could keep the prefix (which we don't have today, so the library URI just looks a bit random):

Declared in package:foo/foo.dart

(I'm happy to change this if we agree, but I'm interested in opinions first)

[bwilkerson]

I like the more compact form (including removing the colon), and I like that the file path has an explanation of why it's there.

We should minimally remove the colon after 'Type' as well, but I have to wonder whether that line actually tells the user anything interesting that isn't already on the line above. I haven't yet been able to find a case where it's different, other than replacing the name of a function/method with the word Function, which doesn't seem useful to me. I'd be tempted to just drop that line completely.

[DanTup]

@bwilkerson the types can be different if it's been narrowed by a condition:

[bwilkerson]

Ah, thanks. We should probably just leave it for now, but I do have to wonder whether there might be a better way (only displaying that line when the promoted type is different, using a more descriptive title than 'Type', inlining the information into the first line, etc.). But doing any of those is a big enough change that I wouldn't want to bundle the changes together.

[kallentu]

Declared in Foo in package:foo/foo.dart

Declared in package:foo/foo.dart

That's a great idea. I like that a lot better and it would make the file path make more sense, like Brian said.

[DanTup]

Any opinions on code versus underscore for the library name? I thought italics would look odd, but seeing them side-by-side I think I might prefer it (it feels less like "code" than the symbol name).

I'll prepare the CL keeping with the current italics, but I'm completely on the fence and happy to change.

[kallentu]

At first, I was thinking code would make more sense for the library name, but we're not changing what it looks like if we don't have the containing class part? Then the library name would be in italics right?

We can keep it consistent with italics?

No strong opinions though.

[DanTup]

but we're not changing what it looks like if we don't have the containing class part?

The code is shared, so if we change to use backticks, it would apply both to "Declared in X in package:y/y.dart" and just "Declared in package:y/y.dart" without the class. The question was more generally whether to use backticks or underscores for the library name for the "Declared in" line.

The changes are here: https://dart-review.googlesource.com/c/sdk/+/433481