diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 81c6f3f8b..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*" -- "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/custom_lint.dart b/packages/riverpod_lint/bin/custom_lint.dart index 0ab7e09a2..718e890f0 100644 --- a/packages/riverpod_lint/bin/custom_lint.dart +++ b/packages/riverpod_lint/bin/custom_lint.dart @@ -13,10 +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 _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]); @@ -25,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', @@ -34,7 +44,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'}; @@ -87,52 +97,60 @@ 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; + final MethodInvocation invocation; - for (final parent in node.parents) { - if (parent is FunctionExpression) { - if (hasFoundFunctionExpression) return false; + late final bool? isWithinBuild = invocation.inBuild; +} - if (parent.isWidgetBuilder ?? false) { - return true; - } +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; - // 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; - } + bool get isSync => this == buildSync || this == callbackSync; + bool get isAsync => this == callbackAfterAsync; +} - final classElement = parent.parents - .whereType() - .firstOrNull - ?.declaredElement2; +mixin _AsyncContextVisitor on AsyncRecursiveVisitor { + AsyncContext context = AsyncContext.buildSync; + @override + Stream? visitMethodDeclaration(MethodDeclaration node) async* { + final last = context; + context = AsyncContext.buildSync; + yield* super.visitMethodDeclaration(node) ?? const Stream.empty(); + context = last; + } - if (classElement == null) return null; - return _consumerWidget.isAssignableFrom(classElement) || - _consumerState.isAssignableFrom(classElement); - } + @override + Stream? visitFunctionExpression(FunctionExpression node) async* { + final last = context; + if (node.isBuild() ?? false) { + context = AsyncContext.buildSync; + } else { + context = AsyncContext.callbackSync; } - return null; + yield* super.visitFunctionExpression(node) ?? const Stream.empty(); + context = last; } - final MethodInvocation invocation; + @override + Stream? visitAwaitExpression(AwaitExpression node) async* { + yield* super.visitAwaitExpression(node) ?? const Stream.empty(); + context = AsyncContext.callbackAfterAsync; + } - late final bool? isWithinBuild = _isWithinBuild(invocation); + @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) { + context = AsyncContext.callbackAfterAsync; + } + } } - mixin _RefLifecycleVisitor on AsyncRecursiveVisitor { /// A Ref/WidgetRef method was invoked. It isn't guaranteed to be watch/listen/read Stream? visitRefInvocation(RefLifecycleInvocation node); @@ -148,13 +166,111 @@ 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; } } } +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 && ast.refPassed) { + 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 && ast.refPassed) { + 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 && ast.refPassed) { + 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 && ast.refPassed) { + 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 = context; + final lastAsync = forceAsync; + forceAsync = true; + context = AsyncContext.callbackAfterAsync; + yield* visitExpression(arg) ?? const Stream.empty(); + context = last; + forceAsync = lastAsync; + } + } + } 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 @@ -261,118 +377,133 @@ 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? visitFunctionExpressionInvocation( - FunctionExpressionInvocation node, - ) async* { - final method = node.staticElement; - 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; - } + 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_ref_after_async', + message: + '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, + ), + ); + // Only need to report the function once + return; } } - 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) { - 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; - } - } + Stream? visitRefInvocation(RefLifecycleInvocation node) async* { + if (context == AsyncContext.callbackAfterAsync) { + // 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.visitMethodInvocation(node) ?? const Stream.empty(); } @override - Stream? visitInstanceCreationExpression( - InstanceCreationExpression node, - ) async* { - final constructor = node.constructorName.staticElement; - if (constructor != null) { - if (_futureOrStream.isAssignableFrom(constructor.enclosingElement3)) { - return; + 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 + } } - final ast = await findAstNodeForElement(constructor.declaration); - if (ast != null) { - yield* visitConstructorDeclaration(ast as ConstructorDeclaration) ?? - const Stream.empty(); + } 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; } +} - bool synchronous = true; - @override - Stream? visitBlockFunctionBody(BlockFunctionBody node) async* { - final last = synchronous; - synchronous = true; - yield* super.visitBlockFunctionBody(node) ?? const Stream.empty(); - synchronous = last; - } +class ProviderSyncMutationVisitor extends AsyncRecursiveVisitor + with _AsyncContextVisitor, _InvocationVisitor { + ProviderSyncMutationVisitor(this.unit); - @override - Stream? visitAwaitExpression(AwaitExpression node) async* { - yield* super.visitAwaitExpression(node) ?? const Stream.empty(); - synchronous = false; - } + final ResolvedUnitResult unit; @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; + 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; + } } } @override - Stream? visitAssignmentExpression( - AssignmentExpression expression, - ) async* { - final mutate = expression.isMutation ?? false; - if (synchronous && mutate) { + Stream? visitExpression(Expression expression) async* { + final lints = super.visitExpression(expression); + yield* lints ?? const Stream.empty(); + final mutate = expression.isMutation; + if (!forceAsync && context == AsyncContext.buildSync && (mutate ?? false)) { yield Lint( code: 'riverpod_no_mutate_sync', message: 'Do not mutate a provider synchronously', @@ -383,6 +514,17 @@ 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; } class _FindProviderCallbackVisitor @@ -416,6 +558,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) { @@ -599,6 +759,59 @@ 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(); + 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(); + final refAfterAsyncGaps = RefAsyncUsageVisitor(unit); + final results2 = node is MethodDeclaration + ? refAfterAsyncGaps.visitMethodDeclaration(node) + : node is FunctionDeclaration + ? 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(); + } + } + Stream _checkValidProviderDeclaration(AstNode providerNode) async* { final declaration = providerNode.parents.whereType().firstOrNull; @@ -818,6 +1031,94 @@ 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; + } + + 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 { /// Null if unknown bool? get isWidgetBuilder { @@ -838,26 +1139,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) { - return null; - } - final targ = left.target; - if (targ is! MethodInvocation) { - return null; - } - final target = targ.target?.staticType; - if (target == null) { - return null; - } - if ((targ.methodName.name == 'watch' || targ.methodName.name == 'read') && - left.propertyName.name == 'state' && - _ref.isAssignableFromType(target)) { - 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 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/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/lib/src/type_checker.dart b/packages/riverpod_lint/lib/src/type_checker.dart index 6bae4c235..720f65084 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; @@ -180,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); @@ -230,29 +225,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 978c7658d..4554af892 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'], @@ -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 @@ -30,11 +33,33 @@ 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:6:3 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:16:5 • Do not mutate a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:20: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, a method was called which mutates a provider synchronously • riverpod_no_mutate_sync - test/goldens/mutate_in_create.dart:22:5 • Do not mutate a provider synchronously, a method was called which mutates a provider synchronously • riverpod_no_mutate_sync + 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: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: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: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: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 @@ -54,8 +79,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 '''); - }, skip: 'TODO must fix'); + }, skip: 'TODO flacky'); } 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/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 621874df2..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 @@ -1,9 +1,11 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:flutter/material.dart'; final a = StateProvider((ref) => 'String'); final b = Provider((ref) { ref.watch(a.notifier).state = ''; + ref.watch(c.notifier).fn(); return false; }); @@ -40,3 +42,120 @@ 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.watch(c.notifier).fn(); + 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, for synchronous, but bad for async usage + 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(); +} + +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(); + ref.invalidate(a); + ref.invalidateSelf(); + 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; +}); + +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/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..c365eab57 --- /dev/null +++ b/packages/riverpod_lint_flutter_test/test/goldens/use_ref_before_async_gaps.dart @@ -0,0 +1,97 @@ +// 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); + } +} + +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); +}); 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