diff --git a/packages/leancode_lint/CHANGELOG.md b/packages/leancode_lint/CHANGELOG.md index 4ccb3a10..b40ae7fc 100644 --- a/packages/leancode_lint/CHANGELOG.md +++ b/packages/leancode_lint/CHANGELOG.md @@ -1,3 +1,8 @@ +# Unreleased + +- Add new custom lints: + - [`avoid_build_context_in_blocs`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#avoid_build_context_in_blocs) + # 25.0.0 - Add new custom lint [`prefer_abstract_final_class`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#prefer_abstract_final_class) @@ -97,7 +102,7 @@ - Remove the following lints which have been removed from Dart: - [`package_api_docs`](https://dart.dev/tools/linter-rules/package_api_docs) - [`unsafe_html`](https://dart.dev/tools/linter-rules/unsafe_html) -- Disable the [`require_trailing_commas`](https://dart.dev/tools/linter-rules/require_trailing_commas) lint as it conflicts with Dart 3.7 formatter (https://github.com/dart-lang/sdk/issues/60119). +- Disable the [`require_trailing_commas`](https://dart.dev/tools/linter-rules/require_trailing_commas) lint as it conflicts with Dart 3.7 formatter (). # 15.1.0 diff --git a/packages/leancode_lint/README.md b/packages/leancode_lint/README.md index c58e1d7b..e9000099 100644 --- a/packages/leancode_lint/README.md +++ b/packages/leancode_lint/README.md @@ -140,6 +140,56 @@ None. +
+avoid_build_context_in_blocs + +### `avoid_build_context_in_blocs` + +**AVOID** letting a `BuildContext` cross into a Bloc/Cubit. + +A `BuildContext` couples business logic to the widget tree, which risks stale +contexts and wrong `InheritedWidget` reads and makes the logic hard to test. The +rule flags both passing a `BuildContext` into a Bloc/Cubit (via a method such as +`add`, or a constructor) and declaring one inside a Bloc/Cubit (as a parameter or +field). A value merely derived from a context (e.g. `MediaQuery.sizeOf(context)`) +is allowed. + +**BAD:** + +```dart +bloc.add(CounterEvent(context)); + +final event = CounterEvent(context); +bloc.add(event); + +class CounterCubit extends Cubit { + CounterCubit() : super(0); + + final BuildContext context; + + void another(BuildContext context) {} +} +``` + +**GOOD:** + +```dart +bloc.add(CounterEvent()); +bloc.add(CounterEvent(MediaQuery.sizeOf(context))); + +class CounterCubit extends Cubit { + CounterCubit() : super(0); + + void another() {} +} +``` + +#### Configuration + +None. + +
+
avoid_catch_error diff --git a/packages/leancode_lint/lib/plugin.dart b/packages/leancode_lint/lib/plugin.dart index 8f5df035..b9289ec4 100644 --- a/packages/leancode_lint/lib/plugin.dart +++ b/packages/leancode_lint/lib/plugin.dart @@ -5,6 +5,7 @@ import 'package:leancode_lint/src/assists/convert_iterable_map_to_collection_for import 'package:leancode_lint/src/assists/convert_positional_to_named_formal.dart'; import 'package:leancode_lint/src/assists/convert_record_into_nominal_type.dart'; import 'package:leancode_lint/src/lints/add_cubit_suffix_for_cubits.dart'; +import 'package:leancode_lint/src/lints/avoid_build_context_in_blocs.dart'; import 'package:leancode_lint/src/lints/avoid_catch_error.dart'; import 'package:leancode_lint/src/lints/avoid_conditional_hooks.dart'; import 'package:leancode_lint/src/lints/avoid_single_child_in_multi_child_widget.dart'; @@ -64,6 +65,7 @@ final class LeanCodeLintPlugin extends Plugin { CatchParameterNames(config: config.catchParameterNames), ) ..registerWarningRule(AvoidCatchError()) + ..registerWarningRule(AvoidBuildContextInBlocs()) ..registerWarningRule(AvoidConditionalHooks()) ..registerWarningRule(HookWidgetDoesNotUseHooks()) ..registerFixForRule( diff --git a/packages/leancode_lint/lib/src/lints/avoid_build_context_in_blocs.dart b/packages/leancode_lint/lib/src/lints/avoid_build_context_in_blocs.dart new file mode 100644 index 00000000..6daa5997 --- /dev/null +++ b/packages/leancode_lint/lib/src/lints/avoid_build_context_in_blocs.dart @@ -0,0 +1,241 @@ +import 'package:analyzer/analysis_rule/analysis_rule.dart'; +import 'package:analyzer/analysis_rule/rule_context.dart'; +import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/token.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:analyzer/error/error.dart'; +import 'package:leancode_lint/src/bloc_utils.dart'; +import 'package:leancode_lint/src/type_checker.dart'; + +/// Warns when a `BuildContext` crosses into a Bloc/Cubit. +/// +/// A `BuildContext` couples business logic to the widget tree, which risks +/// stale contexts and wrong `InheritedWidget` reads and makes the logic hard to +/// test. This rule flags a `BuildContext` on both sides of the boundary: +/// passing one into a Bloc/Cubit (via a method, e.g. `bloc.add(...)`, or a +/// constructor) and declaring one inside a Bloc/Cubit (as a parameter or field). +class AvoidBuildContextInBlocs extends AnalysisRule { + AvoidBuildContextInBlocs() + : super(name: code.lowerCaseName, description: code.problemMessage); + + static const code = LintCode( + 'avoid_build_context_in_blocs', + "Avoid {0} a 'BuildContext' {1} a {2}.", + correctionMessage: + "Remove the 'BuildContext' and pass only the data the {2} needs.", + severity: .WARNING, + ); + + @override + LintCode get diagnosticCode => code; + + @override + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, + ) { + final visitor = _Visitor(this); + registry + ..addMethodInvocation(this, visitor) + ..addInstanceCreationExpression(this, visitor) + ..addClassDeclaration(this, visitor); + } +} + +class _Visitor extends SimpleAstVisitor { + _Visitor(this.rule); + + final AnalysisRule rule; + + static const _buildContextChecker = TypeChecker.fromName( + 'BuildContext', + packageName: 'flutter', + ); + + // Passing side: `bloc.add(...)` and other method calls on a Bloc/Cubit. + @override + void visitMethodInvocation(MethodInvocation node) { + final blocType = determineBlocType(node.realTarget?.staticType?.element); + if (blocType == null) { + return; + } + + _reportContextArguments(node.argumentList, blocType); + } + + // Passing side: `CounterCubit(context)` / `CounterBloc(context)`. + @override + void visitInstanceCreationExpression(InstanceCreationExpression node) { + final blocType = determineBlocType(node.staticType?.element); + if (blocType == null) { + return; + } + + _reportContextArguments(node.argumentList, blocType); + } + + // Declaration side: `BuildContext` parameters and fields inside a Bloc/Cubit. + @override + void visitClassDeclaration(ClassDeclaration node) { + final blocType = determineBlocType(node.declaredFragment?.element); + if (blocType == null) { + return; + } + + final members = switch (node.body) { + BlockClassBody(:final members) => members, + _ => const [], + }; + + for (final member in members) { + switch (member) { + case MethodDeclaration(:final parameters?): + _checkParameters(parameters, blocType); + case ConstructorDeclaration(:final parameters): + _checkParameters(parameters, blocType); + case FieldDeclaration(:final fields): + _checkFields(fields, blocType); + case _: + break; + } + } + } + + void _checkParameters(FormalParameterList parameters, BlocType blocType) { + for (final parameter in parameters.parameters) { + if (parameter case FormalParameter( + :final name?, + declaredFragment: final fragment?, + ) when _isBuildContext(fragment.element.type)) { + rule.reportAtToken( + name, + arguments: ['declaring', 'parameter in', blocType.name], + ); + } + } + } + + void _checkFields(VariableDeclarationList fields, BlocType blocType) { + for (final variable in fields.variables) { + final type = variable.declaredFragment?.element.type; + if (type != null && _isBuildContext(type)) { + rule.reportAtToken( + variable.name, + arguments: ['declaring', 'field in', blocType.name], + ); + } + } + } + + void _reportContextArguments(ArgumentList argumentList, BlocType blocType) { + for (final argument in argumentList.arguments) { + final expression = argument.argumentExpression; + if (_carriesContext(expression, {}, 0)) { + rule.reportAtNode( + expression, + arguments: ['passing', 'to', blocType.name], + ); + } + } + } + + /// Whether [expression] carries a `BuildContext` into the enclosing call. + /// + /// Returns true when the expression is itself a `BuildContext`, when it + /// constructs an object with a `BuildContext` argument (e.g. an event like + /// `CounterEvent(context)`, including nested constructions), or when it is a + /// local variable whose initializer does so. Recursion deliberately does not + /// descend into method/function calls, so a value merely *derived* from a + /// context (e.g. `MediaQuery.sizeOf(context)`) is not flagged. + /// + /// The local-variable trace is best-effort: it only inspects the declaration + /// initializer, not later reassignments. [visited] guards against cycles. + bool _carriesContext( + Expression? expression, + Set visited, + int depth, + ) { + if (expression == null || depth > 20) { + return false; + } + + if (expression.staticType case final type? when _isBuildContext(type)) { + return true; + } + + switch (expression) { + case InstanceCreationExpression(:final argumentList): + for (final argument in argumentList.arguments) { + if (_carriesContext( + argument.argumentExpression, + visited, + depth + 1, + )) { + return true; + } + } + case ParenthesizedExpression(:final expression): + case AsExpression(:final expression): + return _carriesContext(expression, visited, depth + 1); + case PostfixExpression(:final operand, :final operator) + when operator.type == TokenType.BANG: + return _carriesContext(operand, visited, depth + 1); + case SimpleIdentifier(:final LocalVariableElement element?) + when visited.add(element): + final initializer = _localVariableInitializer(expression, element); + if (_carriesContext(initializer, visited, depth + 1)) { + return true; + } + } + + return false; + } + + Expression? _localVariableInitializer( + AstNode reference, + LocalVariableElement element, + ) { + AstNode? body = reference; + while (body != null && body is! FunctionBody) { + body = body.parent; + } + if (body == null) { + return null; + } + + final finder = _InitializerFinder(element); + body.accept(finder); + return finder.initializer; + } + + bool _isBuildContext(DartType type) => + _buildContextChecker.isExactlyType(type); +} + +/// Finds the declaration initializer of a specific [LocalVariableElement]. +class _InitializerFinder extends GeneralizingAstVisitor { + _InitializerFinder(this.element); + + final LocalVariableElement element; + Expression? initializer; + + @override + void visitNode(AstNode node) { + if (initializer == null) { + super.visitNode(node); + } + } + + @override + void visitVariableDeclaration(VariableDeclaration node) { + if (initializer == null && + node.declaredFragment?.element == element && + node.initializer != null) { + initializer = node.initializer; + } + super.visitVariableDeclaration(node); + } +} diff --git a/packages/leancode_lint/test/mock_libraries/bloc.dart b/packages/leancode_lint/test/mock_libraries/bloc.dart index db9f874c..70040e61 100644 --- a/packages/leancode_lint/test/mock_libraries/bloc.dart +++ b/packages/leancode_lint/test/mock_libraries/bloc.dart @@ -15,6 +15,7 @@ abstract class Cubit extends BlocBase { abstract class Bloc extends BlocBase { Bloc(State initialState) : super(initialState); + void add(Event event) {} } '''); super.setUp(); diff --git a/packages/leancode_lint/test/test_cases/avoid_build_context_in_blocs_test.dart b/packages/leancode_lint/test/test_cases/avoid_build_context_in_blocs_test.dart new file mode 100644 index 00000000..f927d8ac --- /dev/null +++ b/packages/leancode_lint/test/test_cases/avoid_build_context_in_blocs_test.dart @@ -0,0 +1,358 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:leancode_lint/src/lints/avoid_build_context_in_blocs.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +import '../assert_ranges.dart'; +import '../mock_libraries.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidBuildContextInBlocsTest); + }); +} + +@reflectiveTest +class AvoidBuildContextInBlocsTest extends AnalysisRuleTest + with MockBloc, MockFlutter { + @override + void setUp() { + rule = AvoidBuildContextInBlocs(); + + super.setUp(); + } + + Future test_passingContextInEvent_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent([this.data]); + final Object? data; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + bloc.add([!CounterEvent(context)!]); +} +'''); + } + + Future test_passingContextDirectly_flagged() async { + const code = ''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + bloc.add(context); +} +'''; + + await assertDiagnostics(code, [ + lint( + code.lastIndexOf('context'), + 'context'.length, + messageContainsAll: ["Avoid passing a 'BuildContext' to a bloc."], + correctionContains: + "Remove the 'BuildContext' and pass only the data the bloc needs.", + ), + ]); + } + + Future test_passingContextViaLocalVariable_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent([this.data]); + final Object? data; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + final event = CounterEvent(context); + bloc.add([!event!]); +} +'''); + } + + Future test_passingContextInNestedEvent_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class Inner { + Inner(this.context); + final BuildContext context; +} + +class CounterEvent { + CounterEvent(this.inner); + final Inner inner; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + bloc.add([!CounterEvent(Inner(context))!]); +} +'''); + } + + Future test_passingContextToConstructor_flagged() async { + const code = ''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterCubit extends Cubit { + CounterCubit(Object data) : super(0); +} + +void f(BuildContext context) { + CounterCubit(context); +} +'''; + + await assertDiagnostics(code, [ + lint( + code.lastIndexOf('context'), + 'context'.length, + messageContainsAll: ["Avoid passing a 'BuildContext' to a cubit."], + correctionContains: + "Remove the 'BuildContext' and pass only the data the cubit needs.", + ), + ]); + } + + Future test_cubitMethodParameter_flagged() async { + const code = ''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterCubit extends Cubit { + CounterCubit() : super(0); + + void another(BuildContext context) {} +} +'''; + + await assertDiagnostics(code, [ + lint( + code.lastIndexOf('context'), + 'context'.length, + messageContainsAll: [ + "Avoid declaring a 'BuildContext' parameter in a cubit.", + ], + ), + ]); + } + + Future test_blocMethodParameter_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterBloc extends Bloc { + CounterBloc() : super(0); + + void another(BuildContext [!context!]) {} +} +'''); + } + + Future test_passingContextToCustomBlocMethod_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterBloc extends Bloc { + CounterBloc() : super(0); + + void doSomething(BuildContext /*[0*/context/*0]*/) {} +} + +void f(CounterBloc bloc, BuildContext context) { + bloc.doSomething(/*[1*/context/*1]*/); +} +'''); + } + + Future test_cubitConstructorParameter_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterCubit extends Cubit { + CounterCubit(BuildContext [!context!]) : super(0); +} +'''); + } + + Future test_cubitField_flagged() async { + const code = ''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterCubit extends Cubit { + CounterCubit() : super(0); + + late final BuildContext context; +} +'''; + + await assertDiagnostics(code, [ + lint( + code.lastIndexOf('context'), + 'context'.length, + messageContainsAll: [ + "Avoid declaring a 'BuildContext' field in a cubit.", + ], + ), + ]); + } + + Future test_passingParenthesizedLocalVariable_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent(this.context); + final BuildContext context; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + final event = CounterEvent(context); + bloc.add([!(event)!]); +} +'''); + } + + Future test_passingCastLocalVariable_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent(this.context); + final BuildContext context; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + final event = CounterEvent(context); + bloc.add([!event as Object!]); +} +'''); + } + + Future test_passingNullAssertedLocalVariable_flagged() async { + await assertDiagnosticsInRanges(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent(this.context); + final BuildContext context; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + final CounterEvent? event = CounterEvent(context) as dynamic; + bloc.add(/*[0*/event!/*0]*/); +} +'''); + } + + Future test_addWithoutContext_ok() async { + await assertNoDiagnostics(''' +import 'package:bloc/bloc.dart'; + +class CounterEvent { + CounterEvent(); +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc) { + bloc.add(CounterEvent()); +} +'''); + } + + Future test_passingContextDerivedValue_ok() async { + await assertNoDiagnostics(''' +import 'package:flutter/material.dart'; +import 'package:bloc/bloc.dart'; + +int deriveFrom(BuildContext context) => 0; + +class CounterEvent { + CounterEvent(this.data); + final int data; +} + +class CounterBloc extends Bloc { + CounterBloc() : super(0); +} + +void f(CounterBloc bloc, BuildContext context) { + bloc.add(CounterEvent(deriveFrom(context))); +} +'''); + } + + Future test_contextParameterOnNonBloc_ok() async { + await assertNoDiagnostics(''' +import 'package:flutter/material.dart'; + +class NotABloc { + void another(BuildContext context) {} +} +'''); + } + + Future test_methodCallOnNonBloc_ok() async { + await assertNoDiagnostics(''' +import 'package:flutter/material.dart'; + +class NotABloc { + void add(Object event) {} +} + +class CounterEvent { + CounterEvent(this.context); + final BuildContext context; +} + +void f(NotABloc notABloc, BuildContext context) { + notABloc.add(CounterEvent(context)); +} +'''); + } +}