Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/src/model/nameable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion lib/src/model/package.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 12 additions & 5 deletions lib/src/model/package_graph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 25 additions & 0 deletions test/end2end/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
}

Expand All @@ -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;
}