Fix global table lookup of non-canonical elements that have canonical counterparts (#2397)
* First stage -- include all references that can be canonicalized
* Cleanups and hashmap conversion
* prefer enclosing combos (filtering accessors)
* dartfmt
* add tests
diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart
index 12a66de..ca288d9 100644
--- a/lib/src/markdown_processor.dart
+++ b/lib/src/markdown_processor.dart
@@ -459,6 +459,10 @@
// conflicts are allowed, but an instance field is allowed to have the
// same name as a named constructor.
_reducePreferConstructorViaIndicators,
+ // Prefer Fields/TopLevelVariables to accessors.
+ // TODO(jcollins-g): Remove after fixing dart-lang/dartdoc#2396 or
+ // exclude Accessors from all lookup tables.
+ _reducePreferCombos,
// Prefer the Dart analyzer's resolution of comment references. We
// can't start from this because of the differences in Dartdoc
// canonicalization.
@@ -547,6 +551,14 @@
}
}
+ void _reducePreferCombos() {
+ var accessors = results.whereType<Accessor>().toList();
+ accessors.forEach((a) {
+ results.remove(a);
+ results.add(a.enclosingCombo);
+ });
+ }
+
void _findTypeParameters() {
if (element is TypeParameters) {
results.addAll((element as TypeParameters).typeParameters.where((p) =>
@@ -648,7 +660,7 @@
prefixToLibrary[codeRefChompedParts.first]?.forEach((l) => l
.modelElementsNameMap[lookup]
?.map(_convertConstructors)
- ?.forEach((m) => _addCanonicalResult(m, _getPreferredClass(m))));
+ ?.forEach((m) => _addCanonicalResult(m)));
}
}
}
@@ -660,8 +672,7 @@
if (codeRefChomped == modelElement.fullyQualifiedNameWithoutLibrary ||
(modelElement is Library &&
codeRefChomped == modelElement.fullyQualifiedName)) {
- _addCanonicalResult(
- _convertConstructors(modelElement), preferredClass);
+ _addCanonicalResult(_convertConstructors(modelElement));
}
}
}
@@ -671,7 +682,7 @@
// Only look for partially qualified matches if we didn't find a fully qualified one.
if (library.modelElementsNameMap.containsKey(codeRefChomped)) {
for (final modelElement in library.modelElementsNameMap[codeRefChomped]) {
- _addCanonicalResult(_convertConstructors(modelElement), preferredClass);
+ _addCanonicalResult(_convertConstructors(modelElement));
}
}
}
@@ -684,15 +695,7 @@
packageGraph.findRefElementCache.containsKey(codeRefChomped)) {
for (var modelElement
in packageGraph.findRefElementCache[codeRefChomped]) {
- // For fully qualified matches, the original preferredClass passed
- // might make no sense. Instead, use the enclosing class from the
- // element in [packageGraph.findRefElementCache], because that element's
- // enclosing class will be preferred from [codeRefChomped]'s perspective.
- _addCanonicalResult(
- _convertConstructors(modelElement),
- modelElement.enclosingElement is Class
- ? modelElement.enclosingElement
- : null);
+ _addCanonicalResult(_convertConstructors(modelElement));
}
}
}
@@ -785,9 +788,8 @@
}
// Add a result, but make it canonical.
- void _addCanonicalResult(ModelElement modelElement, Container tryClass) {
- results.add(packageGraph.findCanonicalModelElementFor(modelElement.element,
- preferredClass: tryClass));
+ void _addCanonicalResult(ModelElement modelElement) {
+ results.add(modelElement.canonicalModelElement);
}
/// _getResultsForClass assumes codeRefChomped might be a member of tryClass (inherited or not)
@@ -804,7 +806,7 @@
} else {
// People like to use 'this' in docrefs too.
if (codeRef == 'this') {
- _addCanonicalResult(tryClass, null);
+ _addCanonicalResult(tryClass);
} else {
// TODO(jcollins-g): get rid of reimplementation of identifier resolution
// or integrate into ModelElement in a simpler way.
@@ -816,7 +818,7 @@
// Fortunately superChains are short, but optimize this if it matters.
superChain.addAll(tryClass.superChain.map((t) => t.element));
for (final c in superChain) {
- _getResultsForSuperChainElement(c, tryClass);
+ _getResultsForSuperChainElement(c);
if (results.isNotEmpty) break;
}
}
@@ -825,12 +827,12 @@
/// Get any possible results for this class in the superChain. Returns
/// true if we found something.
- void _getResultsForSuperChainElement(Class c, Class tryClass) {
+ void _getResultsForSuperChainElement(Class c) {
var membersToCheck = (c.allModelElementsByNamePart[codeRefChomped] ?? [])
.map(_convertConstructors);
for (var modelElement in membersToCheck) {
// [thing], a member of this class
- _addCanonicalResult(modelElement, tryClass);
+ _addCanonicalResult(modelElement);
}
if (codeRefChompedParts.length < 2 ||
codeRefChompedParts[codeRefChompedParts.length - 2] == c.name) {
@@ -838,7 +840,7 @@
(c.allModelElementsByNamePart[codeRefChompedParts.last] ??
<ModelElement>[])
.map(_convertConstructors);
- membersToCheck.forEach((m) => _addCanonicalResult(m, tryClass));
+ membersToCheck.forEach((m) => _addCanonicalResult(m));
}
results.remove(null);
if (results.isNotEmpty) return;
diff --git a/lib/src/model/class.dart b/lib/src/model/class.dart
index d63299d..49d8049 100644
--- a/lib/src/model/class.dart
+++ b/lib/src/model/class.dart
@@ -401,8 +401,7 @@
var accessorMap = <String, List<PropertyAccessorElement>>{};
for (var accessorElement in inheritedAccessorElements) {
var name = accessorElement.name.replaceFirst('=', '');
- accessorMap.putIfAbsent(name, () => []);
- accessorMap[name].add(accessorElement);
+ accessorMap.putIfAbsent(name, () => []).add(accessorElement);
}
// For half-inherited fields, the analyzer only links the non-inherited
diff --git a/lib/src/model/library.dart b/lib/src/model/library.dart
index 2b7a0c1..2a7b962 100644
--- a/lib/src/model/library.dart
+++ b/lib/src/model/library.dart
@@ -2,6 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
+import 'dart:collection';
+
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/element.dart';
@@ -283,8 +285,8 @@
for (var i in element.imports) {
// Ignore invalid imports.
if (i.prefix?.name != null && i.importedLibrary != null) {
- _prefixToLibrary.putIfAbsent(i.prefix?.name, () => {});
- _prefixToLibrary[i.prefix?.name]
+ _prefixToLibrary
+ .putIfAbsent(i.prefix?.name, () => {})
.add(ModelElement.from(i.importedLibrary, library, packageGraph));
}
}
@@ -585,29 +587,29 @@
static String getLibraryName(LibraryElement element) =>
_getLibraryName(element);
- /*late final*/ Map<String, Set<ModelElement>> _modelElementsNameMap;
+ /*late final*/ HashMap<String, Set<ModelElement>> _modelElementsNameMap;
/// Map of [fullyQualifiedNameWithoutLibrary] to all matching [ModelElement]s
/// in this library. Used for code reference lookups.
- Map<String, Set<ModelElement>> get modelElementsNameMap {
+ HashMap<String, Set<ModelElement>> get modelElementsNameMap {
if (_modelElementsNameMap == null) {
- _modelElementsNameMap = <String, Set<ModelElement>>{};
+ _modelElementsNameMap = HashMap<String, Set<ModelElement>>();
allModelElements.forEach((ModelElement modelElement) {
// [definingLibrary] may be null if [element] has been imported or
// exported with a non-normalized URI, like "src//a.dart".
if (modelElement.definingLibrary == null) return;
- _modelElementsNameMap.putIfAbsent(
- modelElement.fullyQualifiedNameWithoutLibrary, () => {});
- _modelElementsNameMap[modelElement.fullyQualifiedNameWithoutLibrary]
+ _modelElementsNameMap
+ .putIfAbsent(
+ modelElement.fullyQualifiedNameWithoutLibrary, () => {})
.add(modelElement);
});
}
return _modelElementsNameMap;
}
- /*late final*/ Map<Element, Set<ModelElement>> _modelElementsMap;
+ /*late final*/ HashMap<Element, Set<ModelElement>> _modelElementsMap;
- Map<Element, Set<ModelElement>> get modelElementsMap {
+ HashMap<Element, Set<ModelElement>> get modelElementsMap {
if (_modelElementsMap == null) {
var results = quiver.concat(<Iterable<ModelElement>>[
library.constants,
@@ -639,13 +641,13 @@
]);
}),
]);
- _modelElementsMap = <Element, Set<ModelElement>>{};
+ _modelElementsMap = HashMap<Element, Set<ModelElement>>();
results.forEach((modelElement) {
- _modelElementsMap.putIfAbsent(modelElement.element, () => {});
- _modelElementsMap[modelElement.element].add(modelElement);
+ _modelElementsMap
+ .putIfAbsent(modelElement.element, () => {})
+ .add(modelElement);
});
- _modelElementsMap.putIfAbsent(element, () => {});
- _modelElementsMap[element].add(this);
+ _modelElementsMap.putIfAbsent(element, () => {}).add(this);
}
return _modelElementsMap;
}
diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart
index 4ba3de3..58c57b4 100644
--- a/lib/src/model/model_element.dart
+++ b/lib/src/model/model_element.dart
@@ -299,8 +299,9 @@
library.packageGraph.allConstructedModelElements[key] = newModelElement;
if (newModelElement is Inheritable) {
var iKey = Tuple2<Element, Library>(e, library);
- library.packageGraph.allInheritableElements.putIfAbsent(iKey, () => {});
- library.packageGraph.allInheritableElements[iKey].add(newModelElement);
+ library.packageGraph.allInheritableElements
+ .putIfAbsent(iKey, () => {})
+ .add(newModelElement);
}
}
}
@@ -557,10 +558,9 @@
preferredClass: preferredClass);
}
+ ModelElement _canonicalModelElement;
// Returns the canonical ModelElement for this ModelElement, or null
// if there isn't one.
- ModelElement _canonicalModelElement;
-
ModelElement get canonicalModelElement =>
_canonicalModelElement ??= buildCanonicalModelElement();
diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart
index 2b39258..7f4ad89 100644
--- a/lib/src/model/package_graph.dart
+++ b/lib/src/model/package_graph.dart
@@ -184,13 +184,13 @@
return _extensions;
}
- Map<String, Set<ModelElement>> _findRefElementCache;
- Map<String, Set<ModelElement>> get findRefElementCache {
+ HashMap<String, Set<ModelElement>> _findRefElementCache;
+ HashMap<String, Set<ModelElement>> get findRefElementCache {
if (_findRefElementCache == null) {
assert(packageGraph.allLibrariesAdded);
- _findRefElementCache = {};
+ _findRefElementCache = HashMap<String, Set<ModelElement>>();
for (final modelElement
- in utils.filterNonDocumented(packageGraph.allLocalModelElements)) {
+ in utils.filterHasCanonical(packageGraph.allModelElements)) {
_findRefElementCache.putIfAbsent(
modelElement.fullyQualifiedNameWithoutLibrary, () => {});
_findRefElementCache.putIfAbsent(
@@ -210,15 +210,15 @@
PackageWarningCounter _packageWarningCounter;
/// All ModelElements constructed for this package; a superset of [allModelElements].
- final Map<Tuple3<Element, Library, Container>, ModelElement>
- allConstructedModelElements = {};
+ final allConstructedModelElements =
+ HashMap<Tuple3<Element, Library, Container>, ModelElement>();
/// Anything that might be inheritable, place here for later lookup.
- final Map<Tuple2<Element, Library>, Set<ModelElement>>
- allInheritableElements = {};
+ final allInheritableElements =
+ HashMap<Tuple2<Element, Library>, Set<ModelElement>>();
/// A mapping of the list of classes which implement each class.
- final Map<Class, List<Class>> _implementors = LinkedHashMap(
+ final _implementors = LinkedHashMap<Class, List<Class>>(
equals: (Class a, Class b) => a.definingClass == b.definingClass,
hashCode: (Class class_) => class_.definingClass.hashCode);
@@ -530,8 +530,9 @@
referredFrom: <Locatable>[topLevelLibrary]);
return;
}
- _libraryElementReexportedBy.putIfAbsent(libraryElement, () => {});
- _libraryElementReexportedBy[libraryElement].add(topLevelLibrary);
+ _libraryElementReexportedBy
+ .putIfAbsent(libraryElement, () => {})
+ .add(topLevelLibrary);
for (var exportedElement in libraryElement.exports) {
_tagReexportsFor(
topLevelLibrary, exportedElement.exportedLibrary, exportedElement);
diff --git a/lib/src/model_utils.dart b/lib/src/model_utils.dart
index 77a1432..3890d27 100644
--- a/lib/src/model_utils.dart
+++ b/lib/src/model_utils.dart
@@ -73,6 +73,11 @@
return null;
}
+Iterable<T> filterHasCanonical<T extends ModelElement>(
+ Iterable<T> maybeHasCanonicalItems) {
+ return maybeHasCanonicalItems.where((me) => me.canonicalModelElement != null);
+}
+
/// Remove elements that aren't documented.
Iterable<T> filterNonDocumented<T extends Documentable>(
Iterable<T> maybeDocumentedItems) {
diff --git a/lib/src/package_config_provider.dart b/lib/src/package_config_provider.dart
index 3221aa4..845137e 100644
--- a/lib/src/package_config_provider.dart
+++ b/lib/src/package_config_provider.dart
@@ -27,8 +27,9 @@
final _packageConfigData = <String, List<package_config.Package>>{};
void addPackageToConfigFor(String location, String name, Uri root) {
- _packageConfigData.putIfAbsent(location, () => []);
- _packageConfigData[location].add(package_config.Package(name, root));
+ _packageConfigData
+ .putIfAbsent(location, () => [])
+ .add(package_config.Package(name, root));
}
@override
diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart
index b8a5756..de96ee8 100644
--- a/lib/src/warnings.dart
+++ b/lib/src/warnings.dart
@@ -482,8 +482,7 @@
errorCount += 1;
}
var warningData = Tuple2<PackageWarning, String>(kind, message);
- countedWarnings.putIfAbsent(element?.element, () => {});
- countedWarnings[element?.element].add(warningData);
+ countedWarnings.putIfAbsent(element?.element, () => {}).add(warningData);
_writeWarning(kind, warningMode, config.verboseWarnings,
element?.fullyQualifiedName, fullMessage);
}
diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart
index 8fc70e9..9b3eda5 100644
--- a/test/end2end/model_test.dart
+++ b/test/end2end/model_test.dart
@@ -15,6 +15,7 @@
import 'package:dartdoc/src/render/parameter_renderer.dart';
import 'package:dartdoc/src/render/typedef_renderer.dart';
import 'package:dartdoc/src/special_elements.dart';
+import 'package:dartdoc/src/tuple.dart';
import 'package:dartdoc/src/warnings.dart';
import 'package:test/test.dart';
@@ -882,6 +883,24 @@
docsAsHtml = doAwesomeStuff.documentationAsHtml;
});
+ test('Verify links to inherited members inside class', () {
+ expect(
+ docsAsHtml,
+ contains(
+ '<a href="${HTMLBASE_PLACEHOLDER}fake/ImplicitProperties/forInheriting.html">ClassWithUnusualProperties.forInheriting</a>'));
+ expect(
+ docsAsHtml,
+ contains(
+ '<a href="%%__HTMLBASE_dartdoc_internal__%%reexport_two/BaseReexported/action.html">ExtendedBaseReexported.action</a></p>'));
+ var doAwesomeStuffWarnings = packageGraph.packageWarningCounter
+ .countedWarnings[doAwesomeStuff.element] ??
+ [];
+ expect(
+ doAwesomeStuffWarnings,
+ isNot(anyElement((Tuple2<PackageWarning, String> e) =>
+ e.item1 == PackageWarning.ambiguousDocReference)));
+ });
+
test('can handle renamed imports', () {
var aFunctionUsingRenamedLib = fakeLibrary.functions
.firstWhere((f) => f.name == 'aFunctionUsingRenamedLib');
diff --git a/testing/test_package/lib/fake.dart b/testing/test_package/lib/fake.dart
index 7aace41..6cccca1 100644
--- a/testing/test_package/lib/fake.dart
+++ b/testing/test_package/lib/fake.dart
@@ -858,6 +858,7 @@
/// {@subCategory Things and Such}
/// {@image https://flutter.io/images/catalog-widget-placeholder.png}
/// {@samples https://flutter.io}
+///
class BaseForDocComments {
/// Takes a [value] and returns a String.
///
@@ -899,6 +900,10 @@
///
/// Reference to something that doesn't exist containing a type parameter [ThisIsNotHereNoWay<MyType>]
///
+ /// Reference to an inherited member: [ClassWithUnusualProperties.forInheriting]
+ ///
+ /// Reference to an inherited member in another library via class name: [ExtendedBaseReexported.action]
+ ///
/// Link to a nonexistent file (erroneously expects base href): [link](SubForDocComments/localMethod.html)
///
/// Link to an existing file: [link](../SubForDocComments/localMethod.html)
diff --git a/testing/test_package/lib/src/somelib.dart b/testing/test_package/lib/src/somelib.dart
index 697ec1f..921d4ee 100644
--- a/testing/test_package/lib/src/somelib.dart
+++ b/testing/test_package/lib/src/somelib.dart
@@ -8,6 +8,13 @@
class AUnicornClass {}
+class BaseReexported {
+ String action;
+}
+
+class ExtendedBaseReexported extends BaseReexported {
+}
+
/// A private extension.
extension _Unseen on Object {
void doYouSeeMe() { }
@@ -21,4 +28,4 @@
/// [_Unseen] is not seen, but [DocumentMe] is.
extension DocumentThisExtensionOnce on String {
String get reportOnString => '$this is wonderful';
-}
\ No newline at end of file
+}
