diff --git a/packages/leancode_lint/CHANGELOG.md b/packages/leancode_lint/CHANGELOG.md index b40ae7fc2..1ea7f1dfc 100644 --- a/packages/leancode_lint/CHANGELOG.md +++ b/packages/leancode_lint/CHANGELOG.md @@ -1,8 +1,9 @@ # Unreleased - Add new custom lints: + - [`avoid_context_read_in_build`](https://github.com/leancodepl/flutter_corelibrary/tree/master/packages/leancode_lint#avoid_context_read_in_build) - [`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) diff --git a/packages/leancode_lint/README.md b/packages/leancode_lint/README.md index e9000099e..595cb982d 100644 --- a/packages/leancode_lint/README.md +++ b/packages/leancode_lint/README.md @@ -306,6 +306,67 @@ None. +
+avoid_context_read_in_build + +### `avoid_context_read_in_build` + +**AVOID** using `context.read` inside a `build` method. + +`read` grabs a value once and never re-subscribes, so using its result to render +leaves the UI stale when the value changes — `watch` (or a `BlocBuilder` / +`BlocSelector`) is what you want. `select` (or `BlocSelector`) works too, and is +preferable when only part of the state is needed. + +Every `read` that executes during `build` is flagged, whatever it is used for: +reading a value, calling a method, or grabbing a bloc/service reference. All +three run on every rebuild, so none of them belong in `build`. Either consume +the value with `watch` / `select` / `BlocBuilder` / `BlocSelector`, or move the +read into a callback. Reads inside deferred interaction callbacks (`onTap`, +`onPressed`) are exempt — that's where `read` is meant to be used; reads inside +builder closures that run during `build` are checked. + +**BAD:** + +```dart +Widget build(BuildContext context) { + final count = context.read().state; + return Text('$count'); +} +``` + +```dart +Widget build(BuildContext context) { + // Fires on every rebuild. + context.read().increment(); + return const SizedBox(); +} +``` + +**GOOD:** + +```dart +Widget build(BuildContext context) { + final count = context.watch().state; + return Text('$count'); +} +``` + +```dart +Widget build(BuildContext context) { + return ElevatedButton( + onPressed: () => context.read().increment(), + child: const Text('+'), + ); +} +``` + +#### Configuration + +None. + +
+
bloc_related_class_naming diff --git a/packages/leancode_lint/lib/plugin.dart b/packages/leancode_lint/lib/plugin.dart index b9289ec41..bf2c49522 100644 --- a/packages/leancode_lint/lib/plugin.dart +++ b/packages/leancode_lint/lib/plugin.dart @@ -8,6 +8,7 @@ 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_context_read_in_build.dart'; import 'package:leancode_lint/src/lints/avoid_single_child_in_multi_child_widget.dart'; import 'package:leancode_lint/src/lints/bloc_related_class_naming.dart'; import 'package:leancode_lint/src/lints/bloc_subclasses_naming.dart'; @@ -77,6 +78,11 @@ final class LeanCodeLintPlugin extends Plugin { NeverDiscardBuildContext.code, RenameDiscardedBuildContextFix.new, ) + ..registerWarningRule(AvoidContextReadInBuild()) + ..registerFixForRule( + AvoidContextReadInBuild.code, + ReplaceContextReadWithWatchFix.new, + ) // TODO: disabled by default until stabilized. Add documentation. ..registerLintRule(ConstructorParametersAndFieldsShouldHaveTheSameOrder()) ..registerWarningRule(AvoidSingleChildInMultiChildWidgets()) diff --git a/packages/leancode_lint/lib/src/lints/avoid_context_read_in_build.dart b/packages/leancode_lint/lib/src/lints/avoid_context_read_in_build.dart new file mode 100644 index 000000000..6a41d6ac4 --- /dev/null +++ b/packages/leancode_lint/lib/src/lints/avoid_context_read_in_build.dart @@ -0,0 +1,143 @@ +import 'package:analysis_server_plugin/edit/dart/correction_producer.dart'; +import 'package:analysis_server_plugin/edit/dart/dart_fix_kind_priority.dart'; +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/visitor.dart'; +import 'package:analyzer/error/error.dart'; +import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart'; +import 'package:analyzer_plugin/utilities/fixes/fixes.dart'; +import 'package:analyzer_plugin/utilities/range_factory.dart'; +import 'package:leancode_lint/src/helpers.dart'; +import 'package:leancode_lint/src/type_checker.dart'; + +/// Warns when `context.read` is called during `build`. +/// +/// `read` grabs a value once and never re-subscribes, so using its result to +/// render leaves the UI stale when the value changes — `watch` (or a +/// `BlocBuilder`/`BlocSelector`) is what's actually wanted. `select` (or +/// `BlocSelector`) works too, and is preferable when only part of the state +/// is needed. +/// +/// Every `read` that executes during build is reported, whatever it is used +/// for: reading a value, calling a method, or grabbing a bloc/service +/// reference. All three run on every rebuild, so none of them belong in +/// `build`. Reads inside deferred interaction callbacks (`onTap`, `onPressed`) +/// are exempt — that is where `read` is meant to be used; reads inside builder +/// closures that run during build are checked. +class AvoidContextReadInBuild extends AnalysisRule { + AvoidContextReadInBuild() + : super(name: code.lowerCaseName, description: code.problemMessage); + + static const code = LintCode( + 'avoid_context_read_in_build', + "Avoid using 'context.read' inside 'build' method.", + correctionMessage: + "Use 'context.watch' or 'context.select' (or BlocBuilder/BlocSelector) to consume the value, or move the read into a callback.", + severity: .WARNING, + ); + + @override + LintCode get diagnosticCode => code; + + @override + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, + ) { + registry.addMethodInvocation(this, _Visitor(this)); + } +} + +class _Visitor extends SimpleAstVisitor { + _Visitor(this.rule); + + final AnalysisRule rule; + + static const _buildContextChecker = TypeChecker.fromName( + 'BuildContext', + packageName: 'flutter', + ); + + @override + void visitMethodInvocation(MethodInvocation node) { + if (node.methodName.name != 'read') { + return; + } + final targetType = node.realTarget?.staticType; + if (targetType == null || + !_buildContextChecker.isAssignableFromType(targetType)) { + return; + } + + if (!_runsDuringBuild(node)) { + return; + } + + rule.reportAtNode(node.methodName); + } + + /// Whether [node] executes during build: it is inside a widget's `build` + /// method, and every closure between [node] and that method declares a + /// `BuildContext` parameter (i.e. is a builder that runs during build, not a + /// deferred interaction callback). + bool _runsDuringBuild(AstNode node) { + for ( + AstNode? current = node.parent; + current != null; + current = current.parent + ) { + if (current is FunctionExpression && + !_declaresBuildContextParameter(current)) { + return false; + } + if (current is MethodDeclaration) { + if (current.name.lexeme != 'build') { + return false; + } + final classDeclaration = current + .thisOrAncestorOfType(); + return classDeclaration != null && isWidgetClass(classDeclaration); + } + } + return false; + } + + bool _declaresBuildContextParameter(FunctionExpression function) { + final parameters = function.parameters?.parameters; + if (parameters == null) { + return false; + } + for (final parameter in parameters) { + final type = parameter.declaredFragment?.element.type; + if (type != null && _buildContextChecker.isAssignableFromType(type)) { + return true; + } + } + return false; + } +} + +class ReplaceContextReadWithWatchFix extends ResolvedCorrectionProducer { + ReplaceContextReadWithWatchFix({required super.context}); + + @override + FixKind get fixKind => const .new( + 'leancode_lint.fix.replaceContextReadWithWatch', + DartFixKindPriority.standard, + "Replace with 'context.watch'", + ); + + @override + CorrectionApplicability get applicability => .singleLocation; + + @override + Future compute(ChangeBuilder builder) async { + await builder.addDartFileEdit( + file, + (builder) => + builder.addSimpleReplacement(range.diagnostic(diagnostic!), 'watch'), + ); + } +} diff --git a/packages/leancode_lint/test/mock_libraries/flutter_bloc.dart b/packages/leancode_lint/test/mock_libraries/flutter_bloc.dart index d4bd4314e..980ffaebf 100644 --- a/packages/leancode_lint/test/mock_libraries/flutter_bloc.dart +++ b/packages/leancode_lint/test/mock_libraries/flutter_bloc.dart @@ -4,7 +4,15 @@ mixin MockFlutterBloc on AnalysisRuleTest { @override void setUp() { newPackage('flutter_bloc').addFile('lib/flutter_bloc.dart', ''' +import 'package:flutter/material.dart'; + export 'package:bloc/bloc.dart'; + +extension BlocContextExtention on BuildContext { + T read() => throw UnimplementedError(); + + T watch() => throw UnimplementedError(); +} '''); super.setUp(); } diff --git a/packages/leancode_lint/test/test_cases/avoid_context_read_in_build_test.dart b/packages/leancode_lint/test/test_cases/avoid_context_read_in_build_test.dart new file mode 100644 index 000000000..4b208c89e --- /dev/null +++ b/packages/leancode_lint/test/test_cases/avoid_context_read_in_build_test.dart @@ -0,0 +1,166 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:leancode_lint/src/lints/avoid_context_read_in_build.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +import '../assert_ranges.dart'; +import '../mock_libraries.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidContextReadInBuildTest); + }); +} + +/// Wraps [buildBody] (the contents of a widget's `build` method) in a source +/// file with the helpers the test cases reference. +String _widget(String buildBody) => + ''' +import 'package:flutter/material.dart'; +import 'package:flutter_bloc/flutter_bloc.dart'; + +class MyCubit extends Cubit { + MyCubit() : super(0); + void doThing() {} +} + +class MyService {} + +class Consumer extends StatelessWidget { + const Consumer({super.key, this.value}); + final Object? value; + @override + Widget build(BuildContext context) => const SizedBox(); +} + +class Button extends StatelessWidget { + const Button({super.key, this.onTap}); + final void Function()? onTap; + @override + Widget build(BuildContext context) => const SizedBox(); +} + +class MyWidget extends StatelessWidget { + const MyWidget({super.key}); + + @override + Widget build(BuildContext context) { +$buildBody + } +} +'''; + +@reflectiveTest +class AvoidContextReadInBuildTest extends AnalysisRuleTest + with MockFlutter, MockBloc, MockFlutterBloc { + @override + void setUp() { + rule = AvoidContextReadInBuild(); + + super.setUp(); + } + + Future test_stateGetter_intoVariable_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + final s = context.[!read!]().state; + return Consumer(value: s);'''), + ); + } + + Future test_stateGetter_inline_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + return Consumer(value: context.[!read!]().state);'''), + ); + } + + Future test_plainValue_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + return Consumer(value: context.[!read!]());'''), + ); + } + + Future test_insideBuilder_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + return Builder( + builder: (context) => Consumer(value: context.[!read!]().state), + );'''), + ); + } + + Future test_methodReceiver_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + context.[!read!]().doThing(); + return const SizedBox();'''), + ); + } + + Future test_blocObjectReference_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + return Consumer(value: context.[!read!]());'''), + ); + } + + /// The tear-off evaluates the read during build, unlike + /// [test_deferredCallback_ok] which defers it until the tap. + Future test_methodTearOff_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + return Button(onTap: context.[!read!]().doThing);'''), + ); + } + + Future test_serviceReference_flagged() async { + await assertDiagnosticsInRanges( + _widget(''' + final s = context.[!read!](); + return Consumer(value: s);'''), + ); + } + + Future test_deferredCallback_ok() async { + await assertNoDiagnostics( + _widget(''' + return Button(onTap: () => context.read().doThing());'''), + ); + } + + Future test_deferredCallbackInsideBuilder_ok() async { + await assertNoDiagnostics( + _widget(''' + return Builder( + builder: (context) => + Button(onTap: () => context.read().doThing()), + );'''), + ); + } + + Future test_watch_ok() async { + await assertNoDiagnostics( + _widget(''' + return Consumer(value: context.watch().state);'''), + ); + } + + Future test_readOutsideWidget_ok() async { + await assertNoDiagnostics(''' +import 'package:flutter/material.dart'; +import 'package:flutter_bloc/flutter_bloc.dart'; + +class MyCubit extends Cubit { + MyCubit() : super(0); +} + +class NotAWidget { + NotAWidget(this.context); + final BuildContext context; + + int build() => context.read().state; +} +'''); + } +}