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

feat: add static code diagnostic consistent-update-render-object#1004

Merged
incendial merged 7 commits into
masterfrom
consistent-update-render-object
Sep 20, 2022
Merged

feat: add static code diagnostic consistent-update-render-object#1004
incendial merged 7 commits into
masterfrom
consistent-update-render-object

Conversation

@incendial

@incendial incendial commented Sep 18, 2022

Copy link
Copy Markdown
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:

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

incendial commented Sep 18, 2022

Copy link
Copy Markdown
Member Author

@fzyzcjy again kindly asking you to take a look 🙂

@incendial

incendial commented Sep 18, 2022

Copy link
Copy Markdown
Member Author

https://github.com/flutter/flutter/blob/4b4c876fb9e960329bd86dc143b6e6776eee8abb/packages/flutter/lib/src/cupertino/text_selection_toolbar.dart#L171 - edge case?
https://github.com/flutter/flutter/blob/b0aa50255b117dd6afd178ddae7f0c7b4a01dc11/packages/flutter/lib/src/cupertino/sliding_segmented_control.dart#L743 - edge case too?
https://github.com/flutter/flutter/blob/b0aa50255b117dd6afd178ddae7f0c7b4a01dc11/packages/flutter/lib/src/cupertino/dialog.dart#L808 - interesting, some constants?
https://github.com/flutter/flutter/blob/d72daf602de72a096b9f5c76468e95b69033412c/packages/flutter/lib/src/cupertino/slider.dart#L303 - constants too
https://github.com/flutter/flutter/blob/b0aa50255b117dd6afd178ddae7f0c7b4a01dc11/packages/flutter/lib/src/cupertino/switch.dart#L377 - same thing with state
https://github.com/flutter/flutter/blob/c150c5a03dd8f17d050521a2bd94e61d6b56f997/packages/flutter/lib/src/material/time_picker.dart#L698 - bug?
https://github.com/flutter/flutter/blob/082ad11422dae822c47808337e18a9d2af5734eb/packages/flutter/lib/src/material/tabs.dart#L302 - oh, that would be hard to handle
https://github.com/flutter/flutter/blob/d3bc2bbcdd814beb343719d9b7fed58b3807675f/packages/flutter/lib/src/material/range_slider.dart#L712 - also state thing
https://github.com/flutter/flutter/blob/2aa348b9407e96ffe4eca8e8f213c7984afad3f7/packages/flutter/lib/src/widgets/sliver_persistent_header.dart#L393-L394 - no update, bug?
https://github.com/flutter/flutter/blob/2aa348b9407e96ffe4eca8e8f213c7984afad3f7/packages/flutter/lib/src/widgets/sliver_persistent_header.dart#L413-L414 - same
https://github.com/flutter/flutter/blob/15739345da5a077bb54a403a505994bffae79783/packages/flutter/lib/src/widgets/basic.dart#L3422-L3423 - also no update, but looks like a false positive, since no fields
https://github.com/flutter/flutter/blob/15739345da5a077bb54a403a505994bffae79783/packages/flutter/lib/src/widgets/basic.dart#L3506-L3507 - same
https://github.com/flutter/flutter/blob/c58dca2a45d2c33c1f206db80519306371ee0eed/packages/flutter/lib/src/widgets/editable_text.dart#L3749 - missing backgroundCursorColor, bug?
https://github.com/flutter/flutter/blob/2aa348b9407e96ffe4eca8e8f213c7984afad3f7/packages/flutter/lib/src/widgets/sliver_prototype_extent_list.dart#L52 - missing update, bug?
https://github.com/flutter/flutter/blob/51bcdb9407211ccaf0616533fe1f9eb0e8f04fca/packages/flutter/lib/src/widgets/performance_overlay.dart#L115 - missing few fields, bug?
https://github.com/flutter/flutter/blob/2aa348b9407e96ffe4eca8e8f213c7984afad3f7/packages/flutter/lib/src/widgets/size_changed_layout_notifier.dart#L54-L55 - expected?
https://github.com/flutter/flutter/blob/2aa348b9407e96ffe4eca8e8f213c7984afad3f7/packages/flutter/lib/src/widgets/list_wheel_scroll_view.dart#L1069 - edge case with context?

@fzyzcjy

fzyzcjy commented Sep 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Let me put a tick box to track it:

Looks like a bug, but it is not revealed b/c the users of this render object never update widget with new value? Anyway bug is bug and should be fixed

Maybe we should follow this code, and add an assertion.

Maybe we should follow this code, and add an assertion.

Dnk why orientation is a final field in render object. IMHO it should be modifyable. So I suspect it is bug.

Also maybe add assertion

Agree bug.

Agree bug.

Agree - and this is very commonly used (the text field!)

childManager in parent is a final field, and I suspect buildcontext will not change (indeed it is the element). so I guess no bug?

Agree bug. Btw this bug is 5yr old!

@fzyzcjy

fzyzcjy commented Sep 18, 2022

Copy link
Copy Markdown
Contributor

Aha this rule catches many more bugs! And they cause more problems - because the bug we caught w/ our previous lint rule only affect performance, while this rule catches logically wrong bugs. Great job!

So, same as before, shall I make a PR to flutter?

@incendial

incendial commented Sep 18, 2022

Copy link
Copy Markdown
Member Author

I'll check your response a bit later (I didn't list all the duplicate cases) and maybe add more links. After that, sure

@fzyzcjy

fzyzcjy commented Sep 18, 2022

Copy link
Copy Markdown
Contributor

Sure, take your time!

Base automatically changed from prefer-checking-for-equls-in-render-object-setters to master September 19, 2022 18:03
@github-actions

github-actions Bot commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown

Dart Code Metrics unused files report of dart_code_metrics. ✅

Summary

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

@github-actions

github-actions Bot commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown

Dart Code Metrics analyze report of dart_code_metrics. ✅

Summary

  • Scanned folders: bin, example, lib, test

  • Total scanned files: 509

  • Total lines of source code: 8805

  • Total classes: 385

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

  • Average Source Lines of Code per method: 6

  • Total tech debt: 2170.0 hours

  • Found issues: 6 ⚠

@codecov

codecov Bot commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1004 (c0f3ae2) into master (e773d30) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   87.62%   87.75%   +0.13%     
==========================================
  Files         320      322       +2     
  Lines        6739     6812      +73     
==========================================
+ Hits         5905     5978      +73     
  Misses        834      834
Impacted Files Coverage Δ
...c/analyzers/lint_analyzer/rules/rules_factory.dart 71.42% <ø> (ø)
...r_object/consistent_update_render_object_rule.dart 100.00% <100.00%> (ø)
..._list/consistent_update_render_object/visitor.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

fzyzcjy commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'll check your response a bit later (I didn't list all the duplicate cases) and maybe add more links. After that, sure

So I guess we are ready to file issues? :) (Given all those git updates)

@incendial

incendial commented Sep 20, 2022

Copy link
Copy Markdown
Member Author

I'll check your response a bit later (I didn't list all the duplicate cases) and maybe add more links. After that, sure

So I guess we are ready to file issues? :) (Given all those git updates)

Sorry, no, I did everything easy and left this one for today 😄 (I also want to clean the rule from false-positives)

@fzyzcjy

fzyzcjy commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

Sure, take your time!

@incendial

incendial commented Sep 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

@fzyzcjy supported most of the false positives, checking the source code again

@incendial

incendial commented Sep 20, 2022

Copy link
Copy Markdown
Member Author

Edge case https://github.com/flutter/flutter/blob/f614c597022fd1fafca698f2fd824a707223c84e/packages/flutter/lib/src/widgets/framework.dart#L4760 ?

@incendial

incendial commented Sep 20, 2022

Copy link
Copy Markdown
Member Author

Left several edge cases not supported (assertions, a super call, method calls, constants) and looks like it's okay to have them.

@sonarqubecloud

sonarqubecloud Bot commented Sep 20, 2022

Copy link
Copy Markdown

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

@incendial
incendial merged commit b235f78 into master Sep 20, 2022
@incendial
incendial deleted the consistent-update-render-object branch September 20, 2022 17:49
@fzyzcjy

fzyzcjy commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

Edge case https://github.com/flutter/flutter/blob/f614c597022fd1fafca698f2fd824a707223c84e/packages/flutter/lib/src/widgets/framework.dart#L4760 ?

Think so. That field is final, so no need to update

@fzyzcjy fzyzcjy mentioned this pull request Sep 21, 2022
Closed
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dkrutskikh dkrutskikh Awaiting requested review from dkrutskikh

Assignees

@incendial incendial

Labels

area-rules type: enhancement New feature or request

Projects

None yet

Milestone

4.19.0

Development

Successfully merging this pull request may close these issues.

2 participants

@incendial @fzyzcjy