conceptadev / conceptadev/mix

DeepCollectionEquality: throws on mismatched collection shapes, and never compares non-List iterables as equal

Open
#1,036 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Dart
Stars
800
Forks
49
Avg merge
3h 39m
Merged PRs (30d)
22

Description

Two issues in `DeepCollectionEquality` (`packages/mix/lib/src/core/internal/deep_collection_equality.dart` on `main`). This class backs `Equatable.==`/`hashCode`, so it is on the equality path for every `Spec` and `Mix` value.

### 1. `equals` throws instead of returning `false` for mismatched shapes

Only the first operand is type-tested; the second is cast blind:

```dart
bool equals(Object? obj1, Object? obj2) {
if (identical(obj1, obj2)) return true;
if (obj1 == null || obj2 == null) return false;

if (obj1 is Map) {
return _mapsEqual(obj1, obj2 as Map); // <-- unguarded
} else if (obj1 is Set) {
return _setsEqual(obj1, obj2 as Set); // <-- unguarded
} else if (obj1 is Iterable) {
return _iterablesEqual(obj1, obj2 as Iterable); // <-- unguarded
}

return obj1 == obj2;
}
```

Repro:

```dart
const DeepCollectionEquality().equals({1}, [1]);
// TypeError: List is not a subtype of Set
```

An equality predicate should answer `false` for mismatched shapes rather than throw. It is reachable from `Equatable.==` via `equatable.dart`, which likewise passes `unit2` unchecked once `unit1` is found to be an `Iterable`/`Map`. In practice `Equatable.==` guards on `runtimeType` first, so hitting this needs a prop whose static type admits differing shapes (`Object?`, or a bare `Iterable`) — but the guard is incidental, not by design.

Note the same file already gets this right one method up, in `_iterablesEqual`, which tests `iter1 is List && iter2 is List`.

### 2. `_iterablesEqual` returns `false` for two equal non-`List` iterables

```dart
bool _iterablesEqual(Iterable iter1, Iterable iter2) {
if (iter1.length != iter2.length) return false;
if (iter1 is List && iter2 is List) {
for (int i = 0; i < iter1.length; i++) {
if (!equals(iter1[i], iter2[i])) return false;
}
return true;
}

return false; // <-- any non-List iterable pair
}
```

This contradicts the method's own doc comment ("The iterables are equal if they have the same length and contain the same
elements in the same order"). `Set` and `Map` are dispatched earlier in `equals`, so the fallthrough is reached by lazy iterables and other non-`List` collections:

```dart
final a = [1, 2].map((e) => e);
final b = [1, 2].map((e) => e);
const DeepCollectionEquality().equals(a, b); // false
```

Consequence for a spec or `Mix` with an `Iterable`-typed prop: it never compares equal to itself, so `==` is always `false` and dependent rebuilds/lerps never short-circuit.

I checked and found no prop or spec field in `mix`, `mix_winds`, or `mix_chart` currently typed as a non-`List` `Iterable`, so this looks latent rather than actively biting.

### Secondary observation (not a correctness bug)

`hash` XORs element hashes for `Iterable`, making it order-insensitive, while `equals` is order-sensitive for `List`. So `[1, 2]` and `[2, 1]` are unequal but hash identically. That is a legal hash (collisions are permitted) and only costs bucket distribution, but it is worth a look if this code is revisited.

### Suggested handling

Issue 1 is a safe fix — guarding both operands only converts a crash into the correct `false`, with no behavior change for any input that works today. **This one is already fixed** in the `mix_core` extraction work (the file moved to `mix_core` verbatim and was corrected there).

Issue 2 is deliberately left alone in that branch: making it compare element-wise changes `==` — and therefore rebuild behavior — for any `Iterable`-typed prop library-wide, and the current tests do not cover it. It seems worth deciding explicitly rather than as a drive-by.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with packages/mix/lib/src/core/internal/deep_collection_equality.dart and trace equals and _iterablesEqual, then compare the related path in equatable.dart. Check whether the mix_core extraction is present before changing anything. Done means mismatched shapes no longer throw, and the non-List iterable behavior has an explicit decision supported by appropriate equality tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
dart, flutter
Domain
design, mobile-dev
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.