Update code to Dart 3.0 #284

lrhn · 2023-05-03T09:46:04Z

Add required mixins. Add some appropriate finals.

Should this be a major version increment?

The final modifiers are breaking. (But so is the lack of mixin modifiers on some classes.)

Fix local variable typos and add comments preparing for 3.0 migration.

pubspec.yaml

natebosch · 2023-05-03T15:44:24Z

Should this be a major version increment?

For most packages, yes.

For this package, which is pinned by flutter, it may be easier to cheat and ship without a major version change.

lrhn · 2023-05-03T15:47:45Z

I don't think we're time-bound on this release. The internal need to make the code 3.0 compatible has been fixed, so staying as 2.18.0 is fine for now.
We want to release a 3.0 version eventually, where we add modifiers (and the sooner we do so, the sooner people can see which modifiers they need to comply with).

lrhn · 2023-05-03T17:34:18Z

(Need to fix CI too.)

natebosch · 2023-05-03T19:19:56Z

CHANGELOG.md

+
+* Require Dart 3.0.
+* Mark mixin classes as `mixin class`.
+
 ## 1.17.2


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

natebosch · 2023-05-03T19:22:09Z

I'm starting an internal presubmit to see if any usage is impacted by the change.

lib/src/unmodifiable_wrappers.dart

natebosch · 2023-05-03T19:32:21Z

lib/src/canonicalized_map.dart

@@ -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> {


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?

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)

(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.

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.

natebosch · 2023-05-03T19:50:39Z

lib/src/equality.dart

@@ -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 {


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.

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.

natebosch · 2023-05-03T20:00:30Z

lib/src/priority_queue.dart

@@ -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> {


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

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 😁

natebosch · 2023-05-03T21:07:45Z

lib/src/equality.dart

@@ -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> {


https://github.com/angulardart/angular/blob/e943f6f0b8f8ae0d64c2c7935d4554f41d86e2cd/angular/lib/src/runtime/check_binding.dart#L186-L190

This has a usage of extends

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.

lrhn · 2023-05-04T08:43:49Z

lib/src/equality.dart

@@ -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> {


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.

lrhn · 2023-05-04T08:46:36Z

lib/src/equality.dart

@@ -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 {


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.

lrhn · 2023-05-04T09:25:27Z

lib/src/priority_queue.dart

@@ -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> {


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 😁

lib/src/unmodifiable_wrappers.dart

lrhn · 2023-05-04T09:29:43Z

lib/src/unmodifiable_wrappers.dart

@@ -116,7 +116,7 @@ class UnmodifiableSetView<E> extends DelegatingSet<E>



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).

lrhn · 2023-05-04T09:36:09Z

lib/src/canonicalized_map.dart

@@ -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> {


(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.

natebosch · 2023-05-26T00:05:57Z

The full set of modifiers in this CL which caused build failures in google3 are CanonicalizedMap, DefaultEquality, MapEquality, DeepCollectionEquality, and HeapPriorityQueue. I think you fixed the usage of HeapPriorityQueue but I think the others will still cause a problem. When I comment out the added modifiers on those classes I can get a TAP global without any related build failures.

Add `Map.pairs` getter for more flexibility with patterns than `entries`.

kevmoo · 2023-05-30T20:00:42Z

lib/src/map_extensions.dart

+
+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));


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

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

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

#289 (comment)

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.

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.

nex3 · 2024-01-03T23:24:30Z

What's the status of this PR? I'm itching to use Map.pairs in my packages.

dannnnthemannnn · 2024-03-15T19:02:43Z

Hey all, I'm trying to use collections.dart and am getting errors like:

packages/collection/src/boollist.dart:231:50:
Error: The class 'NonGrowableListMixin' can't be used as a mixin because it isn't a mixin class nor a mixin.
class _NonGrowableBoolList extends BoolList with NonGrowableListMixin<bool> {

This is pulled in via transitive dependencies.

My dart version:

dart --version
Dart SDK version: 3.3.1 (stable) (Wed Mar 6 13:09:19 2024 +0000) on "macos_arm64"

I see above people saying it isnt a problem but how can I mitigate this?

lrhn added 4 commits April 21, 2023 16:58

Accept SDK version above 3.0.

b78de08

Fix local variable typos and add comments preparing for 3.0 migration.

Update package:collection to Dart 3.0.

8f961ff

WIP

88aa113

Update pubspec.yaml

b6ce4b5

lrhn requested review from natebosch and mit-mit May 3, 2023 09:46

Merge branch '3.0-compat' into 3.0

72a6789

devoncarew reviewed May 3, 2023

View reviewed changes

pubspec.yaml Outdated Show resolved Hide resolved

Base automatically changed from 3.0-compat to master May 3, 2023 17:28

Merge branch 'master' into 3.0

4ef876d

natebosch reviewed May 3, 2023

View reviewed changes

lib/src/unmodifiable_wrappers.dart Show resolved Hide resolved

natebosch reviewed May 3, 2023

View reviewed changes

lrhn added 2 commits May 4, 2023 10:39

Use hat-notation for SDK dependency

0bbee8d

Update unmodifiable_wrappers.dart

c722cdc

lrhn commented May 4, 2023

View reviewed changes

Add extension method for converting map entries to records (#289)

6ac3b5c

Add `Map.pairs` getter for more flexibility with patterns than `entries`.

kevmoo reviewed May 30, 2023

View reviewed changes

natebosch mentioned this pull request Jan 12, 2024

Add flattenedToList and flattenedToSet #328

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update code to Dart 3.0 #284

Update code to Dart 3.0 #284

lrhn commented May 3, 2023

natebosch commented May 3, 2023

lrhn commented May 3, 2023

lrhn commented May 3, 2023

natebosch May 3, 2023

natebosch commented May 3, 2023

natebosch May 3, 2023

natebosch May 3, 2023

lrhn May 4, 2023

natebosch May 4, 2023

natebosch May 3, 2023

lrhn May 4, 2023

natebosch May 3, 2023

lrhn May 4, 2023

natebosch May 3, 2023

lrhn May 4, 2023

lrhn May 4, 2023

lrhn May 4, 2023

lrhn May 4, 2023

lrhn May 4, 2023

lrhn May 4, 2023

natebosch commented May 26, 2023

kevmoo May 30, 2023

natebosch May 30, 2023

natebosch May 30, 2023

lrhn May 31, 2023

nex3 commented Jan 3, 2024

dannnnthemannnn commented Mar 15, 2024

		@@ -116,7 +116,7 @@ class UnmodifiableSetView<E> extends DelegatingSet<E>

Update code to Dart 3.0 #284

Are you sure you want to change the base?

Update code to Dart 3.0 #284

Conversation

lrhn commented May 3, 2023

natebosch commented May 3, 2023

lrhn commented May 3, 2023

lrhn commented May 3, 2023

Choose a reason for hiding this comment

natebosch commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natebosch commented May 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 commented Jan 3, 2024

dannnnthemannnn commented Mar 15, 2024