Skip to content

Commit d755bc2

Browse files
Replace RenderBox.compute* with RenderBox.get* and add @visibleForOverriding (flutter#145503)
`@visibleForOverriding` + `@protected` unfortunately does not catch the case where a `compute*` method was overridden in a subtype and the overide was called in that same type's implementation. I did not add a `flutter_ignore` for this because it doesn't seem there will be false positives.
1 parent 0f685f8 commit d755bc2

File tree

19 files changed

+246
-37
lines changed

19 files changed

+246
-37
lines changed

dev/bots/analyze.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import 'custom_rules/analyze.dart';
2121
import 'custom_rules/avoid_future_catcherror.dart';
2222
import 'custom_rules/no_double_clamp.dart';
2323
import 'custom_rules/no_stop_watches.dart';
24+
import 'custom_rules/render_box_intrinsics.dart';
2425
import 'run_command.dart';
2526
import 'utils.dart';
2627

@@ -174,7 +175,7 @@ Future<void> run(List<String> arguments) async {
174175
// Only run the private lints when the code is free of type errors. The
175176
// lints are easier to write when they can assume, for example, there is no
176177
// inheritance cycles.
177-
final List<AnalyzeRule> rules = <AnalyzeRule>[noDoubleClamp, noStopwatches];
178+
final List<AnalyzeRule> rules = <AnalyzeRule>[noDoubleClamp, noStopwatches, renderBoxIntrinsicCalculation];
178179
final String ruleNames = rules.map((AnalyzeRule rule) => '\n * $rule').join();
179180
printProgress('Analyzing code in the framework with the following rules:$ruleNames');
180181
await analyzeWithRules(flutterRoot, rules,
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// Copyright 2014 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:analyzer/dart/analysis/results.dart';
6+
import 'package:analyzer/dart/ast/ast.dart';
7+
import 'package:analyzer/dart/ast/visitor.dart';
8+
import 'package:analyzer/dart/element/element.dart';
9+
import 'package:analyzer/dart/element/type.dart';
10+
11+
import '../utils.dart';
12+
import 'analyze.dart';
13+
14+
/// Verify that no RenderBox subclasses call compute* instead of get* for
15+
/// computing the intrinsic dimensions. The full list of RenderBox intrinsic
16+
/// methods checked by this rule is listed in [candidates].
17+
final AnalyzeRule renderBoxIntrinsicCalculation = _RenderBoxIntrinsicCalculationRule();
18+
19+
const Map<String, String> candidates = <String, String> {
20+
'computeDryBaseline': 'getDryBaseline',
21+
'computeDryLayout': 'getDryLayout',
22+
'computeDistanceToActualBaseline': 'getDistanceToBaseline, or getDistanceToActualBaseline',
23+
'computeMaxIntrinsicHeight': 'getMaxIntrinsicHeight',
24+
'computeMinIntrinsicHeight': 'getMinIntrinsicHeight',
25+
'computeMaxIntrinsicWidth': 'getMaxIntrinsicWidth',
26+
'computeMinIntrinsicWidth': 'getMinIntrinsicWidth'
27+
};
28+
29+
class _RenderBoxIntrinsicCalculationRule implements AnalyzeRule {
30+
final Map<ResolvedUnitResult, List<(AstNode, String)>> _errors = <ResolvedUnitResult, List<(AstNode, String)>>{};
31+
32+
@override
33+
void applyTo(ResolvedUnitResult unit) {
34+
final _RenderBoxSubclassVisitor visitor = _RenderBoxSubclassVisitor();
35+
unit.unit.visitChildren(visitor);
36+
final List<(AstNode, String)> violationsInUnit = visitor.violationNodes;
37+
if (violationsInUnit.isNotEmpty) {
38+
_errors.putIfAbsent(unit, () => <(AstNode, String)>[]).addAll(violationsInUnit);
39+
}
40+
}
41+
42+
@override
43+
void reportViolations(String workingDirectory) {
44+
if (_errors.isEmpty) {
45+
return;
46+
}
47+
48+
foundError(<String>[
49+
for (final MapEntry<ResolvedUnitResult, List<(AstNode, String)>> entry in _errors.entries)
50+
for (final (AstNode node, String suggestion) in entry.value)
51+
'${locationInFile(entry.key, node, workingDirectory)}: ${node.parent}. Consider calling $suggestion instead.',
52+
'\n${bold}Typically the get* methods should be used to obtain the intrinsics of a RenderBox.$reset',
53+
]);
54+
}
55+
56+
@override
57+
String toString() => 'RenderBox subclass intrinsic calculation best practices';
58+
}
59+
60+
class _RenderBoxSubclassVisitor extends RecursiveAstVisitor<void> {
61+
final List<(AstNode, String)> violationNodes = <(AstNode, String)>[];
62+
63+
static final Map<InterfaceElement, bool> _isRenderBoxClassElementCache = <InterfaceElement, bool>{};
64+
// The cached version, call this method instead of _checkIfImplementsRenderBox.
65+
static bool _implementsRenderBox(InterfaceElement interfaceElement) {
66+
// Framework naming convention: a RenderObject subclass names have "Render" in its name.
67+
if (!interfaceElement.name.contains('Render')) {
68+
return false;
69+
}
70+
return interfaceElement.name == 'RenderBox'
71+
|| _isRenderBoxClassElementCache.putIfAbsent(interfaceElement, () => _checkIfImplementsRenderBox(interfaceElement));
72+
}
73+
74+
static bool _checkIfImplementsRenderBox(InterfaceElement element) {
75+
return element.allSupertypes.any((InterfaceType interface) => _implementsRenderBox(interface.element));
76+
}
77+
78+
// We don't care about directives, comments, or asserts.
79+
@override
80+
void visitImportDirective(ImportDirective node) { }
81+
@override
82+
void visitExportDirective(ExportDirective node) { }
83+
@override
84+
void visitComment(Comment node) { }
85+
@override
86+
void visitAssertStatement(AssertStatement node) { }
87+
88+
@override
89+
void visitClassDeclaration(ClassDeclaration node) {
90+
// Ignore the RenderBox class implementation: that's the only place the
91+
// compute* methods are supposed to be called.
92+
if (node.name.lexeme != 'RenderBox') {
93+
super.visitClassDeclaration(node);
94+
}
95+
}
96+
97+
@override
98+
void visitSimpleIdentifier(SimpleIdentifier node) {
99+
final String? correctMethodName = candidates[node.name];
100+
if (correctMethodName == null) {
101+
return;
102+
}
103+
final bool isCallingSuperImplementation = switch (node.parent) {
104+
PropertyAccess(target: SuperExpression()) ||
105+
MethodInvocation(target: SuperExpression()) => true,
106+
_ => false,
107+
};
108+
if (isCallingSuperImplementation) {
109+
return;
110+
}
111+
final Element? declaredInClassElement = node.staticElement?.declaration?.enclosingElement;
112+
if (declaredInClassElement is InterfaceElement && _implementsRenderBox(declaredInClassElement)) {
113+
violationNodes.add((node, correctMethodName));
114+
}
115+
}
116+
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// Copyright 2014 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import '../../foo/fake_render_box.dart';
6+
7+
mixin ARenderBoxMixin on RenderBox {
8+
@override
9+
void computeMaxIntrinsicWidth() { }
10+
11+
@override
12+
void computeMinIntrinsicWidth() => computeMaxIntrinsicWidth(); // BAD
13+
14+
@override
15+
void computeMinIntrinsicHeight() {
16+
final void Function() f = computeMaxIntrinsicWidth; // BAD
17+
f();
18+
}
19+
}
20+
21+
extension ARenderBoxExtension on RenderBox {
22+
void test() {
23+
computeDryBaseline(); // BAD
24+
computeDryLayout(); // BAD
25+
}
26+
}
27+
28+
class RenderBoxSubclass1 extends RenderBox {
29+
@override
30+
void computeDryLayout() {
31+
computeDistanceToActualBaseline(); // BAD
32+
}
33+
34+
@override
35+
void computeDistanceToActualBaseline() {
36+
computeMaxIntrinsicHeight(); // BAD
37+
}
38+
}
39+
40+
class RenderBoxSubclass2 extends RenderBox with ARenderBoxMixin {
41+
@override
42+
void computeMaxIntrinsicWidth() {
43+
super.computeMinIntrinsicHeight(); // OK
44+
super.computeMaxIntrinsicWidth(); // OK
45+
final void Function() f = super.computeDryBaseline; // OK
46+
f();
47+
}
48+
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Copyright 2014 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
abstract class RenderBox {
6+
void computeDryBaseline() {}
7+
void computeDryLayout() {}
8+
void computeDistanceToActualBaseline() {}
9+
void computeMaxIntrinsicHeight() {}
10+
void computeMinIntrinsicHeight() {}
11+
void computeMaxIntrinsicWidth() {}
12+
void computeMinIntrinsicWidth() {}
13+
}

dev/bots/test/analyze_test.dart

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import '../analyze.dart';
1010
import '../custom_rules/analyze.dart';
1111
import '../custom_rules/no_double_clamp.dart';
1212
import '../custom_rules/no_stop_watches.dart';
13+
import '../custom_rules/render_box_intrinsics.dart';
1314
import '../utils.dart';
1415
import 'common.dart';
1516

@@ -267,4 +268,29 @@ void main() {
267268
'╚═══════════════════════════════════════════════════════════════════════════════\n'
268269
);
269270
});
271+
272+
test('analyze.dart - RenderBox intrinsics', () async {
273+
final String result = await capture(() => analyzeWithRules(
274+
testRootPath,
275+
<AnalyzeRule>[renderBoxIntrinsicCalculation],
276+
includePaths: <String>['packages/flutter/lib'],
277+
), shouldHaveErrors: true);
278+
final String lines = <String>[
279+
'║ packages/flutter/lib/renderbox_intrinsics.dart:12: computeMaxIntrinsicWidth(). Consider calling getMaxIntrinsicWidth instead.',
280+
'║ packages/flutter/lib/renderbox_intrinsics.dart:16: f = computeMaxIntrinsicWidth. Consider calling getMaxIntrinsicWidth instead.',
281+
'║ packages/flutter/lib/renderbox_intrinsics.dart:23: computeDryBaseline(). Consider calling getDryBaseline instead.',
282+
'║ packages/flutter/lib/renderbox_intrinsics.dart:24: computeDryLayout(). Consider calling getDryLayout instead.',
283+
'║ packages/flutter/lib/renderbox_intrinsics.dart:31: computeDistanceToActualBaseline(). Consider calling getDistanceToBaseline, or getDistanceToActualBaseline instead.',
284+
'║ packages/flutter/lib/renderbox_intrinsics.dart:36: computeMaxIntrinsicHeight(). Consider calling getMaxIntrinsicHeight instead.',
285+
]
286+
.map((String line) => line.replaceAll('/', Platform.isWindows ? r'\' : '/'))
287+
.join('\n');
288+
expect(result,
289+
'╔═╡ERROR #1╞════════════════════════════════════════════════════════════════════\n'
290+
'$lines\n'
291+
'║ \n'
292+
'║ Typically the get* methods should be used to obtain the intrinsics of a RenderBox.\n'
293+
'╚═══════════════════════════════════════════════════════════════════════════════\n'
294+
);
295+
});
270296
}

examples/api/lib/widgets/slotted_render_object_widget/slotted_multi_child_render_object_widget_mixin.0.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,8 @@ class RenderDiagonal extends RenderBox
242242
@override
243243
Size computeDryLayout(BoxConstraints constraints) {
244244
const BoxConstraints childConstraints = BoxConstraints();
245-
final Size topLeftSize = _topLeft?.computeDryLayout(childConstraints) ?? Size.zero;
246-
final Size bottomRightSize = _bottomRight?.computeDryLayout(childConstraints) ?? Size.zero;
245+
final Size topLeftSize = _topLeft?.getDryLayout(childConstraints) ?? Size.zero;
246+
final Size bottomRightSize = _bottomRight?.getDryLayout(childConstraints) ?? Size.zero;
247247
return constraints.constrain(Size(
248248
topLeftSize.width + bottomRightSize.width,
249249
topLeftSize.height + bottomRightSize.height,

packages/flutter/lib/foundation.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ export 'package:meta/meta.dart' show
1717
optionalTypeArgs,
1818
protected,
1919
required,
20+
visibleForOverriding,
2021
visibleForTesting;
2122

2223
export 'src/foundation/annotations.dart';

packages/flutter/lib/src/cupertino/dialog.dart

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,11 +1178,11 @@ class _RenderCupertinoDialog extends RenderBox {
11781178
// for buttons to just over 1 button's height to make room for the content
11791179
// section.
11801180
_AlertDialogSizes performRegularLayout({required BoxConstraints constraints, required ChildLayouter layoutChild}) {
1181-
final bool hasDivider = contentSection!.getMaxIntrinsicHeight(computeMaxIntrinsicWidth(0)) > 0.0
1182-
&& actionsSection!.getMaxIntrinsicHeight(computeMaxIntrinsicWidth(0)) > 0.0;
1181+
final bool hasDivider = contentSection!.getMaxIntrinsicHeight(getMaxIntrinsicWidth(0)) > 0.0
1182+
&& actionsSection!.getMaxIntrinsicHeight(getMaxIntrinsicWidth(0)) > 0.0;
11831183
final double dividerThickness = hasDivider ? _dividerThickness : 0.0;
11841184

1185-
final double minActionsHeight = actionsSection!.getMinIntrinsicHeight(computeMaxIntrinsicWidth(0));
1185+
final double minActionsHeight = actionsSection!.getMinIntrinsicHeight(getMaxIntrinsicWidth(0));
11861186

11871187
final Size contentSize = layoutChild(
11881188
contentSection!,
@@ -2017,7 +2017,7 @@ class _RenderCupertinoDialogActions extends RenderBox
20172017
return 0.0;
20182018
} else if (isActionSheet) {
20192019
if (childCount == 1) {
2020-
return firstChild!.computeMaxIntrinsicHeight(width) + dividerThickness;
2020+
return firstChild!.getMaxIntrinsicHeight(width) + dividerThickness;
20212021
}
20222022
if (hasCancelButton && childCount < 4) {
20232023
return _computeMinIntrinsicHeightWithCancel(width);
@@ -2089,7 +2089,7 @@ class _RenderCupertinoDialogActions extends RenderBox
20892089
return 0.0;
20902090
} else if (isActionSheet) {
20912091
if (childCount == 1) {
2092-
return firstChild!.computeMaxIntrinsicHeight(width) + dividerThickness;
2092+
return firstChild!.getMaxIntrinsicHeight(width) + dividerThickness;
20932093
}
20942094
return _computeMaxIntrinsicHeightStacked(width);
20952095
} else if (childCount == 1) {
@@ -2243,7 +2243,7 @@ class _RenderCupertinoDialogActions extends RenderBox
22432243

22442244
// Our height is the accumulated height of all buttons and dividers.
22452245
return constraints.constrain(
2246-
Size(computeMaxIntrinsicWidth(0), verticalOffset),
2246+
Size(getMaxIntrinsicWidth(0), verticalOffset),
22472247
);
22482248
}
22492249
}

packages/flutter/lib/src/material/chip.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1757,7 +1757,7 @@ class _RenderChip extends RenderBox with SlottedContainerRenderObjectMixin<_Chip
17571757
}
17581758

17591759
@override
1760-
double computeMaxIntrinsicHeight(double width) => computeMinIntrinsicHeight(width);
1760+
double computeMaxIntrinsicHeight(double width) => getMinIntrinsicHeight(width);
17611761

17621762
@override
17631763
double? computeDistanceToActualBaseline(TextBaseline baseline) {

packages/flutter/lib/src/material/dropdown_menu.dart

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -885,8 +885,8 @@ class _RenderDropdownMenuBody extends RenderBox
885885
RenderBox? child = firstChild;
886886

887887
final BoxConstraints innerConstraints = BoxConstraints(
888-
maxWidth: width ?? computeMaxIntrinsicWidth(constraints.maxWidth),
889-
maxHeight: computeMaxIntrinsicHeight(constraints.maxHeight),
888+
maxWidth: width ?? getMaxIntrinsicWidth(constraints.maxWidth),
889+
maxHeight: getMaxIntrinsicHeight(constraints.maxHeight),
890890
);
891891
while (child != null) {
892892
if (child == firstChild) {
@@ -927,8 +927,8 @@ class _RenderDropdownMenuBody extends RenderBox
927927
double? maxHeight;
928928
RenderBox? child = firstChild;
929929
final BoxConstraints innerConstraints = BoxConstraints(
930-
maxWidth: width ?? computeMaxIntrinsicWidth(constraints.maxWidth),
931-
maxHeight: computeMaxIntrinsicHeight(constraints.maxHeight),
930+
maxWidth: width ?? getMaxIntrinsicWidth(constraints.maxWidth),
931+
maxHeight: getMaxIntrinsicHeight(constraints.maxHeight),
932932
);
933933

934934
while (child != null) {

0 commit comments

Comments
 (0)