Incompatibility with `@macro`/`@template` and `@docImport`

srawlins commented 6 months ago

@kallentu found this issue.

Background

Dartdoc currently supports a snippets system (@macro/@template) where a block of text from one doc comment can be re-used in other doc comments:

foo.dart:

import 'package:flutter/widget.dart';

/// Doc comment for Foo.
///
/// {@template foo_package.foo}
/// Shared text. This class is used a lot for a [Widget].
/// {@end-template}
class Foo {}

bar.dart:

/// Doc comment for Bar.
///
/// {@macro foo_package.foo}
class Bar {}

When dartdoc generates the docs for this code, it builds a database of all of the @template names and contents, and then replaces each @macro with the corresponding @template text. So for the Bar class's documentation, dartdoc builds a String that looks like:

Doc comment for Bar.

Shared Text. This class is used a lot for a [Widget].

After this textual replacement, we resolve comment references. In this step, dartdoc determines what code element is being referenced by [Widget].

Resolution tomorrow

Before we get into how dartdoc performs resolution today, I'll mention that the goal is that dartdoc will always go to the analyzer for resolution. In taking that step, we hit this problem: When resolving comment references on class Bar, we encounter [Widget], and have no idea what it could be referring to. There is no Widget in the local file, nor in the import namespace. Even if we went to the "doc import" namespace (including elements imported via @docImport), we don't find a Widget.

Resolution today

So how do macro/template snippets work today?

Today, dartdoc doesn't even ask analyzer what the comment references are. Dartdoc looks at each doc comment (after macro/template replacement), and parses it manually for square brackets. Dartdoc has its own determination of what a comment reference even is. Because of this custom resolution, dartdoc specifically allows for comment references in macro/template-replaced text.

Secondly, today we have the "universal scope reference" system, which allows far-flung elements to be referenced in a doc comment. If Widget is not found by analyzer's Scope APIs, then dartdoc seeks it out in non-imported code.

A little deeper

At first glance, it seems like the second issue above could be solved with doc imports. We can add a doc import for 'package:flutter/widgets.dart'.

The first issue is trickier though: If dartdoc stops parsing comments, looking for comment references, and just asks the analyzer, then all comment references in macro/template-replaced code will be missed. Comment references are resolved by the analyzer, made available on Comment.references. This list of references will not include macro/template-replaced code.

srawlins commented 6 months ago

One possible solution would be to move @macro/@template-replacement into the analyzer. The feature would work something like this:

During parsing, when the analyzer sees a {@macro foo} doc directive in a doc comment, it will carry out a replacement action that will find (precisely, via the import namespace) the corresponding @template text is, replace the @macro text, and carry on parsing this comment. Any references in the template will be included in the Comment object's references field.

Presumably that macro/template-replaced text will be considered as the element's doc comment, even in the IDE.

Design decisions:

It seems dangerous and surprising to resolve references after doing the replacement. Should they be resolved instead within the @template, and then the referenced elements are carried over to the comment with @macro?
Similarly, should nested macros (these exist in Flutter, IIUC) be replaced in the template before all being replaced in the comment with @macro?
When carrying out these replacements, the Comment AstNode instance is no longer just representing what was parsed on Comment. What would the offsets mean, etc.? Should we instead create a linked graph of these replacements, such that one Comment points to another Comment (or more likely, step away from AstNodes and have one CommentData-ish instance point to another)?

goderbauer commented 6 months ago

Should they be resolved instead within the @template, and then the referenced elements are carried over to the comment with @macro?

This would have been my naive expectation of how references in templates/macros are resolved.

goderbauer commented 6 months ago

One possible solution would be to move @macro/@template-replacement into the analyzer.

Presumably that macro/template-replaced text will be considered as the element's doc comment, even in the IDE.

Will this enable us to improve the doc experience in the IDE by virtually inserting the @template text into all @macro locations that reference it? That would be awesome!

srawlins commented 6 months ago

Will this enable us to improve the doc experience in the IDE by virtually inserting the @template text into all @macro locations that reference it? That would be awesome!

Yes I do think this is generally desirable, as part of moving dartdoc features into analyzer.

bwilkerson commented 6 months ago

Should they be resolved instead within the @template, and then the referenced elements are carried over to the comment with @macro?

I would also have expected that to be the approach. The imports in the library containing the template should be the ones used to resolve the identifier.

Will this enable us to improve the doc experience in the IDE by virtually inserting the @template text into all @macro locations that reference it?

We already do that, at least for hovers. (I don't think we currently do for code completion because of performance issues, but this change might enable that to work too.)

I don't think we'd be able to "replace" the @macro reference in the source code where it's being referenced, nor do I think that we'd want to, but anywhere else that the comment is displayed, yes, we ought to be able to do the substitution before displaying it.

dart-lang / dartdoc