From b94e8ce27f5ccca5906a1aeeaf7c634167d9a095 Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Tue, 23 Aug 2022 19:12:11 -0600 Subject: [PATCH 1/8] start of mutate in widgets --- packages/riverpod_lint/bin/custom_lint.dart | 105 ++++++++++++------ .../test/goldens/mutate_in_create.dart | 57 ++++++++++ 2 files changed, 127 insertions(+), 35 deletions(-) diff --git a/packages/riverpod_lint/bin/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index 7b1167771..cbdbe6870 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -265,6 +265,29 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { final ResolvedUnitResult unit; + Stream visitCalledFunction(AstNode node) async* { + final results = node is MethodDeclaration + ? visitMethodDeclaration(node) + : node is FunctionDeclaration + ? visitFunctionDeclaration(node) + : null; + if (results != null) { + await for (final _ in results) { + yield Lint( + code: 'riverpod_no_mutate_sync', + message: + 'Do not mutate a provider synchronously, a function was called which mutates a provider synchronously', + location: unit.lintLocationFromOffset( + node.offset, + length: node.length, + ), + ); + // Only need to report the function once + return; + } + } + } + @override Stream? visitFunctionExpressionInvocation( FunctionExpressionInvocation node) async* { @@ -272,23 +295,7 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { if (method != null) { final ast = await findAstNodeForElement(method); if (ast != null) { - final decl = visitFunctionDeclaration(ast as FunctionDeclaration); - if (decl == null) { - return; - } - // If the method mutates synchronously, it is not a problem with the method necessarily, just that we call it synchronously - await for (final _ in decl) { - yield Lint( - code: 'riverpod_no_mutate_sync', - message: - 'Do not mutate a provider synchronously, a function was called which mutates a provider synchronously', - location: unit.lintLocationFromOffset( - node.offset, - length: node.length, - ), - ); - return; - } + yield* visitCalledFunction(ast as FunctionDeclaration); } } yield* super.visitFunctionExpressionInvocation(node) ?? @@ -296,28 +303,13 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { } @override - Stream? visitMethodInvocation(MethodInvocation node) async* { + Stream visitMethodInvocation(MethodInvocation node) async* { final method = node.methodName.staticElement; + print(node.methodName.name); if (method != null) { final ast = await findAstNodeForElement(method.declaration!); if (ast != null) { - final decl = visitMethodDeclaration(ast as MethodDeclaration); - if (decl == null) { - return; - } - // If the method mutates synchronously, it is not a problem with the method necessarily, just that we call it synchronously - await for (final _ in decl) { - yield Lint( - code: 'riverpod_no_mutate_sync', - message: - 'Do not mutate a provider synchronously, a method was called which mutates a provider synchronously', - location: unit.lintLocationFromOffset( - node.offset, - length: node.length, - ), - ); - return; - } + yield* visitCalledFunction(ast); } } @@ -332,7 +324,9 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { if (_futureOrStream.isAssignableFrom(constructor.enclosingElement3)) { return; } + final ast = await findAstNodeForElement(constructor.declaration); + if (ast != null) { yield* visitConstructorDeclaration(ast as ConstructorDeclaration) ?? const Stream.empty(); @@ -343,6 +337,7 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { bool synchronous = true; @override Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { + print('Visiting $node'); final last = synchronous; synchronous = true; yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); @@ -594,6 +589,21 @@ class RiverpodVisitor extends AsyncRecursiveVisitor // 'Provider\n - origin: $node\n - expected: ${dependencies.value}\n - dependencies: ${visitor.dependencies}'); } + @override + Stream visitNode(AstNode node) async* { + yield* super.visitNode(node) ?? const Stream.empty(); + if (node.isWidgetBuild ?? false) { + final syncMutationDetector = ProviderSyncMutationVisitor(unit); + final results = node is MethodDeclaration + ? syncMutationDetector.visitMethodDeclaration(node) + : node is FunctionDeclaration + ? syncMutationDetector.visitFunctionDeclaration(node) + : null; + + yield* results ?? const Stream.empty(); + } + } + Stream _checkValidProviderDeclaration(AstNode providerNode) async* { final declaration = providerNode.parents.whereType().firstOrNull; @@ -809,6 +819,30 @@ extension on FunctionExpressionInvocation { } } +extension on AstNode { + bool? get isWidgetBuild { + final expr = this; + if (expr is FunctionExpression) { + return (this as FunctionExpression).isWidgetBuilder; + } + if (expr is MethodDeclaration) { + if (expr.name2.lexeme != 'build') { + return false; + } + + final classElement = expr.parents + .whereType() + .firstOrNull + ?.declaredElement2; + + if (classElement == null) return null; + return _consumerWidget.isAssignableFrom(classElement) || + _consumerState.isAssignableFrom(classElement); + } + return null; + } +} + extension on FunctionExpression { /// Null if unknown bool? get isWidgetBuilder { @@ -844,6 +878,7 @@ extension on AssignmentExpression { if (target == null) { return null; } + // TODO: if ((targ.methodName.name == 'watch' || targ.methodName.name == 'read') && left.propertyName.name == 'state' && _ref.isAssignableFromType(target)) { diff --git a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart index 621874df2..dc9c57a61 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart @@ -1,4 +1,5 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:flutter/material.dart'; final a = StateProvider((ref) => 'String'); @@ -40,3 +41,59 @@ class C extends StateNotifier { ref.read(a.notifier).state = ''; } } + +class D extends ConsumerWidget { + const D({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context, WidgetRef ref) { + ref.watch(a.notifier).state = ''; + fn(ref); + fn2(ref); + fn3(ref); + ref.fn_g; + ref.fn2_g; + ref.fn3_g; + return Container(); + } + + void fn(WidgetRef ref) { + ref.read(a.notifier).state = ''; + } + + // Not okay + Future fn2(WidgetRef ref) async { + ref.read(a.notifier).state = ''; + await Future.delayed(Duration(seconds: 1)); + } + + // Okay + Future fn3(WidgetRef ref) async { + await Future.delayed(Duration(seconds: 1)); + ref.read(a.notifier).state = ''; + } +} + +extension on WidgetRef { + void fn() { + read(a.notifier).state = ''; + } + + void get fn_g => fn(); + + // Not okay + Future fn2() async { + read(a.notifier).state = ''; + await Future.delayed(Duration(seconds: 1)); + } + + Future get fn2_g => fn2(); + + // Okay + Future fn3() async { + await Future.delayed(Duration(seconds: 1)); + read(a.notifier).state = ''; + } + + Future get fn3_g => fn3(); +} From 9a8b14da432c0192bebbe2395845dd8363e97c67 Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Tue, 23 Aug 2022 20:04:15 -0600 Subject: [PATCH 2/8] add widget support for riverpod_no_mutate_sync --- packages/riverpod_lint/bin/custom_lint.dart | 59 +++++++++++++------ .../riverpod_lint/lib/src/analyzer_utils.dart | 9 +++ .../test/flutter_test.dart | 10 ++++ 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/packages/riverpod_lint/bin/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index cbdbe6870..253aa0eb6 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -265,7 +265,8 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { final ResolvedUnitResult unit; - Stream visitCalledFunction(AstNode node) async* { + Stream visitCalledFunction(AstNode node, + {required AstNode callingNode}) async* { final results = node is MethodDeclaration ? visitMethodDeclaration(node) : node is FunctionDeclaration @@ -278,8 +279,8 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { message: 'Do not mutate a provider synchronously, a function was called which mutates a provider synchronously', location: unit.lintLocationFromOffset( - node.offset, - length: node.length, + callingNode.offset, + length: callingNode.length, ), ); // Only need to report the function once @@ -288,6 +289,30 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { } } + @override + Stream? visitPropertyAccess(PropertyAccess node) async* { + final method = node.propertyName.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method); + if (ast != null) { + yield* visitCalledFunction(ast, callingNode: node); + } + } + yield* super.visitPropertyAccess(node) ?? const Stream.empty(); + } + + @override + Stream? visitPrefixedIdentifier(PrefixedIdentifier node) async* { + final method = node.identifier.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method); + if (ast != null) { + yield* visitCalledFunction(ast, callingNode: node); + } + } + yield* super.visitPrefixedIdentifier(node) ?? const Stream.empty(); + } + @override Stream? visitFunctionExpressionInvocation( FunctionExpressionInvocation node) async* { @@ -295,7 +320,8 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { if (method != null) { final ast = await findAstNodeForElement(method); if (ast != null) { - yield* visitCalledFunction(ast as FunctionDeclaration); + yield* visitCalledFunction(ast as FunctionDeclaration, + callingNode: node); } } yield* super.visitFunctionExpressionInvocation(node) ?? @@ -305,11 +331,10 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { @override Stream visitMethodInvocation(MethodInvocation node) async* { final method = node.methodName.staticElement; - print(node.methodName.name); if (method != null) { final ast = await findAstNodeForElement(method.declaration!); if (ast != null) { - yield* visitCalledFunction(ast); + yield* visitCalledFunction(ast, callingNode: node); } } @@ -337,7 +362,6 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { bool synchronous = true; @override Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { - print('Visiting $node'); final last = synchronous; synchronous = true; yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); @@ -362,8 +386,8 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { @override Stream? visitAssignmentExpression( AssignmentExpression expression) async* { - final mutate = expression.isMutation ?? false; - if (synchronous && mutate) { + final mutate = expression.isMutation; + if (synchronous && (mutate ?? false)) { yield Lint( code: 'riverpod_no_mutate_sync', message: 'Do not mutate a provider synchronously', @@ -867,22 +891,23 @@ extension on AssignmentExpression { // ref.watch(a.notifier).state = ''; bool? get isMutation { final left = leftHandSide; - if (left is! PropertyAccess) { + if (left is! PropertyAccess || left.propertyName.name != 'state') { return null; } final targ = left.target; if (targ is! MethodInvocation) { return null; } - final target = targ.target?.staticType; - if (target == null) { + final methodTarget = targ.methodName.staticElement?.enclosingElement3; + if (methodTarget == null || methodTarget is! InterfaceElement) { return null; } - // TODO: - if ((targ.methodName.name == 'watch' || targ.methodName.name == 'read') && - left.propertyName.name == 'state' && - _ref.isAssignableFromType(target)) { - return true; + if (_ref.isAssignableFromType(methodTarget.thisType) || + _widgetRef.isAssignableFromType(methodTarget.thisType)) { + // TODO: Synchronous listen + if (targ.methodName.name == 'watch' || targ.methodName.name == 'read') { + return true; + } } return false; } diff --git a/packages/riverpod_lint/lib/src/analyzer_utils.dart b/packages/riverpod_lint/lib/src/analyzer_utils.dart index 612773c29..a5fc2fd69 100644 --- a/packages/riverpod_lint/lib/src/analyzer_utils.dart +++ b/packages/riverpod_lint/lib/src/analyzer_utils.dart @@ -7,6 +7,15 @@ import 'package:analyzer/dart/element/element.dart'; Future findAstNodeForElement(Element element) async { final libraryElement = element.library; if (libraryElement == null) return null; + final inSdk = element.library?.isInSdk ?? false; + if (inSdk) { + return null; + } + final libraryName = element.librarySource?.uri.path ?? ''; + // We don't need to visit AST nodes in flutter to check for dependencies, ref usage, etc + if (libraryName.startsWith('flutter/')) { + return null; + } final parsedLibrary = await element.session?.getResolvedLibraryByElement(libraryElement); if (parsedLibrary is! ResolvedLibraryResult) return null; diff --git a/packages/riverpod_lint_flutter_test/test/flutter_test.dart b/packages/riverpod_lint_flutter_test/test/flutter_test.dart index d50da388b..1392ade1f 100644 --- a/packages/riverpod_lint_flutter_test/test/flutter_test.dart +++ b/packages/riverpod_lint_flutter_test/test/flutter_test.dart @@ -30,6 +30,16 @@ Future main() async { test/goldens/final_provider.dart:11:5 • Providers should always be declared as final • riverpod_final_provider test/goldens/final_provider.dart:13:5 • Providers should always be declared as final • riverpod_final_provider test/goldens/final_provider.dart:15:42 • Providers should always be declared as final • riverpod_final_provider + test/goldens/mutate_in_create.dart:7:3 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:17:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:21:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:22:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:23:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:50:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:51:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:52:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:54:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:55:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync test/goldens/read_vs_watch.dart:10:3 • Avoid using ref.read inside the build method of widgets/providers. • riverpod_avoid_read_inside_build test/goldens/read_vs_watch.dart:13:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build test/goldens/read_vs_watch.dart:18:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build From b4de82ca51c8c72fdf162b2a23d343279333ef6b Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Wed, 24 Aug 2022 07:36:22 -0600 Subject: [PATCH 3/8] wip --- packages/riverpod_lint/bin/custom_lint.dart | 277 ++++++++++++------ .../riverpod_lint/bin/test_custom_lint.dart | 2 +- .../test/flutter_test.dart | 36 ++- .../test/goldens/global_providers.dart | 4 + .../test/goldens/mutate_in_create.dart | 40 ++- .../test/goldens/ref_escape_scope.dart | 4 + .../goldens/use_ref_before_async_gaps.dart | 85 ++++++ 7 files changed, 351 insertions(+), 97 deletions(-) create mode 100644 packages/riverpod_lint_flutter_test/test/goldens/global_providers.dart create mode 100644 packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart diff --git a/packages/riverpod_lint/bin/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index 253aa0eb6..4629f7c3b 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -17,6 +17,7 @@ const _providerBase = TypeChecker.fromRuntime(ProviderBase); const _family = TypeChecker.fromRuntime(Family); const _future = TypeChecker.fromRuntime(Future); const _stream = TypeChecker.fromRuntime(Stream); +const _container = TypeChecker.fromRuntime(ProviderContainer); const _providerOrFamily = TypeChecker.any([_providerBase, _family]); const _futureOrStream = TypeChecker.any([_future, _stream]); @@ -133,6 +134,31 @@ class RefLifecycleInvocation { late final bool? isWithinBuild = _isWithinBuild(invocation); } +mixin _AsyncContextVisitor on AsyncRecursiveVisitor { + bool synchronous = true; + @override + Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { + final last = synchronous; + synchronous = true; + yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); + synchronous = last; + } + + @override + Stream? visitAwaitExpression(AwaitExpression node) async* { + yield* super.visitAwaitExpression(node) ?? const Stream.empty(); + synchronous = false; + } + + @override + Stream? visitForStatement(ForStatement node) async* { + // First time through the loop could be synchronous, so wait till the loop is done + yield* super.visitForStatement(node) ?? const Stream.empty(); + if (node.awaitKeyword != null) { + synchronous = false; + } + } +} mixin _RefLifecycleVisitor on AsyncRecursiveVisitor { /// A Ref/WidgetRef method was invoked. It isn't guaranteed to be watch/listen/read Stream? visitRefInvocation(RefLifecycleInvocation node); @@ -155,6 +181,96 @@ mixin _RefLifecycleVisitor on AsyncRecursiveVisitor { } } +mixin _InvocationVisitor + on AsyncRecursiveVisitor, _AsyncContextVisitor { + // Whether the method is forced async + bool forceAsync = false; + bool get asyncBad; + Stream visitCalledFunction(AstNode node, {required AstNode callingNode}); + @override + Stream? visitPropertyAccess(PropertyAccess node) async* { + final method = node.propertyName.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method); + if (ast != null) { + yield* visitCalledFunction(ast, callingNode: node); + } + } + yield* super.visitPropertyAccess(node) ?? const Stream.empty(); + } + + @override + Stream? visitPrefixedIdentifier(PrefixedIdentifier node) async* { + final method = node.identifier.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method); + if (ast != null) { + yield* visitCalledFunction(ast, callingNode: node); + } + } + yield* super.visitPrefixedIdentifier(node) ?? const Stream.empty(); + } + + @override + Stream? visitFunctionExpressionInvocation( + FunctionExpressionInvocation node) async* { + final method = node.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method); + if (ast != null) { + yield* visitCalledFunction(ast as FunctionDeclaration, + callingNode: node); + } + } + yield* super.visitFunctionExpressionInvocation(node) ?? + const Stream.empty(); + } + + @override + Stream? visitMethodInvocation(MethodInvocation node) async* { + final method = node.methodName.staticElement; + if (method != null) { + final ast = await findAstNodeForElement(method.declaration!); + if (ast != null) { + yield* visitCalledFunction(ast, callingNode: node); + } + } + + yield* super.visitMethodInvocation(node) ?? const Stream.empty(); + } + + @override + Stream? visitInstanceCreationExpression( + InstanceCreationExpression node) async* { + final constructor = node.constructorName.staticElement; + if (constructor != null) { + if (_futureOrStream.isAssignableFrom(constructor.enclosingElement3)) { + if (!asyncBad) { + return; + } else { + for (final arg in node.argumentList.arguments) { + // The arguments to a future or stream could be called after a widget is disposed + final last = forceAsync; + forceAsync = true; + yield* visitExpression(arg) ?? const Stream.empty(); + forceAsync = last; + } + } + } else { + final ast = await findAstNodeForElement(constructor.declaration); + + if (ast != null) { + yield* visitConstructorDeclaration(ast as ConstructorDeclaration) ?? + const Stream.empty(); + } + for (final arg in node.argumentList.arguments) { + yield* visitExpression(arg) ?? const Stream.empty(); + } + } + } + } +} + /// Recursively search all the places where a Provider's `ref` is used // TODO handle Ref fn() => ref; class ProviderRefUsageVisitor extends AsyncRecursiveVisitor @@ -260,11 +376,12 @@ class ProviderRefUsageVisitor extends AsyncRecursiveVisitor } } -class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { - ProviderSyncMutationVisitor(this.unit); - +class RefAsyncUsageVisitor extends AsyncRecursiveVisitor + with _AsyncContextVisitor, _InvocationVisitor, _RefLifecycleVisitor { + RefAsyncUsageVisitor(this.unit); final ResolvedUnitResult unit; + @override Stream visitCalledFunction(AstNode node, {required AstNode callingNode}) async* { final results = node is MethodDeclaration @@ -275,9 +392,9 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { if (results != null) { await for (final _ in results) { yield Lint( - code: 'riverpod_no_mutate_sync', + code: 'riverpod_no_ref_after_async', message: - 'Do not mutate a provider synchronously, a function was called which mutates a provider synchronously', + 'Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed', location: unit.lintLocationFromOffset( callingNode.offset, length: callingNode.length, @@ -290,96 +407,52 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { } @override - Stream? visitPropertyAccess(PropertyAccess node) async* { - final method = node.propertyName.staticElement; - if (method != null) { - final ast = await findAstNodeForElement(method); - if (ast != null) { - yield* visitCalledFunction(ast, callingNode: node); - } - } - yield* super.visitPropertyAccess(node) ?? const Stream.empty(); - } - - @override - Stream? visitPrefixedIdentifier(PrefixedIdentifier node) async* { - final method = node.identifier.staticElement; - if (method != null) { - final ast = await findAstNodeForElement(method); - if (ast != null) { - yield* visitCalledFunction(ast, callingNode: node); - } + Stream? visitRefInvocation(RefLifecycleInvocation node) async* { + if (!synchronous || forceAsync) { + // TODO Allow checking mounted in stateful widgets and checking mounted in StateNotifiers + yield Lint( + code: 'riverpod_no_ref_after_async', + message: 'Do not use ref after async gaps in flutter widgets.', + location: unit.lintLocationFromOffset( + node.invocation.offset, + length: node.invocation.length, + ), + ); } - yield* super.visitPrefixedIdentifier(node) ?? const Stream.empty(); } @override - Stream? visitFunctionExpressionInvocation( - FunctionExpressionInvocation node) async* { - final method = node.staticElement; - if (method != null) { - final ast = await findAstNodeForElement(method); - if (ast != null) { - yield* visitCalledFunction(ast as FunctionDeclaration, - callingNode: node); - } - } - yield* super.visitFunctionExpressionInvocation(node) ?? - const Stream.empty(); - } + bool get asyncBad => true; +} - @override - Stream visitMethodInvocation(MethodInvocation node) async* { - final method = node.methodName.staticElement; - if (method != null) { - final ast = await findAstNodeForElement(method.declaration!); - if (ast != null) { - yield* visitCalledFunction(ast, callingNode: node); - } - } +class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor + with _AsyncContextVisitor, _InvocationVisitor { + ProviderSyncMutationVisitor(this.unit); - yield* super.visitMethodInvocation(node) ?? const Stream.empty(); - } + final ResolvedUnitResult unit; @override - Stream? visitInstanceCreationExpression( - InstanceCreationExpression node) async* { - final constructor = node.constructorName.staticElement; - if (constructor != null) { - if (_futureOrStream.isAssignableFrom(constructor.enclosingElement3)) { + Stream visitCalledFunction(AstNode node, + {required AstNode callingNode}) async* { + final results = node is MethodDeclaration + ? visitMethodDeclaration(node) + : node is FunctionDeclaration + ? visitFunctionDeclaration(node) + : null; + if (results != null) { + await for (final _ in results) { + yield Lint( + code: 'riverpod_no_mutate_sync', + message: + 'Do not mutate a provider synchronously, a function was called which mutates a provider synchronously', + location: unit.lintLocationFromOffset( + callingNode.offset, + length: callingNode.length, + ), + ); + // Only need to report the function once return; } - - final ast = await findAstNodeForElement(constructor.declaration); - - if (ast != null) { - yield* visitConstructorDeclaration(ast as ConstructorDeclaration) ?? - const Stream.empty(); - } - } - } - - bool synchronous = true; - @override - Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { - final last = synchronous; - synchronous = true; - yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); - synchronous = last; - } - - @override - Stream? visitAwaitExpression(AwaitExpression node) async* { - yield* super.visitAwaitExpression(node) ?? const Stream.empty(); - synchronous = false; - } - - @override - Stream? visitForStatement(ForStatement node) async* { - // First time through the loop could be synchronous, so wait till the loop is done - yield* super.visitForStatement(node) ?? const Stream.empty(); - if (node.awaitKeyword != null) { - synchronous = false; } } @@ -398,6 +471,9 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor { ); } } + + @override + bool get asyncBad => false; } class _FindProviderCallbackVisitor @@ -613,6 +689,29 @@ class RiverpodVisitor extends AsyncRecursiveVisitor // 'Provider\n - origin: $node\n - expected: ${dependencies.value}\n - dependencies: ${visitor.dependencies}'); } + @override + Stream visitTopLevelVariableDeclaration( + TopLevelVariableDeclaration node) async* { + yield* super.visitTopLevelVariableDeclaration(node) ?? const Stream.empty(); + for (final variable in node.variables.variables) { + final type = variable.declaredElement2?.type; + + if (type != null && _container.isAssignableFromType(type)) { + yield Lint( + code: 'riverpod_global_container', + message: 'This container is global', + correction: + 'Make the container non-global by creating it in your main and assigning it to a UncontrolledProviderScope.', + severity: LintSeverity.warning, + location: unit.lintLocationFromOffset( + variable.offset, + length: variable.length, + ), + ); + } + } + } + @override Stream visitNode(AstNode node) async* { yield* super.visitNode(node) ?? const Stream.empty(); @@ -623,8 +722,14 @@ class RiverpodVisitor extends AsyncRecursiveVisitor : node is FunctionDeclaration ? syncMutationDetector.visitFunctionDeclaration(node) : null; - yield* results ?? const Stream.empty(); + final refAfterAsyncGaps = RefAsyncUsageVisitor(unit); + final results2 = node is MethodDeclaration + ? refAfterAsyncGaps.visitMethodDeclaration(node) + : node is FunctionDeclaration + ? refAfterAsyncGaps.visitFunctionDeclaration(node) + : null; + yield* results2 ?? const Stream.empty(); } } diff --git a/packages/riverpod_lint/bin/test_custom_lint.dart b/packages/riverpod_lint/bin/test_custom_lint.dart index 94f366d5e..bdffcf8f6 100644 --- a/packages/riverpod_lint/bin/test_custom_lint.dart +++ b/packages/riverpod_lint/bin/test_custom_lint.dart @@ -14,7 +14,7 @@ import 'custom_lint.dart'; void main() async { final collection = AnalysisContextCollection( includedPaths: [canonicalize('../riverpod_lint_flutter_test/')]); - const test = 'mutate_in_create'; + const test = 'use_ref_before_async_gaps'; final file = canonicalize('../riverpod_lint_flutter_test/test/goldens/$test.dart'); final watcher = FileWatcher(file); diff --git a/packages/riverpod_lint_flutter_test/test/flutter_test.dart b/packages/riverpod_lint_flutter_test/test/flutter_test.dart index 1392ade1f..6f33c5e3b 100644 --- a/packages/riverpod_lint_flutter_test/test/flutter_test.dart +++ b/packages/riverpod_lint_flutter_test/test/flutter_test.dart @@ -4,7 +4,7 @@ import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; Future main() async { - test('goldens', () async { + test('goldens', timeout: Timeout(Duration(seconds: 45)), () async { final result = await Process.run( 'flutter', ['pub', 'run', 'custom_lint'], @@ -30,16 +30,28 @@ Future main() async { test/goldens/final_provider.dart:11:5 • Providers should always be declared as final • riverpod_final_provider test/goldens/final_provider.dart:13:5 • Providers should always be declared as final • riverpod_final_provider test/goldens/final_provider.dart:15:42 • Providers should always be declared as final • riverpod_final_provider + test/goldens/global_providers.dart:3:7 • This container is global • riverpod_global_container + test/goldens/global_providers.dart:4:7 • This container is global • riverpod_global_container + test/goldens/global_providers.dart:4:42 • This container is global • riverpod_global_container test/goldens/mutate_in_create.dart:7:3 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:17:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:21:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:22:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:8:3 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:18:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:22:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync test/goldens/mutate_in_create.dart:23:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:50:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:51:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:24:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:51:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync test/goldens/mutate_in_create.dart:52:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:54:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:53:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:54:5 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async test/goldens/mutate_in_create.dart:55:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:55:5 • Do not use ref after async gaps in flutter widgets. • riverpod_no_ref_after_async + test/goldens/mutate_in_create.dart:56:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:57:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:105:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:109:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:110:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:111:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:135:3 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync test/goldens/read_vs_watch.dart:10:3 • Avoid using ref.read inside the build method of widgets/providers. • riverpod_avoid_read_inside_build test/goldens/read_vs_watch.dart:13:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build test/goldens/read_vs_watch.dart:18:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build @@ -59,8 +71,14 @@ Future main() async { test/goldens/read_vs_watch.dart:114:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build test/goldens/read_vs_watch.dart:128:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build test/goldens/ref_escape_scope.dart:7:12 • Ref escaped the scope via a function or return expression. • riverpod_ref_escape_scope - test/goldens/ref_escape_scope.dart:33:32 • Ref escaped its scope to another widget. • riverpod_ref_escape_scope - test/goldens/ref_escape_scope.dart:42:32 • Ref escaped its scope to another widget. • riverpod_ref_escape_scope + test/goldens/ref_escape_scope.dart:37:32 • Ref escaped its scope to another widget. • riverpod_ref_escape_scope + test/goldens/ref_escape_scope.dart:46:32 • Ref escaped its scope to another widget. • riverpod_ref_escape_scope + test/goldens/use_ref_before_async_gaps.dart:48:11 • Do not use ref after async gaps in flutter widgets. • riverpod_no_ref_after_async + test/goldens/use_ref_before_async_gaps.dart:51:9 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async + test/goldens/use_ref_before_async_gaps.dart:53:9 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async + test/goldens/use_ref_before_async_gaps.dart:55:9 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async + test/goldens/use_ref_before_async_gaps.dart:57:15 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async + test/goldens/use_ref_before_async_gaps.dart:59:9 • Do not use ref after async gaps in flutter widgets. • riverpod_no_ref_after_async '''); }); } diff --git a/packages/riverpod_lint_flutter_test/test/goldens/global_providers.dart b/packages/riverpod_lint_flutter_test/test/goldens/global_providers.dart new file mode 100644 index 000000000..ee95833d2 --- /dev/null +++ b/packages/riverpod_lint_flutter_test/test/goldens/global_providers.dart @@ -0,0 +1,4 @@ +import 'package:hooks_riverpod/hooks_riverpod.dart'; + +final _container1 = ProviderContainer(); +final _container2 = ProviderContainer(), _container3 = ProviderContainer(); diff --git a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart index dc9c57a61..45bc1e39f 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart @@ -5,6 +5,7 @@ final a = StateProvider((ref) => 'String'); final b = Provider((ref) { ref.watch(a.notifier).state = ''; + ref.watch(c.notifier).fn(); return false; }); @@ -51,6 +52,7 @@ class D extends ConsumerWidget { fn(ref); fn2(ref); fn3(ref); + ref.watch(c.notifier).fn(); ref.fn_g; ref.fn2_g; ref.fn3_g; @@ -67,7 +69,7 @@ class D extends ConsumerWidget { await Future.delayed(Duration(seconds: 1)); } - // Okay + // Okay, for synchronous, but bad for async usage Future fn3(WidgetRef ref) async { await Future.delayed(Duration(seconds: 1)); ref.read(a.notifier).state = ''; @@ -97,3 +99,39 @@ extension on WidgetRef { Future get fn3_g => fn3(); } + +class E extends ChangeNotifier { + E(this.ref) { + ref.read(a.notifier).state = ''; + Future.delayed(Duration(milliseconds: 10), () { + ref.read(a.notifier).state = ''; + }); + ref.read(a.notifier).state = ''; + fn(); + fn2(); + fn3(); + } + final Ref ref; + + void fn() { + ref.read(a.notifier).state = ''; + } + + // Not okay + Future fn2() async { + ref.read(a.notifier).state = ''; + await Future.delayed(Duration(seconds: 1)); + } + + // Okay + Future fn3() async { + await Future.delayed(Duration(seconds: 1)); + ref.read(a.notifier).state = ''; + } +} + +final f = ChangeNotifierProvider((ref) { + final e = E(ref); + e.fn(); + return e; +}); diff --git a/packages/riverpod_lint_flutter_test/test/goldens/ref_escape_scope.dart b/packages/riverpod_lint_flutter_test/test/goldens/ref_escape_scope.dart index 826df8fab..557d12c54 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/ref_escape_scope.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/ref_escape_scope.dart @@ -27,6 +27,10 @@ class C extends ConsumerWidget { BadWidgetB(ref: ref), ]); } + + void fn(WidgetRef ref) { + ref.read(a); + } } class BadWidgetA extends StatelessWidget { diff --git a/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart b/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart new file mode 100644 index 000000000..52eeb842e --- /dev/null +++ b/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart @@ -0,0 +1,85 @@ +// Tests that the ref is used before async gaps in widget methods + +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:flutter/material.dart'; + +final a = Provider((ref) => ''); +final b = StateProvider((ref) => ''); + +/// GOOD +class C extends ConsumerWidget { + @override + Widget build(BuildContext context, WidgetRef ref) { + final bn = ref.watch(b.notifier); + return ElevatedButton( + onPressed: () async { + ref.read(a); + final n = ref.read(b.notifier); + fn2(ref); + await Future.delayed(Duration(seconds: 1)); + n.state = ''; + bn.state = ''; + }, + child: Text('Hi'), + ); + } + + Future fn2(WidgetRef ref) async { + ref.read(a); + ref.invalidate(b); + ref.listen(b, (_, __) {}); + ref.refresh(b); + } +} + +/// BAD +class D extends ConsumerWidget { + @override + Widget build(BuildContext context, WidgetRef ref) { + return ElevatedButton( + onPressed: () async { + // Okay + ref.read(a); + ref.invalidate(b); + ref.listen(b, (_, __) {}); + ref.refresh(b); + Future.delayed(Duration(milliseconds: 10), () { + // Bad ref is used in future callback + ref.read(b.notifier).state = ''; + }); + // Bad (ref is used after async in fn) + fn(ref); + // Bad (ref is used after async in fn2) + fn2(ref); + // Bad (ref is used after async in fn3) + fn3(ref); + // Bad (ref is used after async in fn4) + await fn4(ref); + // Bad + ref.read(b.notifier).state = ''; + }, + child: Text('Hi'), + ); + } + + Future fn(WidgetRef ref) async { + await Future.delayed(Duration(seconds: 1)); + ref.read(b.notifier).state = ''; + } + + Future fn2(WidgetRef ref) async { + await Future.delayed(Duration(seconds: 1)); + ref.invalidate(b); + } + + Future fn3(WidgetRef ref) async { + await Future.delayed(Duration(seconds: 1)); + + ref.listen(b, (_, __) {}); + } + + Future fn4(WidgetRef ref) async { + await Future.delayed(Duration(seconds: 1)); + ref.refresh(b); + } +} From 4024f174d5569ae86b7b2cff218c3125226f00af Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Thu, 25 Aug 2022 22:05:41 -0600 Subject: [PATCH 4/8] add invalidate, invalidateSelf, refresh to mutation check, and add ref.read on autoDispose lint, refactor async context detection. --- packages/riverpod_lint/bin/custom_lint.dart | 276 +++++++++++++----- .../riverpod_lint/bin/test_custom_lint.dart | 2 +- .../riverpod_lint/lib/src/type_checker.dart | 30 -- .../test/flutter_test.dart | 10 +- .../test/goldens/auto_dispose_read.dart | 22 ++ .../test/goldens/mutate_in_create.dart | 24 ++ .../goldens/use_ref_before_async_gaps.dart | 12 + 7 files changed, 263 insertions(+), 113 deletions(-) create mode 100644 packages/riverpod_lint_flutter_test/test/goldens/auto_dispose_read.dart diff --git a/packages/riverpod_lint/bin/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index 4629f7c3b..361445bfb 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -13,11 +13,19 @@ import 'package:riverpod/riverpod.dart'; import 'package:riverpod_lint/src/analyzer_utils.dart'; import 'package:riverpod_lint/src/type_checker.dart'; -const _providerBase = TypeChecker.fromRuntime(ProviderBase); -const _family = TypeChecker.fromRuntime(Family); -const _future = TypeChecker.fromRuntime(Future); -const _stream = TypeChecker.fromRuntime(Stream); -const _container = TypeChecker.fromRuntime(ProviderContainer); +const _providerBase = + TypeChecker.fromName('ProviderBase', packageName: 'riverpod'); +const _autoDispose = + TypeChecker.fromName('AutoDisposeProviderBase', packageName: 'riverpod'); +const _family = TypeChecker.fromName('Family', packageName: 'riverpod'); +const _future = TypeChecker.fromUrl( + 'dart:async#Future', +); +const _stream = TypeChecker.fromUrl( + 'dart:async#Stream', +); +const _container = + TypeChecker.fromName('ProviderContainer', packageName: 'riverpod'); const _providerOrFamily = TypeChecker.any([_providerBase, _family]); const _futureOrStream = TypeChecker.any([_future, _stream]); @@ -35,7 +43,7 @@ const _consumerState = TypeChecker.fromName( 'ConsumerState', packageName: 'flutter_riverpod', ); -const _ref = TypeChecker.fromRuntime(Ref); +const _ref = TypeChecker.fromName('Ref', packageName: 'riverpod'); /// [Ref] methods that can make a provider depend on another provider. const _refBinders = {'read', 'watch', 'listen'}; @@ -88,66 +96,49 @@ mixin _ProviderCreationVisitor on AsyncRecursiveVisitor { class RefLifecycleInvocation { RefLifecycleInvocation._(this.invocation); - // Whether the invocation is inside or outside the build method of a provider/widget - // Null if unknown - static bool? _isWithinBuild(AstNode node) { - var hasFoundFunctionExpression = false; - - for (final parent in node.parents) { - if (parent is FunctionExpression) { - if (hasFoundFunctionExpression) return false; - - if (parent.isWidgetBuilder ?? false) { - return true; - } - - // Since anonymous functions could be the build of a provider, - // we need to check their ancestors for more information. - hasFoundFunctionExpression = true; - } - if (parent is InstanceCreationExpression) { - return parent.isProviderCreation; - } - if (parent is FunctionExpressionInvocation) { - return parent.isProviderCreation; - } - if (parent is MethodDeclaration) { - if (hasFoundFunctionExpression || parent.name2.lexeme != 'build') { - return false; - } - - final classElement = parent.parents - .whereType() - .firstOrNull - ?.declaredElement2; + final MethodInvocation invocation; - if (classElement == null) return null; - return _consumerWidget.isAssignableFrom(classElement) || - _consumerState.isAssignableFrom(classElement); - } - } - return null; - } + late final bool? isWithinBuild = invocation.inBuild; +} - final MethodInvocation invocation; +enum AsyncContext { + // Synchronous context in build of widget / initState or Provider + buildSync, + // Synchronous context in callback function + callbackSync, + // In callback, but after await / await_for + callbackAfterAsync; - late final bool? isWithinBuild = _isWithinBuild(invocation); + bool get isSync => this == buildSync || this == callbackSync; + bool get isAsync => this == callbackAfterAsync; } mixin _AsyncContextVisitor on AsyncRecursiveVisitor { - bool synchronous = true; + AsyncContext context = AsyncContext.buildSync; @override - Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { - final last = synchronous; - synchronous = true; - yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); - synchronous = last; + Stream? visitMethodDeclaration(MethodDeclaration node) async* { + final last = context; + context = AsyncContext.buildSync; + yield* super.visitMethodDeclaration(node) ?? const Stream.empty(); + context = last; + } + + @override + Stream? visitFunctionExpression(FunctionExpression node) async* { + final last = context; + if (node.isBuild() ?? false) { + context = AsyncContext.buildSync; + } else { + context = AsyncContext.callbackSync; + } + yield* super.visitFunctionExpression(node) ?? const Stream.empty(); + context = last; } @override Stream? visitAwaitExpression(AwaitExpression node) async* { yield* super.visitAwaitExpression(node) ?? const Stream.empty(); - synchronous = false; + context = AsyncContext.callbackAfterAsync; } @override @@ -155,7 +146,7 @@ mixin _AsyncContextVisitor on AsyncRecursiveVisitor { // First time through the loop could be synchronous, so wait till the loop is done yield* super.visitForStatement(node) ?? const Stream.empty(); if (node.awaitKeyword != null) { - synchronous = false; + context = AsyncContext.callbackAfterAsync; } } } @@ -174,7 +165,8 @@ mixin _RefLifecycleVisitor on AsyncRecursiveVisitor { } if (_ref.isAssignableFrom(targetType) || - _widgetRef.isAssignableFrom(targetType)) { + _widgetRef.isAssignableFrom(targetType) || + _container.isAssignableFrom(targetType)) { final refInvocStream = visitRefInvocation(RefLifecycleInvocation._(node)); if (refInvocStream != null) yield* refInvocStream; } @@ -250,10 +242,13 @@ mixin _InvocationVisitor } else { for (final arg in node.argumentList.arguments) { // The arguments to a future or stream could be called after a widget is disposed - final last = forceAsync; + final last = context; + final lastAsync = forceAsync; forceAsync = true; + context = AsyncContext.callbackAfterAsync; yield* visitExpression(arg) ?? const Stream.empty(); - forceAsync = last; + context = last; + forceAsync = lastAsync; } } } else { @@ -408,7 +403,7 @@ class RefAsyncUsageVisitor extends AsyncRecursiveVisitor @override Stream? visitRefInvocation(RefLifecycleInvocation node) async* { - if (!synchronous || forceAsync) { + if (context == AsyncContext.callbackAfterAsync) { // TODO Allow checking mounted in stateful widgets and checking mounted in StateNotifiers yield Lint( code: 'riverpod_no_ref_after_async', @@ -457,10 +452,11 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor } @override - Stream? visitAssignmentExpression( - AssignmentExpression expression) async* { + Stream? visitExpression(Expression expression) async* { + final lints = super.visitExpression(expression); + yield* lints ?? const Stream.empty(); final mutate = expression.isMutation; - if (synchronous && (mutate ?? false)) { + if (!forceAsync && context == AsyncContext.buildSync && (mutate ?? false)) { yield Lint( code: 'riverpod_no_mutate_sync', message: 'Do not mutate a provider synchronously', @@ -472,6 +468,14 @@ class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor } } + @override + Stream? visitNode(AstNode node) { + if (!forceAsync && context == AsyncContext.buildSync) { + return super.visitNode(node); + } + return null; + } + @override bool get asyncBad => false; } @@ -507,6 +511,24 @@ class RiverpodVisitor extends AsyncRecursiveVisitor ), ); } + final arg = + node.invocation.argumentList.arguments.firstOrNull?.staticType; + + if (arg != null && _autoDispose.isAssignableFromType(arg)) { + yield Lint( + code: 'riverpod_avoid_read_on_autoDispose', + message: 'Avoid using ref.read on an autoDispose provider', + correction: ''' +Instead use: + final listener = ref.listen(${node.invocation.argumentList.arguments.first}, (_, __){}); + final currentValue = listener.read(); +Then dispose of the listener when you no longer need the autoDispose provider to be kept alive.''', + location: unit.lintLocationFromOffset( + node.invocation.offset, + length: node.invocation.length, + ), + ); + } break; case 'watch': if (node.isWithinBuild == false) { @@ -730,6 +752,14 @@ class RiverpodVisitor extends AsyncRecursiveVisitor ? refAfterAsyncGaps.visitFunctionDeclaration(node) : null; yield* results2 ?? const Stream.empty(); + } else if (node.isInitState ?? false) { + final syncMutationDetector = ProviderSyncMutationVisitor(unit); + final results = node is MethodDeclaration + ? syncMutationDetector.visitMethodDeclaration(node) + : node is FunctionDeclaration + ? syncMutationDetector.visitFunctionDeclaration(node) + : null; + yield* results ?? const Stream.empty(); } } @@ -970,6 +1000,69 @@ extension on AstNode { } return null; } + + bool? isBuild({bool hasFoundFunctionExpression = false}) { + final node = this; + + if (node is FunctionExpression) { + if (hasFoundFunctionExpression) { + return false; + } + if (node.isWidgetBuilder ?? false) { + return true; + } + + return parent?.isBuild(hasFoundFunctionExpression: true); + } + if (node is InstanceCreationExpression) { + return node.isProviderCreation; + } + if (node is FunctionExpressionInvocation) { + return node.isProviderCreation; + } + if (node is MethodDeclaration) { + if (hasFoundFunctionExpression || node.name2.lexeme != 'build') { + return false; + } + + final classElement = node.parents + .whereType() + .firstOrNull + ?.declaredElement2; + + if (classElement == null) return null; + return _consumerWidget.isAssignableFrom(classElement) || + _consumerState.isAssignableFrom(classElement); + } + return parent?.isBuild( + hasFoundFunctionExpression: hasFoundFunctionExpression); + } + + bool? get inBuild { + final inBuild = parent?.isBuild(); + if (inBuild ?? false) { + return inBuild; + } + return inBuild; + } + + bool? get isInitState { + final expr = this; + if (expr is MethodDeclaration) { + if (expr.name2.lexeme != 'initState') { + return false; + } + + final classElement = expr.parents + .whereType() + .firstOrNull + ?.declaredElement2; + + if (classElement == null) return null; + return _consumerState.isAssignableFrom(classElement); + } + return null; + } } extension on FunctionExpression { @@ -992,28 +1085,49 @@ extension on InstanceCreationExpression { } } -extension on AssignmentExpression { +extension on Expression { // ref.watch(a.notifier).state = ''; bool? get isMutation { - final left = leftHandSide; - if (left is! PropertyAccess || left.propertyName.name != 'state') { - return null; - } - final targ = left.target; - if (targ is! MethodInvocation) { - return null; - } - final methodTarget = targ.methodName.staticElement?.enclosingElement3; - if (methodTarget == null || methodTarget is! InterfaceElement) { - return null; - } - if (_ref.isAssignableFromType(methodTarget.thisType) || - _widgetRef.isAssignableFromType(methodTarget.thisType)) { - // TODO: Synchronous listen - if (targ.methodName.name == 'watch' || targ.methodName.name == 'read') { - return true; + final expr = this; + if (expr is AssignmentExpression) { + final left = expr.leftHandSide; + if (left is! PropertyAccess || left.propertyName.name != 'state') { + return null; + } + final targ = left.target; + if (targ is! MethodInvocation) { + return null; + } + final methodTarget = targ.methodName.staticElement?.enclosingElement3; + if (methodTarget == null || methodTarget is! InterfaceElement) { + return null; + } + if (_ref.isAssignableFromType(methodTarget.thisType) || + _widgetRef.isAssignableFromType(methodTarget.thisType)) { + // TODO: Synchronous listen + if (targ.methodName.name == 'watch' || targ.methodName.name == 'read') { + return true; + } + } + return false; + } else if (expr is MethodInvocation) { + if (expr.methodName.name == 'refresh' || + expr.methodName.name == 'invalidate' || + expr.methodName.name == 'invalidateSelf') { + final methodTarget = expr.methodName.staticElement?.enclosingElement3; + if (methodTarget == null || methodTarget is! InterfaceElement) { + return null; + } + if (_ref.isAssignableFromType(methodTarget.thisType) || + _widgetRef.isAssignableFromType(methodTarget.thisType)) { + return true; + } else { + return false; + } + } else { + return false; } } - return false; + return null; } } diff --git a/packages/riverpod_lint/bin/test_custom_lint.dart b/packages/riverpod_lint/bin/test_custom_lint.dart index bdffcf8f6..94f366d5e 100644 --- a/packages/riverpod_lint/bin/test_custom_lint.dart +++ b/packages/riverpod_lint/bin/test_custom_lint.dart @@ -14,7 +14,7 @@ import 'custom_lint.dart'; void main() async { final collection = AnalysisContextCollection( includedPaths: [canonicalize('../riverpod_lint_flutter_test/')]); - const test = 'use_ref_before_async_gaps'; + const test = 'mutate_in_create'; final file = canonicalize('../riverpod_lint_flutter_test/test/goldens/$test.dart'); final watcher = FileWatcher(file); diff --git a/packages/riverpod_lint/lib/src/type_checker.dart b/packages/riverpod_lint/lib/src/type_checker.dart index 6bae4c235..98e376583 100644 --- a/packages/riverpod_lint/lib/src/type_checker.dart +++ b/packages/riverpod_lint/lib/src/type_checker.dart @@ -4,8 +4,6 @@ // Code imported from source_gen -import 'dart:mirrors' hide SourceLocation; - import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/constant/value.dart'; @@ -34,11 +32,6 @@ abstract class TypeChecker { /// ``` const factory TypeChecker.any(Iterable checkers) = _AnyChecker; - /// Create a new [TypeChecker] backed by a runtime [type]. - /// - /// This implementation uses `dart:mirrors` (runtime reflection). - const factory TypeChecker.fromRuntime(Type type) = _MirrorTypeChecker; - /// Create a new [TypeChecker] backed by a static [type]. const factory TypeChecker.fromStatic(DartType type) = _LibraryTypeChecker; @@ -230,29 +223,6 @@ class _LibraryTypeChecker extends TypeChecker { String toString() => urlOfElement(_type.element2!); } -// Checks a runtime type against a static type. -class _MirrorTypeChecker extends TypeChecker { - const _MirrorTypeChecker(this._type) : super._(); - - static Uri _uriOf(ClassMirror mirror) => - normalizeUrl((mirror.owner! as LibraryMirror).uri) - .replace(fragment: MirrorSystem.getName(mirror.simpleName)); - - // Precomputed type checker for types that already have been used. - static final _cache = Expando(); - - final Type _type; - - TypeChecker get _computed => - _cache[this] ??= TypeChecker.fromUrl(_uriOf(reflectClass(_type))); - - @override - bool isExactly(Element element) => _computed.isExactly(element); - - @override - String toString() => _computed.toString(); -} - @immutable class _NamedChecker extends TypeChecker { const _NamedChecker(this._name, {required this.packageName}) : super._(); diff --git a/packages/riverpod_lint_flutter_test/test/flutter_test.dart b/packages/riverpod_lint_flutter_test/test/flutter_test.dart index 6f33c5e3b..5f384ae40 100644 --- a/packages/riverpod_lint_flutter_test/test/flutter_test.dart +++ b/packages/riverpod_lint_flutter_test/test/flutter_test.dart @@ -12,6 +12,9 @@ Future main() async { ); expect(result.stdout, ''' + test/goldens/auto_dispose_read.dart:11:5 • Avoid using ref.read inside the build method of widgets/providers. • riverpod_avoid_read_inside_build + test/goldens/auto_dispose_read.dart:11:5 • Avoid using ref.read on an autoDispose provider • riverpod_avoid_read_on_autoDispose + test/goldens/auto_dispose_read.dart:20:3 • Avoid using ref.read on an autoDispose provider • riverpod_avoid_read_on_autoDispose test/goldens/avoid_dynamic_provider.dart:10:9 • Providers should be either top level variables or static properties • riverpod_avoid_dynamic_provider test/goldens/avoid_dynamic_provider.dart:14:9 • Providers should be either top level variables or static properties • riverpod_avoid_dynamic_provider test/goldens/avoid_dynamic_provider.dart:15:9 • Providers should be either top level variables or static properties • riverpod_avoid_dynamic_provider @@ -51,7 +54,12 @@ Future main() async { test/goldens/mutate_in_create.dart:109:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync test/goldens/mutate_in_create.dart:110:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync test/goldens/mutate_in_create.dart:111:5 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:135:3 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:112:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:113:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:137:3 • Do not mutate a provider synchronously, a function was called which mutates a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:151:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:152:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync + test/goldens/mutate_in_create.dart:153:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync test/goldens/read_vs_watch.dart:10:3 • Avoid using ref.read inside the build method of widgets/providers. • riverpod_avoid_read_inside_build test/goldens/read_vs_watch.dart:13:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build test/goldens/read_vs_watch.dart:18:5 • Avoid using ref.watch outside the build method of widgets/providers. • riverpod_avoid_watch_outside_build diff --git a/packages/riverpod_lint_flutter_test/test/goldens/auto_dispose_read.dart b/packages/riverpod_lint_flutter_test/test/goldens/auto_dispose_read.dart new file mode 100644 index 000000000..8c763bd71 --- /dev/null +++ b/packages/riverpod_lint_flutter_test/test/goldens/auto_dispose_read.dart @@ -0,0 +1,22 @@ +import 'package:flutter/material.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; + +final a = Provider.autoDispose((ref) {}); + +class B extends ConsumerWidget { + const B({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context, WidgetRef ref) { + ref.read(a); + return Container(); + } +} + +void main() { + // Example of test usage + final container = ProviderContainer(); + // Lint + container.read(a); + container.dispose(); +} diff --git a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart index 45bc1e39f..271de823e 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/mutate_in_create.dart @@ -109,6 +109,8 @@ class E extends ChangeNotifier { ref.read(a.notifier).state = ''; fn(); fn2(); + ref.invalidate(a); + ref.invalidateSelf(); fn3(); } final Ref ref; @@ -135,3 +137,25 @@ final f = ChangeNotifierProvider((ref) { e.fn(); return e; }); + +class G extends ConsumerStatefulWidget { + const G({Key? key}) : super(key: key); + + @override + ConsumerState createState() => _GState(); +} + +class _GState extends ConsumerState { + @override + void initState() { + ref.read(a.notifier).state = ''; + ref.invalidate(a); + ref.invalidate(a); + super.initState(); + } + + @override + Widget build(BuildContext context) { + return Container(); + } +} diff --git a/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart b/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart index 52eeb842e..c365eab57 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart @@ -83,3 +83,15 @@ class D extends ConsumerWidget { ref.refresh(b); } } + +final stream = StreamProvider((ref) async* { + // No lint after async + await Future.delayed(Duration(seconds: 1)); + ref.watch(a); +}); + +final future = FutureProvider((ref) async* { + await Future.delayed(Duration(seconds: 1)); + // No lint after async + ref.watch(a); +}); From a3bc17d03b26d82f6f76561dc05485c3fb84642a Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Fri, 9 Sep 2022 20:03:49 -0600 Subject: [PATCH 5/8] ignore analysis of linter test directory --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 81c6f3f8b..89cb2fcdc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -47,7 +47,7 @@ jobs: run: melos exec --depends-on="build_runner" -- "dart run build_runner build --delete-conflicting-outputs" - name: Analyze - run: melos exec --ignore="codemod_riverpod_test*" -- "flutter analyze" + run: melos exec --ignore="codemod_riverpod_test*" --ignore="riverpod_lint_flutter_test" -- "flutter analyze" - name: Run tests run: melos run test From 08167042a1ba0202b7a9ed583be41e86281fad0c Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Fri, 9 Sep 2022 20:24:58 -0600 Subject: [PATCH 6/8] minor updates --- .github/workflows/build.yml | 6 +++--- packages/riverpod_lint/bin/test_custom_lint.dart | 2 ++ website/pubspec.yaml | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 89cb2fcdc..6bd048439 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -25,7 +25,7 @@ jobs: steps: - uses: actions/checkout@v2 - - uses: subosito/flutter-action@v1 + - uses: subosito/flutter-action@v2 with: channel: ${{ matrix.channel }} @@ -41,13 +41,13 @@ jobs: run: melos bootstrap --ignore "codemod_riverpod_*,riverpod_cli" - name: Check format - run: melos exec --ignore="website_snipets" -- "flutter format --set-exit-if-changed ." + run: melos exec --ignore="website_snippets" -- "flutter format --set-exit-if-changed ." - name: Generate run: melos exec --depends-on="build_runner" -- "dart run build_runner build --delete-conflicting-outputs" - name: Analyze - run: melos exec --ignore="codemod_riverpod_test*" --ignore="riverpod_lint_flutter_test" -- "flutter analyze" + run: melos exec --ignore="codemod_riverpod_test*,riverpod_lint_flutter_test" -- "flutter analyze" - name: Run tests run: melos run test diff --git a/packages/riverpod_lint/bin/test_custom_lint.dart b/packages/riverpod_lint/bin/test_custom_lint.dart index c2400e067..e2f6fb58a 100644 --- a/packages/riverpod_lint/bin/test_custom_lint.dart +++ b/packages/riverpod_lint/bin/test_custom_lint.dart @@ -1,4 +1,6 @@ +// TODO: Replace with custom_lint/basic_runner.dart once merged into custom_lint // Hotreloader and watcher are really more of dev-dependencies + // ignore_for_file: depend_on_referenced_packages import 'dart:developer' as dev; diff --git a/website/pubspec.yaml b/website/pubspec.yaml index 8966ece67..d3989e875 100644 --- a/website/pubspec.yaml +++ b/website/pubspec.yaml @@ -1,4 +1,4 @@ -name: website_snipets +name: website_snippets publish_to: "none" version: 1.0.0+1 From 3fdd1247698aeab3c55409522075dce08d39898a Mon Sep 17 00:00:00 2001 From: Tim Whiting Date: Fri, 9 Sep 2022 21:57:06 -0600 Subject: [PATCH 7/8] unskip the test and slight optimization on checking for ref usages --- packages/riverpod_lint/bin/custom_lint.dart | 46 +++++++++++++++++-- .../riverpod_lint/lib/src/type_checker.dart | 6 ++- .../test/flutter_test.dart | 2 +- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/packages/riverpod_lint/bin/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index 7c3da4d47..718e890f0 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -34,6 +34,7 @@ const _widgetState = TypeChecker.fromName('State', packageName: 'flutter'); const _widgetRef = TypeChecker.fromName('WidgetRef', packageName: 'flutter_riverpod'); +const _anyRef = TypeChecker.any([_widgetRef, _ref]); const _consumerWidget = TypeChecker.fromName( 'ConsumerWidget', @@ -184,7 +185,7 @@ mixin _InvocationVisitor final method = node.propertyName.staticElement; if (method != null) { final ast = await findAstNodeForElement(method); - if (ast != null) { + if (ast != null && ast.refPassed) { yield* visitCalledFunction(ast, callingNode: node); } } @@ -196,7 +197,7 @@ mixin _InvocationVisitor final method = node.identifier.staticElement; if (method != null) { final ast = await findAstNodeForElement(method); - if (ast != null) { + if (ast != null && ast.refPassed) { yield* visitCalledFunction(ast, callingNode: node); } } @@ -210,7 +211,7 @@ mixin _InvocationVisitor final method = node.staticElement; if (method != null) { final ast = await findAstNodeForElement(method); - if (ast != null) { + if (ast != null && ast.refPassed) { yield* visitCalledFunction( ast as FunctionDeclaration, callingNode: node, @@ -226,7 +227,7 @@ mixin _InvocationVisitor final method = node.methodName.staticElement; if (method != null) { final ast = await findAstNodeForElement(method.declaration!); - if (ast != null) { + if (ast != null && ast.refPassed) { yield* visitCalledFunction(ast, callingNode: node); } } @@ -427,6 +428,43 @@ class RefAsyncUsageVisitor extends AsyncRecursiveVisitor bool get asyncBad => true; } +extension on FormalParameterList { + bool get hasRefParameter { + return parameters.any((p) { + final type = p.declaredElement?.type; + return type != null && _anyRef.isAssignableFromType(type); + }); + } +} + +extension on InterfaceOrAugmentationElement { + bool get hasRefField => + fields.any((f) => _anyRef.isAssignableFromType(f.type)); +} + +extension RefPassed on AstNode { + bool get refPassed { + final node = this; + if (node is MethodDeclaration) { + if (!(node.parameters?.hasRefParameter ?? false)) { + final enclosingElement = node.declaredElement2?.enclosingElement3; + if (enclosingElement is ExtensionElement && + !_anyRef.isAssignableFromType(enclosingElement.extendedType)) { + return false; // If ref is not passed in, there is no way the ref could be used within the the called function + } else if (enclosingElement is InterfaceOrAugmentationElement && + !enclosingElement.hasRefField) { + return false; // If ref is not passed in and not a field, there is no way the ref could be used within the the called function + } + } + } else if (node is FunctionDeclaration) { + if (!(node.functionExpression.parameters?.hasRefParameter ?? false)) { + return false; // If ref is not passed in, there is no way the ref could be used within the the called function + } + } + return true; + } +} + class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor with _AsyncContextVisitor, _InvocationVisitor { ProviderSyncMutationVisitor(this.unit); diff --git a/packages/riverpod_lint/lib/src/type_checker.dart b/packages/riverpod_lint/lib/src/type_checker.dart index 98e376583..720f65084 100644 --- a/packages/riverpod_lint/lib/src/type_checker.dart +++ b/packages/riverpod_lint/lib/src/type_checker.dart @@ -173,8 +173,10 @@ abstract class TypeChecker { (element is ClassElement && element.allSupertypes.any(isExactlyType)); /// Returns `true` if [staticType] can be assigned to this type. - bool isAssignableFromType(DartType staticType) => - isAssignableFrom(staticType.element2!); + // ignore: avoid_bool_literals_in_conditional_expressions + bool isAssignableFromType(DartType staticType) => staticType.element2 == null + ? false + : isAssignableFrom(staticType.element2!); /// Returns `true` if representing the exact same class as [element]. bool isExactly(Element element); diff --git a/packages/riverpod_lint_flutter_test/test/flutter_test.dart b/packages/riverpod_lint_flutter_test/test/flutter_test.dart index e8a05f7ca..5f384ae40 100644 --- a/packages/riverpod_lint_flutter_test/test/flutter_test.dart +++ b/packages/riverpod_lint_flutter_test/test/flutter_test.dart @@ -88,5 +88,5 @@ Future main() async { test/goldens/use_ref_before_async_gaps.dart:57:15 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async test/goldens/use_ref_before_async_gaps.dart:59:9 • Do not use ref after async gaps in flutter widgets. • riverpod_no_ref_after_async '''); - }, skip: 'TODO must fix'); + }); } From b9f4861594ffee23a9409808016249b908b1c202 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 11 Sep 2022 12:01:50 +0200 Subject: [PATCH 8/8] Apply suggestions from code review --- packages/riverpod_lint_flutter_test/test/flutter_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/riverpod_lint_flutter_test/test/flutter_test.dart b/packages/riverpod_lint_flutter_test/test/flutter_test.dart index 5f384ae40..4554af892 100644 --- a/packages/riverpod_lint_flutter_test/test/flutter_test.dart +++ b/packages/riverpod_lint_flutter_test/test/flutter_test.dart @@ -88,5 +88,5 @@ Future main() async { test/goldens/use_ref_before_async_gaps.dart:57:15 • Do not use ref after async gaps in flutter widgets, a function was called which uses ref after a widget could be disposed • riverpod_no_ref_after_async test/goldens/use_ref_before_async_gaps.dart:59:9 • Do not use ref after async gaps in flutter widgets. • riverpod_no_ref_after_async '''); - }); + }, skip: 'TODO flacky'); }