Feat: adding new lint: avoid_missing_dispose #469
Conversation
github-actions
bot
added
the
p: leancode_lint
Artur-Wisniewski
added
enhancement
packages/leancode_lint/test/lints_test_app/lib/avoid_missing_dispose_test.dart
Outdated
Show resolved
Hide resolved
packages/leancode_lint/test/lints_test_app/lib/avoid_missing_dispose_test.dart
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
p: leancode_lint
packages/leancode_lint/test/lints_test_app/lib/avoid_conditional_hooks_test.dart
Outdated
Show resolved
Hide resolved
Artur-Wisniewski
force-pushed
the
feat/avoid-missing-dispose
branch
from
5c77f66 to
2f5f1fd
Compare
| bool _isDisposable(InterfaceType type) => | ||
| type.methods2.any((method) => method.name3 == 'dispose') || | ||
| type.element3.inheritedMembers.entries.any( | ||
| (entry) => | ||
| entry.key.name == 'dispose' && | ||
| entry.value.baseElement is MethodElement2, | ||
| ); |
There was a problem hiding this comment.
That's very nice. I'd also consider adding close and cancel methods but I'm not sure because it might give more false-positive cases 🙈
There was a problem hiding this comment.
👍 (#469 (comment)) — I don't think there'll be false positives 🤔
There was a problem hiding this comment.
I will make 2 other lints for closeable and for cancelable resources based on this code in this PR.
There was a problem hiding this comment.
Or maybe this one could be configurable? 😏
There was a problem hiding this comment.
I've added configuration for this close, cancel, and dispose
packages/leancode_lint/test/lints_test_app/lib/avoid_conditional_hooks_test.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new lint rule called missing_cleanup that enforces proper disposal of resources in Flutter StatefulWidget classes. The lint detects when disposable resources (controllers, streams, timers, etc.) are not properly cleaned up in the dispose() method, helping prevent memory leaks.
- Implements the
missing_cleanuplint rule with configurable cleanup methods and ignored types - Adds comprehensive test cases covering various scenarios including StatefulWidget, StatelessWidget, and Bloc classes
- Includes detailed documentation with configuration options and usage examples
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/leancode_lint/lib/lints/missing_cleanup.dart |
Main implementation of the missing_cleanup lint rule with configuration support |
packages/leancode_lint/test/lints_test_app/lib/missing_cleanup/missing_cleanup_test.dart |
Test cases for the lint rule covering various disposal scenarios |
packages/leancode_lint/test/lints_test_app/lib/missing_cleanup/missing_cleanup_dispose_bloc_test.dart |
Test cases specifically for Bloc disposal patterns |
packages/leancode_lint/lib/leancode_lint.dart |
Registration of the new lint rule in the plugin |
packages/leancode_lint/README.md |
Documentation for the new lint rule with examples and configuration |
packages/leancode_lint/pubspec.yaml |
Added yaml dependency for configuration parsing |
packages/leancode_lint/test/lints_test_app/analysis_options.yaml |
Configuration example for the lint rule |
packages/leancode_lint/test/lints_test_app/lib/avoid_conditional_hooks_test.dart |
Added ignore directive for the new lint |
Comments suppressed due to low confidence (2)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Artur-Wisniewski
force-pushed
the
feat/avoid-missing-dispose
branch
from
30ecd4a to
6783459
Compare
|
|
||
| Resources such as controllers, stream controllers or focus nodes must be cleanup in the `dispose()` method to prevent memory leaks. | ||
|
|
||
| **BAD:**` |
There was a problem hiding this comment.
| **BAD:**` | |
| **BAD:** |
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| controller = TextEditingController(); | ||
| } |
There was a problem hiding this comment.
I find this GOOD example confusing: is it good because of the dispose in the void dispose() method of the Widget or is it good because the controller is initialized in void initState()? I would minimize the diff between the BAD and GOOD example
| - `ignored_types` - an optional YamlList - skips dispose checks for specified types. This allows disabling the lint rule for classes where dispose method checks are not needed. | ||
| - `ignore` - A required String - name of the instance to ignore | ||
| - `from_package` - A required String - name of the source package | ||
| - `cleanup_methods` - an optional YamlMap - controls which disposal methods the lint rule should recognize and check for. By default, the rule looks for `dispose`, `close`, and `cancel` methods. You can selectively enable or disable checking for each of these methods. |
There was a problem hiding this comment.
I would not use package:yaml jargon here (YamlList, YamlMap). End users don't know what that is. Just saying it is a list and respectively a map with keys X Y Z is enough
| - `ignored_types` - an optional YamlList - skips dispose checks for specified types. This allows disabling the lint rule for classes where dispose method checks are not needed. | ||
| - `ignore` - A required String - name of the instance to ignore | ||
| - `from_package` - A required String - name of the source package | ||
| - `cleanup_methods` - an optional YamlMap - controls which disposal methods the lint rule should recognize and check for. By default, the rule looks for `dispose`, `close`, and `cancel` methods. You can selectively enable or disable checking for each of these methods. |
There was a problem hiding this comment.
What is the purpose of this configuration? What is the usecase of disabling some of them globally (rather than putting a type in the ignored_types list)?
There was a problem hiding this comment.
I would also write a custom class that has a cleanup method to see if it is also detected by the lint
| ); | ||
|
|
||
| return widgetTypeChecker.isExactlyType(type) || | ||
| widgetTypeChecker.isSuperTypeOf(type); |
There was a problem hiding this comment.
Doesn't being exactly the type imply being a supertype? i.e. the check for isExactlyType is redundant?
| if (_isWidgetClass(classNode) && | ||
| !_isFieldUsedByConstructor(classNode, node)) { | ||
| reporter.atNode(node, code); | ||
| return; | ||
| } |
There was a problem hiding this comment.
How does this check work? Why it matters if something is used in the constructor?
| void visitExpressionStatement(ExpressionStatement node) { | ||
| if (node.expression | ||
| case MethodInvocation( |
There was a problem hiding this comment.
Since you only care about expressions that are MethodInvocations, you can just override the visitMethodInvocation method of the visitor
| target: SimpleIdentifier(:final name), | ||
| ) && | ||
| final invocation when name == targetName) { |
There was a problem hiding this comment.
Don't compare by name. Variables can be shadowed. I am pretty sure something in the AST gives identity to variables (element?)
|
|
||
| final String targetName; | ||
|
|
||
| final List<InvocationExpression> _disposeExpressions = []; |
There was a problem hiding this comment.
The result does not seem to be ever used. Since this visitor is already quite expensive, I would make it return early. Namely, if a cleanup is found, immediately stop the visitor and just return bool true
For that you might need to override the RecursiveAstVisitor which can stop the recursing
No description provided.