Skip to content
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

Update code to Dart 3.0 #284

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 1.18.0

* Require Dart 3.0.
* Mark mixin classes as `mixin class`.
* Added a `Map.pairs` extension method to make it easier to work with maps using
Dart 3 record types.

## 1.17.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version was never published, so we can probably just drop the changelog entry.


* Accept Dart SDK versions above 3.0.
Expand Down
1 change: 1 addition & 0 deletions lib/collection.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export 'src/functions.dart';
export 'src/iterable_extensions.dart';
export 'src/iterable_zip.dart';
export 'src/list_extensions.dart';
export 'src/map_extensions.dart';
export 'src/priority_queue.dart';
export 'src/queue_list.dart';
export 'src/union_set.dart';
Expand Down
7 changes: 4 additions & 3 deletions lib/src/boollist.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import 'unmodifiable_wrappers.dart' show NonGrowableListMixin;
/// A space-efficient list of boolean values.
///
/// Uses list of integers as internal storage to reduce memory usage.
abstract /*mixin*/ class BoolList with ListMixin<bool> {
abstract final class BoolList with ListMixin<bool> {
static const int _entryShift = 5;

static const int _bitsPerEntry = 32;
Expand Down Expand Up @@ -180,7 +180,7 @@ abstract /*mixin*/ class BoolList with ListMixin<bool> {
}
}

class _GrowableBoolList extends BoolList {
final class _GrowableBoolList extends BoolList {
static const int _growthFactor = 2;

_GrowableBoolList._withCapacity(int length, int capacity)
Expand Down Expand Up @@ -228,7 +228,8 @@ class _GrowableBoolList extends BoolList {
}
}

class _NonGrowableBoolList extends BoolList with NonGrowableListMixin<bool> {
final class _NonGrowableBoolList extends BoolList
with NonGrowableListMixin<bool> {
_NonGrowableBoolList._withCapacity(int length, int capacity)
: super._(
Uint32List(BoolList._lengthInWords(capacity)),
Expand Down
2 changes: 1 addition & 1 deletion lib/src/canonicalized_map.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import 'dart:collection';
/// more efficient than a [LinkedHashMap] with a custom equality operator
/// because it only canonicalizes each key once, rather than doing so for each
/// comparison.
class CanonicalizedMap<C, K, V> implements Map<K, V> {
final class CanonicalizedMap<C, K, V> implements Map<K, V> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has subclasses in the ecosystem.

https://github.com/dart-lang/http_parser/blob/bbe37dd228ec59f58a73df4b328ef747757165c7/lib/src/case_insensitive_map.dart#L10

I think this looks like a legitimately useful pattern. Perhaps this class could be base instead of final?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess even marking it as base will be breaking for this use case... not sure if we want to leave it loose, or ignore the fact that it is breaking (and submit a fix at the usage site ahead of time)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Why is making it base breaking? I probably wouldn't make it base because it doesn't need to be, but this use case is extending, so making it base shouldn't break it. Just require being base or final itself, and being final seems fine for that class too.)

That subclass doesn't really need to be a class. It's an implementation strategy for the Map interface, so it's an implementation class, but the class isn't important.
Just getting a Map<String, V> is what it exposes

It could just be a function:

Map<String, V> caseInsensitiveMap<V>({Map<String, V>? from}) => 
    from == null 
          ? CanonicalizedMap<String, String, V>(_toLowerCase) 
          : CanonicalizedMap<String, String, V>.from(from, _toLowerCase);

That would be breaking, since it's not how it was originally written, and we can't really replace the unnamed constructor with something returning a different type.)

But it can use a wrapper and delegation and preserve the unnecessary class:

final class CaseInsensitiveMap<V> extends DelegatingMap<String, V> {
   CaseInsensitiveMap() : super(CanonicalizedMap<String, String, V>(_toLowerCase));
   CaseInsensitiveMap.from(Map<String, V> other) 
       : super(CanonicalizedMap<String, String, V>.from(from, _toLowerCase));

   static String _toLowerCase(String string) => string.toLowerCase();
}

In either case, the result doesn't need to be-a CanonicalizedMap. That's leaking an implementation detail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is making it base breaking?

It could be a quirk of google3 - when I made it base I saw a build failure until I also marked the subclass as base.

In either case, the result doesn't need to be-a CanonicalizedMap. That's leaking an implementation detail.

Yeah, it's not a good design and we should fix it. We just need to decide what types of misuses that exist in google3 or that we can find in pub that we want to avoid breaking, and which we are OK with breaking.

final C Function(K) _canonicalize;

final bool Function(K)? _isValidKeyFn;
Expand Down
2 changes: 1 addition & 1 deletion lib/src/combined_wrappers/combined_iterable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import 'combined_iterator.dart';
/// lazily accessing individual iterable instances. This means that if the
/// underlying iterables change, the [CombinedIterableView] will reflect those
/// changes.
class CombinedIterableView<T> extends IterableBase<T> {
final class CombinedIterableView<T> extends IterableBase<T> {
/// The iterables that this combines.
final Iterable<Iterable<T>> _iterables;

Expand Down
2 changes: 1 addition & 1 deletion lib/src/combined_wrappers/combined_iterator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/// The iterator for `CombinedIterableView` and `CombinedListView`.
///
/// Moves through each iterable's iterator in sequence.
class CombinedIterator<T> implements Iterator<T> {
final class CombinedIterator<T> implements Iterator<T> {
/// The iterators that this combines, or `null` if done iterating.
///
/// Because this comes from a call to [Iterable.map], it's lazy and will
Expand Down
29 changes: 15 additions & 14 deletions lib/src/equality.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import 'comparators.dart';
const int _hashMask = 0x7fffffff;

/// A generic equality relation on objects.
abstract class Equality<E> {
abstract interface class Equality<E> {
const factory Equality() = DefaultEquality<E>;

/// Compare two elements for being equal.
Expand Down Expand Up @@ -46,7 +46,7 @@ abstract class Equality<E> {
///
/// It's also possible to pass an additional equality instance that should be
/// used to compare the value itself.
class EqualityBy<E, F> implements Equality<E> {
final class EqualityBy<E, F> implements Equality<E> {
final F Function(E) _comparisonKey;

final Equality<F> _inner;
Expand Down Expand Up @@ -81,7 +81,7 @@ class EqualityBy<E, F> implements Equality<E> {
/// Note that [equals] and [hash] take `Object`s rather than `E`s. This allows
/// `E` to be inferred as `Null` in const contexts where `E` wouldn't be a
/// compile-time constant, while still allowing the class to be used at runtime.
class DefaultEquality<E> implements Equality<E> {
final class DefaultEquality<E> implements Equality<E> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended an implementation class for the Equality interface, and implementation classes should be final.

I can see how this can also be used as a skeleton implementation, where you override only the parts that differ from default equality. In which case making it open makes sense.
It was not the intent, but it's not a completely unreasonable use. So, let's open.

const DefaultEquality();
@override
bool equals(Object? e1, Object? e2) => e1 == e2;
Expand All @@ -92,7 +92,7 @@ class DefaultEquality<E> implements Equality<E> {
}

/// Equality of objects that compares only the identity of the objects.
class IdentityEquality<E> implements Equality<E> {
final class IdentityEquality<E> implements Equality<E> {
const IdentityEquality();
@override
bool equals(E e1, E e2) => identical(e1, e2);
Expand All @@ -109,7 +109,7 @@ class IdentityEquality<E> implements Equality<E> {
/// The [equals] and [hash] methods accepts `null` values,
/// even if the [isValidKey] returns `false` for `null`.
/// The [hash] of `null` is `null.hashCode`.
class IterableEquality<E> implements Equality<Iterable<E>> {
final class IterableEquality<E> implements Equality<Iterable<E>> {
final Equality<E?> _elementEquality;
const IterableEquality(
[Equality<E> elementEquality = const DefaultEquality<Never>()])
Expand Down Expand Up @@ -161,7 +161,7 @@ class IterableEquality<E> implements Equality<Iterable<E>> {
/// The [equals] and [hash] methods accepts `null` values,
/// even if the [isValidKey] returns `false` for `null`.
/// The [hash] of `null` is `null.hashCode`.
class ListEquality<E> implements Equality<List<E>> {
final class ListEquality<E> implements Equality<List<E>> {
final Equality<E> _elementEquality;
const ListEquality(
[Equality<E> elementEquality = const DefaultEquality<Never>()])
Expand Down Expand Up @@ -202,7 +202,7 @@ class ListEquality<E> implements Equality<List<E>> {
bool isValidKey(Object? o) => o is List<E>;
}

abstract class _UnorderedEquality<E, T extends Iterable<E>>
abstract final class _UnorderedEquality<E, T extends Iterable<E>>
implements Equality<T> {
final Equality<E> _elementEquality;

Expand Down Expand Up @@ -251,7 +251,8 @@ abstract class _UnorderedEquality<E, T extends Iterable<E>>
/// Two iterables are considered equal if they have the same number of elements,
/// and the elements of one set can be paired with the elements
/// of the other iterable, so that each pair are equal.
class UnorderedIterableEquality<E> extends _UnorderedEquality<E, Iterable<E>> {
final class UnorderedIterableEquality<E>
extends _UnorderedEquality<E, Iterable<E>> {
const UnorderedIterableEquality(
[super.elementEquality = const DefaultEquality<Never>()]);

Expand All @@ -271,7 +272,7 @@ class UnorderedIterableEquality<E> extends _UnorderedEquality<E, Iterable<E>> {
/// The [equals] and [hash] methods accepts `null` values,
/// even if the [isValidKey] returns `false` for `null`.
/// The [hash] of `null` is `null.hashCode`.
class SetEquality<E> extends _UnorderedEquality<E, Set<E>> {
final class SetEquality<E> extends _UnorderedEquality<E, Set<E>> {
const SetEquality([super.elementEquality = const DefaultEquality<Never>()]);

@override
Expand All @@ -282,7 +283,7 @@ class SetEquality<E> extends _UnorderedEquality<E, Set<E>> {
///
/// The class represents a map entry as a single object,
/// using a combined hashCode and equality of the key and value.
class _MapEntry {
final class _MapEntry {
final MapEquality equality;
final Object? key;
final Object? value;
Expand All @@ -309,7 +310,7 @@ class _MapEntry {
/// The [equals] and [hash] methods accepts `null` values,
/// even if the [isValidKey] returns `false` for `null`.
/// The [hash] of `null` is `null.hashCode`.
class MapEquality<K, V> implements Equality<Map<K, V>> {
final class MapEquality<K, V> implements Equality<Map<K, V>> {
final Equality<K> _keyEquality;
final Equality<V> _valueEquality;
const MapEquality(
Expand Down Expand Up @@ -372,7 +373,7 @@ class MapEquality<K, V> implements Equality<Map<K, V>> {
/// for `equals(e1, e2)` and `equals(e2, e1)`. This can happen if one equality
/// considers only `e1` a valid key, and not `e2`, but an equality which is
/// checked later, allows both.
class MultiEquality<E> implements Equality<E> {
final class MultiEquality<E> implements Equality<E> {
final Iterable<Equality<E>> _equalities;

const MultiEquality(Iterable<Equality<E>> equalities)
Expand Down Expand Up @@ -418,7 +419,7 @@ class MultiEquality<E> implements Equality<E> {
///
/// A list is only equal to another list, likewise for sets and maps. All other
/// iterables are compared as iterables only.
class DeepCollectionEquality implements Equality {
final class DeepCollectionEquality implements Equality {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mockito extends this class.

https://github.com/dart-lang/mockito/blob/56173fa356c7017e1d2137ce3600963518bf497d/lib/src/invocation_matcher.dart#L155

We could rewrite that to be implements Equality and use a const DeepCollectionEquality() to reuse the required behavior - I might take a look at doing that anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how the recursive behavior of creating equalities with this as member equality makes this a reasonable base class.

A subclass can override equals to check for more types, the call super.equals to check the existing types, and still get called recursively with the object which checks for new types.

So, it's actually a reasonable use-case to subclass and extend. Let's open it.

final Equality _base;
final bool _unordered;
const DeepCollectionEquality([Equality base = const DefaultEquality<Never>()])
Expand Down Expand Up @@ -476,7 +477,7 @@ class DeepCollectionEquality implements Equality {
/// String equality that's insensitive to differences in ASCII case.
///
/// Non-ASCII characters are compared as-is, with no conversion.
class CaseInsensitiveEquality implements Equality<String> {
final class CaseInsensitiveEquality implements Equality<String> {
const CaseInsensitiveEquality();

@override
Expand Down
1 change: 1 addition & 0 deletions lib/src/functions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'dart:collection';
import 'dart:math' as math;

import 'map_extensions.dart';
import 'utils.dart';

/// Creates a new map from [map] with new keys and values.
Expand Down
8 changes: 8 additions & 0 deletions lib/src/map_extensions.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// 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.

extension MapExtensions<K, V> on Map<K, V> {
/// Like [Map.entries], but returns each entry as a record.
Iterable<(K, V)> get pairs => entries.map((e) => (e.key, e.value));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting...but why? What's the upside here? What do users get?

We're already going to allocate (however short-lived) MapEnty instances.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main benefit is being able to loop over pairs with variables named something other than "key" and "value".

#289 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're still hoping to make MapEntry an inline class over a (K, V), and if we are able to do that this .pairs extension will be useless. I think it's worth having it early in this package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it will be over (K, V). More likely it's ({K key, V value}), since that will allow dynamic-typed code to keep working.

}
4 changes: 2 additions & 2 deletions lib/src/priority_queue.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import 'utils.dart';
/// If elements override [Object.==], the `comparison` function must
/// always give equal objects the same priority,
/// otherwise [contains] or [remove] might not work correctly.
abstract class PriorityQueue<E> {
abstract interface class PriorityQueue<E> {
/// Creates an empty [PriorityQueue].
///
/// The created [PriorityQueue] is a plain [HeapPriorityQueue].
Expand Down Expand Up @@ -168,7 +168,7 @@ abstract class PriorityQueue<E> {
/// and is linear, O(n).
/// * The [toSet] operation effectively adds each element to the new set, taking
/// an expected O(n*log(n)) time.
class HeapPriorityQueue<E> implements PriorityQueue<E> {
final class HeapPriorityQueue<E> implements PriorityQueue<E> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an internal class which extends this.

/// A [PriorityQueue] that works like a normal queue for elements that compare
/// as equal, that is, returns them in the order they were added.
///
/// Takes an optional [Comparator] parameter that should be used instead of the
/// default [Comparable.compare].
class _StablePriorityQueue<T extends Comparable<Object>>
    extends HeapPriorityQueue<T> {

It seems to record the values that are added in a second collection and uses it to set the order for "equal" elements. At first glance I don't see a clean way to implement it without extends

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a very good example of subclassing.

For example that implementation doesn't work correctly if the same item is added more than once, and there are other elements which compare equal (comparison result zero), but are not == equal. (It's based on keeping a secondary priority on the side, but in an equality-based hash map, and it updates that priority on each add, so if the same (==) value occurs more than once, adding it again can change how elements already inside the heap compares. That's a recipe for disaster.)

And it's lucky that the implementation of addAll in HeapPriorityQueue doesn't call add, because then it would be double-registering its elements. Classic fragile base-class issue.

Wrapping seems like it would be easier and safer than subclassing in this situation. ("Favor composition over inheritance"!)

import "package:collection/collection.dart" show HeapPriorityQueue;

class _StablePriorityQueue<T> {
  final HeapPriorityQueue<(T, int)> _queue;
  int _counter = 0;

  _StablePriorityQueue(Comparator<T> comparison)
      : _queue = HeapPriorityQueue(_orderedComparator<T>(comparison));

  static int Function((T, int), (T, int)) _orderedComparator<T>(
          int Function(T, T) comparison) =>
      ((T, int) a, (T, int) b) {
        var result = comparison(a.$1, b.$1);
        if (result != 0) return result;
        return a.$2.compareTo(b.$2); // or just: return a.$2 - b.$2;
      };

  void add(T element) {
    _queue.add((element, _counter++));
  }

  bool get isNotEmpty => _queue.isNotEmpty;

  T removeFirst() => _queue.removeFirst().$1;

  void clear() {
    _queue.clear();
    _counter = 0; // For good measure, not really needed.
  }
}

It's a private class, so by not extending a larger interface, it only needs to care about the features that are actually needed by the code in the same library. If it doesn't use removeAll, it doesn't need to implement it. If it doesn't use remove(T), it doesn't have to worry about whether the implementation can support it (this one can't, and that's OK).

It even works if you add the same element more than once, because the comparison is stable and based only on the values in the queue, not extra data on the side.
And it's shorter and simpler, even with the helper class you'd need before we had records.

After talking it up so much, I' guess I should just go and make the change 😁

/// Initial capacity of a queue when created, or when added to after a
/// [clear].
///
Expand Down
6 changes: 4 additions & 2 deletions lib/src/queue_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import 'dart:collection';
// dart:collection. The only changes are to implement List and to remove methods
// that are redundant with ListMixin. Remove or simplify it when issue 21330 is
// fixed.
class QueueList<E> extends Object with ListMixin<E> implements Queue<E> {
interface class QueueList<E> extends Object
with ListMixin<E>
implements Queue<E> {
/// Adapts [source] to be a `QueueList<T>`.
///
/// Any time the class would produce an element that is not a [T], the element
Expand Down Expand Up @@ -274,7 +276,7 @@ class QueueList<E> extends Object with ListMixin<E> implements Queue<E> {
}
}

class _CastQueueList<S, T> extends QueueList<T> {
final class _CastQueueList<S, T> extends QueueList<T> {
final QueueList<S> _delegate;

// Assigns invalid values for head/tail because it uses the delegate to hold
Expand Down
2 changes: 1 addition & 1 deletion lib/src/union_set_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import 'union_set.dart';
/// }
/// }
/// ```
class UnionSetController<E> {
interface class UnionSetController<E> {
/// The [UnionSet] that provides a view of the union of sets in `this`.
final UnionSet<E> set;

Expand Down
6 changes: 3 additions & 3 deletions lib/src/unmodifiable_wrappers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class NonGrowableListView<E> extends DelegatingList<E>

/// Mixin class that implements a throwing version of all list operations that
/// change the List's length.
abstract class NonGrowableListMixin<E> implements List<E> {
abstract mixin class NonGrowableListMixin<E> implements List<E> {
natebosch marked this conversation as resolved.
Show resolved Hide resolved
static Never _throw() {
throw UnsupportedError('Cannot change the length of a fixed-length list');
}
Expand Down Expand Up @@ -116,7 +116,7 @@ class UnmodifiableSetView<E> extends DelegatingSet<E>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to self: The UnmodifiableSetView, and all similar classes, should wrap the return value of cast (or any other modifiable view on the original that it exposes).

/// Mixin class that implements a throwing version of all set operations that
/// change the Set.
abstract /*mixin*/ class UnmodifiableSetMixin<E> implements Set<E> {
abstract mixin class UnmodifiableSetMixin<E> implements Set<E> {
static Never _throw() {
throw UnsupportedError('Cannot modify an unmodifiable Set');
}
Expand Down Expand Up @@ -164,7 +164,7 @@ abstract /*mixin*/ class UnmodifiableSetMixin<E> implements Set<E> {

/// Mixin class that implements a throwing version of all map operations that
/// change the Map.
abstract /*mixin*/ class UnmodifiableMapMixin<K, V> implements Map<K, V> {
abstract mixin class UnmodifiableMapMixin<K, V> implements Map<K, V> {
static Never _throw() {
throw UnsupportedError('Cannot modify an unmodifiable Map');
}
Expand Down
4 changes: 2 additions & 2 deletions pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
name: collection
version: 1.17.2
version: 1.18.0
description: Collections and utilities functions and classes related to collections.
repository: https://github.com/dart-lang/collection

environment:
sdk: ">=2.18.0 <4.0.0"
sdk: ^3.0.0

dev_dependencies:
lints: ^2.0.0
Expand Down
17 changes: 17 additions & 0 deletions test/extensions_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,23 @@ void main() {
});
});
});

group('Map', () {
group('pairs', () {
test('empty map', () {
expect(<int, int>{}.pairs, isEmpty);
});

test('single pair', () {
expect(<int, int>{1: 2}.pairs, equals([(1, 2)]));
});

test('multiple pairs', () {
expect(<int, int>{1: 2, 3: 4, 5: 6}.pairs,
equals([(1, 2), (3, 4), (5, 6)]));
});
});
});
}

/// Creates a plain iterable not implementing any other class.
Expand Down