Skip to content
This repository was archived by the owner on Jul 16, 2023. It is now read-only.

feat: add use-setstate-synchronously rule #1120

Merged
merged 12 commits into from
Dec 28, 2022

Conversation

Desdaemon
Copy link
Contributor

@Desdaemon Desdaemon commented Dec 24, 2022

What is the purpose of this pull request? (put an "X" next to an item)

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain:

#1117

What changes did you make? (Give an overview)

  • Basic implementation
  • Rule customization
  • Flow analysis
    • if checks
    • while checks
    • Ternaries
    • And-chains (mounted && ..)
    • Or-chains (!mounted || ..)

Is there anything you'd like reviewers to focus on?

Should this rule try to be as comprehensive as possible and check for all cases, or should the current heuristics for determining mounted-ness be sufficient?

@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch 2 times, most recently from 8fd91e1 to 3c6e2a2 Compare December 25, 2022 03:49
@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch from 3c6e2a2 to f97a9ea Compare December 25, 2022 03:53
@incendial incendial self-requested a review December 25, 2022 10:46
@incendial
Copy link
Member

@Desdaemon could you please update the changelog?

Should this rule try to be as comprehensive as possible and check for all cases, or should the current heuristics for determining mounted-ness be sufficient?

The most important that the rule shouldn't produce false-positives. So, if I'm not mistaken, covering And-chains (mounted && ..) is a must.

Copy link
Member

@incendial incendial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great result! Left a few comments. Haven't seen @pragma('vm:prefer-inline') in action before 🙂

@Desdaemon Desdaemon changed the title feat: add avoid-async-setstate rule feat: add use-setstate-synchronously rule Dec 25, 2022
@Desdaemon Desdaemon requested a review from incendial December 26, 2022 01:09
@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch 2 times, most recently from 0606a41 to 4f1389f Compare December 26, 2022 03:09
Copy link
Member

@incendial incendial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! The only question I have - will || create false positives? I'll later run this rule on the Flutter codebase (or you can, if you want to) to know for sure, but it'd be better if we avoid as many false positives as possible

@Desdaemon
Copy link
Contributor Author

There's one I haven't found a way to cleanly implement:

if (!mounted || foo) return;
setState(); // should be mounted here

if (!mounted && bar || foo) return;
setState(); // LINT

When the first or-chain diverges, it should also have the effect of guaranteeing the widget is mounted, but currently it does not do so. If there are other false positives I'll prioritize fixing them as well.

@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch from 4f1389f to 42893bd Compare December 26, 2022 14:17
@codecov
Copy link

codecov bot commented Dec 26, 2022

Codecov Report

Merging #1120 (2111ddd) into master (a352607) will increase coverage by 0.17%.
The diff coverage is 95.48%.

@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
+ Coverage   85.19%   85.37%   +0.17%     
==========================================
  Files         342      347       +5     
  Lines        7540     7673     +133     
==========================================
+ Hits         6424     6551     +127     
- Misses       1116     1122       +6     
Impacted Files Coverage Δ
...c/analyzers/lint_analyzer/rules/rules_factory.dart 75.00% <ø> (ø)
...es/rules_list/use_setstate_synchronously/fact.dart 87.50% <87.50%> (ø)
...rules_list/use_setstate_synchronously/visitor.dart 93.22% <93.22%> (ø)
.../rules_list/use_setstate_synchronously/config.dart 100.00% <100.00%> (ø)
...rules_list/use_setstate_synchronously/helpers.dart 100.00% <100.00%> (ø)
...synchronously/use_setstate_synchronously_rule.dart 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Desdaemon
Copy link
Contributor Author

I'll later run this rule on the Flutter codebase (or you can, if you want to) to know for sure, but it'd be better if we avoid as many false positives as possible

I ran this on the Flutter package on the stable branch, the only violation it detected was this situation:

https://github.com/flutter/flutter/blob/135454af32477f815a7525073027a3ff9eff1bfd/packages/flutter/lib/src/material/refresh_indicator.dart#L432-L439

I think this is too specific for this rule to handle, but otherwise no false positives yet. I'll run this on other codebases as well to cover as many edge cases as possible.

@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch from 0054192 to 667d125 Compare December 27, 2022 05:14
@Desdaemon Desdaemon requested a review from incendial December 27, 2022 05:15
@incendial
Copy link
Member

incendial commented Dec 27, 2022

I think this is too specific for this rule to handle

Yeah, looks questionable, tbh (I mean, the code).

I'll run this on other codebases as well to cover as many edge cases as possible.

I've noticed a fix for a false positive, if you can also add tests - that'd be great. And after that I'm ready to merge, so tell me when you're ready.

@incendial incendial added type: enhancement New feature or request area-rules labels Dec 27, 2022
@incendial incendial added this to the 5.4.0 milestone Dec 27, 2022
@Desdaemon Desdaemon force-pushed the avoid-async-setstate branch from fb1170e to 75d8762 Compare December 27, 2022 06:58
@Desdaemon
Copy link
Contributor Author

@incendial That should be my last batch of changes, should be ready to review and merge.

@incendial
Copy link
Member

@Desdaemon please add tests for the config (example) and I'll merge

@incendial
Copy link
Member

And please check the failing tests

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@incendial incendial merged commit ea9eaad into dart-code-checker:master Dec 28, 2022
@incendial
Copy link
Member

@Desdaemon thank you for the contribution! 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-rules type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants