diff --git a/lib/main.dart b/lib/main.dart index b0a50bfc..40977ac1 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -4,7 +4,8 @@ import 'package:solid_lints/src/common/parameter_parser/analysis_options_loader. import 'package:solid_lints/src/lints/avoid_debug_print_in_release/avoid_debug_print_in_release_rule.dart'; import 'package:solid_lints/src/lints/avoid_global_state/avoid_global_state_rule.dart'; import 'package:solid_lints/src/lints/avoid_non_null_assertion/avoid_non_null_assertion_rule.dart'; -import 'package:solid_lints/src/lints/proper_super_calls/proper_super_calls_rule.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart'; /// The entry point for the Solid Lints analyser server plugin. /// @@ -23,17 +24,18 @@ class SolidLintsPlugin extends Plugin { @override void register(PluginRegistry registry) { final analysisLoader = AnalysisOptionsLoader(); - registry.registerLintRule( + final lintRules = [ AvoidGlobalStateRule(analysisLoader), - ); - registry.registerLintRule( AvoidNonNullAssertionRule(), - ); - registry.registerLintRule( AvoidDebugPrintInReleaseRule(), - ); - registry.registerLintRule( - ProperSuperCallsRule(), - ); + AvoidReturningWidgetsRule( + analysisOptionsLoader: analysisLoader, + parametersParser: AvoidReturningWidgetsParameters.fromJson, + ), + ]; + + for (final lintRule in lintRules) { + registry.registerLintRule(lintRule); + } } } diff --git a/lib/src/common/parameters/excluded_identifiers_list_parameter.dart b/lib/src/common/parameters/excluded_identifiers_list_parameter.dart index 8a4b1097..526201a4 100644 --- a/lib/src/common/parameters/excluded_identifiers_list_parameter.dart +++ b/lib/src/common/parameters/excluded_identifiers_list_parameter.dart @@ -82,7 +82,7 @@ class ExcludedIdentifiersListParameter { final classDeclaration = node.thisOrAncestorOfType(); return classDeclaration != null && - classDeclaration.name.toString() == className; + classDeclaration.namePart.typeName.lexeme == className; } } } diff --git a/lib/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart b/lib/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart index 6bd15911..c90f2d97 100644 --- a/lib/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart +++ b/lib/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart @@ -1,11 +1,9 @@ -import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/dart/element/type.dart'; -import 'package:analyzer/error/listener.dart'; -import 'package:custom_lint_builder/custom_lint_builder.dart'; +import 'package:analyzer/analysis_rule/rule_context.dart'; +import 'package:analyzer/analysis_rule/rule_visitor_registry.dart'; +import 'package:analyzer/error/error.dart'; import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart'; -import 'package:solid_lints/src/models/rule_config.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/visitors/avoid_returning_widgets_visitor.dart'; import 'package:solid_lints/src/models/solid_lint_rule.dart'; -import 'package:solid_lints/src/utils/types_utils.dart'; /// A rule which warns about returning widgets from functions and methods. /// @@ -54,64 +52,38 @@ class AvoidReturningWidgetsRule /// This lint rule represents /// the error whether we return a widget. static const lintName = 'avoid_returning_widgets'; - static const _override = 'override'; - AvoidReturningWidgetsRule._(super.config); + static const _code = LintCode( + lintName, + 'Returning a widget from a function is considered an anti-pattern. ' + 'Unless you are overriding an existing method, ' + 'consider extracting your widget to a separate class.', + ); - /// Creates a new instance of [AvoidReturningWidgetsRule] - /// based on the lint configuration. - factory AvoidReturningWidgetsRule.createRule(CustomLintConfigs configs) { - final rule = RuleConfig( - configs: configs, - name: lintName, - paramsParser: AvoidReturningWidgetsParameters.fromJson, - problemMessage: (_) => - 'Returning a widget from a function is considered an anti-pattern. ' - 'Unless you are overriding an existing method, ' - 'consider extracting your widget to a separate class.', - ); + @override + DiagnosticCode get diagnosticCode => _code; - return AvoidReturningWidgetsRule._(rule); - } + /// Creates a new instance of [AvoidReturningWidgetsRule] + AvoidReturningWidgetsRule({ + required super.analysisOptionsLoader, + required super.parametersParser, + }) : super.withParameters( + name: _code.lowerCaseName, + description: _code.problemMessage, + ); @override - void run( - CustomLintResolver resolver, - DiagnosticReporter reporter, - CustomLintContext context, + void registerNodeProcessors( + RuleVisitorRegistry registry, + RuleContext context, ) { - context.registry.addDeclaration((node) { - // Check if declaration is function or method, - // simultaneously checks if return type is [DartType] - final DartType? returnType = switch (node) { - FunctionDeclaration(returnType: TypeAnnotation(:final type?)) || - MethodDeclaration(returnType: TypeAnnotation(:final type?)) => - type, - _ => null, - }; - - if (returnType == null) { - return; - } - - final isWidgetReturned = hasWidgetType(returnType); + super.registerNodeProcessors(registry, context); - final isIgnored = config.parameters.exclude.shouldIgnore(node); + final parameters = getParametersForContext(context) ?? + AvoidReturningWidgetsParameters.empty(); - final isOverriden = switch (node) { - FunctionDeclaration(:final functionExpression) => - functionExpression.parent is MethodDeclaration && - (functionExpression.parent! as MethodDeclaration) - .metadata - .any((m) => m.name.name == _override), - MethodDeclaration(:final metadata) => - metadata.any((m) => m.name.name == _override), - _ => false, - }; + final visitor = AvoidReturningWidgetsVisitor(this, parameters); - if (isWidgetReturned && !isOverriden && !isIgnored) { - reporter.atNode(node, code); - } - }); + registry.addCompilationUnit(this, visitor); } } diff --git a/lib/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart b/lib/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart index 7947ea64..14c27036 100644 --- a/lib/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart +++ b/lib/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart @@ -11,6 +11,13 @@ class AvoidReturningWidgetsParameters { required this.exclude, }); + /// Empty [AvoidReturningWidgetsParameters] model, excludes nothing. + factory AvoidReturningWidgetsParameters.empty() { + return AvoidReturningWidgetsParameters( + exclude: ExcludedIdentifiersListParameter(exclude: []), + ); + } + /// Method for creating from json data factory AvoidReturningWidgetsParameters.fromJson(Map json) { return AvoidReturningWidgetsParameters( diff --git a/lib/src/lints/avoid_returning_widgets/visitors/avoid_returning_widgets_visitor.dart b/lib/src/lints/avoid_returning_widgets/visitors/avoid_returning_widgets_visitor.dart new file mode 100644 index 00000000..b81f4c39 --- /dev/null +++ b/lib/src/lints/avoid_returning_widgets/visitors/avoid_returning_widgets_visitor.dart @@ -0,0 +1,84 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart'; +import 'package:solid_lints/src/utils/types_utils.dart'; + +/// A visitor that reports on functions that return widgets. +class AvoidReturningWidgetsVisitor extends RecursiveAstVisitor { + final AvoidReturningWidgetsRule _rule; + final AvoidReturningWidgetsParameters _parameters; + + /// Creates a new instance of [AvoidReturningWidgetsVisitor] + AvoidReturningWidgetsVisitor(this._rule, this._parameters); + + @override + void visitFunctionDeclaration(FunctionDeclaration node) { + super.visitFunctionDeclaration(node); + + _visitDeclaration(node); + } + + @override + void visitMethodDeclaration(MethodDeclaration node) { + super.visitMethodDeclaration(node); + + _visitDeclaration(node); + } + + @override + void visitFunctionDeclarationStatement(FunctionDeclarationStatement node) { + super.visitFunctionDeclarationStatement(node); + + _visitDeclaration(node.functionDeclaration); + } + + void _visitDeclaration(Declaration node) { + if (node is! FunctionDeclaration && node is! MethodDeclaration) { + return; + } + + final returnType = switch (node) { + Declaration( + declaredFragment: ExecutableFragment( + element: ExecutableElement(type: FunctionType(:final returnType)) + ) + ) => + returnType, + MethodDeclaration(returnType: TypeAnnotation(:final type)) => type, + FunctionDeclaration(returnType: TypeAnnotation(:final type)) => type, + _ => null, + }; + if (returnType == null) return; + + final isWidgetReturned = hasWidgetType(returnType); + if (!isWidgetReturned) return; + + final isIgnored = _parameters.exclude.shouldIgnore(node); + if (isIgnored) return; + + if (_isOverridden(node)) return; + + _rule.reportAtNode(node); + } + + bool _isOverridden(Declaration node) { + return switch (node) { + Declaration( + declaredFragment: Fragment( + element: Element( + name: final String name, + enclosingElement: final InterfaceElement enclosingElement + ) + ), + ) => + enclosingElement.getInheritedMember( + Name.forLibrary(enclosingElement.library, name), + ) != + null, + _ => false, + }; + } +} diff --git a/lint_test/avoid_returning_widget_test/analysis_options.yaml b/lint_test/avoid_returning_widget_test/analysis_options.yaml deleted file mode 100644 index 0953ea1e..00000000 --- a/lint_test/avoid_returning_widget_test/analysis_options.yaml +++ /dev/null @@ -1,11 +0,0 @@ -analyzer: - plugins: - - ../custom_lint - -custom_lint: - rules: - - avoid_returning_widgets: - exclude: - - class_name: ExcludeWidget - method_name: excludeWidgetMethod - - method_name: excludeMethod diff --git a/lint_test/avoid_returning_widget_test/avoid_returning_widget_test.dart b/lint_test/avoid_returning_widget_test/avoid_returning_widget_test.dart deleted file mode 100644 index ad4398ba..00000000 --- a/lint_test/avoid_returning_widget_test/avoid_returning_widget_test.dart +++ /dev/null @@ -1,96 +0,0 @@ -// ignore_for_file: unused_element, prefer_match_file_name -// ignore_for_file: member_ordering - -/// Check returning a widget fail -/// `avoid_returning_widgets` - -import 'package:flutter/material.dart'; - -// expect_lint: avoid_returning_widgets -Widget avoidReturningWidgets() => const SizedBox(); - -class BaseWidget extends StatelessWidget { - const BaseWidget({super.key}); - - // Not allowed even though overriding it is alllowed - // expect_lint: avoid_returning_widgets - Widget get box => SizedBox(); - - // expect_lint: avoid_returning_widgets - Widget decoratedBox() { - return DecoratedBox(decoration: BoxDecoration()); - } - - @override - Widget build(BuildContext context) { - return const Placeholder(); - } -} - -class MyWidget extends BaseWidget { - const MyWidget({super.key}); - - // expect_lint: avoid_returning_widgets - Widget _test1() => const SizedBox(); - - // expect_lint: avoid_returning_widgets - Widget _test2() { - return const SizedBox( - child: SizedBox(), - ); - } - - // expect_lint: avoid_returning_widgets - Widget get _test3 => const SizedBox(); - - // Allowed - @override - Widget decoratedBox() { - return super.decoratedBox(); - } - - // Allowed - @override - Widget get box => ColoredBox(color: Colors.pink); - - // Allowed - @override - Widget build(BuildContext context) { - return const SizedBox(); - } -} - -// expect_lint: avoid_returning_widgets -Widget build() { - return Offstage(); -} - -// no lint -SizedBox excludeMethod() => const SizedBox(); - -class ExcludeWidget extends StatelessWidget { - const ExcludeWidget({super.key}); - - @override - Widget build(BuildContext context) { - return const Placeholder(); - } - - // no lint - Widget excludeWidgetMethod() => const SizedBox(); - - // expect_lint: avoid_returning_widgets - Widget excludeWidgetMethod2() => const SizedBox(); -} - -class NotExcludeWidget extends StatelessWidget { - const NotExcludeWidget({super.key}); - - @override - Widget build(BuildContext context) { - return const Placeholder(); - } - - // expect_lint: avoid_returning_widgets - Widget excludeWidgetMethod() => const SizedBox(); -} diff --git a/pubspec.yaml b/pubspec.yaml index f85b1309..a97a55de 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -18,12 +18,9 @@ dependencies: path: ^1.9.1 yaml: ^3.1.3 # These packages are required for pana analysis to run correctly - flutter: - sdk: flutter test: ^1.25.14 dev_dependencies: args: ^2.6.0 analyzer_testing: ^0.1.9 test_reflective_loader: ^0.3.0 - diff --git a/test/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule_test.dart b/test/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule_test.dart new file mode 100644 index 00000000..f780c3df --- /dev/null +++ b/test/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule_test.dart @@ -0,0 +1,296 @@ +import 'package:analyzer_testing/analysis_rule/analysis_rule.dart'; +import 'package:analyzer_testing/utilities/utilities.dart'; +import 'package:solid_lints/src/common/parameter_parser/analysis_options_loader.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/avoid_returning_widgets_rule.dart'; +import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart'; +import 'package:test_reflective_loader/test_reflective_loader.dart'; + +void main() { + defineReflectiveSuite(() { + defineReflectiveTests(AvoidReturningWidgetsRuleTest); + }); +} + +@reflectiveTest +class AvoidReturningWidgetsRuleTest extends AnalysisRuleTest { + static const _importFlutterWidgets = "import 'package:flutter/widgets.dart';"; + static const _mockFlutterWidgetsContent = ''' +abstract class Widget { + final String key; + + const Widget({required this.key}); +} + +class StatelessWidget implements Widget { + const StatelessWidget({super.key}); + + @override + Widget build(BuildContext context); +} + +class StatefulWidget implements Widget { + const StatefulWidget({super.key}); + + @override + Widget build(BuildContext context); +} + +abstract interface class BuildContext {} + +class Placeholder extends StatelessWidget { + const Placeholder({super.key}); + + @override + Widget build(BuildContext context) => throw 'unimplemented'; +} + +class SizedBox extends Widget { + final Widget? child; + + const SizedBox({this.child}); + + @override + Widget build(BuildContext context) => child ?? const SizedBox(); +} + +class BoxDecoration extends Widget { + const BoxDecoration(); + + @override + Widget build(BuildContext context) => throw 'unimplemented'; +} + +class DecoratedBox extends Widget { + const DecoratedBox({required this.decoration}); + + final BoxDecoration decoration; + + @override + Widget build(BuildContext context) => throw 'unimplemented'; +} +'''; + static const _mockAnalysisOptionsContent = ''' +plugins: + solid_lints: + diagnostics: + avoid_returning_widgets: + exclude: + - class_name: ExcludeWidget + method_name: excludeWidgetMethod + - method_name: excludeMethod + '''; + + void _addBaseWidgetFile() { + newFile('$testPackageLibPath/base_widget.dart', ''' +$_importFlutterWidgets +class BaseWidget extends StatelessWidget { + const BaseWidget({super.key}); + + Widget get box => SizedBox(); + + Widget decoratedBox() => DecoratedBox(decoration: BoxDecoration()); + + set box(Widget value) { + throw 'unimplemented'; + } +} +'''); + } + + @override + void setUp() { + rule = AvoidReturningWidgetsRule( + analysisOptionsLoader: AnalysisOptionsLoader( + resourceProvider: resourceProvider, + ), + parametersParser: AvoidReturningWidgetsParameters.fromJson, + ); + newPackage('flutter') + ..addFile('lib/widgets.dart', _mockFlutterWidgetsContent); + super.setUp(); + + newAnalysisOptionsYamlFile( + testPackageRootPath, + '''${analysisOptionsContent(rules: [rule.name])} +$_mockAnalysisOptionsContent''', + ); + } + + Future test_reports_on_static_function() async { + await assertDiagnostics( + ''' +$_importFlutterWidgets + +Widget avoidReturningWidgets() => const SizedBox(); + +Widget build() { + return SizedBox(); +} +''', + [lint(40, 51), lint(93, 39)], + ); + } + + Future test_reports_on_methods() async { + await assertDiagnostics( + ''' +$_importFlutterWidgets + +class BaseWidget extends StatelessWidget { + const BaseWidget({super.key}); + + Widget decoratedBox() { + return DecoratedBox(decoration: BoxDecoration()); + } +} +''', + [lint(119, 81)], + ); + } + + Future test_reports_on_getters_but_not_setters() async { + await assertDiagnostics( + ''' +$_importFlutterWidgets + +class BaseWidget extends StatelessWidget { + const BaseWidget({super.key}); + + Widget get box => SizedBox(); + + set box(Widget value) { + throw 'unimplemented'; + } +} +''', + [lint(119, 29)], + ); + } + + Future test_reports_on_private_members() async { + _addBaseWidgetFile(); + + await assertDiagnostics( + ''' +$_importFlutterWidgets +import 'base_widget.dart'; + +class MyWidget extends BaseWidget { + const MyWidget({super.key}); + + Widget _test1() => const SizedBox(); + + Widget _test2() { + return const SizedBox( + child: SizedBox(), + ); + } + + Widget get _test3 => const SizedBox(); + + @override + Widget decoratedBox() { + return super.decoratedBox(); + } + + @override + Widget get box => SizedBox(); + + @override + Widget build(BuildContext context) { + return const SizedBox(); + } +} +''', + [ + lint(137, 36), + lint(177, 80), + lint(261, 38), + ], + ); + } + + Future test_does_not_report_on_overridden_members() async { + _addBaseWidgetFile(); + + // Shouldn't report even if not annotated with @override + await assertNoDiagnostics( + ''' +$_importFlutterWidgets +import 'base_widget.dart'; + +class MyWidget extends BaseWidget { + const MyWidget({super.key}); + + Widget decoratedBox() { + return super.decoratedBox(); + } + + Widget get box => SizedBox(); + + Widget build(BuildContext context) { + return const SizedBox(); + } +} +''', + ); + } + + Future test_does_not_report_on_excluded() async { + await assertNoDiagnostics( + ''' +$_importFlutterWidgets + +SizedBox excludeMethod() => const SizedBox(); + +class ExcludeWidget extends StatelessWidget { + const ExcludeWidget({super.key}); + + @override + Widget build(BuildContext context) { + return const Placeholder(); + } + + Widget excludeWidgetMethod() => const SizedBox(); +} + +''', + ); + } + + Future test_reports_on_non_matching_excluded() async { + await assertDiagnostics( + ''' +$_importFlutterWidgets + +SizedBox excludeMethod() => const SizedBox(); + +class ExcludeWidget extends StatelessWidget { + const ExcludeWidget({super.key}); + + @override + Widget build(BuildContext context) { + return const Placeholder(); + } + + Widget notExcludeWidgetMethod() => const Placeholder(); +} + +class NotExcludeWidget extends StatelessWidget { + const NotExcludeWidget({super.key}); + + @override + Widget build(BuildContext context) { + return const Placeholder(); + } + + Widget excludeWidgetMethod() => const SizedBox(); +} +''', + [ + lint(260, 55), + lint(498, 49), + ], + ); + } +}