-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add unnecessary_stateful_widgets #3725
base: main
Are you sure you want to change the base?
Conversation
/cc @goderbauer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this do when run over the flutter repository? Does it find anything? Any false alarms?
} | ||
|
||
// State public | ||
class MyWidget4 extends StatefulWidget // OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok. Sounds good then.
State<MyWidget3> createState() => _MyWidget3State(); | ||
} | ||
|
||
class _MyWidget3State extends State<MyWidget3> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the cases in which the lint does and doesn't trigger (as documented in the test_data
).
The lint implementation needs a review from somebody more knowledgable in the linter space... :)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document what this method returns?
Fixes #3683