From 65ea983191ba4bd06ae7de648443136a81b18586 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Thu, 6 Apr 2023 22:49:56 +0200 Subject: [PATCH] Fix scoped_providers_should_specify_dependencies incorrectly triggering on functions other than "main" fixes #2340 --- packages/riverpod_lint/CHANGELOG.md | 1 + .../lib/src/lints/missing_provider_scope.dart | 4 +- ...providers_should_specify_dependencies.dart | 28 ++++++-- .../src/lints/unsupported_provider_value.dart | 2 - .../goldens/lints/missing_provider_scope.dart | 14 ++++ ...providers_should_specify_dependencies.dart | 69 ++++++++++++++++++- 6 files changed, 106 insertions(+), 12 deletions(-) diff --git a/packages/riverpod_lint/CHANGELOG.md b/packages/riverpod_lint/CHANGELOG.md index 6382e3eb6..c3c8d2a5e 100644 --- a/packages/riverpod_lint/CHANGELOG.md +++ b/packages/riverpod_lint/CHANGELOG.md @@ -1,6 +1,7 @@ ## Unreleased fix - Disable unsupported_provider_value when a notifier returns "this" +- Fix scoped_providers_should_specify_dependencies incorrectly triggering on functions other than "main" ## 1.1.7 - 2023-04-06 diff --git a/packages/riverpod_lint/lib/src/lints/missing_provider_scope.dart b/packages/riverpod_lint/lib/src/lints/missing_provider_scope.dart index 780091b93..bab6da2b0 100644 --- a/packages/riverpod_lint/lib/src/lints/missing_provider_scope.dart +++ b/packages/riverpod_lint/lib/src/lints/missing_provider_scope.dart @@ -6,6 +6,8 @@ import 'package:collection/collection.dart'; import 'package:custom_lint_builder/custom_lint_builder.dart'; import 'package:riverpod_analyzer_utils/riverpod_analyzer_utils.dart'; +import 'scoped_providers_should_specify_dependencies.dart'; + class MissingProviderScope extends DartLintRule { const MissingProviderScope() : super(code: _code); @@ -22,7 +24,7 @@ class MissingProviderScope extends DartLintRule { CustomLintContext context, ) { context.registry.addMethodInvocation((node) { - if (node.methodName.name != 'runApp') return; + if (!node.methodName.isFlutterRunApp) return; final function = node.function; if (function is! SimpleIdentifier) return; final functionElement = function.staticElement; diff --git a/packages/riverpod_lint/lib/src/lints/scoped_providers_should_specify_dependencies.dart b/packages/riverpod_lint/lib/src/lints/scoped_providers_should_specify_dependencies.dart index 8737026c5..f736a766e 100644 --- a/packages/riverpod_lint/lib/src/lints/scoped_providers_should_specify_dependencies.dart +++ b/packages/riverpod_lint/lib/src/lints/scoped_providers_should_specify_dependencies.dart @@ -5,6 +5,20 @@ import 'package:riverpod_analyzer_utils/riverpod_analyzer_utils.dart'; import '../riverpod_custom_lint.dart'; +extension SimpleIdentifierX on SimpleIdentifier { + bool get isFlutterRunApp { + if (name != 'runApp') return false; + + final library = staticElement?.library; + if (library == null) return false; + final libraryUri = Uri.tryParse(library.identifier); + if (libraryUri == null) return false; + + return libraryUri.scheme == 'package' && + libraryUri.pathSegments.first == 'flutter'; + } +} + class ScopedProvidersShouldSpecifyDependencies extends RiverpodLintRule { const ScopedProvidersShouldSpecifyDependencies() : super(code: _code); @@ -62,18 +76,20 @@ class ScopedProvidersShouldSpecifyDependencies extends RiverpodLintRule { bool isProviderScopeScoped( ProviderScopeInstanceCreationExpression expression, ) { - final hasParent = expression.node.argumentList.arguments + final hasParentParameter = expression.node.argumentList.arguments .whereType() // TODO handle parent:null. // This might be doable by checking that the expression's // static type is non-nullable .any((e) => e.name.label.name == 'parent'); - if (hasParent) return true; + if (hasParentParameter) return true; - final enclosingFunction = - expression.node.thisOrAncestorOfType(); + // in runApp(ProviderScope(..)) the direct parent of the ProviderScope + // is an ArgumentList. + final enclosingExpression = expression.node.parent?.parent; - // Any ProviderScope without parent outside of the main are considered "scoped". - return enclosingFunction == null || enclosingFunction.name.lexeme != 'main'; + // If the ProviderScope isn't directly as a child of runApp, it is scoped + return enclosingExpression is! MethodInvocation || + !enclosingExpression.methodName.isFlutterRunApp; } } diff --git a/packages/riverpod_lint/lib/src/lints/unsupported_provider_value.dart b/packages/riverpod_lint/lib/src/lints/unsupported_provider_value.dart index 66b5e54ad..dfd768048 100644 --- a/packages/riverpod_lint/lib/src/lints/unsupported_provider_value.dart +++ b/packages/riverpod_lint/lib/src/lints/unsupported_provider_value.dart @@ -1,5 +1,3 @@ -import 'package:analyzer/dart/element/type.dart'; -import 'package:analyzer/dart/element/type_system.dart'; import 'package:analyzer/error/listener.dart'; import 'package:custom_lint_builder/custom_lint_builder.dart'; import 'package:riverpod_analyzer_utils/riverpod_analyzer_utils.dart'; diff --git a/packages/riverpod_lint_flutter_test/test/goldens/lints/missing_provider_scope.dart b/packages/riverpod_lint_flutter_test/test/goldens/lints/missing_provider_scope.dart index 0d75cc9e7..a5ea5d684 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/lints/missing_provider_scope.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/lints/missing_provider_scope.dart @@ -15,6 +15,20 @@ void main() { ); } +void definitelyNotAMain() { + // expect_lint: missing_provider_scope + runApp( + MyApp(), + ); + runApp(ProviderScope(child: MyApp())); + runApp( + UncontrolledProviderScope( + container: ProviderContainer(), + child: MyApp(), + ), + ); +} + class MyApp extends StatelessWidget { const MyApp({super.key}); diff --git a/packages/riverpod_lint_flutter_test/test/goldens/lints/scoped_providers_should_specify_dependencies.dart b/packages/riverpod_lint_flutter_test/test/goldens/lints/scoped_providers_should_specify_dependencies.dart index 72768c2f5..3d3d49d4d 100644 --- a/packages/riverpod_lint_flutter_test/test/goldens/lints/scoped_providers_should_specify_dependencies.dart +++ b/packages/riverpod_lint_flutter_test/test/goldens/lints/scoped_providers_should_specify_dependencies.dart @@ -1,4 +1,7 @@ -import 'package:flutter/material.dart'; +// ignore_for_file: unused_local_variable + +import 'package:flutter/material.dart' as flutter; +import 'package:flutter/material.dart' hide runApp; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:riverpod_annotation/riverpod_annotation.dart'; @@ -13,6 +16,9 @@ external int unimplementedScoped(); @riverpod int root(RootRef ref) => 0; +// A fake runApp to check that we lint only on the official Flutter's runApp +void runApp(Widget widget) {} + void main() { final rootContainer = ProviderContainer( overrides: [ @@ -22,7 +28,6 @@ void main() { ], ); - // ignore: unused_local_variable final scopedContainer = ProviderContainer( parent: rootContainer, overrides: [ @@ -33,7 +38,7 @@ void main() { ], ); - runApp( + flutter.runApp( ProviderScope( overrides: [ scopedProvider.overrideWith((ref) => 0), @@ -45,6 +50,64 @@ void main() { ); runApp( + ProviderScope( + overrides: [ + scopedProvider.overrideWith((ref) => 0), + unimplementedScopedProvider.overrideWith((ref) => 0), + // This is not a Flutter's runApp, so the ProviderScope is considered scoped + // expect_lint: scoped_providers_should_specify_dependencies + rootProvider.overrideWith((ref) => 0), + ], + child: Container(), + ), + ); + + flutter.runApp( + ProviderScope( + parent: rootContainer, + overrides: [ + scopedProvider.overrideWith((ref) => 0), + unimplementedScopedProvider.overrideWith((ref) => 0), + // expect_lint: scoped_providers_should_specify_dependencies + rootProvider.overrideWith((ref) => 0), + ], + child: Container(), + ), + ); +} + +// Regression tests for https://github.com/rrousselGit/riverpod/issues/2340 +void definitelyNotAMain() { + final rootContainer = ProviderContainer( + overrides: [ + scopedProvider.overrideWith((ref) => 0), + unimplementedScopedProvider.overrideWith((ref) => 0), + rootProvider.overrideWith((ref) => 0), + ], + ); + + final scopedContainer = ProviderContainer( + parent: rootContainer, + overrides: [ + scopedProvider.overrideWith((ref) => 0), + unimplementedScopedProvider.overrideWith((ref) => 0), + // expect_lint: scoped_providers_should_specify_dependencies + rootProvider.overrideWith((ref) => 0), + ], + ); + + flutter.runApp( + ProviderScope( + overrides: [ + scopedProvider.overrideWith((ref) => 0), + unimplementedScopedProvider.overrideWith((ref) => 0), + rootProvider.overrideWith((ref) => 0), + ], + child: Container(), + ), + ); + + flutter.runApp( ProviderScope( parent: rootContainer, overrides: [