dart-lang / dartdoc

API documentation tool for Dart.
https://pub.dev/packages/dartdoc
BSD 3-Clause "New" or "Revised" License
463 stars 117 forks source link

[doc_imports] Differences with analyzer and Dartdoc's comment resolver #3762

Open kallentu opened 1 month ago

kallentu commented 1 month ago

General Problem

When looking for a reference with the same name as other references in scope, which should the comment reference resolver pick?

For example, this can happen when a class member doc comment references a name that is both a field and a parameter.

Resolution today

Dartdoc has its own algorithm to search through children nodes and parent nodes (universal reference resolving).

This algorithm can be seen in CommentReferable.referenceBy.

Resolution tomorrow

The analyzer has a different algorithm with Scope that makes for (IMO) better reference resolving, but this does mean that there are a couple of differences and changes.

I'll list out a bunch of examples where this happens with output from some dummy warnings I've generated.

Case 1: Analyzer prefers parameters over fields.

This should be an acceptable change that the analyzer prefers the Parameter. If we want the Field, we should have doc writers write [Class.field] as an escape hatch.

We might need to migrate the references that intend to be referencing fields.

An example of this is below:

https://github.com/dart-lang/sdk/blob/51d7b8477d4abe6cc54b203dcf6fae3a45f0c7db/sdk/lib/convert/html_escape.dart#L141

build-sdk-docs:   error: [analyzerRef DIFFERENT Parameter escapeSlash null other result is Field escapeSlash %%__HTMLBASE_dartdoc_internal__%%dart-convert/HtmlEscapeMode/escapeSlash.html]
build-sdk-docs:     from dart-convert.HtmlEscapeMode.HtmlEscapeMode: (file:///usr/local/google/home/kallentu/dart-sdk/sdk/out/ReleaseX64/dart-sdk/lib/convert/html_escape.dart:142:9)

Case 2: Analyzer prefers a field over parameter.

/flutter/packages/flutter/lib/src/material/theme_data.dart for [applyElevationOverlayColor]

flutter-docs:   error: [analyzerRef DIFFERENT Field applyElevationOverlayColor material/ThemeData/applyElevationOverlayColor.html other result is Parameter applyElevationOverlayColor null]
flutter-docs:     from material.ThemeData.from: (file:///tmp/flutterOOFHUB/packages/flutter/lib/src/material/theme_data.dart:781:21)

/flutter/packages/flutter/lib/src/material/colors.dart for [value]

flutter-docs:   error: [analyzerRef DIFFERENT Field value dart-ui/Color/value.html other result is Parameter value null]
flutter-docs:     from material.MaterialColor.MaterialColor: (file:///tmp/flutterOOFHUB/packages/flutter/lib/src/material/colors.dart:100:9)

/flutter/packages/flutter/lib/src/foundation/diagnostics.dart for [hashCode]

flutter-docs:   error: [analyzerRef DIFFERENT Field hashCode dart-core/Object/hashCode.html other result is Field hashCode material/RadioThemeData/hashCode.html]
flutter-docs:     from material.RadioThemeData.toStringShort: (file:///tmp/flutterOOFHUB/packages/flutter/lib/src/foundation/diagnostics.dart:3013:10)

Bonus Case 3: Analyzer finding references where Dartdoc couldn't before

This is more of a general win.

[File] in this doc comment wouldn't be linked, but is now linked to the correct class.

bwilkerson commented 1 month ago

The analyzer has a different algorithm with Scope that makes for (IMO) better reference resolving ...

I agree. Using the language-defined scope lookup makes more sense, especially given that users already have to learn about scopes and shouldn't have to learn a different mechanism for deciding which names will be found.