Improve assertion messaging in RenderBox.size#128910
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). 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. |
github-actions
Bot
added
the
framework
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Hey @matthew-carroll thanks for looking into improving the error message. Can you add. test that checks both cases of this error?
| assert(size._owner == this); | ||
| if (RenderObject.debugActiveLayout != null && | ||
| !RenderObject.debugActiveLayout!.debugDoingThisLayoutWithCallback) { | ||
| assert( |
There was a problem hiding this comment.
This should probably still be wrapped in an assertion.
| "that child's layout().", | ||
| ); | ||
| // The Flutter pipeline is currently running a layout pass. | ||
| // TODO: explain why we care that debugDoingThisLayoutWithCallback |
There was a problem hiding this comment.
Tracked it back to #74402 for full context. Some widgets like RenderEditableTextState will access RenderBox.size when rebuilding.
There was a problem hiding this comment.
So that property refers to a "rebuild"? Can you define that term for me? I've seen it used a few different ways.
There was a problem hiding this comment.
I cannot, I am not familiar with the specific change. The docs for debugDoingThisLayoutWithCallback say Whether invokeLayoutCallback for this render object is currently running. Certainly room to improve the docs there though. I'll file a bug.
There was a problem hiding this comment.
invokeLayoutCallback is quite a bit more diverse than just the concept of "rebuild", I believe.
invokeLayoutCallback may run by a RenderObject any number of times within that RenderObject's layout() behavior. Inside of that callback, you're permitted to mutate the element and render tree during layout, which is otherwise prohibited.
The double and triple negatives are throwing me, so I still can't tell the intention. It looks like if we're in a layout callback, we don't bother checking any conditions? We let the size access go through no matter what? This is the kind of stuff I was trying to improve. The intention of these conditions is really hard to figure out.
| // This RenderBox is not in the middle of running its own layout, which means some other | ||
| // object is trying to access the size. | ||
| if (RenderObject.debugActiveLayout != parent) { | ||
| throw AssertionError( |
There was a problem hiding this comment.
Instead of an AssertionError can we be even more helpful with a full FlutterError? You can add an ErrorHint for these helpful suggestions to resolve the issue.
Given that I'm not adding or removing any intentional condition, shouldn't there already be tests for this? I was OK spending the time to write up different comments and error messages, but I purposefully only made non-behavior changes so that I wouldn't trigger the testing policy. If I'm still supposed to write tests, even for this change, then this is probably as far as I take the PR. As for changing the type of thing being thrown - sure, I can make that whatever you want. |
|
It looks like a lot of tests are currently failing, so if an existing test just needs an update, that would definitely check the box. Otherwise, per the bot we'll need Hixie to exempt tests for this. As a heads up, Hixie may ask if it's ok for this change to get accidentally reverted later, since tests would prevent something like that happening. 😉 |
|
@Piinks do you happen to know the lint rules for composing strings and escaping single-quotes? In my original commit, I escaped apostrophes because they conflicted with the string delimiters. But then the analysis said I used unnecessary escapes. So I removed the escapes. Now I've broken compilation because I have apostrophes that conflict with the string delimiters. I'm not sure what the analyzer wants from me... |
|
I think the analyzer expects single quotes, unless they are already part of the string, which in that case it prefers double quotes for the string so the escapes for single quotes are not necessary. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
(Triage): @matthew-carroll Do you still have plans to work on this and address the feedback given above? |
|
I'm giving up. It's proven far too difficult to interpret and resolve the test failures. |
3 participants
I'm working on a group of custom
RenderObjects and I keep hitting an assertion that's a bit ambiguous. I think it would be a good idea to add clarity to the associated assertion.One outstanding question: What does
!RenderObject.debugActiveLayout!.debugDoingThisLayoutWithCallbackmean, and why do we care?