Replace RenderBox.compute* with RenderBox.get* and add @visibleForOverriding (#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.
This commit is contained in:
LongCatIsLooong 2024-03-21 19:44:55 -07:00 committed by GitHub
parent 0f685f88c6
commit d755bc222b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 246 additions and 37 deletions

View file

@ -21,6 +21,7 @@ import 'custom_rules/analyze.dart';
import 'custom_rules/avoid_future_catcherror.dart';
import 'custom_rules/no_double_clamp.dart';
import 'custom_rules/no_stop_watches.dart';
import 'custom_rules/render_box_intrinsics.dart';
import 'run_command.dart';
import 'utils.dart';
@ -174,7 +175,7 @@ Future<void> run(List<String> arguments) async {
// Only run the private lints when the code is free of type errors. The
// lints are easier to write when they can assume, for example, there is no
// inheritance cycles.
final List<AnalyzeRule> rules = <AnalyzeRule>[noDoubleClamp, noStopwatches];
final List<AnalyzeRule> rules = <AnalyzeRule>[noDoubleClamp, noStopwatches, renderBoxIntrinsicCalculation];
final String ruleNames = rules.map((AnalyzeRule rule) => '\n * $rule').join();
printProgress('Analyzing code in the framework with the following rules:$ruleNames');
await analyzeWithRules(flutterRoot, rules,

View file

@ -0,0 +1,116 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import '../utils.dart';
import 'analyze.dart';
/// Verify that no RenderBox subclasses call compute* instead of get* for
/// computing the intrinsic dimensions. The full list of RenderBox intrinsic
/// methods checked by this rule is listed in [candidates].
final AnalyzeRule renderBoxIntrinsicCalculation = _RenderBoxIntrinsicCalculationRule();
const Map<String, String> candidates = <String, String> {
'computeDryBaseline': 'getDryBaseline',
'computeDryLayout': 'getDryLayout',
'computeDistanceToActualBaseline': 'getDistanceToBaseline, or getDistanceToActualBaseline',
'computeMaxIntrinsicHeight': 'getMaxIntrinsicHeight',
'computeMinIntrinsicHeight': 'getMinIntrinsicHeight',
'computeMaxIntrinsicWidth': 'getMaxIntrinsicWidth',
'computeMinIntrinsicWidth': 'getMinIntrinsicWidth'
};
class _RenderBoxIntrinsicCalculationRule implements AnalyzeRule {
final Map<ResolvedUnitResult, List<(AstNode, String)>> _errors = <ResolvedUnitResult, List<(AstNode, String)>>{};
@override
void applyTo(ResolvedUnitResult unit) {
final _RenderBoxSubclassVisitor visitor = _RenderBoxSubclassVisitor();
unit.unit.visitChildren(visitor);
final List<(AstNode, String)> violationsInUnit = visitor.violationNodes;
if (violationsInUnit.isNotEmpty) {
_errors.putIfAbsent(unit, () => <(AstNode, String)>[]).addAll(violationsInUnit);
}
}
@override
void reportViolations(String workingDirectory) {
if (_errors.isEmpty) {
return;
}
foundError(<String>[
for (final MapEntry<ResolvedUnitResult, List<(AstNode, String)>> entry in _errors.entries)
for (final (AstNode node, String suggestion) in entry.value)
'${locationInFile(entry.key, node, workingDirectory)}: ${node.parent}. Consider calling $suggestion instead.',
'\n${bold}Typically the get* methods should be used to obtain the intrinsics of a RenderBox.$reset',
]);
}
@override
String toString() => 'RenderBox subclass intrinsic calculation best practices';
}
class _RenderBoxSubclassVisitor extends RecursiveAstVisitor<void> {
final List<(AstNode, String)> violationNodes = <(AstNode, String)>[];
static final Map<InterfaceElement, bool> _isRenderBoxClassElementCache = <InterfaceElement, bool>{};
// The cached version, call this method instead of _checkIfImplementsRenderBox.
static bool _implementsRenderBox(InterfaceElement interfaceElement) {
// Framework naming convention: a RenderObject subclass names have "Render" in its name.
if (!interfaceElement.name.contains('Render')) {
return false;
}
return interfaceElement.name == 'RenderBox'
|| _isRenderBoxClassElementCache.putIfAbsent(interfaceElement, () => _checkIfImplementsRenderBox(interfaceElement));
}
static bool _checkIfImplementsRenderBox(InterfaceElement element) {
return element.allSupertypes.any((InterfaceType interface) => _implementsRenderBox(interface.element));
}
// We don't care about directives, comments, or asserts.
@override
void visitImportDirective(ImportDirective node) { }
@override
void visitExportDirective(ExportDirective node) { }
@override
void visitComment(Comment node) { }
@override
void visitAssertStatement(AssertStatement node) { }
@override
void visitClassDeclaration(ClassDeclaration node) {
// Ignore the RenderBox class implementation: that's the only place the
// compute* methods are supposed to be called.
if (node.name.lexeme != 'RenderBox') {
super.visitClassDeclaration(node);
}
}
@override
void visitSimpleIdentifier(SimpleIdentifier node) {
final String? correctMethodName = candidates[node.name];
if (correctMethodName == null) {
return;
}
final bool isCallingSuperImplementation = switch (node.parent) {
PropertyAccess(target: SuperExpression()) ||
MethodInvocation(target: SuperExpression()) => true,
_ => false,
};
if (isCallingSuperImplementation) {
return;
}
final Element? declaredInClassElement = node.staticElement?.declaration?.enclosingElement;
if (declaredInClassElement is InterfaceElement && _implementsRenderBox(declaredInClassElement)) {
violationNodes.add((node, correctMethodName));
}
}
}

View file

@ -0,0 +1,48 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
import '../../foo/fake_render_box.dart';
mixin ARenderBoxMixin on RenderBox {
@override
void computeMaxIntrinsicWidth() { }
@override
void computeMinIntrinsicWidth() => computeMaxIntrinsicWidth(); // BAD
@override
void computeMinIntrinsicHeight() {
final void Function() f = computeMaxIntrinsicWidth; // BAD
f();
}
}
extension ARenderBoxExtension on RenderBox {
void test() {
computeDryBaseline(); // BAD
computeDryLayout(); // BAD
}
}
class RenderBoxSubclass1 extends RenderBox {
@override
void computeDryLayout() {
computeDistanceToActualBaseline(); // BAD
}
@override
void computeDistanceToActualBaseline() {
computeMaxIntrinsicHeight(); // BAD
}
}
class RenderBoxSubclass2 extends RenderBox with ARenderBoxMixin {
@override
void computeMaxIntrinsicWidth() {
super.computeMinIntrinsicHeight(); // OK
super.computeMaxIntrinsicWidth(); // OK
final void Function() f = super.computeDryBaseline; // OK
f();
}
}

View file

@ -0,0 +1,13 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
abstract class RenderBox {
void computeDryBaseline() {}
void computeDryLayout() {}
void computeDistanceToActualBaseline() {}
void computeMaxIntrinsicHeight() {}
void computeMinIntrinsicHeight() {}
void computeMaxIntrinsicWidth() {}
void computeMinIntrinsicWidth() {}
}

View file

@ -10,6 +10,7 @@ import '../analyze.dart';
import '../custom_rules/analyze.dart';
import '../custom_rules/no_double_clamp.dart';
import '../custom_rules/no_stop_watches.dart';
import '../custom_rules/render_box_intrinsics.dart';
import '../utils.dart';
import 'common.dart';
@ -267,4 +268,29 @@ void main() {
'╚═══════════════════════════════════════════════════════════════════════════════\n'
);
});
test('analyze.dart - RenderBox intrinsics', () async {
final String result = await capture(() => analyzeWithRules(
testRootPath,
<AnalyzeRule>[renderBoxIntrinsicCalculation],
includePaths: <String>['packages/flutter/lib'],
), shouldHaveErrors: true);
final String lines = <String>[
'║ packages/flutter/lib/renderbox_intrinsics.dart:12: computeMaxIntrinsicWidth(). Consider calling getMaxIntrinsicWidth instead.',
'║ packages/flutter/lib/renderbox_intrinsics.dart:16: f = computeMaxIntrinsicWidth. Consider calling getMaxIntrinsicWidth instead.',
'║ packages/flutter/lib/renderbox_intrinsics.dart:23: computeDryBaseline(). Consider calling getDryBaseline instead.',
'║ packages/flutter/lib/renderbox_intrinsics.dart:24: computeDryLayout(). Consider calling getDryLayout instead.',
'║ packages/flutter/lib/renderbox_intrinsics.dart:31: computeDistanceToActualBaseline(). Consider calling getDistanceToBaseline, or getDistanceToActualBaseline instead.',
'║ packages/flutter/lib/renderbox_intrinsics.dart:36: computeMaxIntrinsicHeight(). Consider calling getMaxIntrinsicHeight instead.',
]
.map((String line) => line.replaceAll('/', Platform.isWindows ? r'\' : '/'))
.join('\n');
expect(result,
'╔═╡ERROR #1╞════════════════════════════════════════════════════════════════════\n'
'$lines\n'
'\n'
'║ Typically the get* methods should be used to obtain the intrinsics of a RenderBox.\n'
'╚═══════════════════════════════════════════════════════════════════════════════\n'
);
});
}

View file

@ -242,8 +242,8 @@ class RenderDiagonal extends RenderBox
@override
Size computeDryLayout(BoxConstraints constraints) {
const BoxConstraints childConstraints = BoxConstraints();
final Size topLeftSize = _topLeft?.computeDryLayout(childConstraints) ?? Size.zero;
final Size bottomRightSize = _bottomRight?.computeDryLayout(childConstraints) ?? Size.zero;
final Size topLeftSize = _topLeft?.getDryLayout(childConstraints) ?? Size.zero;
final Size bottomRightSize = _bottomRight?.getDryLayout(childConstraints) ?? Size.zero;
return constraints.constrain(Size(
topLeftSize.width + bottomRightSize.width,
topLeftSize.height + bottomRightSize.height,

View file

@ -17,6 +17,7 @@ export 'package:meta/meta.dart' show
optionalTypeArgs,
protected,
required,
visibleForOverriding,
visibleForTesting;
export 'src/foundation/annotations.dart';

View file

@ -1178,11 +1178,11 @@ class _RenderCupertinoDialog extends RenderBox {
// for buttons to just over 1 button's height to make room for the content
// section.
_AlertDialogSizes performRegularLayout({required BoxConstraints constraints, required ChildLayouter layoutChild}) {
final bool hasDivider = contentSection!.getMaxIntrinsicHeight(computeMaxIntrinsicWidth(0)) > 0.0
&& actionsSection!.getMaxIntrinsicHeight(computeMaxIntrinsicWidth(0)) > 0.0;
final bool hasDivider = contentSection!.getMaxIntrinsicHeight(getMaxIntrinsicWidth(0)) > 0.0
&& actionsSection!.getMaxIntrinsicHeight(getMaxIntrinsicWidth(0)) > 0.0;
final double dividerThickness = hasDivider ? _dividerThickness : 0.0;
final double minActionsHeight = actionsSection!.getMinIntrinsicHeight(computeMaxIntrinsicWidth(0));
final double minActionsHeight = actionsSection!.getMinIntrinsicHeight(getMaxIntrinsicWidth(0));
final Size contentSize = layoutChild(
contentSection!,
@ -2017,7 +2017,7 @@ class _RenderCupertinoDialogActions extends RenderBox
return 0.0;
} else if (isActionSheet) {
if (childCount == 1) {
return firstChild!.computeMaxIntrinsicHeight(width) + dividerThickness;
return firstChild!.getMaxIntrinsicHeight(width) + dividerThickness;
}
if (hasCancelButton && childCount < 4) {
return _computeMinIntrinsicHeightWithCancel(width);
@ -2089,7 +2089,7 @@ class _RenderCupertinoDialogActions extends RenderBox
return 0.0;
} else if (isActionSheet) {
if (childCount == 1) {
return firstChild!.computeMaxIntrinsicHeight(width) + dividerThickness;
return firstChild!.getMaxIntrinsicHeight(width) + dividerThickness;
}
return _computeMaxIntrinsicHeightStacked(width);
} else if (childCount == 1) {
@ -2243,7 +2243,7 @@ class _RenderCupertinoDialogActions extends RenderBox
// Our height is the accumulated height of all buttons and dividers.
return constraints.constrain(
Size(computeMaxIntrinsicWidth(0), verticalOffset),
Size(getMaxIntrinsicWidth(0), verticalOffset),
);
}
}

View file

@ -1757,7 +1757,7 @@ class _RenderChip extends RenderBox with SlottedContainerRenderObjectMixin<_Chip
}
@override
double computeMaxIntrinsicHeight(double width) => computeMinIntrinsicHeight(width);
double computeMaxIntrinsicHeight(double width) => getMinIntrinsicHeight(width);
@override
double? computeDistanceToActualBaseline(TextBaseline baseline) {

View file

@ -885,8 +885,8 @@ class _RenderDropdownMenuBody extends RenderBox
RenderBox? child = firstChild;
final BoxConstraints innerConstraints = BoxConstraints(
maxWidth: width ?? computeMaxIntrinsicWidth(constraints.maxWidth),
maxHeight: computeMaxIntrinsicHeight(constraints.maxHeight),
maxWidth: width ?? getMaxIntrinsicWidth(constraints.maxWidth),
maxHeight: getMaxIntrinsicHeight(constraints.maxHeight),
);
while (child != null) {
if (child == firstChild) {
@ -927,8 +927,8 @@ class _RenderDropdownMenuBody extends RenderBox
double? maxHeight;
RenderBox? child = firstChild;
final BoxConstraints innerConstraints = BoxConstraints(
maxWidth: width ?? computeMaxIntrinsicWidth(constraints.maxWidth),
maxHeight: computeMaxIntrinsicHeight(constraints.maxHeight),
maxWidth: width ?? getMaxIntrinsicWidth(constraints.maxWidth),
maxHeight: getMaxIntrinsicHeight(constraints.maxHeight),
);
while (child != null) {

View file

@ -1317,7 +1317,7 @@ class _RenderDecoration extends RenderBox with SlottedContainerRenderObjectMixin
@override
double computeMaxIntrinsicHeight(double width) {
return computeMinIntrinsicHeight(width);
return getMinIntrinsicHeight(width);
}
@override
@ -1325,7 +1325,7 @@ class _RenderDecoration extends RenderBox with SlottedContainerRenderObjectMixin
final RenderBox? input = this.input;
return input == null
? 0.0
: _boxParentData(input).offset.dy + (input.computeDistanceToActualBaseline(baseline) ?? 0.0);
: _boxParentData(input).offset.dy + (input.getDistanceToActualBaseline(baseline) ?? 0.0);
}
// Records where the label was painted.

View file

@ -1273,7 +1273,7 @@ class _RenderListTile extends RenderBox with SlottedContainerRenderObjectMixin<_
@override
double computeMaxIntrinsicHeight(double width) {
return computeMinIntrinsicHeight(width);
return getMinIntrinsicHeight(width);
}
@override

View file

@ -1177,7 +1177,7 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox {
@override
double? computeDistanceToActualBaseline(TextBaseline baseline) {
// The baseline of this widget is the baseline of its child
final BaselineOffset childOffset = BaselineOffset(child?.computeDistanceToActualBaseline(baseline));
final BaselineOffset childOffset = BaselineOffset(child?.getDistanceToActualBaseline(baseline));
return switch (direction) {
Axis.horizontal => childOffset + borderSide.width,
Axis.vertical => childOffset + leadingBorderSide.width,

View file

@ -1771,6 +1771,7 @@ abstract class RenderBox extends RenderObject {
/// See also:
///
/// * [computeMinIntrinsicWidth], which has usage examples.
@visibleForOverriding
@protected
double computeMaxIntrinsicWidth(double height) {
return 0.0;
@ -1845,6 +1846,7 @@ abstract class RenderBox extends RenderObject {
/// * [computeMinIntrinsicWidth], which has usage examples.
/// * [computeMaxIntrinsicHeight], which computes the smallest height beyond
/// which increasing the height never decreases the preferred width.
@visibleForOverriding
@protected
double computeMinIntrinsicHeight(double width) {
return 0.0;
@ -1922,6 +1924,7 @@ abstract class RenderBox extends RenderObject {
/// See also:
///
/// * [computeMinIntrinsicWidth], which has usage examples.
@visibleForOverriding
@protected
double computeMaxIntrinsicHeight(double width) {
return 0.0;
@ -1995,6 +1998,7 @@ abstract class RenderBox extends RenderObject {
/// return a valid answer. In such cases, the function should call
/// [debugCannotComputeDryLayout] from within an assert and return a dummy
/// value of `const Size(0, 0)`.
@visibleForOverriding
@protected
Size computeDryLayout(covariant BoxConstraints constraints) {
assert(debugCannotComputeDryLayout(
@ -2093,6 +2097,7 @@ abstract class RenderBox extends RenderObject {
/// updating the widget tree, violating the "dry" contract. In such cases the
/// [RenderBox] must call [debugCannotComputeDryLayout] in an assert, and
/// return a dummy baseline offset value (such as `null`).
@visibleForOverriding
@protected
double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) {
assert(debugCannotComputeDryLayout(
@ -2405,6 +2410,7 @@ abstract class RenderBox extends RenderObject {
/// computation, call [getDistanceToActualBaseline] on the child (not
/// [computeDistanceToActualBaseline], the internal implementation, and not
/// [getDistanceToBaseline], the public entry point for this API).
@visibleForOverriding
@protected
double? computeDistanceToActualBaseline(TextBaseline baseline) {
assert(_debugDoingBaseline, 'Please see the documentation for computeDistanceToActualBaseline for the required calling conventions of this method.');

View file

@ -1922,7 +1922,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
}
@override
double computeMinIntrinsicHeight(double width) => computeMaxIntrinsicHeight(width);
double computeMinIntrinsicHeight(double width) => getMaxIntrinsicHeight(width);
@override
double computeMaxIntrinsicHeight(double width) {

View file

@ -563,10 +563,8 @@ class RenderAspectRatio extends RenderProxyBox {
@override
void performLayout() {
size = computeDryLayout(constraints);
if (child != null) {
child!.layout(BoxConstraints.tight(size));
}
size = getDryLayout(constraints);
child?.layout(BoxConstraints.tight(size));
}
@override
@ -661,7 +659,7 @@ class RenderIntrinsicWidth extends RenderProxyBox {
@override
double computeMinIntrinsicWidth(double height) {
return computeMaxIntrinsicWidth(height);
return getMaxIntrinsicWidth(height);
}
@override
@ -679,7 +677,7 @@ class RenderIntrinsicWidth extends RenderProxyBox {
return 0.0;
}
if (!width.isFinite) {
width = computeMaxIntrinsicWidth(double.infinity);
width = getMaxIntrinsicWidth(double.infinity);
}
assert(width.isFinite);
final double height = child!.getMinIntrinsicHeight(width);
@ -692,7 +690,7 @@ class RenderIntrinsicWidth extends RenderProxyBox {
return 0.0;
}
if (!width.isFinite) {
width = computeMaxIntrinsicWidth(double.infinity);
width = getMaxIntrinsicWidth(double.infinity);
}
assert(width.isFinite);
final double height = child!.getMaxIntrinsicHeight(width);
@ -802,7 +800,7 @@ class RenderIntrinsicHeight extends RenderProxyBox {
@override
double computeMinIntrinsicHeight(double width) {
return computeMaxIntrinsicHeight(width);
return getMaxIntrinsicHeight(width);
}
Size _computeSize({required ChildLayouter layoutChild, required BoxConstraints constraints}) {

View file

@ -800,7 +800,7 @@ class RenderTable extends RenderBox {
@override
double computeMaxIntrinsicHeight(double width) {
return computeMinIntrinsicHeight(width);
return getMinIntrinsicHeight(width);
}
double? _baselineDistance;

View file

@ -396,7 +396,7 @@ class RenderWrap extends RenderBox
}
return width;
case Axis.vertical:
return computeDryLayout(BoxConstraints(maxHeight: height)).width;
return getDryLayout(BoxConstraints(maxHeight: height)).width;
}
}
@ -412,7 +412,7 @@ class RenderWrap extends RenderBox
}
return width;
case Axis.vertical:
return computeDryLayout(BoxConstraints(maxHeight: height)).width;
return getDryLayout(BoxConstraints(maxHeight: height)).width;
}
}
@ -420,7 +420,7 @@ class RenderWrap extends RenderBox
double computeMinIntrinsicHeight(double width) {
switch (direction) {
case Axis.horizontal:
return computeDryLayout(BoxConstraints(maxWidth: width)).height;
return getDryLayout(BoxConstraints(maxWidth: width)).height;
case Axis.vertical:
double height = 0.0;
RenderBox? child = firstChild;
@ -436,7 +436,7 @@ class RenderWrap extends RenderBox
double computeMaxIntrinsicHeight(double width) {
switch (direction) {
case Axis.horizontal:
return computeDryLayout(BoxConstraints(maxWidth: width)).height;
return getDryLayout(BoxConstraints(maxWidth: width)).height;
case Axis.vertical:
double height = 0.0;
RenderBox? child = firstChild;

View file

@ -353,22 +353,22 @@ class _RenderScaledInlineWidget extends RenderBox with RenderObjectWithChildMixi
@override
double computeMaxIntrinsicHeight(double width) {
return (child?.computeMaxIntrinsicHeight(width / scale) ?? 0.0) * scale;
return (child?.getMaxIntrinsicHeight(width / scale) ?? 0.0) * scale;
}
@override
double computeMaxIntrinsicWidth(double height) {
return (child?.computeMaxIntrinsicWidth(height / scale) ?? 0.0) * scale;
return (child?.getMaxIntrinsicWidth(height / scale) ?? 0.0) * scale;
}
@override
double computeMinIntrinsicHeight(double width) {
return (child?.computeMinIntrinsicHeight(width / scale) ?? 0.0) * scale;
return (child?.getMinIntrinsicHeight(width / scale) ?? 0.0) * scale;
}
@override
double computeMinIntrinsicWidth(double height) {
return (child?.computeMinIntrinsicWidth(height / scale) ?? 0.0) * scale;
return (child?.getMinIntrinsicWidth(height / scale) ?? 0.0) * scale;
}
@override
@ -382,7 +382,7 @@ class _RenderScaledInlineWidget extends RenderBox with RenderObjectWithChildMixi
@override
Size computeDryLayout(BoxConstraints constraints) {
assert(!constraints.hasBoundedHeight);
final Size unscaledSize = child?.computeDryLayout(BoxConstraints(maxWidth: constraints.maxWidth / scale)) ?? Size.zero;
final Size unscaledSize = child?.getDryLayout(BoxConstraints(maxWidth: constraints.maxWidth / scale)) ?? Size.zero;
return constraints.constrain(unscaledSize * scale);
}