Fix Ref.invalidate/Ref.refresh not throwing on circular dependency (#…

…3155) fixes #2336
rrousselGit · Nov 26, 2023 · b7056b5 · b7056b5
1 parent 036eac1
commit b7056b5
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 68 deletions.
diff --git a/packages/riverpod/CHANGELOG.md b/packages/riverpod/CHANGELOG.md
@@ -1,3 +1,7 @@
+## Unreleased fix
+
+- Fix `Ref.invalidate`/`Ref.refresh` not throwing on circular dependency.
+
 ## 2.4.8 - 2023-11-20
 
 Fix exceptions when using multiple root `ProviderContainers`/`ProviderScopes`.

diff --git a/packages/riverpod/lib/src/framework/element.dart b/packages/riverpod/lib/src/framework/element.dart
@@ -287,6 +287,7 @@ abstract class ProviderElementBase<State> implements Ref<State>, Node {
 
   @override
   void invalidate(ProviderOrFamily provider) {
+    assert(_debugAssertCanDependOn(provider), '');
     _container.invalidate(provider);
   }
 
@@ -615,7 +616,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu
     );
   }
 
-  bool _debugAssertCanDependOn(ProviderListenable<Object?> listenable) {
+  bool _debugAssertCanDependOn(ProviderListenableOrFamily listenable) {
     assert(
       () {
         if (listenable is! ProviderBase<Object?>) return true;
@@ -678,6 +679,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu
   @override
   T refresh<T>(Refreshable<T> provider) {
     _assertNotOutdated();
+    assert(_debugAssertCanDependOn(provider), '');
     return _container.refresh(provider);
   }
 

diff --git a/packages/riverpod/lib/src/framework/foundation.dart b/packages/riverpod/lib/src/framework/foundation.dart
@@ -2,7 +2,7 @@ part of '../framework.dart';
 
 /// A common interface shared by [ProviderBase] and [Family]
 @sealed
-abstract class ProviderOrFamily {
+abstract class ProviderOrFamily implements ProviderListenableOrFamily {
   /// A common interface shared by [ProviderBase] and [Family]
   const ProviderOrFamily({
     required this.name,
@@ -81,6 +81,9 @@ abstract class ProviderOrFamily {
   final Iterable<ProviderOrFamily>? allTransitiveDependencies;
 }
 
+/// A shared interface between [ProviderListenable] and [Family].
+abstract class ProviderListenableOrFamily {}
+
 /// Computes the list of all dependencies of a provider.
 @internal
 Set<ProviderOrFamily>? computeAllTransitiveDependencies(
@@ -132,7 +135,7 @@ String shortHash(Object? object) {
 ///
 /// Should override ==/hashCode when possible
 @immutable
-mixin ProviderListenable<State> {
+mixin ProviderListenable<State> implements ProviderListenableOrFamily {
   /// Starts listening to this transformer
   ProviderSubscription<State> addListener(
     Node node,

diff --git a/packages/riverpod/test/framework/provider_element_test.dart b/packages/riverpod/test/framework/provider_element_test.dart
@@ -125,103 +125,160 @@ void main() {
     });
   });
 
-  group('ref.invalidate on families', () {
-    test('recomputes providers associated with the family', () async {
-      final container = createContainer();
-      final listener = Listener<String>();
-      final listener2 = Listener<String>();
-      final listener3 = Listener<int>();
-      var result = 0;
-      final unrelated = Provider((ref) => result);
-      final provider = Provider.family<String, int>((r, i) => '$result-$i');
+  group('ref.refresh', () {
+    test('Throws if a circular dependency is detected', () {
+      // Regression test for https://github.com/rrousselGit/riverpod/issues/2336
       late Ref ref;
-      final another = Provider((r) {
+      final a = Provider((r) {
         ref = r;
+        return 0;
       });
+      final b = Provider((r) => r.watch(a));
+      final container = createContainer();
 
-      container.read(another);
+      container.read(b);
 
-      container.listen(provider(0), listener.call, fireImmediately: true);
-      container.listen(provider(1), listener2.call, fireImmediately: true);
-      container.listen(unrelated, listener3.call, fireImmediately: true);
+      expect(
+        () => ref.refresh(b),
+        throwsA(isA<CircularDependencyError>()),
+      );
+    });
+  });
 
-      verifyOnly(listener, listener(null, '0-0'));
-      verifyOnly(listener2, listener2(null, '0-1'));
-      verifyOnly(listener3, listener3(null, 0));
+  group('ref.invalidate', () {
+    test('Throws if a circular dependency is detected', () {
+      // Regression test for https://github.com/rrousselGit/riverpod/issues/2336
+      late Ref ref;
+      final a = Provider((r) {
+        ref = r;
+        return 0;
+      });
+      final b = Provider((r) => r.watch(a));
+      final container = createContainer();
 
-      ref.invalidate(provider);
-      ref.invalidate(provider);
-      result = 1;
+      container.read(b);
 
-      verifyNoMoreInteractions(listener);
-      verifyNoMoreInteractions(listener2);
-      verifyNoMoreInteractions(listener3);
+      expect(
+        () => ref.invalidate(b),
+        throwsA(isA<CircularDependencyError>()),
+      );
+    });
 
-      await container.pump();
+    test('Circular dependency ignores families', () {
+      late Ref ref;
+      final a = Provider((r) {
+        ref = r;
+        return 0;
+      });
+      final b = Provider.family<int, int>((r, id) => r.watch(a));
+      final container = createContainer();
+
+      container.read(b(0));
 
-      verifyOnly(listener, listener('0-0', '1-0'));
-      verifyOnly(listener2, listener2('0-1', '1-1'));
-      verifyNoMoreInteractions(listener3);
+      expect(
+        () => ref.invalidate(b),
+        returnsNormally,
+      );
     });
 
-    test('clears only on the closest family override', () async {
+    test('triggers a rebuild on next frame', () async {
+      final container = createContainer();
+      final listener = Listener<int>();
+      var result = 0;
+      final provider = Provider((r) => result);
       late Ref ref;
       final another = Provider((r) {
         ref = r;
       });
-      var result = 0;
-      final provider = Provider.family<int, int>((r, i) => result);
-      final listener = Listener<int>();
-      final listener2 = Listener<int>();
-      final root = createContainer();
-      final container = createContainer(
-        parent: root,
-        overrides: [provider, another],
-      );
 
+      container.listen(provider, listener.call);
       container.read(another);
-      root.listen(provider(0), listener.call, fireImmediately: true);
-      container.listen(provider(1), listener2.call, fireImmediately: true);
-
-      verifyOnly(listener, listener(null, 0));
-      verifyOnly(listener2, listener2(null, 0));
+      verifyZeroInteractions(listener);
 
+      ref.invalidate(provider);
       ref.invalidate(provider);
       result = 1;
 
-      verifyNoMoreInteractions(listener);
-      verifyNoMoreInteractions(listener2);
+      verifyZeroInteractions(listener);
 
       await container.pump();
 
-      verifyOnly(listener2, listener2(0, 1));
-      verifyNoMoreInteractions(listener);
+      verifyOnly(listener, listener(0, 1));
     });
-  });
 
-  test('ref.invalidate triggers a rebuild on next frame', () async {
-    final container = createContainer();
-    final listener = Listener<int>();
-    var result = 0;
-    final provider = Provider((r) => result);
-    late Ref ref;
-    final another = Provider((r) {
-      ref = r;
-    });
+    group('on families', () {
+      test('recomputes providers associated with the family', () async {
+        final container = createContainer();
+        final listener = Listener<String>();
+        final listener2 = Listener<String>();
+        final listener3 = Listener<int>();
+        var result = 0;
+        final unrelated = Provider((ref) => result);
+        final provider = Provider.family<String, int>((r, i) => '$result-$i');
+        late Ref ref;
+        final another = Provider((r) {
+          ref = r;
+        });
 
-    container.listen(provider, listener.call);
-    container.read(another);
-    verifyZeroInteractions(listener);
+        container.read(another);
 
-    ref.invalidate(provider);
-    ref.invalidate(provider);
-    result = 1;
+        container.listen(provider(0), listener.call, fireImmediately: true);
+        container.listen(provider(1), listener2.call, fireImmediately: true);
+        container.listen(unrelated, listener3.call, fireImmediately: true);
 
-    verifyZeroInteractions(listener);
+        verifyOnly(listener, listener(null, '0-0'));
+        verifyOnly(listener2, listener2(null, '0-1'));
+        verifyOnly(listener3, listener3(null, 0));
 
-    await container.pump();
+        ref.invalidate(provider);
+        ref.invalidate(provider);
+        result = 1;
+
+        verifyNoMoreInteractions(listener);
+        verifyNoMoreInteractions(listener2);
+        verifyNoMoreInteractions(listener3);
+
+        await container.pump();
+
+        verifyOnly(listener, listener('0-0', '1-0'));
+        verifyOnly(listener2, listener2('0-1', '1-1'));
+        verifyNoMoreInteractions(listener3);
+      });
+
+      test('clears only on the closest family override', () async {
+        late Ref ref;
+        final another = Provider((r) {
+          ref = r;
+        });
+        var result = 0;
+        final provider = Provider.family<int, int>((r, i) => result);
+        final listener = Listener<int>();
+        final listener2 = Listener<int>();
+        final root = createContainer();
+        final container = createContainer(
+          parent: root,
+          overrides: [provider, another],
+        );
+
+        container.read(another);
+        root.listen(provider(0), listener.call, fireImmediately: true);
+        container.listen(provider(1), listener2.call, fireImmediately: true);
+
+        verifyOnly(listener, listener(null, 0));
+        verifyOnly(listener2, listener2(null, 0));
+
+        ref.invalidate(provider);
+        result = 1;
+
+        verifyNoMoreInteractions(listener);
+        verifyNoMoreInteractions(listener2);
 
-    verifyOnly(listener, listener(0, 1));
+        await container.pump();
+
+        verifyOnly(listener2, listener2(0, 1));
+        verifyNoMoreInteractions(listener);
+      });
+    });
   });
 
   group('ref.invalidateSelf', () {