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

feat: add static code diagnostic check-for-equals-in-render-object-setters #1003

Merged
merged 4 commits into from Sep 19, 2022

Conversation

incendial
Copy link
Member

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:

#997

@incendial incendial added type: enhancement New feature or request area-rules labels Sep 16, 2022
@incendial incendial added this to the 4.19.0 milestone Sep 16, 2022
@incendial incendial self-assigned this Sep 16, 2022
@incendial
Copy link
Member Author

incendial commented Sep 16, 2022

@fzyzcjy if you have some time to take a look and test the rule - that'd be very much appreciated!

@incendial incendial changed the title feat: add static code diagnostic prefer-checking-for-equals-in-render-object-setters feat: add static code diagnostic check-for-equals-in-render-object-setters Sep 16, 2022
@github-actions
Copy link

github-actions bot commented Sep 16, 2022

Dart Code Metrics unused files report of dart_code_metrics. ✅

Summary

  • Scanned package folders: bin, example, lib
  • No unused files found! ✅

@github-actions
Copy link

github-actions bot commented Sep 16, 2022

Dart Code Metrics analyze report of dart_code_metrics. ✅

Summary

  • Scanned folders: bin, example, lib, test

  • Total scanned files: 506

  • Total lines of source code: 8716

  • Total classes: 379

  • Average Cyclomatic Number per line of code: 0.36 / 1

  • Average Source Lines of Code per method: 6

  • Total tech debt: 2146.0 hours

  • Found issues: 6 ⚠

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 16, 2022

Sure, let me have a look :)

As for testing, may not have time now, but you can run it on the flutter/flutter repo, and even possibly fire a bug :)

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 16, 2022

Good job! LGTM especially on the demo in the tests. But not sure how many false positives will happen in the real codebase - may need to run it actually :)

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #1003 (f5861ae) into master (36baaa5) will increase coverage by 0.09%.
The diff coverage is 98.24%.

@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
+ Coverage   87.36%   87.46%   +0.09%     
==========================================
  Files         311      313       +2     
  Lines        6571     6628      +57     
==========================================
+ Hits         5741     5797      +56     
- Misses        830      831       +1     
Impacted Files Coverage Δ
...c/analyzers/lint_analyzer/rules/rules_factory.dart 71.42% <ø> (ø)
...k_for_equals_in_render_object_setters/visitor.dart 97.43% <97.43%> (ø)
...heck_for_equals_in_render_object_setters_rule.dart 100.00% <100.00%> (ø)
lib/src/utils/flutter_types_utils.dart 100.00% <100.00%> (ø)

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

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 16, 2022

https://github.com/flutter/flutter/blob/7fba37848c9f3007795be2d67a3207067b7a0894/packages/flutter/lib/src/rendering/platform_view.dart#L323

I strongly suspect this is a bug. That is what I have said in the issue title - "maybe even apply this linter to check potential bugs in Flutter source code" :)

What about giving a PR to flutter repo fixing this 🎉


More details:

Where is the controller from?

image

Where is _UiKitPlatformView used?

image

Thus, seems like this is really a bug.

@incendial
Copy link
Member Author

What about giving a PR to flutter repo fixing this 🎉

Do you want to do it? It's mostly your effort finding this 🙂

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 16, 2022

Do you want to do it?

Sure 🙂

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 17, 2022

@incendial Not very sure. The only interesting call is:

image

However it is named "insertRenderObjectChild", where by saying "insert" I suspect it will not happen unconditionally on every update. So I guess it is normal.

@incendial incendial merged commit e773d30 into master Sep 19, 2022
@incendial incendial deleted the prefer-checking-for-equls-in-render-object-setters branch September 19, 2022 18:03
@sonarcloud
Copy link

sonarcloud bot commented Sep 19, 2022

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
No Duplication information No Duplication information

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.

None yet

2 participants