Repository: rrousselGit/flutter_hooks (dart)

Issue:
[BUG] shouldPreserveState may wrongfully preserve state when it shouldn't
Was looking into the library's code to see how the hook keys are being compared, when I noticed what I think to be a potential bug?

The (unmodified) library code in question is:
```dart
static bool shouldPreserveState(Hook<Object?> hook1, Hook<Object?> hook2) {
    final p1 = hook1.keys;
    final p2 = hook2.keys;

    if (p1 == p2) {
      return true;
    }
    // if one list is null and the other one isn't, or if they have different sizes
    if (p1 == null || p2 == null || p1.length != p2.length) {
      return false;
    }

    final i1 = p1.iterator;
    final i2 = p2.iterator;
    // ignore: literal_only_boolean_expressions, returns will abort the loop
    while (true) {
      if (!i1.moveNext() || !i2.moveNext()) {
        return true;
      }

      final curr1 = i1.current;
      final curr2 = i2.current;

      if (curr1 is num && curr2 is num) {
        // Checks if both are NaN
        if (curr1.isNaN && curr2.isNaN) {
          return true;
        }

        // Checks if one is 0.0 and the other is -0.0
        if (curr1 == 0 && curr2 == 0) {
          return curr1.isNegative == curr2.isNegative;
        }
      }

      if (curr1 != curr2) {
        return false;
      }
    }
  }
  ```

## The problematic snippets

Both of those checks can lead to state preservation **before all keys are compared**. 

```dart
// Checks if both are NaN
        if (curr1.isNaN && curr2.isNaN) {
          return true;
        }
```
and
```dart 
// Checks if one is 0.0 and the other is -0.0
        if (curr1 == 0 && curr2 == 0) {
          return curr1.isNegative == curr2.isNegative;
        }
```

## The solution?
```dart
// Checks if both are NaN
        if (curr1.isNaN && curr2.isNaN) {
          continue;
        }
```
and
```dart 
// Checks if one is 0.0 and the other is -0.0
        if (curr1 == 0 && curr2 == 0) {
          if (curr1.isNegative != curr2.isNegative) {
            return false;
          } else {
            continue;
          }
        }
```

P.s. Hopefully I'm not missing something in my analysis

Pull request description:
fixes #384

Patch:
diff --git a/packages/flutter_hooks/CHANGELOG.md b/packages/flutter_hooks/CHANGELOG.md
index 89e31791..6a4e95c8 100644
--- a/packages/flutter_hooks/CHANGELOG.md
+++ b/packages/flutter_hooks/CHANGELOG.md
@@ -1,3 +1,7 @@
+## Unreleased patch
+
+- Fixed key matching algorithm incorrectly not checking all keys if a 0 or a NaN is present (thanks to @AlexDochioiu)
+
 ## 0.20.1 - 2023-08-29
 
 - `useListenableSelector` now supports `null` listeners (thanks to @JWo1F)
diff --git a/packages/flutter_hooks/lib/src/framework.dart b/packages/flutter_hooks/lib/src/framework.dart
index d3104a9b..36ff6081 100644
--- a/packages/flutter_hooks/lib/src/framework.dart
+++ b/packages/flutter_hooks/lib/src/framework.dart
@@ -183,12 +183,15 @@ Calling them outside of build method leads to an unstable state and is therefore
       if (curr1 is num && curr2 is num) {
         // Checks if both are NaN
         if (curr1.isNaN && curr2.isNaN) {
-          return true;
+          continue;
         }
 
         // Checks if one is 0.0 and the other is -0.0
         if (curr1 == 0 && curr2 == 0) {
-          return curr1.isNegative == curr2.isNegative;
+          if (curr1.isNegative != curr2.isNegative) {
+            return false;
+          }
+          continue;
         }
       }
 


Test patch:
diff --git a/packages/flutter_hooks/test/use_effect_test.dart b/packages/flutter_hooks/test/use_effect_test.dart
index d3a0c430..a6c42aa4 100644
--- a/packages/flutter_hooks/test/use_effect_test.dart
+++ b/packages/flutter_hooks/test/use_effect_test.dart
@@ -268,6 +268,29 @@ void main() {
     verifyNoMoreInteractions(effect);
   });
 
+  testWidgets(
+      'calls effect when first key is two matching NaN but second key is different',
+      (tester) async {
+    // Regression test for https://github.com/rrousselGit/flutter_hooks/issues/384
+    parameters = [double.nan, 0];
+    await tester.pumpWidget(builder());
+
+    verifyInOrder([
+      effect(),
+      unrelated(),
+    ]);
+    verifyNoMoreInteractions(effect);
+
+    parameters = [double.nan, 1];
+    await tester.pumpWidget(builder());
+
+    verifyInOrder([
+      effect(),
+      unrelated(),
+    ]);
+    verifyNoMoreInteractions(effect);
+  });
+
   testWidgets(
       'calls effect when one of keys is changed from 0.0 to -0.0 and vice versa',
       (tester) async {
@@ -298,6 +321,29 @@ void main() {
     ]);
     verifyNoMoreInteractions(effect);
   });
+
+  testWidgets(
+      'calls effect when first key is two matching 0 with same sign, but second key is different',
+      (tester) async {
+    // Regression test for https://github.com/rrousselGit/flutter_hooks/issues/384
+    parameters = [-0, 42];
+    await tester.pumpWidget(builder());
+
+    verifyInOrder([
+      effect(),
+      unrelated(),
+    ]);
+    verifyNoMoreInteractions(effect);
+
+    parameters = [-0, 43];
+    await tester.pumpWidget(builder());
+
+    verifyInOrder([
+      effect(),
+      unrelated(),
+    ]);
+    verifyNoMoreInteractions(effect);
+  });
 }
 
 class MockEffect extends Mock {