From a8bc82c5c3c54737ebaed04b1a31fa93fa839f5f Mon Sep 17 00:00:00 2001 From: David Schneider Date: Tue, 27 Aug 2024 12:36:42 +0200 Subject: [PATCH] Distinguish between signal reads and creations for build-method access lint --- .../example/analysis_options.yaml | 4 +- packages/signals_lint/example/lib/main.dart | 17 +++ .../signals_lint/example/lib/newfile.dart | 9 ++ packages/signals_lint/lib/signals_lint.dart | 1 + .../lints/avoid_create_in_build_method.dart | 132 +++++------------- .../lib/src/lints/definitions.dart | 57 ++++++++ .../lib/src/lints/extensions.dart | 76 ++++++++++ 7 files changed, 200 insertions(+), 96 deletions(-) create mode 100644 packages/signals_lint/example/lib/newfile.dart create mode 100644 packages/signals_lint/lib/src/lints/definitions.dart create mode 100644 packages/signals_lint/lib/src/lints/extensions.dart diff --git a/packages/signals_lint/example/analysis_options.yaml b/packages/signals_lint/example/analysis_options.yaml index c421a8bc..3b0cd54b 100644 --- a/packages/signals_lint/example/analysis_options.yaml +++ b/packages/signals_lint/example/analysis_options.yaml @@ -22,8 +22,8 @@ linter: # `// ignore_for_file: name_of_lint` syntax on the line or in the file # producing the lint. rules: - # avoid_print: false # Uncomment to disable the `avoid_print` rule - # prefer_single_quotes: true # Uncomment to enable the `prefer_single_quotes` rule + # avoid_print: false # Uncomment to disable the `avoid_print` rule + # prefer_single_quotes: true # Uncomment to enable the `prefer_single_quotes` rule # Additional information about this file can be found at # https://dart.dev/guides/language/analysis-options diff --git a/packages/signals_lint/example/lib/main.dart b/packages/signals_lint/example/lib/main.dart index fb7bd079..41a805ac 100644 --- a/packages/signals_lint/example/lib/main.dart +++ b/packages/signals_lint/example/lib/main.dart @@ -1,3 +1,4 @@ +import 'package:example/newfile.dart'; import 'package:flutter/material.dart'; import 'package:signals/signals_flutter.dart'; @@ -25,12 +26,28 @@ class MyHomePage extends StatefulWidget { class _MyHomePageState extends State { late final counter = createSignal(context, 1); + Signal field() => counter; + Signal get sameFileGetter => Signal(1); + var abc = Abc().externalGetter; + @override Widget build(BuildContext context) { + var counter3 = Counter(1); + final counterX = () => sameFileGetter; + final counter2 = sameFileGetter; + final third = counter3.y; + final other = Counter(2).y; + final nun = counter3.externalGetter; + final n = Signal(1); return Text('Count: $counter'); } } class Counter extends ValueNotifier { Counter(super.value); + + final x = Signal(4); + final y = Counter(1).x; + + Signal get externalGetter => Signal(1); } diff --git a/packages/signals_lint/example/lib/newfile.dart b/packages/signals_lint/example/lib/newfile.dart new file mode 100644 index 00000000..be4b340b --- /dev/null +++ b/packages/signals_lint/example/lib/newfile.dart @@ -0,0 +1,9 @@ +import 'package:example/main.dart'; +import 'package:signals/signals.dart'; + +class Abc { + final x = Signal(4); + final y = Counter(1).x; + + Signal get externalGetter => Signal(1); +} diff --git a/packages/signals_lint/lib/signals_lint.dart b/packages/signals_lint/lib/signals_lint.dart index 73d2af81..432d6591 100644 --- a/packages/signals_lint/lib/signals_lint.dart +++ b/packages/signals_lint/lib/signals_lint.dart @@ -2,6 +2,7 @@ library signals_lint; import 'package:custom_lint_builder/custom_lint_builder.dart'; + import 'src/fixes/wrap_with_watch.dart'; import 'src/lints/avoid_create_in_build_method.dart'; diff --git a/packages/signals_lint/lib/src/lints/avoid_create_in_build_method.dart b/packages/signals_lint/lib/src/lints/avoid_create_in_build_method.dart index 7db5613c..793ae00d 100644 --- a/packages/signals_lint/lib/src/lints/avoid_create_in_build_method.dart +++ b/packages/signals_lint/lib/src/lints/avoid_create_in_build_method.dart @@ -1,46 +1,16 @@ import 'package:analyzer/dart/ast/ast.dart'; -import 'package:analyzer/error/error.dart'; import 'package:analyzer/error/listener.dart'; import 'package:custom_lint_builder/custom_lint_builder.dart'; +import 'package:signals_lint/src/lints/definitions.dart'; +import 'package:signals_lint/src/lints/extensions.dart'; const TypeChecker buildContextType = TypeChecker.fromName( 'BuildContext', packageName: 'flutter', ); -const buildMethod = 'build'; - -const _errorMessage = ''' -Signals should not be created in the build method because will -create a new signal every time the element is rebuilt. -'''; - -const _correctionMessage = ''' -Create the new signals outside the build() method in the class or globally. - -For StatelessWidgets you can define the signals as a static variable, pass in -from the constructor or declare globally. - -```diff -+ final counter = signal(0); -... -@override -Widget build(BuildContext context) { -- final counter = signal(0); - return ...; -} -``` -'''; - class SignalsAvoidCreateInBuildMethod extends DartLintRule { - const SignalsAvoidCreateInBuildMethod() : super(code: _code); - - static const _code = LintCode( - name: 'signals_avoid_create_in_build_method', - problemMessage: _errorMessage, - correctionMessage: _correctionMessage, - errorSeverity: ErrorSeverity.WARNING, - ); + const SignalsAvoidCreateInBuildMethod() : super(code: lintCode); @override void run( @@ -48,69 +18,43 @@ class SignalsAvoidCreateInBuildMethod extends DartLintRule { ErrorReporter reporter, CustomLintContext context, ) { - const types = [ - TypeChecker.fromName( - 'Signal', - packageName: 'signals_core', - ), - TypeChecker.fromName( - 'Computed', - packageName: 'signals_core', - ), - TypeChecker.fromName( - 'SignalValueNotifier', - packageName: 'signals_flutter', - ), - TypeChecker.fromName( - 'SignalValueListenable', - packageName: 'signals_flutter', - ), - ]; - - context.registry.addVariableDeclaration((node) { - final element = node.declaredElement; - if (element == null) return; - - final ancestor = node.thisOrAncestorMatching((method) { - final isMethod = - method is MethodDeclaration && method.name.lexeme == buildMethod; - if (!isMethod) return false; - - if (_findStateClass(node) != null) { - return types.any((e) => e.isAssignableFromType(element.type)); + context.registry.addExpression( + (node) { + if (node.staticType?.isSignal() != true) return; + + final hasMatch = node.hierarchyMatches((node) { + if (node is! Expression || node.staticType?.isSignal() != true) { + return false; + } + if (node is InstanceCreationExpression && + node.isWidgetBuildMember()) { + return true; + } + final checkCreatesSignal = switch (node) { + PropertyAccess(:final realTarget) => realTarget, + Identifier() => node, + InvocationExpression(:final function) => function, + _ => null, + }; + return checkCreatesSignal != null && + node.isWidgetBuildMember() && + createsSignal(checkCreatesSignal); + }); + + if (hasMatch) { + reporter.atNode(node, code); } + }, + ); + } - return false; - }); - - if (ancestor != null) { - reporter.reportErrorForElement(code, element); - } + bool createsSignal(Expression expression) { + return expression.hierarchyMatches((node) { + return (node is InstanceCreationExpression && + node.thisOrAncestorOfType() != null) || + node.isIdentifier() || + node.createsSignal() || + (node is FunctionExpression && node.body.createsSignal()); }); } } - -const TypeChecker stateClass = TypeChecker.fromName( - 'State', - packageName: 'flutter', -); - -const TypeChecker statelessClass = TypeChecker.fromName( - 'StatelessWidget', - packageName: 'flutter', -); - -AstNode? _findStateClass(AstNode node) { - return node.parent?.thisOrAncestorMatching((node) { - if (node is! ClassDeclaration) return false; - - /// Looking for the class which is a [ConsumerState] - final extendsClause = node.extendsClause; - if (extendsClause == null) return false; - final extendsType = extendsClause.superclass.type; - if (extendsType == null) return false; - - return stateClass.isExactlyType(extendsType) || - statelessClass.isExactlyType(extendsType); - }); -} diff --git a/packages/signals_lint/lib/src/lints/definitions.dart b/packages/signals_lint/lib/src/lints/definitions.dart new file mode 100644 index 00000000..a01ef3b4 --- /dev/null +++ b/packages/signals_lint/lib/src/lints/definitions.dart @@ -0,0 +1,57 @@ +import 'package:analyzer/error/error.dart'; +import 'package:custom_lint_builder/custom_lint_builder.dart'; + +const lintCode = LintCode( + name: 'signals_avoid_create_in_build_method', + problemMessage: _errorMessage, + correctionMessage: _correctionMessage, + errorSeverity: ErrorSeverity.WARNING, +); + +const _errorMessage = ''' +This expression causes a new Signal instance to be created on every build, +instead of accessing an existing one. +'''; + +const _correctionMessage = ''' +Create the new signals outside the build() method in the class or globally. + +For StatelessWidgets you can define the signals as a static variable, pass in +from the constructor or declare globally. + +```diff ++ final counter = signal(0); +... +@override +Widget build(BuildContext context) { +- final counter = signal(0); + return ...; +} +``` +'''; + +const signalTypes = [ + TypeChecker.fromName( + 'Signal', + packageName: 'signals_core', + ), + TypeChecker.fromName( + 'Computed', + packageName: 'signals_core', + ), + TypeChecker.fromName( + 'SignalValueNotifier', + packageName: 'signals_flutter', + ), + TypeChecker.fromName( + 'SignalValueListenable', + packageName: 'signals_flutter', + ), +]; + +const widgetClasses = [ + TypeChecker.fromName('State', packageName: 'flutter'), + TypeChecker.fromName('StatelessWidget', packageName: 'flutter'), +]; + +const buildMethodName = 'build'; diff --git a/packages/signals_lint/lib/src/lints/extensions.dart b/packages/signals_lint/lib/src/lints/extensions.dart new file mode 100644 index 00000000..b5847002 --- /dev/null +++ b/packages/signals_lint/lib/src/lints/extensions.dart @@ -0,0 +1,76 @@ +import 'package:analyzer/dart/ast/ast.dart'; +import 'package:analyzer/dart/element/element.dart'; +import 'package:analyzer/dart/element/type.dart'; +import 'package:signals_lint/src/lints/definitions.dart'; + +extension AstNodeX on AstNode { + bool createsSignal() { + if (this case Identifier(staticElement: final ExecutableElement element)) { + if (element case PropertyAccessorElement(isSynthetic: false)) { + return element.returnType.isSignal(); + } + } + return false; + } + + bool isIdentifier() { + final candidate = switch (this) { + PrefixedIdentifier(:final prefix) => prefix.staticElement, + Identifier(:final staticElement) => staticElement, + _ => null, + }; + return candidate?.declaration?.calledInBuild() ?? false; + } + + bool hierarchyMatches(bool Function(AstNode node) predicate) => + thisOrAncestorMatching(predicate) != null; + + bool isWidgetBuildMember() => _isWidgetMember && _isBuildMember; + + bool get _isBuildMember => hierarchyMatches( + (node) => + node is MethodDeclaration && node.name.lexeme == buildMethodName, + ); + + bool get _isWidgetMember { + return parent != null && + parent!.hierarchyMatches((node) { + if (node + case ClassDeclaration( + extendsClause: ExtendsClause( + superclass: NamedType(:final DartType type) + ) + )) { + return type.isTypeWidgetClass(); + } + return false; + }); + } +} + +extension TypeX on DartType { + bool isSignal() => + signalTypes.any((checker) => checker.isAssignableFromType(this)); + + bool isTypeWidgetClass() => + widgetClasses.any((checker) => checker.isAssignableFromType(this)); +} + +extension ElementX on Element { + bool calledInBuild() { + final ancestor = thisOrAncestorMatching( + (element) { + if (element case MethodElement(name: buildMethodName)) { + final widgetParent = element.enclosingElement.thisOrAncestorMatching( + (element) => element._isElementWidgetClass()); + return widgetParent != null; + } + return false; + }, + ); + return ancestor != null; + } + + bool _isElementWidgetClass() => + widgetClasses.any((checker) => checker.isAssignableFrom(this)); +}