Comment references support for VS Code documentation popup

[Sominemo]

Is your feature request related to a problem? Please describe.
It's frustrating to manually search for an identifier in code when it's used in an entities' documentation as a comment reference.

Describe the solution you'd like
Documentation popups provided by Dart Code need to add support for active links when comment references are used.

Describe alternatives you've considered
Since comment references are supported in the editing window, I usually jump to the doc comment itself, and then click the comment reference from there.

Additional context
As Effective Dart states, you can use square brackets to refer to fields and methods, and this annotation is used by dartdoc to generate active links in documentation.

The references are already being highlighted correctly in the code itself with working "Go to Definition" etc. options, but not in the VS Code's documentation popup, these display plain text in square brackets instead.

[DanTup]

This will require the markdown parser can provide is offsets/lengths for the references in a block:

dart-lang/markdown#369

[sadespresso]

Do we have any update on this 😁

[DanTup]

No updates yet. This still requires changes to the Markdown package first.

[Number-3434]

Any updates yet?

@srawlins This depends on Provide offest/lengths for nodes. Could this be fixed easily (and quickly)?

[srawlins]

Could this be fixed easily (and quickly)?

No it would be a pretty big effort. Right now the markdown package does not even parse Markdown text into a syntax tree which represents the Markdown text, dart-lang/markdown#364. It goes straight into an AST for the rendered HTML. :(

[DanTup]

This came up again in #4975 and I was thinking a bit more about it. I wonder whether offsets from the markdown parser is really what we need for these. It seems we already have the comment references in the AST (which we use for Semantic Tokens and Go-to-Definition), but for hovers we only have access to the Element model. I'm not sure that re-parsing the markdown content to get these references is the right approach.

@bwilkerson since in the hover we only have a single Element to look at, I wonder if we could fetch the AST for the unit that contains it, locate the dartdoc node for that element, and then visit it to find the comment references. It does mean fetching a resolved AST for a file that might not be open (and therefore not cached) in every hover request though and I don't know if that's great.

[bwilkerson]

If I'm understanding the request correctly, we want to insert a link in place of a comment reference, which will require not only knowing the range of text to replace but also the target of the link. I can't think of any existing way other than getting the resolved AST to get the target.

We could try adding this support to see what the performance implications are, but probably only if it could be done quickly (because I don't want to expend too much effort if we might not be able to use it. If it adds too large a delay in getting hover information there might be some optimizations we could consider (such as storing more information in the element model, or having a special "resolve this comment only" mode in the resolver), but those are both likely to be fairly large projects.

... since in the hover we only have a single Element to look at ...

It's possible that the doc comment for a given element might not be in the compilation unit in which the element is declared. For members that override an inherited member we allow the docs to be copied from an overridden member, which might cause us to need to look in multiple compilation units in order to find it. Adding a field to Element to record the file / element / etc. where the doc comment was found would ensure a single resolution request, but that's again more work.

[Number-3434]

@bwilkerson To clarify, the idea is to parse all possible references in the dartdoc comment with blind faith (i.e. without checking if they are valid in markdown, only checking if they can be linked) that they will be used, then add the references to the bottom.

Markdown supports reference-style-links.

To get familiar with it, copy the following:

/// [int] [value]
///
/// [int]: https://api.flutter.dev/flutter/dart-core/int-class.html
void foo(int value) {  }

...then, every instance of valid [int]s will then be rendered with the link.

The idea is to make use of the fact that a comment reference has the same syntax as a reference-style-link.

The method described in #4975 means you won't replace anything in the dartdoc comment, you are just appending the references to the end.

VSCode's markdown parser will then parse the results, and do all the reference linking automatically.

This is designed specifically to bypass the need for knowing the exact range of text.

For example, given the following:

/// [foo]
/// ```dart
/// /// [int]
/// ```
void foo() {  }

...the original dartdoc comment will be left unchanged, but foo and int would be recognized as valid references, and appended to the end of the comment.

int would not be rendered as a link, since it is not valid.

[DanTup]

@Number-3434

Actually, my idea was as Brian understood. The problem here isn't specifically knowing the range of the reference, but rather knowing what references there are so we know what target links we need to include (whether inline, or appended).

For example if the markdown contains [MyClass] then we'd need to a) know that it includes this reference and b) know where the declaration of MyClass is. That information will generally come together, so to have the target we'll probably also have the offset/length of the reference.

My comments above about this being complicated by not having the offset/length was a bit misguided. For some of the other requests (like adding semantic tokens for code blocks inside the comments) we probably do need to re-parse the markdown and know where those ranges are, but for these references we should instead use the existing data that's already been parsed.

@bwilkerson

It's possible that the doc comment for a given element might not be in the compilation unit in which the element is declared. For members that override an inherited member we allow the docs to be copied from an overridden member, which might cause us to need to look in multiple compilation units in order to find it.

You're right - although I think this is already handled because we already do this traversal to find the string documentation today (we compute the documentedElement by looking through the related elements to find one with non-null docs). So I think once we've done that, we could use the final element as the one to fetch a resolved AST for.

Something to try out I guess, although I don't know if it's something I'll get to in the immediate future.

[bwilkerson]

... we could use the final element as the one to fetch a resolved AST for.

Good point. That simplifies the performance question by making it an O(1) operation.

Something to try out I guess ...

I think a good first step would be to compute the ResolvedUnitResult for the unit containing the doc comment, without doing anything with the result, and see what kind of performance impact that would have. Not sure how much effort that would require, and definitely not our highest priority task.

[Number-3434]

Just out of curiosity, once you've found the [references] in the dartdoc, how hard is it to get the actual reference path (e.g. ~/path-to-sdk/path-to-int-class#L10)?

[DanTup]

@Number-3434 that probably depends on how we found them. If we got them via the AST, we should already have a route to that info. If we discover them by parsing the markdown, we'd need to resolve the identifiers based on the scope. I don't know how difficult this would be (because I'm not familiar with the code that normally does this when parsing the docs), but it feels like duplicating work that's already done (and may be something else place to update if a feature like dart-lang/sdk#50702 is implemented).

I think it would make more sense to try out the ideas above before looking at other ways to do this.

[Number-3434]

So to summarise, the problem is that you're possibly concerned about performance if the reference is in a file you haven't already got an AST for?

Btw, roughly how long does it take to create a AST?

Maybe there could be a temporary setting for this (e.g. dart.commentReferences), which defaults to "off", and has a value of "openFiles", which will make comment references be parsed only if there is already and AST for it?

[bwilkerson]

... it feels like duplicating work that's already done (and may be something else place to update if a feature like dart-lang/sdk#50702 is implemented).

It would definitely duplicate existing code. I don't know how hard it would be to rewrite the code to be sharable, but I agree that it's better to try the more straightforward implementation first and consider implementations like this if the simple path isn't performant enough.

If you're interested, we are actively implementing the doc import proposal. Unless we find some technical issues that prevent it I'm fairly confident that that's going to happen. Which is a good thing because then the analyzer and dartdoc will have the same semantics for resolving these references, allowing the analyzer to have better support for dartdoc in the future.

Btw, roughly how long does it take to create a AST?

I don't have those numbers at hand (and don't remember), but it is, of course, dependent on the length of the source file being parsed and/or resolved.

Maybe there could be a temporary setting for this ...

There could be, but my guess is that most users want hovers primarily for code that's outside the libraries currently in open editors, so I'm not sure how much value that would add. I might be wrong about that, of course.