Fix debug getters crashing in release mode with LateInitializationError#183697
Fix debug getters crashing in release mode with LateInitializationError#183697futpib wants to merge 1 commit into
Conversation
…e crash in release mode Initialize result to false instead of using late, so these getters return false gracefully in release/profile mode instead of throwing LateInitializationError. Fixes flutter#183696
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
The pull request updates three debug getters (debugNeedsLayout, debugNeedsPaint, debugNeedsCompositedLayerUpdate) in RenderObject by initializing a local bool variable result to false instead of using the late keyword. This change likely addresses a potential runtime error in release mode where the assert block (which assigns result) is stripped, leaving the late variable uninitialized if it were to be read outside the assert, or simply provides a default value for consistency.
| /// This is only set in debug mode. In general, render objects should not need | ||
| /// to condition their runtime behavior on whether they are dirty or not, | ||
| /// since they should only be marked dirty immediately prior to being laid | ||
| /// out and painted. In release builds, this throws. |
There was a problem hiding this comment.
Seeing how its mentioned in the comments that this throws, I think this is by design to make it loud if you unintentionally use these methods in release mode.
What do you gain by silencing this error?
There was a problem hiding this comment.
I was just frustrated that my app worked in debug and did not in production, this kind of defeats the purpose of debug build that's easy to debug when you also have to debug the production build. Also debugNeedsSemanticsUpdate returns false silently in production.
If you insist these should throw in production but not in debug I propose we make flutter analyze warn use of these getters (I'm willing to contribute that if it's not an absurd amount of effort).
There was a problem hiding this comment.
Sounds great. The avoid_print lint could be a great place to start
debugNeedsPaint,debugNeedsLayout, anddebugNeedsCompositedLayerUpdateuselate bool resultinitialized only inside anassert()block. In release/profile mode, where asserts are stripped, accessing these getters throwsLateInitializationError. This changeslate bool resulttobool result = false, so the getters returnfalsegracefully outside debug mode while preserving existing debug-mode behavior.Fixes #183696
Pre-launch Checklist
///).