Skip to content

Commit 7f46b5d

Browse files
jcollins-gjcollins-g
authored
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
1 parent 3087e5f commit 7f46b5d

File tree

11 files changed

+98
-58
lines changed

11 files changed

+98
-58
lines changed

‎lib/src/markdown_processor.dart

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,10 @@ class _MarkdownCommentReference {
459459
// conflicts are allowed, but an instance field is allowed to have the
460460
// same name as a named constructor.
461461
_reducePreferConstructorViaIndicators,
462+
// Prefer Fields/TopLevelVariables to accessors.
463+
// TODO(jcollins-g): Remove after fixing dart-lang/dartdoc#2396 or
464+
// exclude Accessors from all lookup tables.
465+
_reducePreferCombos,
462466
// Prefer the Dart analyzer's resolution of comment references. We
463467
// can't start from this because of the differences in Dartdoc
464468
// canonicalization.
@@ -547,6 +551,14 @@ class _MarkdownCommentReference {
547551
}
548552
}
549553

554+
void _reducePreferCombos() {
555+
var accessors = results.whereType<Accessor>().toList();
556+
accessors.forEach((a) {
557+
results.remove(a);
558+
results.add(a.enclosingCombo);
559+
});
560+
}
561+
550562
void _findTypeParameters() {
551563
if (element is TypeParameters) {
552564
results.addAll((element as TypeParameters).typeParameters.where((p) =>
@@ -648,7 +660,7 @@ class _MarkdownCommentReference {
648660
prefixToLibrary[codeRefChompedParts.first]?.forEach((l) => l
649661
.modelElementsNameMap[lookup]
650662
?.map(_convertConstructors)
651-
?.forEach((m) => _addCanonicalResult(m, _getPreferredClass(m))));
663+
?.forEach((m) => _addCanonicalResult(m)));
652664
}
653665
}
654666
}
@@ -660,8 +672,7 @@ class _MarkdownCommentReference {
660672
if (codeRefChomped == modelElement.fullyQualifiedNameWithoutLibrary ||
661673
(modelElement is Library &&
662674
codeRefChomped == modelElement.fullyQualifiedName)) {
663-
_addCanonicalResult(
664-
_convertConstructors(modelElement), preferredClass);
675+
_addCanonicalResult(_convertConstructors(modelElement));
665676
}
666677
}
667678
}
@@ -671,7 +682,7 @@ class _MarkdownCommentReference {
671682
// Only look for partially qualified matches if we didn't find a fully qualified one.
672683
if (library.modelElementsNameMap.containsKey(codeRefChomped)) {
673684
for (final modelElement in library.modelElementsNameMap[codeRefChomped]) {
674-
_addCanonicalResult(_convertConstructors(modelElement), preferredClass);
685+
_addCanonicalResult(_convertConstructors(modelElement));
675686
}
676687
}
677688
}
@@ -684,15 +695,7 @@ class _MarkdownCommentReference {
684695
packageGraph.findRefElementCache.containsKey(codeRefChomped)) {
685696
for (var modelElement
686697
in packageGraph.findRefElementCache[codeRefChomped]) {
687-
// For fully qualified matches, the original preferredClass passed
688-
// might make no sense. Instead, use the enclosing class from the
689-
// element in [packageGraph.findRefElementCache], because that element's
690-
// enclosing class will be preferred from [codeRefChomped]'s perspective.
691-
_addCanonicalResult(
692-
_convertConstructors(modelElement),
693-
modelElement.enclosingElement is Class
694-
? modelElement.enclosingElement
695-
: null);
698+
_addCanonicalResult(_convertConstructors(modelElement));
696699
}
697700
}
698701
}
@@ -785,9 +788,8 @@ class _MarkdownCommentReference {
785788
}
786789

787790
// Add a result, but make it canonical.
788-
void _addCanonicalResult(ModelElement modelElement, Container tryClass) {
789-
results.add(packageGraph.findCanonicalModelElementFor(modelElement.element,
790-
preferredClass: tryClass));
791+
void _addCanonicalResult(ModelElement modelElement) {
792+
results.add(modelElement.canonicalModelElement);
791793
}
792794

793795
/// _getResultsForClass assumes codeRefChomped might be a member of tryClass (inherited or not)
@@ -804,7 +806,7 @@ class _MarkdownCommentReference {
804806
} else {
805807
// People like to use 'this' in docrefs too.
806808
if (codeRef == 'this') {
807-
_addCanonicalResult(tryClass, null);
809+
_addCanonicalResult(tryClass);
808810
} else {
809811
// TODO(jcollins-g): get rid of reimplementation of identifier resolution
810812
// or integrate into ModelElement in a simpler way.
@@ -816,7 +818,7 @@ class _MarkdownCommentReference {
816818
// Fortunately superChains are short, but optimize this if it matters.
817819
superChain.addAll(tryClass.superChain.map((t) => t.element));
818820
for (final c in superChain) {
819-
_getResultsForSuperChainElement(c, tryClass);
821+
_getResultsForSuperChainElement(c);
820822
if (results.isNotEmpty) break;
821823
}
822824
}
@@ -825,20 +827,20 @@ class _MarkdownCommentReference {
825827

826828
/// Get any possible results for this class in the superChain. Returns
827829
/// true if we found something.
828-
void _getResultsForSuperChainElement(Class c, Class tryClass) {
830+
void _getResultsForSuperChainElement(Class c) {
829831
var membersToCheck = (c.allModelElementsByNamePart[codeRefChomped] ?? [])
830832
.map(_convertConstructors);
831833
for (var modelElement in membersToCheck) {
832834
// [thing], a member of this class
833-
_addCanonicalResult(modelElement, tryClass);
835+
_addCanonicalResult(modelElement);
834836
}
835837
if (codeRefChompedParts.length < 2 ||
836838
codeRefChompedParts[codeRefChompedParts.length - 2] == c.name) {
837839
membersToCheck =
838840
(c.allModelElementsByNamePart[codeRefChompedParts.last] ??
839841
<ModelElement>[])
840842
.map(_convertConstructors);
841-
membersToCheck.forEach((m) => _addCanonicalResult(m, tryClass));
843+
membersToCheck.forEach((m) => _addCanonicalResult(m));
842844
}
843845
results.remove(null);
844846
if (results.isNotEmpty) return;

‎lib/src/model/class.dart

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -401,8 +401,7 @@ class Class extends Container
401401
var accessorMap = <String, List<PropertyAccessorElement>>{};
402402
for (var accessorElement in inheritedAccessorElements) {
403403
var name = accessorElement.name.replaceFirst('=', '');
404-
accessorMap.putIfAbsent(name, () => []);
405-
accessorMap[name].add(accessorElement);
404+
accessorMap.putIfAbsent(name, () => []).add(accessorElement);
406405
}
407406

408407
// For half-inherited fields, the analyzer only links the non-inherited

‎lib/src/model/library.dart

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5+
import 'dart:collection';
6+
57
import 'package:analyzer/dart/analysis/results.dart';
68
import 'package:analyzer/dart/ast/ast.dart';
79
import 'package:analyzer/dart/element/element.dart';
@@ -283,8 +285,8 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
283285
for (var i in element.imports) {
284286
// Ignore invalid imports.
285287
if (i.prefix?.name != null && i.importedLibrary != null) {
286-
_prefixToLibrary.putIfAbsent(i.prefix?.name, () => {});
287-
_prefixToLibrary[i.prefix?.name]
288+
_prefixToLibrary
289+
.putIfAbsent(i.prefix?.name, () => {})
288290
.add(ModelElement.from(i.importedLibrary, library, packageGraph));
289291
}
290292
}
@@ -585,29 +587,29 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
585587
static String getLibraryName(LibraryElement element) =>
586588
_getLibraryName(element);
587589

588-
/*late final*/ Map<String, Set<ModelElement>> _modelElementsNameMap;
590+
/*late final*/ HashMap<String, Set<ModelElement>> _modelElementsNameMap;
589591

590592
/// Map of [fullyQualifiedNameWithoutLibrary] to all matching [ModelElement]s
591593
/// in this library. Used for code reference lookups.
592-
Map<String, Set<ModelElement>> get modelElementsNameMap {
594+
HashMap<String, Set<ModelElement>> get modelElementsNameMap {
593595
if (_modelElementsNameMap == null) {
594-
_modelElementsNameMap = <String, Set<ModelElement>>{};
596+
_modelElementsNameMap = HashMap<String, Set<ModelElement>>();
595597
allModelElements.forEach((ModelElement modelElement) {
596598
// [definingLibrary] may be null if [element] has been imported or
597599
// exported with a non-normalized URI, like "src//a.dart".
598600
if (modelElement.definingLibrary == null) return;
599-
_modelElementsNameMap.putIfAbsent(
600-
modelElement.fullyQualifiedNameWithoutLibrary, () => {});
601-
_modelElementsNameMap[modelElement.fullyQualifiedNameWithoutLibrary]
601+
_modelElementsNameMap
602+
.putIfAbsent(
603+
modelElement.fullyQualifiedNameWithoutLibrary, () => {})
602604
.add(modelElement);
603605
});
604606
}
605607
return _modelElementsNameMap;
606608
}
607609

608-
/*late final*/ Map<Element, Set<ModelElement>> _modelElementsMap;
610+
/*late final*/ HashMap<Element, Set<ModelElement>> _modelElementsMap;
609611

610-
Map<Element, Set<ModelElement>> get modelElementsMap {
612+
HashMap<Element, Set<ModelElement>> get modelElementsMap {
611613
if (_modelElementsMap == null) {
612614
var results = quiver.concat(<Iterable<ModelElement>>[
613615
library.constants,
@@ -639,13 +641,13 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
639641
]);
640642
}),
641643
]);
642-
_modelElementsMap = <Element, Set<ModelElement>>{};
644+
_modelElementsMap = HashMap<Element, Set<ModelElement>>();
643645
results.forEach((modelElement) {
644-
_modelElementsMap.putIfAbsent(modelElement.element, () => {});
645-
_modelElementsMap[modelElement.element].add(modelElement);
646+
_modelElementsMap
647+
.putIfAbsent(modelElement.element, () => {})
648+
.add(modelElement);
646649
});
647-
_modelElementsMap.putIfAbsent(element, () => {});
648-
_modelElementsMap[element].add(this);
650+
_modelElementsMap.putIfAbsent(element, () => {}).add(this);
649651
}
650652
return _modelElementsMap;
651653
}

‎lib/src/model/model_element.dart

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -299,8 +299,9 @@ abstract class ModelElement extends Canonicalization
299299
library.packageGraph.allConstructedModelElements[key] = newModelElement;
300300
if (newModelElement is Inheritable) {
301301
var iKey = Tuple2<Element, Library>(e, library);
302-
library.packageGraph.allInheritableElements.putIfAbsent(iKey, () => {});
303-
library.packageGraph.allInheritableElements[iKey].add(newModelElement);
302+
library.packageGraph.allInheritableElements
303+
.putIfAbsent(iKey, () => {})
304+
.add(newModelElement);
304305
}
305306
}
306307
}
@@ -557,10 +558,9 @@ abstract class ModelElement extends Canonicalization
557558
preferredClass: preferredClass);
558559
}
559560

561+
ModelElement _canonicalModelElement;
560562
// Returns the canonical ModelElement for this ModelElement, or null
561563
// if there isn't one.
562-
ModelElement _canonicalModelElement;
563-
564564
ModelElement get canonicalModelElement =>
565565
_canonicalModelElement ??= buildCanonicalModelElement();
566566

‎lib/src/model/package_graph.dart

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,13 @@ class PackageGraph {
184184
return _extensions;
185185
}
186186

187-
Map<String, Set<ModelElement>> _findRefElementCache;
188-
Map<String, Set<ModelElement>> get findRefElementCache {
187+
HashMap<String, Set<ModelElement>> _findRefElementCache;
188+
HashMap<String, Set<ModelElement>> get findRefElementCache {
189189
if (_findRefElementCache == null) {
190190
assert(packageGraph.allLibrariesAdded);
191-
_findRefElementCache = {};
191+
_findRefElementCache = HashMap<String, Set<ModelElement>>();
192192
for (final modelElement
193-
in utils.filterNonDocumented(packageGraph.allLocalModelElements)) {
193+
in utils.filterHasCanonical(packageGraph.allModelElements)) {
194194
_findRefElementCache.putIfAbsent(
195195
modelElement.fullyQualifiedNameWithoutLibrary, () => {});
196196
_findRefElementCache.putIfAbsent(
@@ -210,15 +210,15 @@ class PackageGraph {
210210
PackageWarningCounter _packageWarningCounter;
211211

212212
/// All ModelElements constructed for this package; a superset of [allModelElements].
213-
final Map<Tuple3<Element, Library, Container>, ModelElement>
214-
allConstructedModelElements = {};
213+
final allConstructedModelElements =
214+
HashMap<Tuple3<Element, Library, Container>, ModelElement>();
215215

216216
/// Anything that might be inheritable, place here for later lookup.
217-
final Map<Tuple2<Element, Library>, Set<ModelElement>>
218-
allInheritableElements = {};
217+
final allInheritableElements =
218+
HashMap<Tuple2<Element, Library>, Set<ModelElement>>();
219219

220220
/// A mapping of the list of classes which implement each class.
221-
final Map<Class, List<Class>> _implementors = LinkedHashMap(
221+
final _implementors = LinkedHashMap<Class, List<Class>>(
222222
equals: (Class a, Class b) => a.definingClass == b.definingClass,
223223
hashCode: (Class class_) => class_.definingClass.hashCode);
224224

@@ -530,8 +530,9 @@ class PackageGraph {
530530
referredFrom: <Locatable>[topLevelLibrary]);
531531
return;
532532
}
533-
_libraryElementReexportedBy.putIfAbsent(libraryElement, () => {});
534-
_libraryElementReexportedBy[libraryElement].add(topLevelLibrary);
533+
_libraryElementReexportedBy
534+
.putIfAbsent(libraryElement, () => {})
535+
.add(topLevelLibrary);
535536
for (var exportedElement in libraryElement.exports) {
536537
_tagReexportsFor(
537538
topLevelLibrary, exportedElement.exportedLibrary, exportedElement);

‎lib/src/model_utils.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ AstNode getAstNode(
7373
return null;
7474
}
7575

76+
Iterable<T> filterHasCanonical<T extends ModelElement>(
77+
Iterable<T> maybeHasCanonicalItems) {
78+
return maybeHasCanonicalItems.where((me) => me.canonicalModelElement != null);
79+
}
80+
7681
/// Remove elements that aren't documented.
7782
Iterable<T> filterNonDocumented<T extends Documentable>(
7883
Iterable<T> maybeDocumentedItems) {

‎lib/src/package_config_provider.dart

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,9 @@ class FakePackageConfigProvider implements PackageConfigProvider {
2727
final _packageConfigData = <String, List<package_config.Package>>{};
2828

2929
void addPackageToConfigFor(String location, String name, Uri root) {
30-
_packageConfigData.putIfAbsent(location, () => []);
31-
_packageConfigData[location].add(package_config.Package(name, root));
30+
_packageConfigData
31+
.putIfAbsent(location, () => [])
32+
.add(package_config.Package(name, root));
3233
}
3334

3435
@override

‎lib/src/warnings.dart

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -482,8 +482,7 @@ class PackageWarningCounter {
482482
errorCount += 1;
483483
}
484484
var warningData = Tuple2<PackageWarning, String>(kind, message);
485-
countedWarnings.putIfAbsent(element?.element, () => {});
486-
countedWarnings[element?.element].add(warningData);
485+
countedWarnings.putIfAbsent(element?.element, () => {}).add(warningData);
487486
_writeWarning(kind, warningMode, config.verboseWarnings,
488487
element?.fullyQualifiedName, fullMessage);
489488
}

‎test/end2end/model_test.dart

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import 'package:dartdoc/src/render/model_element_renderer.dart';
1515
import 'package:dartdoc/src/render/parameter_renderer.dart';
1616
import 'package:dartdoc/src/render/typedef_renderer.dart';
1717
import 'package:dartdoc/src/special_elements.dart';
18+
import 'package:dartdoc/src/tuple.dart';
1819
import 'package:dartdoc/src/warnings.dart';
1920
import 'package:test/test.dart';
2021

@@ -882,6 +883,24 @@ void main() {
882883
docsAsHtml = doAwesomeStuff.documentationAsHtml;
883884
});
884885

886+
test('Verify links to inherited members inside class', () {
887+
expect(
888+
docsAsHtml,
889+
contains(
890+
'<a href="${HTMLBASE_PLACEHOLDER}fake/ImplicitProperties/forInheriting.html">ClassWithUnusualProperties.forInheriting</a>'));
891+
expect(
892+
docsAsHtml,
893+
contains(
894+
'<a href="%%__HTMLBASE_dartdoc_internal__%%reexport_two/BaseReexported/action.html">ExtendedBaseReexported.action</a></p>'));
895+
var doAwesomeStuffWarnings = packageGraph.packageWarningCounter
896+
.countedWarnings[doAwesomeStuff.element] ??
897+
[];
898+
expect(
899+
doAwesomeStuffWarnings,
900+
isNot(anyElement((Tuple2<PackageWarning, String> e) =>
901+
e.item1 == PackageWarning.ambiguousDocReference)));
902+
});
903+
885904
test('can handle renamed imports', () {
886905
var aFunctionUsingRenamedLib = fakeLibrary.functions
887906
.firstWhere((f) => f.name == 'aFunctionUsingRenamedLib');

‎testing/test_package/lib/fake.dart

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -858,6 +858,7 @@ class ExtraSpecialList<E> extends SpecialList {}
858858
/// {@subCategory Things and Such}
859859
/// {@image https://flutter.io/images/catalog-widget-placeholder.png}
860860
/// {@samples https://flutter.io}
861+
///
861862
class BaseForDocComments {
862863
/// Takes a [value] and returns a String.
863864
///
@@ -899,6 +900,10 @@ class BaseForDocComments {
899900
///
900901
/// Reference to something that doesn't exist containing a type parameter [ThisIsNotHereNoWay<MyType>]
901902
///
903+
/// Reference to an inherited member: [ClassWithUnusualProperties.forInheriting]
904+
///
905+
/// Reference to an inherited member in another library via class name: [ExtendedBaseReexported.action]
906+
///
902907
/// Link to a nonexistent file (erroneously expects base href): [link](SubForDocComments/localMethod.html)
903908
///
904909
/// Link to an existing file: [link](../SubForDocComments/localMethod.html)

‎testing/test_package/lib/src/somelib.dart

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ class YetAnotherClass {}
88

99
class AUnicornClass {}
1010

11+
class BaseReexported {
12+
String action;
13+
}
14+
15+
class ExtendedBaseReexported extends BaseReexported {
16+
}
17+
1118
/// A private extension.
1219
extension _Unseen on Object {
1320
void doYouSeeMe() { }
@@ -21,4 +28,4 @@ extension on List {
2128
/// [_Unseen] is not seen, but [DocumentMe] is.
2229
extension DocumentThisExtensionOnce on String {
2330
String get reportOnString => '$this is wonderful';
24-
}
31+
}

0 commit comments

Comments
 (0)