Fix avoid_unnecessary_setstate false-positives (#88)

* Add repro

* Cleanup naming

* Properly filter methods calling setState

* Allow async calls to setState

* Fix using original DCM code

* More docs

* Fixup comment

* Positive boolean

* Add constructor doc comment

---------

Co-authored-by: Yurii Prykhodko <[email protected]>

lib/lints/avoid_unnecessary_setstate/avoid_unnecessary_set_state_rule.dart

@@ -7,6 +7,38 @@ import 'package:solid_lints/models/solid_lint_rule.dart';
 /// A rule which 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.
+///
+/// Cases where setState is unnecessary:
+/// - synchronous calls inside State lifecycle methods:
+///   - initState
+///   - didUpdateWidget
+///   - didChangeDependencies
+/// - synchronous calls inside `build` method
+///
+/// Nested synchronous setState invocations are also disallowed.
+///
+/// Calling setState in the aforementioned methods is allowed for:
+/// - async methods
+/// - callbacks
+///
+/// Example:
+/// ```dart
+/// void initState() {
+///   setState(() => foo = 'bar');  // lint
+///   changeState();                // lint
+///   triggerFetch();               // OK
+///   stream.listen((event) => setState(() => foo = event)); // OK
+/// }
+///
+/// void changeState() {
+///   setState(() => foo = 'bar');
+/// }
+///
+/// void triggerFetch() async {
+///   await fetch();
+///   if (mounted) setState(() => foo = 'bar');
+/// }
+/// ```
 class AvoidUnnecessarySetStateRule extends SolidLintRule {
   /// The lint name of this lint rule that represents
   /// the error whether we use setState in inappropriate way.
@@ -32,9 +64,8 @@ class AvoidUnnecessarySetStateRule extends SolidLintRule {
     ErrorReporter reporter,
     CustomLintContext context,
   ) {
-    final visitor = AvoidUnnecessarySetStateVisitor();
-
     context.registry.addClassDeclaration((node) {
+      final visitor = AvoidUnnecessarySetStateVisitor();
       visitor.visitClassDeclaration(node);
       for (final element in visitor.setStateInvocations) {
         reporter.reportErrorForNode(code, element);

lib/lints/avoid_unnecessary_setstate/visitor/avoid_unnecessary_set_state_method_visitor.dart

@@ -28,35 +28,47 @@ import 'package:analyzer/dart/ast/visitor.dart';
 /// necessary
 class AvoidUnnecessarySetStateMethodVisitor extends RecursiveAstVisitor<void> {
   final Set<String> _classMethodsNames;
-  final Iterable<FunctionBody> _bodies;
+  final Set<FunctionBody> _classMethodBodies;
 
   final _setStateInvocations = <MethodInvocation>[];
+  final _classMethodsInvocations = <MethodInvocation>[];
 
-  /// All setState invocations
+  /// Invocations of setState within visited methods.
   Iterable<MethodInvocation> get setStateInvocations => _setStateInvocations;
 
-  /// Constructor for AvoidUnnecessarySetStateMethodVisitor
-  AvoidUnnecessarySetStateMethodVisitor(this._classMethodsNames, this._bodies);
+  /// Invocations of methods within visited methods.
+  Iterable<MethodInvocation> get classMethodsInvocations =>
+      _classMethodsInvocations;
+
+  /// Constructor for [AvoidUnnecessarySetStateMethodVisitor]
+  AvoidUnnecessarySetStateMethodVisitor(
+    this._classMethodsNames,
+    this._classMethodBodies,
+  );
 
   @override
   void visitMethodInvocation(MethodInvocation node) {
     super.visitMethodInvocation(node);
 
     final name = node.methodName.name;
-    final notInBody = _isNotInFunctionBody(node);
+    final isNotInCallback = !_isInCallback(node);
 
-    if (name == 'setState' && notInBody) {
-      _setStateInvocations.add(node);
-    } else if (_classMethodsNames.contains(name) &&
-        notInBody &&
-        node.realTarget == null) {
+    if (name == 'setState' && isNotInCallback) {
       _setStateInvocations.add(node);
+      return;
+    }
+    final isClassMethod = _classMethodsNames.contains(name);
+    final isReturnValueUsed = node.realTarget != null;
+
+    if (isClassMethod && isNotInCallback && !isReturnValueUsed) {
+      _classMethodsInvocations.add(node);
     }
   }
 
-  bool _isNotInFunctionBody(MethodInvocation node) =>
+  bool _isInCallback(MethodInvocation node) =>
       node.thisOrAncestorMatching(
-        (parent) => parent is FunctionBody && !_bodies.contains(parent),
-      ) ==
+        (parent) =>
+            parent is FunctionBody && !_classMethodBodies.contains(parent),
+      ) !=
       null;
 }

lib/lints/avoid_unnecessary_setstate/visitor/avoid_unnecessary_set_state_visitor.dart

@@ -23,6 +23,7 @@
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
+import 'package:collection/collection.dart';
 import 'package:solid_lints/lints/avoid_unnecessary_setstate/visitor/avoid_unnecessary_set_state_method_visitor.dart';
 import 'package:solid_lints/utils/types_utils.dart';
 
@@ -37,7 +38,7 @@ class AvoidUnnecessarySetStateVisitor extends RecursiveAstVisitor<void> {
 
   final _setStateInvocations = <MethodInvocation>[];
 
-  /// All setState invocations in checkedMethods
+  /// Unnecessary setState invocations
   Iterable<MethodInvocation> get setStateInvocations => _setStateInvocations;
 
   @override
@@ -49,22 +50,69 @@ class AvoidUnnecessarySetStateVisitor extends RecursiveAstVisitor<void> {
       return;
     }
 
-    final declarations = node.members.whereType<MethodDeclaration>().toList();
+    final methods = node.members.whereType<MethodDeclaration>();
     final classMethodsNames =
-        declarations.map((declaration) => declaration.name.lexeme).toSet();
-    final bodies = declarations.map((declaration) => declaration.body).toList();
-    final methods = declarations
-        .where((member) => _checkedMethods.contains(member.name.lexeme))
-        .toList();
-
-    for (final method in methods) {
-      final visitor =
-          AvoidUnnecessarySetStateMethodVisitor(classMethodsNames, bodies);
+        methods.map((declaration) => declaration.name.lexeme).toSet();
+    final methodBodies = methods.map((declaration) => declaration.body).toSet();
+
+    final checkedMethods = methods.where(_isMethodChecked);
+    final uncheckedMethods = methods.whereNot(_isMethodChecked);
+
+    // memo for visited methods; prevents checking the same method twice
+    final methodToHasSetState = <String, bool>{};
+
+    for (final method in checkedMethods) {
+      final visitor = AvoidUnnecessarySetStateMethodVisitor(
+        classMethodsNames,
+        methodBodies,
+      );
       method.visitChildren(visitor);
 
-      _setStateInvocations.addAll([
-        ...visitor.setStateInvocations,
-      ]);
+      _setStateInvocations.addAll(visitor.setStateInvocations);
+      _setStateInvocations.addAll(
+        visitor.classMethodsInvocations
+            .where(
+              (invocation) => _containsSetState(
+                methodToHasSetState,
+                classMethodsNames,
+                methodBodies,
+                uncheckedMethods.firstWhere(
+                  (method) => method.name.lexeme == invocation.methodName.name,
+                ),
+              ),
+            )
+            .toList(),
+      );
+    }
+  }
+
+  bool _isMethodChecked(MethodDeclaration m) =>
+      _checkedMethods.contains(m.name.lexeme);
+
+  bool _containsSetState(
+    Map<String, bool> methodToHasSetState,
+    Set<String> classMethodsNames,
+    Set<FunctionBody> bodies,
+    MethodDeclaration declaration,
+  ) {
+    final type = declaration.returnType?.type;
+    if (type != null && (type.isDartAsyncFuture || type.isDartAsyncFutureOr)) {
+      return false;
     }
+
+    final name = declaration.name.lexeme;
+    if (methodToHasSetState.containsKey(name) && methodToHasSetState[name]!) {
+      return true;
+    }
+
+    final visitor =
+        AvoidUnnecessarySetStateMethodVisitor(classMethodsNames, bodies);
+    declaration.visitChildren(visitor);
+
+    final hasSetState = visitor.setStateInvocations.isNotEmpty;
+
+    methodToHasSetState[name] = hasSetState;
+
+    return hasSetState;
   }
 }

lint_test/avoid_unnecessary_setstate_test.dart

@@ -40,6 +40,8 @@ class _MyWidgetState extends State<MyWidget> {
   void initState() {
     super.initState();
 
+    methodWithoutSetState();
+
     // expect_lint: avoid_unnecessary_setstate
     setState(() {
       _myString = "Hello";
@@ -71,6 +73,10 @@ class _MyWidgetState extends State<MyWidget> {
     });
   }
 
+  void methodWithoutSetState() {
+    _myString = 'hey';
+  }
+
   @override
   Widget build(BuildContext context) {
     // expect_lint: avoid_unnecessary_setstate
@@ -90,6 +96,7 @@ class _MyWidgetState extends State<MyWidget> {
 
     return ElevatedButton(
       onPressed: myStateUpdateMethod,
+      onHover: (_) => methodWithoutSetState(),
       onLongPress: () {
         setState(() {
           _myString = 'data';