Stabilize the global table insertion order in the new lookup code (#2712)

* Stabilize the global table insertion order in the new lookup code.

* Keep using all packages -- might be necessary in some cases and I've done all my testing that way so far.

* Make byName more stable via hashCode.

* No need to rename it

Author: jcollins-g

lib/src/model/nameable.dart

@@ -35,5 +35,10 @@ abstract class Nameable {
   String toString() => name;
 }
 
-int byName(Nameable a, Nameable b) =>
-    compareAsciiLowerCaseNatural(a.name, b.name);
+int byName(Nameable a, Nameable b) {
+  var stringCompare = compareAsciiLowerCaseNatural(a.name, b.name);
+  if (stringCompare == 0) {
+    return a.hashCode.compareTo(b.hashCode);
+  }
+  return stringCompare;
+}

lib/src/model/package.dart

@@ -406,7 +406,7 @@ class Package extends LibraryContainer
   Map<String, CommentReferable> get referenceChildren {
     if (_referenceChildren == null) {
       _referenceChildren = {};
-      _referenceChildren.addEntries(allLibraries.generateEntries());
+      _referenceChildren.addEntries(publicLibrariesSorted.generateEntries());
       // Do not override any preexisting data, and insert based on the
       // public library sort order.
       // TODO(jcollins-g): warn when results require package-global

lib/src/model/package_graph.dart

@@ -1030,19 +1030,26 @@ class PackageGraph with CommentReferable, Nameable {
   Map<String, CommentReferable> get referenceChildren {
     if (_referenceChildren == null) {
       _referenceChildren = {};
+      // 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()..sort(byName);
+      var sortedDocumentedPackages = documentedPackages.toList()..sort(byName);
       // Packages are the top priority.
-      _referenceChildren.addEntries(packages.generateEntries());
+      _referenceChildren.addEntries(sortedPackages.generateEntries());
 
       // Libraries are next.
       // TODO(jcollins-g): Warn about directly referencing libraries out of
-      // scope?
-      _referenceChildren.addEntriesIfAbsent(documentedPackages
+      // scope?  Doing this is always going to be ambiguous and potentially
+      // confusing.
+      _referenceChildren.addEntriesIfAbsent(sortedDocumentedPackages
           .expand((p) => p.publicLibrariesSorted)
           .generateEntries());
 
       // TODO(jcollins-g): Warn about directly referencing top level items
-      // out of scope?
-      _referenceChildren.addEntriesIfAbsent(documentedPackages
+      // out of scope?  Doing this will be even more ambiguous and
+      // potentially confusing than doing so with libraries.
+      _referenceChildren.addEntriesIfAbsent(sortedDocumentedPackages
           .expand((p) => p.publicLibrariesSorted)
           .expand((l) => [
                 ...l.publicConstants,

test/end2end/model_test.dart

@@ -4970,6 +4970,16 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
         expect(byName(b, a), 1);
       });
     }
+
+    test('sort order is stable when necessary', () {
+      var a = StringNameHashCode('a', 12);
+      var b = StringNameHashCode('b', 12);
+      var aa = StringNameHashCode('a', 14);
+      expect(byName(a, aa), -1);
+      expect(byName(a, b), -1);
+      expect(byName(b, a), 1);
+      expect(byName(aa, b), -1);
+    });
   });
 }
 
@@ -4982,3 +4992,18 @@ class StringName extends Nameable {
   @override
   String toString() => name;
 }
+
+class StringNameHashCode extends Nameable {
+  @override
+  final String name;
+  @override
+  final int hashCode;
+
+  StringNameHashCode(this.name, this.hashCode);
+
+  @override
+  String toString() => name;
+
+  @override
+  bool operator ==(Object other) => super == other;
+}