Avoid Lists of MapEntries. (#3807)

In the existing code, a few APIs like `CommentReferableEntryGenerators.generateEntries` and `Container.extraReferenceChildren` used `List<MapEntry<...>>` which I've found super confusing. They are pretty hard to reason about, and also some "put all if absent" code was employed to wrangle them. But it turns out that given Map `a` and Map `b`, `a.putAllIfAbsent(b)` is the same as `b.addAll(a)`. So this code can be simplified all around by just using Maps as they are meant to be used. This code is a no-op; same results, just simpler.
dart-lang · Jul 1, 2024 · 6a02ffb · 6a02ffb
1 parent 87853b2
commit 6a02ffb
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 128 deletions.
diff --git a/lib/src/model/comment_referable.dart b/lib/src/model/comment_referable.dart
@@ -228,25 +228,14 @@ extension CommentReferableEntryGenerators on Iterable<CommentReferable> {
             r.referenceName: r,
       };
 
-  /// Generates entries from this Iterable.
-  Iterable<MapEntry<String, CommentReferable>> generateEntries() =>
-      map((r) => MapEntry(r.referenceName, r));
+  /// A mapping from each [CommentReferable]'s name to itself.
+  Map<String, CommentReferable> get asMapByName => {
+        for (var r in this) r.referenceName: r,
+      };
 
   /// Returns all values not of this type.
   List<CommentReferable> whereNotType<T>() => [
         for (var referable in this)
           if (referable is! T) referable,
       ];
 }
-
-/// A set of utility methods to add entries to
-/// [CommentReferable.referenceChildren].
-extension CommentReferableEntryBuilder on Map<String, CommentReferable> {
-  /// Like [Map.putIfAbsent] except works on an iterable of entries.
-  void addEntriesIfAbsent(
-      Iterable<MapEntry<String, CommentReferable>> entries) {
-    for (var entry in entries) {
-      if (!containsKey(entry.key)) this[entry.key] = entry.value;
-    }
-  }
-}
diff --git a/lib/src/model/container.dart b/lib/src/model/container.dart
@@ -221,33 +221,23 @@ abstract class Container extends ModelElement
   /// For subclasses to add items after the main pass but before the
   /// parameter-global.
   @visibleForOverriding
-  Iterable<MapEntry<String, CommentReferable>> get extraReferenceChildren;
+  Map<String, CommentReferable> get extraReferenceChildren;
 
   @override
   @mustCallSuper
-  late final Map<String, CommentReferable> referenceChildren = () {
-    var referenceChildren = <String, CommentReferable>{
-      for (var element in allModelElements
-          .whereNotType<Accessor>()
-          .whereNotType<Constructor>())
-        element.referenceName: element,
-    };
-
-    referenceChildren.addEntriesIfAbsent(extraReferenceChildren);
-    // Process unscoped parameters last to make sure they don't override
-    // other options.
-    for (var modelElement in allModelElements) {
+  late final Map<String, CommentReferable> referenceChildren = {
+    for (var modelElement in allModelElements)
       // Don't complain about references to parameter names, but prefer
       // referring to anything else.
       // TODO(jcollins-g): Figure out something good to do in the ecosystem
       // here to wean people off the habit of unscoped parameter references.
-      if (modelElement.hasParameters) {
-        referenceChildren
-            .addEntriesIfAbsent(modelElement.parameters.generateEntries());
-      }
-    }
-    return referenceChildren;
-  }();
+      if (modelElement.hasParameters) ...modelElement.parameters.asMapByName,
+    ...extraReferenceChildren,
+    for (var element in allModelElements
+        .whereNotType<Accessor>()
+        .whereNotType<Constructor>())
+      element.referenceName: element,
+  };
 
   @override
   Iterable<CommentReferable> get referenceParents => [definingLibrary, library];

diff --git a/lib/src/model/extension.dart b/lib/src/model/extension.dart
@@ -107,8 +107,7 @@ class Extension extends Container {
 
   @override
   @visibleForOverriding
-  Iterable<MapEntry<String, CommentReferable>> get extraReferenceChildren =>
-      const [];
+  Map<String, CommentReferable> get extraReferenceChildren => const {};
 
   @override
   String get relationshipsClass => 'clazz-relationships';

diff --git a/lib/src/model/extension_type.dart b/lib/src/model/extension_type.dart
@@ -84,8 +84,7 @@ class ExtensionType extends InheritingContainer with Constructable {
 
   @override
   @visibleForOverriding
-  Iterable<MapEntry<String, CommentReferable>> get extraReferenceChildren =>
-      const [];
+  Map<String, CommentReferable> get extraReferenceChildren => const {};
 
   @override
   String get relationshipsClass => 'clazz-relationships';

diff --git a/lib/src/model/inheriting_container.dart b/lib/src/model/inheriting_container.dart
@@ -29,41 +29,35 @@ mixin Constructable implements InheritingContainer {
 
   @override
   @visibleForOverriding
-  Iterable<MapEntry<String, CommentReferable>>
-      get extraReferenceChildren sync* {
-    yield* _constructorGenerator(constructors);
-    // TODO(jcollins-g): wean important users off of relying on static method
-    // inheritance (dart-lang/dartdoc#2698)
-    for (var container in superChain.wherePublic
-        .map((t) => t.modelElement)
-        .whereType<Container>()) {
-      for (var modelElement in [
-        ...container.staticFields,
-        ...container.staticMethods,
-      ]) {
-        yield MapEntry(modelElement.referenceName, modelElement);
-      }
-      if (container is Constructable) {
-        yield* _constructorGenerator(container.constructors);
-      }
-    }
-  }
+  Map<String, CommentReferable> get extraReferenceChildren => {
+        for (var container in superChain.wherePublic
+            .map((t) => t.modelElement)
+            .whereType<Container>()) ...{
+          if (container is Constructable)
+            ..._mapConstructorsByName(container.constructors),
+          for (var modelElement in [
+            // TODO(jcollins-g): wean important users off of relying on static
+            // method inheritance (dart-lang/dartdoc#2698).
+            ...container.staticFields, ...container.staticMethods
+          ])
+            modelElement.referenceName: modelElement,
+        },
+        ..._mapConstructorsByName(constructors),
+      };
 
   @override
   bool get hasPublicConstructors => publicConstructorsSorted.isNotEmpty;
 
-  static Iterable<MapEntry<String, CommentReferable>> _constructorGenerator(
-      Iterable<Constructor> source) sync* {
-    for (var constructor in source) {
-      yield MapEntry(constructor.referenceName, constructor);
-      yield MapEntry(
-          '${constructor.enclosingElement.referenceName}.${constructor.referenceName}',
-          constructor);
-      if (constructor.isUnnamedConstructor) {
-        yield MapEntry('new', constructor);
-      }
-    }
-  }
+  static Map<String, CommentReferable> _mapConstructorsByName(
+          Iterable<Constructor> constructors) =>
+      {
+        for (var constructor in constructors) ...{
+          constructor.referenceName: constructor,
+          '${constructor.enclosingElement.referenceName}.${constructor.referenceName}':
+              constructor,
+          if (constructor.isUnnamedConstructor) 'new': constructor,
+        },
+      };
 }
 
 /// A [Container] that participates in inheritance in Dart.

diff --git a/lib/src/model/library.dart b/lib/src/model/library.dart
@@ -386,8 +386,8 @@ class Library extends ModelElement
     var referenceChildrenBuilder = <String, CommentReferable>{};
     var definedNamesModelElements =
         element.exportNamespace.definedNames.values.map(getModelForElement);
-    referenceChildrenBuilder.addEntries(
-        definedNamesModelElements.whereNotType<Accessor>().generateEntries());
+    referenceChildrenBuilder
+        .addAll(definedNamesModelElements.whereNotType<Accessor>().asMapByName);
     // TODO(jcollins-g): warn and get rid of this case where it shows up.
     // If a user is hiding parts of a prefix import, the user should not
     // refer to hidden members via the prefix, because that can be

diff --git a/lib/src/model/mixin.dart b/lib/src/model/mixin.dart
@@ -46,8 +46,7 @@ class Mixin extends InheritingContainer {
 
   @override
   @visibleForOverriding
-  Iterable<MapEntry<String, CommentReferable>> get extraReferenceChildren =>
-      const [];
+  Map<String, CommentReferable> get extraReferenceChildren => const {};
 
   @override
   bool get hasModifiers => super.hasModifiers || hasPublicSuperclassConstraints;

diff --git a/lib/src/model/package.dart b/lib/src/model/package.dart
@@ -376,14 +376,15 @@ class Package extends LibraryContainer
 
   @override
   late final Map<String, CommentReferable> referenceChildren = {
-    for (var library in publicLibrariesSorted) library.referenceName: library,
-  }
-    // Do not override any preexisting data, and insert based on the
-    // public library sort order.
-    // TODO(jcollins-g): warn when results require package-global
-    // lookups like this.
-    ..addEntriesIfAbsent(
-        publicLibrariesSorted.expand((l) => l.referenceChildren.entries));
+    // Do not override any preexisting data, and insert based on the public
+    // library sort order.
+    // TODO(jcollins-g): warn when results require package-global lookups like
+    // this.
+    for (var library in publicLibrariesSorted) ...{
+      library.referenceName: library,
+      ...library.referenceChildren,
+    }
+  };
 
   @override
   Iterable<CommentReferable> get referenceParents => [packageGraph];

diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart
@@ -990,43 +990,42 @@ class PackageGraph with CommentReferable, Nameable {
 
   @override
   late final Map<String, CommentReferable> referenceChildren = () {
-    var children = <String, CommentReferable>{};
-    // We have to use a stable order or otherwise references depending
-    // on ambiguous resolution (see below) will change where they
-    // resolve based on internal implementation details.
+    // We have to use a stable order or otherwise references depending on
+    // ambiguous resolution (see below) will change where they resolve based on
+    // internal implementation details.
     var sortedPackages = packages.toList(growable: false)..sort(byName);
     var sortedDocumentedPackages = _documentedPackages.toList(growable: false)
       ..sort(byName);
-    // Packages are the top priority.
-    children.addEntries(sortedPackages.generateEntries());
-
-    // Libraries are next.
-    // TODO(jcollins-g): Warn about directly referencing libraries out of
-    // scope?  Doing this is always going to be ambiguous and potentially
-    // confusing.
-    children.addEntriesIfAbsent(sortedDocumentedPackages
-        .expand((p) => p.publicLibrariesSorted)
-        .generateEntries());
-
-    // TODO(jcollins-g): Warn about directly referencing top level items
-    // out of scope?  Doing this will be even more ambiguous and
-    // potentially confusing than doing so with libraries.
-    children.addEntriesIfAbsent(sortedDocumentedPackages
-        .expand((p) => p.publicLibrariesSorted)
-        .expand((l) => [
-              ...l.constants.wherePublic,
-              ...l.functions.wherePublic,
-              ...l.properties.wherePublic,
-              ...l.typedefs.wherePublic,
-              ...l.extensions.wherePublic,
-              ...l.extensionTypes.wherePublic,
-              ...l.classes.wherePublic,
-              ...l.enums.wherePublic,
-              ...l.mixins.wherePublic,
-            ])
-        .generateEntries());
-
-    return children;
+    return {
+      // TODO(jcollins-g): Warn about directly referencing top level items out
+      // of scope?  Doing this will be even more ambiguous and potentially
+      // confusing than doing so with libraries.
+      ...sortedDocumentedPackages
+          .expand((p) => p.publicLibrariesSorted)
+          .expand((l) => [
+                ...l.constants.wherePublic,
+                ...l.functions.wherePublic,
+                ...l.properties.wherePublic,
+                ...l.typedefs.wherePublic,
+                ...l.extensions.wherePublic,
+                ...l.extensionTypes.wherePublic,
+                ...l.classes.wherePublic,
+                ...l.enums.wherePublic,
+                ...l.mixins.wherePublic,
+              ])
+          .asMapByName,
+
+      // Libraries are next.
+      // TODO(jcollins-g): Warn about directly referencing libraries out of
+      // scope?  Doing this is always going to be ambiguous and potentially
+      // confusing.
+      ...sortedDocumentedPackages
+          .expand((p) => p.publicLibrariesSorted)
+          .asMapByName,
+
+      // Packages are the top priority.
+      ...sortedPackages.asMapByName,
+    };
   }();
 
   @override

diff --git a/test/src/utils.dart b/test/src/utils.dart
@@ -301,20 +301,20 @@ Future<void> writeDartdocResources(ResourceProvider resourceProvider) async {
   }
 }
 
-/// For comparison purposes, return an equivalent [MatchingLinkResult]
-/// for the defining element returned.  May return [originalResult].
-/// We do this to eliminate canonicalization effects from comparison,
-/// as the original lookup code returns canonicalized results and the
-/// new lookup code is only guaranteed to return equivalent results.
+/// For comparison purposes, return an equivalent [MatchingLinkResult] for the
+/// defining element returned. May return [originalResult]. We do this to
+/// eliminate canonicalization effects from comparison, as the original lookup
+/// code returns canonicalized results and the new lookup code is only
+/// guaranteed to return equivalent results.
 MatchingLinkResult definingLinkResult(MatchingLinkResult originalResult) {
-  var definingReferable =
-      originalResult.commentReferable?.definingCommentReferable;
-
-  if (definingReferable != null &&
-      definingReferable != originalResult.commentReferable) {
-    return MatchingLinkResult(definingReferable);
+  var commentReferable = originalResult.commentReferable;
+  if (commentReferable == null) {
+    return originalResult;
   }
-  return originalResult;
+  var definingReferable = commentReferable.definingCommentReferable;
+  return definingReferable == originalResult.commentReferable
+      ? originalResult
+      : MatchingLinkResult(definingReferable);
 }
 
 MatchingLinkResult referenceLookup(Warnable element, String codeRef) =>