Add lint rule for debug-only RenderObject getters#183706
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new lint rule to prevent the incorrect use of debug-only RenderObject getters outside of assert statements. The implementation of the new rule is solid, and it includes corresponding tests and updates to existing code to suppress legitimate usages. My feedback includes a suggestion to make the type checking in the new lint rule more robust by verifying the library URI of RenderObject in addition to its name.
9184015 to
fbb08bf
Compare
|
@futpib it looks like this does not currently compile, can you take a look? |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. 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. |
a82127b to
5db02fb
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new analyzer rule, avoidDebugOnlyRenderingGetters, which prevents the use of specific RenderObject debug getters outside of assert statements or debug-only contexts. The changes include the rule implementation, a refactor of the hasInlineIgnore utility to accept raw content and line information, and new test fixtures. Feedback was provided regarding the strict library URI validation in the rule, which may prevent it from correctly identifying violations within the test environment's mock objects.
c1cece6 to
1d45ce9
Compare
…serts debugNeedsLayout, debugNeedsPaint, debugNeedsCompositedLayerUpdate, and debugNeedsSemanticsUpdate are only meaningful in debug mode. This custom analyze rule warns when they are used outside of assert statements, helping prevent subtle bugs where these getters silently return false in release builds. Legitimate non-assert usages (e.g. in WidgetInspectorService, which only runs in debug/profile mode) can be suppressed with a flutter_ignore comment directive.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Replace custom _hasTrailingFlutterIgnore with shared hasInlineIgnore from utils.dart, as suggested in review - Fix compilation error: library.source.uri → library.uri (source getter doesn't exist on LibraryElement in analyzer 10.1.0) - Generalize hasInlineIgnore to accept String content + LineInfo instead of ParseStringResult, so it works with ResolvedUnitResult too
The test infrastructure uses fake RenderObject classes without package config, so library URIs won't match package:flutter/... — consistent with how render_box_intrinsics.dart checks by name only.
1d45ce9 to
acd7ca6
Compare
|
@Piinks I think it's fixed now as far as I can see without ci |
There was a problem hiding this comment.
I will be hesitate about enforcing this in framework mainly for cases that debug widget like widget inspector or error widget has valid use case to call these debug method, but I think this is something that is suitable lint for flutter_lint that is used for flutter app #185091
Piinks
left a comment
There was a problem hiding this comment.
Should this have captured
?Looking at #184627, I found we are pretty inconsistent about calling these methods or not outside of debug mode in the framework, I wonder if this lint is too prohibitive for those cases where we already do call on these methods.
Also, what about debugDoingLayout, debugDoingPaint, debugDoingThisResize, debugDoingThisLayout, and debugCanParentUseSize?
Chatting with @chunhtai, they filed #185091, we might need to approach this more comprehensively to address the inconsistencies we have in the framework.
| } | ||
|
|
||
| /// Returns true if [enclosingElement] is `RenderObject` or a subclass of it. | ||
| static bool _isRenderObjectSubclass(InterfaceElement enclosingElement) { |
There was a problem hiding this comment.
The _isRenderObjectSubclass check currently relies on the class name RenderObject. While sufficient for the Flutter repo's internal analysis, it could be made more robust by verifying the library URI to avoid false positives with unrelated classes that happen to be named RenderObject.
Making this a flutter_lint is a great idea @chunhtai! |
Adds a custom analyze rule that warns when debug-only RenderObject getters (
debugNeedsLayout,debugNeedsPaint,debugNeedsCompositedLayerUpdate,debugNeedsSemanticsUpdate) are used outside ofassertstatements.These getters are only meaningful in debug mode — in release builds they silently return
false. Using them in non-assert code can lead to subtle bugs where the app behaves differently in debug vs release mode.Legitimate non-assert usages (e.g. in
WidgetInspectorService, which only runs in debug/profile mode) can be suppressed with a// flutter_ignore: debug_only_rendering_getter (see analyze.dart)comment.Follow-up to #183697, fixes #183696.
Pre-launch Checklist
///).