diff --git a/CHANGELOG.md b/CHANGELOG.md index 5678526f78..56445421bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * Remove deprecated `Lines of Executable Code` metric, use `Source lines of Code` instead. +* Add static code diagnostic `avoid-unnecessary-setstate`. * Changed the supported `analyzer` version to `^1.7.0`. ## 3.2.2 @@ -11,7 +12,7 @@ ## 3.2.1 -* Remove unnecessary scan by `Lines of Executable Code` +* Remove unnecessary scan by `Lines of Executable Code`. ## 3.2.0 diff --git a/README.md b/README.md index 35a1e7117e..07823abbe0 100644 --- a/README.md +++ b/README.md @@ -340,6 +340,7 @@ Rules configuration is [described here](#configuring-a-rules-entry). ### Flutter specific - [avoid-returning-widgets](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/avoid-returning-widgets.md)   [![Configurable](https://img.shields.io/badge/-configurable-informational)](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/avoid-returning-widgets.md#config-example) +- [avoid-unnecessary-setstate](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/avoid-unnecessary-setstate.md) ### Intl specific diff --git a/doc/rules/avoid-unnecessary-setstate.md b/doc/rules/avoid-unnecessary-setstate.md new file mode 100644 index 0000000000..1a7ace6346 --- /dev/null +++ b/doc/rules/avoid-unnecessary-setstate.md @@ -0,0 +1,151 @@ +# Avoid unnecessary setState + +## Rule id + +avoid-unnecessary-setstate + +## Description + +Warns when `setState` is called inside `initState`, `didUpdateWidget` or `build` methods and when it's called from a `sync` method that is called inside those methods. + +Calling setState in those cases will lead to an additional widget rerender which is bad for performance. + +Consider changing state directly without calling `setState`. + +Additional resources: + +* + +### Example + +Bad: + +```dart +class MyWidget extends StatefulWidget { + @override + _MyWidgetState createState() => _MyWidgetState(); +} + +class _MyWidgetState extends State { + String myString = ''; + + @override + void initState() { + super.initState(); + + // LINT + setState(() { + myString = "Hello"; + }); + + if (condition) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + myStateUpdateMethod(); // LINT + } + + @override + void didUpdateWidget(MyWidget oldWidget) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + void myStateUpdateMethod() { + setState(() { + myString = "Hello"; + }); + } + + @override + Widget build(BuildContext context) { + // LINT + setState(() { + myString = "Hello"; + }); + + if (condition) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + myStateUpdateMethod(); // LINT + + return ElevatedButton( + onPressed: () => myStateUpdateMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + } +} +``` + +Good: + +```dart +class MyWidget extends StatefulWidget { + @override + _MyWidgetState createState() => _MyWidgetState(); +} + +class _MyWidgetState extends State { + String myString = ''; + + final classWithMethod = SomeClassWithMethod(); + + @override + void initState() { + super.initState(); + + myString = "Hello"; + + classWithMethod.myMethod(); + myAsyncMethod(); + } + + @override + void didUpdateWidget(MyWidget oldWidget) { + myString = "Hello"; + } + + void myStateUpdateMethod() { + setState(() { + myString = "Hello"; + }); + } + + Future myAsyncMethod() async { + final data = await service.fetchData(); + + setState(() { + myString = data; + }); + } + + @override + Widget build(BuildContext context) { + myAsyncMethod(); + + return ElevatedButton( + onPressed: () => myStateUpdateMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + } +} +``` diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart b/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart index 715428321f..f2d1acdddd 100644 --- a/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart +++ b/lib/src/analyzers/lint_analyzer/rules/rules_factory.dart @@ -3,6 +3,7 @@ import 'rules_list/avoid_late_keyword/avoid_late_keyword.dart'; import 'rules_list/avoid_non_null_assertion/avoid_non_null_assertion.dart'; import 'rules_list/avoid_preserve_whitespace_false/avoid_preserve_whitespace_false.dart'; import 'rules_list/avoid_returning_widgets/avoid_returning_widgets.dart'; +import 'rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate.dart'; import 'rules_list/avoid_unused_parameters/avoid_unused_parameters.dart'; import 'rules_list/binary_expression_operand_order/binary_expression_operand_order.dart'; import 'rules_list/component_annotation_arguments_ordering/component_annotation_arguments_ordering.dart'; @@ -30,6 +31,8 @@ final _implementedRules = )>{ AvoidPreserveWhitespaceFalseRule(config), AvoidReturningWidgetsRule.ruleId: (config) => AvoidReturningWidgetsRule(config), + AvoidUnnecessarySetStateRule.ruleId: (config) => + AvoidUnnecessarySetStateRule(config), AvoidUnusedParametersRule.ruleId: (config) => AvoidUnusedParametersRule(config), BinaryExpressionOperandOrderRule.ruleId: (config) => diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate.dart new file mode 100644 index 0000000000..ebfa7c50f9 --- /dev/null +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate.dart @@ -0,0 +1,60 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:collection/collection.dart'; + +import '../../../../../utils/node_utils.dart'; +import '../../../../models/internal_resolved_unit_result.dart'; +import '../../../../models/issue.dart'; +import '../../../../models/severity.dart'; +import '../../models/rule.dart'; +import '../../models/rule_documentation.dart'; +import '../../rule_utils.dart'; + +part 'visitor.dart'; + +class AvoidUnnecessarySetStateRule extends Rule { + static const String ruleId = 'avoid-unnecessary-setstate'; + + static const _warningMessage = + 'Avoid calling unnecessary setState. Consider changing the state directly.'; + static const _methodWarningMessage = 'Avoid calling a method with setState.'; + + AvoidUnnecessarySetStateRule([Map config = const {}]) + : super( + id: ruleId, + documentation: const RuleDocumentation( + name: 'Avoid returning widgets', + brief: + 'Warns when `setState` is called inside `initState`, `didUpdateWidget` or `build` methods and when it is called from a `sync` method that is called inside those methods.', + ), + severity: readSeverity(config, Severity.warning), + excludes: readExcludes(config), + ); + + @override + Iterable check(InternalResolvedUnitResult source) { + final _visitor = _Visitor(); + + source.unit.visitChildren(_visitor); + + return [ + ..._visitor.setStateInvocations.map((invocation) => createIssue( + rule: this, + location: nodeLocation( + node: invocation, + source: source, + ), + message: _warningMessage, + )), + ..._visitor.classMethodsInvocations.map((invocation) => createIssue( + rule: this, + location: nodeLocation( + node: invocation, + source: source, + ), + message: _methodWarningMessage, + )) + ]; + } +} diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/visitor.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/visitor.dart new file mode 100644 index 0000000000..9520abed13 --- /dev/null +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/visitor.dart @@ -0,0 +1,112 @@ +part of 'avoid_unnecessary_setstate.dart'; + +class _Visitor extends RecursiveAstVisitor { + static const _checkedMethods = ['initState', 'didUpdateWidget', 'build']; + + final _setStateInvocations = []; + final _classMethodsInvocations = []; + + Iterable get setStateInvocations => _setStateInvocations; + Iterable get classMethodsInvocations => + _classMethodsInvocations; + + @override + void visitClassDeclaration(ClassDeclaration node) { + super.visitClassDeclaration(node); + + final type = node.extendsClause?.superclass.type; + if (type == null || !_hasWidgetStateType(type)) { + return; + } + + final methods = node.members + .whereType() + .where((member) => _checkedMethods.contains(member.name.name)) + .toList(); + final restMethods = node.members + .whereType() + .where((member) => !_checkedMethods.contains(member.name.name)) + .toList(); + final restMethodsNames = + restMethods.map((method) => method.name.name).toList(); + + final visitedRestMethods = {}; + + for (final method in methods) { + final visitor = _MethodVisitor(restMethodsNames); + method.visitChildren(visitor); + + _setStateInvocations.addAll(visitor.setStateInvocations); + _classMethodsInvocations.addAll( + visitor.classMethodsInvocations + .where((invocation) => _containsSetState( + visitedRestMethods, + restMethods.firstWhere((method) => + method.name.name == invocation.methodName.name), + )) + .toList(), + ); + } + } + + bool _hasWidgetStateType(DartType type) => + _isWidgetState(type) || _isSubclassOfWidgetState(type); + + bool _isSubclassOfWidgetState(DartType? type) => + type is InterfaceType && + type.allSupertypes.firstWhereOrNull(_isWidgetState) != null; + + bool _isWidgetState(DartType? type) => type?.element?.displayName == 'State'; + + bool _containsSetState( + Map visitedRestMethods, + MethodDeclaration declaration, + ) { + final type = declaration.returnType?.type; + if (type != null && (type.isDartAsyncFuture || type.isDartAsyncFutureOr)) { + return false; + } + + final name = declaration.name.name; + if (visitedRestMethods.containsKey(name) && visitedRestMethods[name]!) { + return true; + } + + final visitor = _MethodVisitor([]); + declaration.visitChildren(visitor); + + final hasSetState = visitor.setStateInvocations.isNotEmpty; + + visitedRestMethods[name] = hasSetState; + return hasSetState; + } +} + +class _MethodVisitor extends RecursiveAstVisitor { + final Iterable classMethodsNames; + + final _setStateInvocations = []; + final _classMethodsInvocations = []; + + Iterable get setStateInvocations => _setStateInvocations; + Iterable get classMethodsInvocations => + _classMethodsInvocations; + + _MethodVisitor(this.classMethodsNames); + + @override + void visitMethodInvocation(MethodInvocation node) { + super.visitMethodInvocation(node); + + final name = node.methodName.name; + + if (name == 'setState' && + node.thisOrAncestorOfType() == null) { + _setStateInvocations.add(node); + } else if (classMethodsNames.contains(name) && + node.thisOrAncestorOfType() == null && + node.realTarget == null) { + _classMethodsInvocations.add(node); + } + } +} diff --git a/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate_test.dart b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate_test.dart new file mode 100644 index 0000000000..33f46874ce --- /dev/null +++ b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate_test.dart @@ -0,0 +1,76 @@ +@TestOn('vm') +import 'package:dart_code_metrics/src/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/avoid_unnecessary_setstate.dart'; +import 'package:dart_code_metrics/src/analyzers/models/severity.dart'; +import 'package:test/test.dart'; + +import '../../../../../helpers/rule_test_helper.dart'; + +// ignore_for_file: no_adjacent_strings_in_list + +const _examplePath = 'avoid_unnecessary_setstate/examples/example.dart'; +const _stateSubclassExamplePath = + 'avoid_unnecessary_setstate/examples/state_subclass_example.dart'; + +void main() { + group('AvoidUnnecessarySetStateRule', () { + test('initialization', () async { + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidUnnecessarySetStateRule().check(unit); + + RuleTestHelper.verifyInitialization( + issues: issues, + ruleId: 'avoid-unnecessary-setstate', + severity: Severity.warning, + ); + }); + + test('reports about found issues', () async { + final unit = await RuleTestHelper.resolveFromFile(_examplePath); + final issues = AvoidUnnecessarySetStateRule().check(unit); + + RuleTestHelper.verifyIssues( + issues: issues, + startOffsets: [302, 392, 606, 941, 1031, 455, 1294], + startLines: [16, 22, 35, 57, 63, 27, 78], + startColumns: [5, 7, 5, 5, 7, 5, 5], + endOffsets: [348, 442, 652, 987, 1081, 465, 1304], + locationTexts: [ + 'setState(() {\n' + ' myString = "Hello";\n' + ' })', + 'setState(() {\n' + ' myString = "Hello";\n' + ' })', + 'setState(() {\n' + ' myString = "Hello";\n' + ' })', + 'setState(() {\n' + ' myString = "Hello";\n' + ' })', + 'setState(() {\n' + ' myString = "Hello";\n' + ' })', + 'myMethod()', + 'myMethod()', + ], + messages: [ + 'Avoid calling unnecessary setState. Consider changing the state directly.', + 'Avoid calling unnecessary setState. Consider changing the state directly.', + 'Avoid calling unnecessary setState. Consider changing the state directly.', + 'Avoid calling unnecessary setState. Consider changing the state directly.', + 'Avoid calling unnecessary setState. Consider changing the state directly.', + 'Avoid calling a method with setState.', + 'Avoid calling a method with setState.', + ], + ); + }); + + test('reports no issues with State subclass', () async { + final unit = + await RuleTestHelper.resolveFromFile(_stateSubclassExamplePath); + final issues = AvoidUnnecessarySetStateRule().check(unit); + + RuleTestHelper.verifyNoIssues(issues); + }); + }); +} diff --git a/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/example.dart b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/example.dart new file mode 100644 index 0000000000..bcdd131db8 --- /dev/null +++ b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/example.dart @@ -0,0 +1,109 @@ +class MyWidget extends StatefulWidget { + @override + _MyWidgetState createState() => _MyWidgetState(); +} + +class _MyWidgetState extends State { + String myString = ''; + + final classWithMethod = SomeClassWithMethod(); + + @override + void initState() { + super.initState(); + + // LINT + setState(() { + myString = "Hello"; + }); + + if (condition) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + myMethod(); // LINT + classWithMethod.myMethod(); + myAsyncMethod(); + } + + @override + void didUpdateWidget(MyWidget oldWidget) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + void myMethod() { + setState(() { + myString = "Hello"; + }); + } + + Future myAsyncMethod() async { + final data = await service.fetchData(); + + setState(() { + myString = data; + }); + } + + @override + Widget build(BuildContext context) { + // LINT + setState(() { + myString = "Hello"; + }); + + if (condition) { + // LINT + setState(() { + myString = "Hello"; + }); + } + + final widget = ElevatedButton( + onPressed: () => myMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + + myMethod(); // LINT + myAsyncMethod(); + + return ElevatedButton( + onPressed: () => myMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + } +} + +class ElevatedButton { + final Function onPressed; + final Function onLongPress; + final dynamic child; + + const ElevatedButton( + this.onPressed, + this.onLongPress, + this.child, + ); +} + +class SomeClassWithMethod { + void myMethod() {} +} + +class State {} diff --git a/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/state_subclass_example.dart b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/state_subclass_example.dart new file mode 100644 index 0000000000..80124f1100 --- /dev/null +++ b/test/analyzers/lint_analyzer/rules/rules_list/avoid_unnecessary_setstate/examples/state_subclass_example.dart @@ -0,0 +1,75 @@ +class MyWidget extends StatefulWidget { + @override + _MyWidgetState createState() => _MyWidgetState(); +} + +class _MyWidgetState extends BaseState { + String myString = ''; + + @override + void initState() { + super.initState(); + + classWithMethod.myMethod(); + myAsyncMethod(); + } + + void myMethod() { + setState(() { + myString = "Hello"; + }); + } + + Future myAsyncMethod() async { + final data = await service.fetchData(); + + setState(() { + myString = data; + }); + } + + @override + Widget build(BuildContext context) { + final widget = ElevatedButton( + onPressed: () => myMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + + myAsyncMethod(); + + return ElevatedButton( + onPressed: () => myMethod(), + onLongPress: () { + setState(() { + myString = data; + }); + }, + child: Text('PRESS'), + ); + } +} + +class ElevatedButton { + final Function onPressed; + final Function onLongPress; + final dynamic child; + + const ElevatedButton( + this.onPressed, + this.onLongPress, + this.child, + ); +} + +class SomeClassWithMethod { + void myMethod() {} +} + +class State {} + +class BaseState extends State {}