diff --git a/CHANGELOG.md b/CHANGELOG.md index 288ea100bd..4cb2999092 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * Add static code diagnostics `always-remove-listener` and `avoid-unnecessary-setstate`. +* Improve static code diagnostic `avoid-returning-widgets`. * Remove deprecated `Lines of Executable Code` metric, use `Source lines of Code` instead. * Changed the supported `analyzer` version to `^1.7.0`. diff --git a/README.md b/README.md index dd122587e7..94e7fce616 100644 --- a/README.md +++ b/README.md @@ -339,7 +339,7 @@ Rules configuration is [described here](#configuring-a-rules-entry). ### Flutter specific -- [always-remove-listener](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/always_remove_listener.md) +- [always-remove-listener](https://github.com/dart-code-checker/dart-code-metrics/blob/master/doc/rules/always-remove-listener.md) - [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) diff --git a/doc/rules/always_remove_listener.md b/doc/rules/always-remove-listener.md similarity index 100% rename from doc/rules/always_remove_listener.md rename to doc/rules/always-remove-listener.md diff --git a/doc/rules/avoid-returning-widgets.md b/doc/rules/avoid-returning-widgets.md index a8985c649e..bb4e8e884c 100644 --- a/doc/rules/avoid-returning-widgets.md +++ b/doc/rules/avoid-returning-widgets.md @@ -8,9 +8,12 @@ avoid-returning-widgets ## Description -Warns when a method or function returns a Widget or subclass of a Widget. The following patterns will not trigger the rule: +Warns when a method, function or getter returns a Widget or subclass of a Widget. + +The following patterns will not trigger the rule: - Widget `build` method overrides. +- Class method that is passed to a builder. - Functions with [functional_widget](https://pub.dev/packages/functional_widget) package annotations. Extracting widgets to a method is considered as a Flutter anti-pattern, because when Flutter rebuilds widget tree, it calls the function all the time, making more processor time for the operations. @@ -19,12 +22,16 @@ Consider creating a separate widget instead of a function or method. Additional resources: -* -* -* -* +- +- +- +- + +Use `ignored-names` configuration, if you want to ignore a function or method name. + +Use `ignored-annotations` configuration, if you want to override default ignored annotation list. -Use `ignored-names` configuration, if you want to ignore a function or method name. Use `ignored-annotations` configuration, if you want to override default ignored annotation list. For example: +For example: ### Config example @@ -51,6 +58,8 @@ class MyWidget extends StatelessWidget { const MyWidget(); // LINT + Widget _getWidget() => Container(); + Widget _buildShinyWidget() { return Container( child: Column( @@ -68,7 +77,7 @@ class MyWidget extends StatelessWidget { children: [ Text('Text!'), ... - _buildShinyWidget(), + _buildShinyWidget(), // LINT ], ); } @@ -109,3 +118,20 @@ class _MyShinyWidget extends StatelessWidget { } } ``` + +Good: + +```dart +class MyWidget extends StatelessWidget { + Widget _buildMyWidget(BuildContext context) { + return Container(); + } + + @override + Widget build(BuildContext context) { + return Builder( + builder: _buildMyWidget, + ); + } +} +``` diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets.dart index c4919a1db9..e506b17a4a 100644 --- a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets.dart +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets.dart @@ -14,11 +14,15 @@ import '../../rule_utils.dart'; part 'config_parser.dart'; part 'visitor.dart'; +part 'visit_declaration.dart'; class AvoidReturningWidgetsRule extends Rule { static const String ruleId = 'avoid-returning-widgets'; static const _warningMessage = 'Avoid returning widgets from a function.'; + static const _getterWarningMessage = 'Avoid returning widgets from a getter.'; + static const _globalFunctionWarningMessage = + 'Avoid returning widgets from a global function.'; final Iterable _ignoredNames; final Iterable _ignoredAnnotations; @@ -43,16 +47,34 @@ class AvoidReturningWidgetsRule extends Rule { source.unit.visitChildren(_visitor); - return _visitor.declarations - .map((declaration) => createIssue( - rule: this, - location: nodeLocation( - node: declaration, - source: source, - withCommentOrMetadata: true, - ), - message: _warningMessage, - )) - .toList(growable: false); + return [ + ..._visitor.invocations.map((invocation) => createIssue( + rule: this, + location: nodeLocation( + node: invocation, + source: source, + withCommentOrMetadata: true, + ), + message: _warningMessage, + )), + ..._visitor.getters.map((getter) => createIssue( + rule: this, + location: nodeLocation( + node: getter, + source: source, + withCommentOrMetadata: true, + ), + message: _getterWarningMessage, + )), + ..._visitor.globalFunctions.map((globalFunction) => createIssue( + rule: this, + location: nodeLocation( + node: globalFunction, + source: source, + withCommentOrMetadata: true, + ), + message: _globalFunctionWarningMessage, + )), + ]; } } diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visit_declaration.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visit_declaration.dart new file mode 100644 index 0000000000..55d633b1d0 --- /dev/null +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visit_declaration.dart @@ -0,0 +1,71 @@ +part of 'avoid_returning_widgets.dart'; + +Declaration? _visitDeclaration( + Declaration node, + SimpleIdentifier name, + TypeAnnotation? returnType, + Iterable ignoredNames, + Iterable ignoredAnnotations, { + required bool isSetter, +}) { + final hasIgnoredAnnotation = node.metadata.any( + (node) => + ignoredAnnotations.contains(node.name.name) && + node.atSign.type == TokenType.AT, + ); + + if (!hasIgnoredAnnotation && + !isSetter && + !_isIgnored(name.name, ignoredNames)) { + final type = returnType?.type; + if (type != null && _hasWidgetType(type)) { + return node; + } + } + + return null; +} + +bool _hasWidgetType(DartType type) => + _isWidgetOrSubclass(type) || + _isIterable(type) || + _isList(type) || + _isFuture(type); + +bool _isIterable(DartType type) => + type.isDartCoreIterable && + type is InterfaceType && + _isWidgetOrSubclass(type.typeArguments.firstOrNull); + +bool _isList(DartType type) => + type.isDartCoreList && + type is InterfaceType && + _isWidgetOrSubclass(type.typeArguments.firstOrNull); + +bool _isFuture(DartType type) => + type.isDartAsyncFuture && + type is InterfaceType && + _isWidgetOrSubclass(type.typeArguments.firstOrNull); + +bool _isIgnored( + String name, + Iterable ignoredNames, +) => + name == 'build' || ignoredNames.contains(name); + +bool _isWidgetOrSubclass(DartType? type) => + _isWidget(type) || _isSubclassOfWidget(type); + +bool _isWidget(DartType? type) => + type?.getDisplayString(withNullability: false) == 'Widget'; + +bool _isSubclassOfWidget(DartType? type) => + type is InterfaceType && type.allSupertypes.any(_isWidget); + +bool _isStateOrSubclass(DartType? type) => + _isWidgetState(type) || _isSubclassOfWidgetState(type); + +bool _isWidgetState(DartType? type) => type?.element?.displayName == 'State'; + +bool _isSubclassOfWidgetState(DartType? type) => + type is InterfaceType && type.allSupertypes.any(_isWidgetState); diff --git a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visitor.dart b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visitor.dart index 70156ea29a..02bb54f1ae 100644 --- a/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visitor.dart +++ b/lib/src/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/visitor.dart @@ -1,90 +1,132 @@ part of 'avoid_returning_widgets.dart'; class _Visitor extends RecursiveAstVisitor { + final _invocations = []; + final _getters = []; + final _globalFunctions = []; + + final Iterable _ignoredNames; + final Iterable _ignoredAnnotations; + + Iterable get invocations => _invocations; + Iterable get getters => _getters; + Iterable get globalFunctions => _globalFunctions; + + _Visitor(this._ignoredNames, this._ignoredAnnotations); + + @override + void visitFunctionDeclaration(FunctionDeclaration node) { + if (node.parent is! CompilationUnit) { + return; + } + + final declaration = _visitDeclaration( + node, + node.name, + node.returnType, + _ignoredNames, + _ignoredAnnotations, + isSetter: node.isSetter, + ); + + if (declaration != null) { + _globalFunctions.add(declaration); + } + } + + @override + void visitClassDeclaration(ClassDeclaration node) { + final classType = node.extendsClause?.superclass.type; + if (!_isWidgetOrSubclass(classType) && !_isStateOrSubclass(classType)) { + return; + } + + final declarationsVisitor = _DeclarationsVisitor( + _ignoredNames, + _ignoredAnnotations, + ); + node.visitChildren(declarationsVisitor); + + final names = declarationsVisitor.declarations + .map((declaration) => declaration.declaredElement?.name) + .whereType() + .toSet(); + + final invocationsVisitor = _InvocationsVisitor(names); + node.visitChildren(invocationsVisitor); + + _invocations.addAll(invocationsVisitor.invocations); + _getters.addAll(declarationsVisitor.getters); + } +} + +class _InvocationsVisitor extends RecursiveAstVisitor { + final _invocations = []; + + final Set _declarationNames; + + Iterable get invocations => _invocations; + + _InvocationsVisitor(this._declarationNames); + + @override + void visitMethodInvocation(MethodInvocation node) { + if (_declarationNames.contains(node.methodName.name) && + node.realTarget == null) { + _invocations.add(node); + } + } +} + +class _DeclarationsVisitor extends RecursiveAstVisitor { final _declarations = []; + final _getters = []; final Iterable _ignoredNames; final Iterable _ignoredAnnotations; Iterable get declarations => _declarations; + Iterable get getters => _getters; - _Visitor(this._ignoredNames, this._ignoredAnnotations); + _DeclarationsVisitor(this._ignoredNames, this._ignoredAnnotations); @override void visitMethodDeclaration(MethodDeclaration node) { super.visitMethodDeclaration(node); - _visitDeclaration( + final declaration = _visitDeclaration( node, node.name, node.returnType, + _ignoredNames, + _ignoredAnnotations, isSetter: node.isSetter, ); + + if (declaration != null) { + if (node.isGetter) { + _getters.add(declaration); + } else { + _declarations.add(declaration); + } + } } @override void visitFunctionDeclaration(FunctionDeclaration node) { super.visitFunctionDeclaration(node); - _visitDeclaration( + final declaration = _visitDeclaration( node, node.name, node.returnType, + _ignoredNames, + _ignoredAnnotations, isSetter: node.isSetter, ); - } - - void _visitDeclaration( - Declaration node, - SimpleIdentifier name, - TypeAnnotation? returnType, { - required bool isSetter, - }) { - final hasIgnoredAnnotation = node.metadata.any( - (node) => - _ignoredAnnotations.contains(node.name.name) && - node.atSign.type == TokenType.AT, - ); - if (!hasIgnoredAnnotation && !isSetter && !_isIgnored(name.name)) { - final type = returnType?.type; - if (type != null && _hasWidgetType(type)) { - _declarations.add(node); - } + if (declaration != null) { + _declarations.add(declaration); } } - - bool _hasWidgetType(DartType type) => - _isWidget(type) || - _isSubclassOfWidget(type) || - _isIterable(type) || - _isList(type) || - _isFuture(type); - - bool _isIterable(DartType type) => - type.isDartCoreIterable && - type is InterfaceType && - (_isWidget(type.typeArguments.firstOrNull) || - _isSubclassOfWidget(type.typeArguments.firstOrNull)); - - bool _isList(DartType type) => - type.isDartCoreList && - type is InterfaceType && - (_isWidget(type.typeArguments.firstOrNull) || - _isSubclassOfWidget(type.typeArguments.firstOrNull)); - - bool _isFuture(DartType type) => - type.isDartAsyncFuture && - type is InterfaceType && - (_isWidget(type.typeArguments.firstOrNull) || - _isSubclassOfWidget(type.typeArguments.firstOrNull)); - - bool _isWidget(DartType? type) => - type?.getDisplayString(withNullability: false) == 'Widget'; - - bool _isSubclassOfWidget(DartType? type) => - type is InterfaceType && type.allSupertypes.any(_isWidget); - - bool _isIgnored(String name) => - name == 'build' || _ignoredNames.contains(name); } diff --git a/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets_test.dart b/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets_test.dart index 9b710c92e8..9d4f43a2e1 100644 --- a/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets_test.dart +++ b/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/avoid_returning_widgets_test.dart @@ -26,22 +26,21 @@ void main() { RuleTestHelper.verifyIssues( issues: issues, - startOffsets: [88, 175, 289, 358, 485, 614, 749, 1012], - startLines: [6, 11, 20, 25, 30, 35, 41, 55], - startColumns: [3, 3, 3, 3, 3, 3, 1, 1], - endOffsets: [127, 231, 344, 414, 542, 677, 784, 1087], + startOffsets: [755, 791, 827, 859, 892, 961, 292, 1016, 1271], + startLines: [36, 38, 40, 42, 44, 47, 16, 53, 67], + startColumns: [5, 5, 5, 5, 5, 14, 3, 1, 1], + endOffsets: [776, 812, 844, 877, 915, 984, 331, 1071, 1346], locationTexts: [ + '_localBuildMyWidget()', + '_getWidgetsIterable()', + '_getWidgetsList()', + '_getWidgetFuture()', + '_buildMyWidget(context)', + '_buildMyWidget(context)', 'Widget get widgetGetter => Container();', - 'Widget _getMyShinyWidget() {\n' - ' return Container();\n' - ' }', - 'Container _getContainer() {\n' - ' return Container();\n' - ' }', - 'Iterable _getWidgetsIterable() => [Container()];', - 'List _getWidgetsList() => [Container()].toList();', - 'Future _getWidgetFuture() => Future.value(Container());', - 'Widget _getWidget() => Container();', + 'Widget _globalBuildMyWidget() {\n' + ' return Container();\n' + '}', '@ignoredAnnotation\n' 'Widget _getWidgetWithIgnoredAnnotation() => Container();', ], @@ -52,8 +51,9 @@ void main() { 'Avoid returning widgets from a function.', 'Avoid returning widgets from a function.', 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', + 'Avoid returning widgets from a getter.', + 'Avoid returning widgets from a global function.', + 'Avoid returning widgets from a global function.', ], ); }); @@ -63,7 +63,7 @@ void main() { final config = { 'ignored-names': [ '_getWidgetFuture', - '_getWidget', + '_buildMyWidget', ], 'ignored-annotations': [ 'ignoredAnnotation', @@ -74,20 +74,18 @@ void main() { RuleTestHelper.verifyIssues( issues: issues, - startOffsets: [88, 175, 289, 358, 485, 814, 879, 944], - startLines: [6, 11, 20, 25, 30, 45, 48, 51], - startColumns: [3, 3, 3, 3, 3, 1, 1, 1], - endOffsets: [127, 231, 344, 414, 542, 877, 942, 1002], + startOffsets: [755, 791, 827, 292, 1016, 1073, 1138, 1203], + startLines: [36, 38, 40, 16, 53, 57, 60, 63], + startColumns: [5, 5, 5, 3, 1, 1, 1, 1], + endOffsets: [776, 812, 844, 331, 1071, 1136, 1201, 1261], locationTexts: [ + '_localBuildMyWidget()', + '_getWidgetsIterable()', + '_getWidgetsList()', 'Widget get widgetGetter => Container();', - 'Widget _getMyShinyWidget() {\n' - ' return Container();\n' - ' }', - 'Container _getContainer() {\n' - ' return Container();\n' - ' }', - 'Iterable _getWidgetsIterable() => [Container()];', - 'List _getWidgetsList() => [Container()].toList();', + 'Widget _globalBuildMyWidget() {\n' + ' return Container();\n' + '}', '@FunctionalWidget\n' 'Widget _getFunctionalWidget() => Container();', '@swidget\n' @@ -99,11 +97,11 @@ void main() { 'Avoid returning widgets from a function.', 'Avoid returning widgets from a function.', 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', - 'Avoid returning widgets from a function.', + 'Avoid returning widgets from a getter.', + 'Avoid returning widgets from a global function.', + 'Avoid returning widgets from a global function.', + 'Avoid returning widgets from a global function.', + 'Avoid returning widgets from a global function.', ], ); }); diff --git a/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/examples/example.dart b/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/examples/example.dart index 1b392670be..42ee9d3718 100644 --- a/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/examples/example.dart +++ b/test/analyzers/lint_analyzer/rules/rules_list/avoid_returning_widgets/examples/example.dart @@ -1,46 +1,58 @@ -class SomeWidget extends StatelessWidget { +class MyWidget extends StatelessWidget { + Widget _buildMyWidget(BuildContext context) { + return Container(); + } + @override - Widget build() {} + Widget build(BuildContext context) { + return Builder( + builder: _buildMyWidget, + ); + } +} +class AnotherWidget extends StatelessWidget { // LINT Widget get widgetGetter => Container(); String get stringGetter => ''; - // LINT - Widget _getMyShinyWidget() { - return Container(); - } + Iterable _getWidgetsIterable() => [Container()]; - String _getString() { - return ''; - } + List _getWidgetsList() => [Container()].toList(); - // LINT - Container _getContainer() { + Future _getWidgetFuture() => Future.value(Container()); + + Widget _buildMyWidget(BuildContext context) { return Container(); } - // LINT - Iterable _getWidgetsIterable() => [Container()]; + @override + Widget build(BuildContext context) { + Widget _localBuildMyWidget() { + return Container(); + } - Iterable _getWidgetsIterable() => ['string']; + _localBuildMyWidget(); // LINT - // LINT - List _getWidgetsList() => [Container()].toList(); + _getWidgetsIterable(); // LINT - List _getStringsList() => ['string'].toList(); + _getWidgetsList(); // LINT - // LINT - Future _getWidgetFuture() => Future.value(Container()); + _getWidgetFuture(); // LINT + + _buildMyWidget(context); // LINT - Future _otherStringFuture() => Future.value(''); + return Container( + child: _buildMyWidget(context), // LINT + ); + } } // LINT -Widget _getWidget() => Container(); - -String _getString() => ''; +Widget _globalBuildMyWidget() { + return Container(); +} @FunctionalWidget Widget _getFunctionalWidget() => Container(); @@ -55,10 +67,6 @@ Widget _getHookFunctionalWidget() => Container(); @ignoredAnnotation Widget _getWidgetWithIgnoredAnnotation() => Container(); -class Widget {} - -class Container extends Widget {} - class FunctionalWidget { const FunctionalWidget(); } @@ -69,3 +77,19 @@ const hwidget = FunctionalWidget(); class IgnoredAnnotation { const IgnoredAnnotation(); } + +class Widget {} + +class Container extends Widget { + final Widget? child; + + const Container({this.child}); +} + +class Builder extends Widget { + final Widget Function(BuildContext) builder; + + const Builder(this.builder); +} + +class StatelessWidget extends Widget {}