Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Update code to Dart 3.0 #284

Closed
wants to merge 9 commits into from
Closed

Update code to Dart 3.0 #284

wants to merge 9 commits into from

Conversation

lrhn
Copy link
Contributor

@lrhn lrhn commented May 3, 2023

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

lrhn added 4 commits April 21, 2023 16:58
@lrhn lrhn requested review from natebosch and mit-mit May 3, 2023 09:46
pubspec.yaml Outdated Show resolved Hide resolved
@natebosch
Copy link
Contributor

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
Copy link
Contributor Author

lrhn commented May 3, 2023

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

Base automatically changed from 3.0-compat to master May 3, 2023 17:28
@lrhn
Copy link
Contributor Author

lrhn commented May 3, 2023

(Need to fix CI too.)


* Require Dart 3.0.
* Mark mixin classes as `mixin class`.

## 1.17.2
Copy link
Contributor

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.

@natebosch
Copy link
Contributor

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

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

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
Contributor

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
Contributor 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
Contributor

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.

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

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

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

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
Contributor 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 😁

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

Choose a reason for hiding this comment

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

Copy link
Contributor 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.

@@ -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
Contributor 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.

@@ -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
Contributor 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.

@@ -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
Contributor 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 😁

lib/src/unmodifiable_wrappers.dart Show resolved Hide resolved
@@ -116,7 +116,7 @@ class UnmodifiableSetView<E> extends DelegatingSet<E>

Copy link
Contributor 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).

@@ -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
Contributor 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.

@natebosch
Copy link
Contributor

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

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
Contributor

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
Contributor

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
Contributor

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

@nex3
Copy link
Contributor

nex3 commented Jan 3, 2024

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

@dannnnthemannnn
Copy link

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?

@mosuem
Copy link
Contributor

mosuem commented Oct 21, 2024

Closing as the dart-lang/collection repository is merged into the dart-lang/core monorepo. Please re-open this PR there!

@mosuem mosuem closed this Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants