Add unnecessary_stateful_widgets #3725
Conversation
|
/cc @goderbauer |
| } | ||
|
|
||
| // State public | ||
| class MyWidget4 extends StatefulWidget // OK |
There was a problem hiding this comment.
What's the reasoning behind marking this as OK? I think, naively I would have marked this with the lint as well...
There was a problem hiding this comment.
Removing the state would be a breaking change as someone could have referenced this type outside. For instance in flutter package PopupMenuButtonState is one of these cases (and the reason why I added this exception).
There was a problem hiding this comment.
Oh ok. Sounds good then.
| State<MyWidget3> createState() => _MyWidget3State(); | ||
| } | ||
|
|
||
| class _MyWidget3State extends State<MyWidget3> { |
There was a problem hiding this comment.
This should really be linted as well since the state is not really holding on to any state...
There was a problem hiding this comment.
But, of course, if a member i holds on to anything stateful (say a FocusNode or a ScrollController) it shouldn't lint. I guess, we probably can't differentiate between these cases?
There was a problem hiding this comment.
I started with only a rule to catch usage of setState but on the flutter codebase I face several false positives with State having fields. As you mentioned it's not easy to catch what field is stateful.
There was a problem hiding this comment.
If i remove the absence of fields in state condition, here's the list of diagnotics
info • Unnecessary StatefulWidget • packages/flutter/lib/src/material/about.dart:530:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/lib/src/material/bottom_sheet.dart:67:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/lib/src/material/bottom_sheet.dart:350:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/lib/src/material/dropdown.dart:95:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/lib/src/widgets/fade_in_image.dart:61:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/test/material/reorderable_list_test.dart:1782:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/test/widgets/animated_list_test.dart:494:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/test/widgets/heroes_test.dart:162:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/test/widgets/inherited_model_test.dart:55:7 • unnecessary_stateful_widgets
info • Unnecessary StatefulWidget • packages/flutter/test/widgets/slivers_block_global_key_test.dart:11:7 • unnecessary_stateful_widgets
A lot of them are not simple/impossible to make statelessWidget
There was a problem hiding this comment.
I went through these just for fun. There are three different classes:
Class a) Lots of these clearly need to be StatefulWidgets because they have non-final properties that represent state:
- packages/flutter/lib/src/material/bottom_sheet.dart:350:7
- packages/flutter/lib/src/widgets/fade_in_image.dart:61:7 • unnecessary_stateful_widgets
- packages/flutter/test/material/reorderable_list_test.dart:1782:7
*packages/flutter/test/widgets/heroes_test.dart:162:7 - packages/flutter/test/widgets/inherited_model_test.dart:55:7
class b) Then there are a bunch that have final properties, but they are caching state, so these are also expected to be stateful widgets:
- packages/flutter/lib/src/material/about.dart:530:7
- packages/flutter/test/widgets/slivers_block_global_key_test.dart:11:7
- packages/flutter/lib/src/material/bottom_sheet.dart:67:7
class c) Last, but not least, there is a single instance that could be migrated to a stateless widget, but it really is more convenient as a stateful widget to access State.context:
- packages/flutter/lib/src/material/dropdown.dart:95:7
Detecting when a final field is state and when not (class b) is likely not possible. And it looks like with the current definition of using fields as an indicator we are not really missing anything useful with this lint. So, yeah, the current definition of the lint makes sense to me then and we can resolve this comment.
On packages/flutter : flutter/flutter#112296
The exceptions I added in this linter PR is the result of a too broaden application. |
|
How about checking if an override is a default one, so it is not preventing the lint from being triggered? E.g. @override
void dispose() => super.dispose();The relevant check is in FlutterConvertToStatelessWidget assist. |
Thanks for the suggestion. This case should be catch by unnecessary_overrides. Not sure it is worth adding complexity here to catch things like this. This |
|
I added some comment over at: flutter/flutter#112296 (comment) I wonder if we should omit the lint if the State implementation is actually using functionality of the |
a14n
force-pushed
the
unnecessary_stateful_widgets
branch
from
366f3ed
to
cad2266
Compare
|
I updated the lint to have an exception if the WidgetState uses a member of its parent. I think this PR is now ready from a functional point of view. |
|
I want to double check that we're "accepting" the new lint request at #3683. Then we'll review. |
There was a problem hiding this comment.
Technically this looks good to me. The proposal has been accepted.
CC @pq @bwilkerson if you care to review from a technical perspective.
| } | ||
| } | ||
|
|
||
| bool _visit(Element? methodElement) { |
There was a problem hiding this comment.
Can you document what this method returns?
Fixes #3683