rrousselGit / flutter_hooks

React hooks for Flutter. Hooks are a new kind of object that manages a Widget life-cycles. They are used to increase code sharing between widgets and as a complete replacement for StatefulWidget.
MIT License
3.13k stars 179 forks source link

[BUG] shouldPreserveState may wrongfully preserve state when it shouldn't #384

Closed AlexDochioiu closed 1 year ago

AlexDochioiu commented 1 year ago

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:

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.

// Checks if both are NaN
        if (curr1.isNaN && curr2.isNaN) {
          return true;
        }

and

// Checks if one is 0.0 and the other is -0.0
        if (curr1 == 0 && curr2 == 0) {
          return curr1.isNegative == curr2.isNegative;
        }

The solution?

// Checks if both are NaN
        if (curr1.isNaN && curr2.isNaN) {
          continue;
        }

and

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

rrousselGit commented 1 year ago

Good catch, I missed it during code reviews. Fixing