computeDryLayout access size bad (#164663)

Asserts if `RenderBox.size` is accessed in `computeDryLayout` Also changes `x is RenderObject` to `x != null` when x's static type is `RenderObject?`. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
2025-03-12 09:15:06 -07:00 · 2025-03-12 09:15:06 -07:00 · 963f23e30e
commit 963f23e30e
parent a3f63a75b0
7 changed files with 140 additions and 105 deletions
--- a/packages/flutter/lib/src/rendering/animated_size.dart
+++ b/packages/flutter/lib/src/rendering/animated_size.dart
@ -242,6 +242,8 @@ class RenderAnimatedSize extends RenderAligningShiftedBox {
    return _sizeTween.evaluate(_animation);
  }

+  late Size _currentSize;
+
  @override
  void performLayout() {
    _lastValue = _controller.value;
@ -249,7 +251,7 @@ class RenderAnimatedSize extends RenderAligningShiftedBox {
    final BoxConstraints constraints = this.constraints;
    if (child == null || constraints.isTight) {
      _controller.stop();
-      size = _sizeTween.begin = _sizeTween.end = constraints.smallest;
+      size = _currentSize = _sizeTween.begin = _sizeTween.end = constraints.smallest;
      _state = RenderAnimatedSizeState.start;
      child?.layout(constraints);
      return;
@ -268,7 +270,7 @@ class RenderAnimatedSize extends RenderAligningShiftedBox {
        _layoutUnstable();
    }

-    size = constraints.constrain(_animatedSize!);
+    size = _currentSize = constraints.constrain(_animatedSize!);
    alignChild();

    if (size.width < _sizeTween.end!.width || size.height < _sizeTween.end!.height) {
@ -292,7 +294,7 @@ class RenderAnimatedSize extends RenderAligningShiftedBox {
        return constraints.constrain(childSize);
      case RenderAnimatedSizeState.stable:
        if (_sizeTween.end != childSize) {
-          return constraints.constrain(size);
+          return constraints.constrain(_currentSize);
        } else if (_controller.value == _controller.upperBound) {
          return constraints.constrain(childSize);
        }
--- a/packages/flutter/lib/src/rendering/box.dart
+++ b/packages/flutter/lib/src/rendering/box.dart
@ -2262,7 +2262,6 @@ abstract class RenderBox extends RenderObject {
            !doingRegularLayout ||
            debugDoingThisResize ||
            debugDoingThisLayout ||
-            _computingThisDryLayout ||
            RenderObject.debugActiveLayout == parent && size._canBeUsedByParent;
        assert(
          sizeAccessAllowed,
@ -2273,16 +2272,29 @@ abstract class RenderBox extends RenderObject {
          'trying to access a child\'s size, pass "parentUsesSize: true" to '
          "that child's layout() in ${objectRuntimeType(this, 'RenderBox')}.performLayout.",
        );
+        final RenderBox? renderBoxDoingDryLayout =
+            _computingThisDryLayout
+                ? this
+                : (parent is RenderBox && parent._computingThisDryLayout ? parent : null);
+
+        assert(
+          renderBoxDoingDryLayout == null,
+          'RenderBox.size accessed in '
+          '${objectRuntimeType(renderBoxDoingDryLayout, 'RenderBox')}.computeDryLayout. '
+          "The computeDryLayout method must not access the RenderBox's own size, or the size of its child, "
+          "because it's established in performLayout or performResize using different BoxConstraints.",
+        );
+
        final RenderBox? renderBoxDoingDryBaseline =
            _computingThisDryBaseline
                ? this
                : (parent is RenderBox && parent._computingThisDryBaseline ? parent : null);
        assert(
          renderBoxDoingDryBaseline == null,
+
          'RenderBox.size accessed in '
-          '${objectRuntimeType(renderBoxDoingDryBaseline, 'RenderBox')}.computeDryBaseline.'
-          'The computeDryBaseline method must not access '
-          '${renderBoxDoingDryBaseline == this ? "the RenderBox's own size" : "the size of its child"},'
+          '${objectRuntimeType(renderBoxDoingDryBaseline, 'RenderBox')}.computeDryBaseline. '
+          "The computeDryBaseline method must not access the RenderBox's own size, or the size of its child, "
          "because it's established in performLayout or performResize using different BoxConstraints.",
        );
        assert(size == _size);
@ -2740,7 +2752,7 @@ abstract class RenderBox extends RenderObject {
        } finally {
          RenderObject.debugCheckingIntrinsics = false;
        }
-        if (_debugDryLayoutCalculationValid && dryLayoutSize != size) {
+        if (_debugDryLayoutCalculationValid && dryLayoutSize != _size) {
          throw FlutterError.fromParts(<DiagnosticsNode>[
            ErrorSummary(
              'The size given to the ${objectRuntimeType(this, 'RenderBox')} class differs from the size computed by computeDryLayout.',
--- a/packages/flutter/lib/src/rendering/object.dart
+++ b/packages/flutter/lib/src/rendering/object.dart
@ -2594,7 +2594,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
  void scheduleInitialLayout() {
    assert(!_debugDisposed);
    assert(attached);
-    assert(parent is! RenderObject);
+    assert(parent == null);
    assert(!owner!._debugDoingLayout);
    assert(_relayoutBoundary == null);
    _relayoutBoundary = this;
@ -2712,7 +2712,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
    assert(!_debugDoingThisResize);
    assert(!_debugDoingThisLayout);
    final bool isRelayoutBoundary =
-        !parentUsesSize || sizedByParent || constraints.isTight || parent is! RenderObject;
+        !parentUsesSize || sizedByParent || constraints.isTight || parent == null;
    final RenderObject relayoutBoundary = isRelayoutBoundary ? this : parent!._relayoutBoundary!;
    assert(() {
      _debugCanParentUseSize = parentUsesSize;
@ -3077,8 +3077,8 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
      return;
    }
    _needsCompositingBitsUpdate = true;
-    if (parent is RenderObject) {
-      final RenderObject parent = this.parent!;
+    final RenderObject? parent = this.parent;
+    if (parent != null) {
      if (parent._needsCompositingBitsUpdate) {
        return;
      }
@ -3089,9 +3089,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
      }
    }
    // parent is fine (or there isn't one), but we are dirty
-    if (owner != null) {
-      owner!._nodesNeedingCompositingBitsUpdate.add(this);
-    }
+    owner?._nodesNeedingCompositingBitsUpdate.add(this);
  }

  late bool _needsCompositing; // initialized in the constructor
@ -3299,7 +3297,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
    assert(_layerHandle.layer != null);
    assert(!_layerHandle.layer!.attached);
    RenderObject? node = parent;
-    while (node is RenderObject) {
+    while (node != null) {
      if (node.isRepaintBoundary) {
        if (node._layerHandle.layer == null) {
          // Looks like the subtree here has never been painted. Let it handle itself.
@ -3324,7 +3322,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
  void scheduleInitialPaint(ContainerLayer rootLayer) {
    assert(rootLayer.attached);
    assert(attached);
-    assert(parent is! RenderObject);
+    assert(parent == null);
    assert(!owner!._debugDoingPaint);
    assert(isRepaintBoundary);
    assert(_layerHandle.layer == null);
@ -3342,7 +3340,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
    assert(!_debugDisposed);
    assert(rootLayer.attached);
    assert(attached);
-    assert(parent is! RenderObject);
+    assert(parent == null);
    assert(!owner!._debugDoingPaint);
    assert(isRepaintBoundary);
    assert(_layerHandle.layer != null); // use scheduleInitialPaint the first time
@ -3391,8 +3389,8 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
    }
    assert(() {
      if (_needsCompositingBitsUpdate) {
-        if (parent is RenderObject) {
-          final RenderObject parent = this.parent!;
+        final RenderObject? parent = this.parent;
+        if (parent != null) {
          bool visitedByParent = false;
          parent.visitChildren((RenderObject child) {
            if (child == this) {
@ -3669,7 +3667,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
  void scheduleInitialSemantics() {
    assert(!_debugDisposed);
    assert(attached);
-    assert(parent is! RenderObject);
+    assert(parent == null);
    assert(!owner!._debugDoingSemantics);
    assert(_semantics.parentDataDirty || !_semantics.built);
    assert(owner!._semanticsOwner != null);
@ -4005,14 +4003,12 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
    Duration duration = Duration.zero,
    Curve curve = Curves.ease,
  }) {
-    if (parent is RenderObject) {
-      parent!.showOnScreen(
-        descendant: descendant ?? this,
-        rect: rect,
-        duration: duration,
-        curve: curve,
-      );
-    }
+    parent?.showOnScreen(
+      descendant: descendant ?? this,
+      rect: rect,
+      duration: duration,
+      curve: curve,
+    );
  }

  /// Adds a debug representation of a [RenderObject] optimized for including in
--- a/packages/flutter/test/material/list_tile_test.dart
+++ b/packages/flutter/test/material/list_tile_test.dart
@ -2605,15 +2605,12 @@ void main() {
    expect(trailingOffset.dy - tileOffset.dy, topPosition);
  });

-  testWidgets('Leading/Trailing exceeding list tile width throws exception', (
-    WidgetTester tester,
-  ) async {
-    List<dynamic> exceptions = <dynamic>[];
-    FlutterExceptionHandler? oldHandler = FlutterError.onError;
-    FlutterError.onError = (FlutterErrorDetails details) {
-      exceptions.add(details.exception);
-    };
+  group('Leading/Trailing exceeding list tile width throws exception', () {
+    final List<Object> exceptions = <Object>[];
+    final FlutterExceptionHandler? oldHandler = FlutterError.onError;
+    tearDown(exceptions.clear);

+    void onError(FlutterErrorDetails details) => exceptions.add(details.exception);
    Widget buildListTile({Widget? leading, Widget? trailing}) {
      return MaterialApp(
        home: Material(
@ -2624,61 +2621,59 @@ void main() {
      );
    }

-    // Test a trailing widget that exceeds the list tile width.
-    // 16 (content padding) + 61 (leading width) + 24 (content padding) = 101.
-    // List tile width is 100 as a result, an exception should be thrown.
-    await tester.pumpWidget(buildListTile(leading: const SizedBox(width: 61)));
+    testWidgets('leading', (WidgetTester tester) async {
+      // Test a leading widget that exceeds the list tile width.
+      // 16 (content padding) + 61 (leading width) + 24 (content padding) = 101.
+      // List tile width is 100 as a result, an exception should be thrown.
+      FlutterError.onError = onError;
+      await tester.pumpWidget(buildListTile(leading: const SizedBox(width: 61)));
+      FlutterError.onError = oldHandler;

-    FlutterError.onError = oldHandler;
-    expect(exceptions.first.runtimeType, FlutterError);
-    FlutterError error = exceptions.first as FlutterError;
-    expect(error.diagnostics.length, 3);
-    expect(
-      error.diagnostics[0].toStringDeep(),
-      'Leading widget consumes the entire tile width (including\nListTile.contentPadding).\n',
-    );
-    expect(
-      error.diagnostics[1].toStringDeep(),
-      'Either resize the tile width so that the leading widget plus any\n'
-      'content padding do not exceed the tile width, or use a sized\n'
-      'widget, or consider replacing ListTile with a custom widget.\n',
-    );
-    expect(
-      error.diagnostics[2].toStringDeep(),
-      'See also:\n'
-      'https://api.flutter.dev/flutter/material/ListTile-class.html#material.ListTile.4\n',
-    );
+      final FlutterError error = exceptions.first as FlutterError;
+      expect(error.diagnostics.length, 3);
+      expect(
+        error.diagnostics[0].toStringDeep(),
+        'Leading widget consumes the entire tile width (including\nListTile.contentPadding).\n',
+      );
+      expect(
+        error.diagnostics[1].toStringDeep(),
+        'Either resize the tile width so that the leading widget plus any\n'
+        'content padding do not exceed the tile width, or use a sized\n'
+        'widget, or consider replacing ListTile with a custom widget.\n',
+      );
+      expect(
+        error.diagnostics[2].toStringDeep(),
+        'See also:\n'
+        'https://api.flutter.dev/flutter/material/ListTile-class.html#material.ListTile.4\n',
+      );
+    });

-    exceptions = <dynamic>[];
-    oldHandler = FlutterError.onError;
-    FlutterError.onError = (FlutterErrorDetails details) {
-      exceptions.add(details.exception);
-    };
+    testWidgets('trailing', (WidgetTester tester) async {
+      // Test a trailing widget that exceeds the list tile width.
+      // 16 (content padding) + 61 (trailing width) + 24 (content padding) = 101.
+      // List tile width is 100 as a result, an exception should be thrown.
+      FlutterError.onError = onError;
+      await tester.pumpWidget(buildListTile(trailing: const SizedBox(width: 61)));
+      FlutterError.onError = oldHandler;

-    // Test a trailing widget that exceeds the list tile width.
-    // 16 (content padding) + 61 (trailing width) + 24 (content padding) = 101.
-    // List tile width is 100 as a result, an exception should be thrown.
-    await tester.pumpWidget(buildListTile(trailing: const SizedBox(width: 61)));
-
-    FlutterError.onError = oldHandler;
-    expect(exceptions.first.runtimeType, FlutterError);
-    error = exceptions.first as FlutterError;
-    expect(error.diagnostics.length, 3);
-    expect(
-      error.diagnostics[0].toStringDeep(),
-      'Trailing widget consumes the entire tile width (including\nListTile.contentPadding).\n',
-    );
-    expect(
-      error.diagnostics[1].toStringDeep(),
-      'Either resize the tile width so that the trailing widget plus any\n'
-      'content padding do not exceed the tile width, or use a sized\n'
-      'widget, or consider replacing ListTile with a custom widget.\n',
-    );
-    expect(
-      error.diagnostics[2].toStringDeep(),
-      'See also:\n'
-      'https://api.flutter.dev/flutter/material/ListTile-class.html#material.ListTile.4\n',
-    );
+      final FlutterError error = exceptions.first as FlutterError;
+      expect(error.diagnostics.length, 3);
+      expect(
+        error.diagnostics[0].toStringDeep(),
+        'Trailing widget consumes the entire tile width (including\nListTile.contentPadding).\n',
+      );
+      expect(
+        error.diagnostics[1].toStringDeep(),
+        'Either resize the tile width so that the trailing widget plus any\n'
+        'content padding do not exceed the tile width, or use a sized\n'
+        'widget, or consider replacing ListTile with a custom widget.\n',
+      );
+      expect(
+        error.diagnostics[2].toStringDeep(),
+        'See also:\n'
+        'https://api.flutter.dev/flutter/material/ListTile-class.html#material.ListTile.4\n',
+      );
+    });
  });

  group('Material 2', () {
--- a/packages/flutter/test/rendering/box_test.dart
+++ b/packages/flutter/test/rendering/box_test.dart
@ -48,6 +48,18 @@ class BadBaselineRenderBox extends RenderBox {
  }
 }

+class InvalidSizeAccessInDryLayoutBox extends RenderBox {
+  @override
+  Size computeDryLayout(covariant BoxConstraints constraints) {
+    return constraints.constrain(hasSize ? size : Size.infinite);
+  }
+
+  @override
+  void performLayout() {
+    size = getDryLayout(constraints);
+  }
+}
+
 void main() {
  TestRenderingFlutterBinding.ensureInitialized();

@ -231,6 +243,31 @@ void main() {
    }
  });

+  test('Invalid size access error message', () {
+    final InvalidSizeAccessInDryLayoutBox testBox = InvalidSizeAccessInDryLayoutBox();
+
+    late FlutterErrorDetails errorDetails;
+    final FlutterExceptionHandler? oldHandler = FlutterError.onError;
+    FlutterError.onError = (FlutterErrorDetails details) {
+      errorDetails = details;
+    };
+    try {
+      testBox.layout(const BoxConstraints.tightFor(width: 100.0, height: 100.0));
+    } finally {
+      FlutterError.onError = oldHandler;
+    }
+
+    expect(
+      errorDetails.toString().replaceAll('\n', ' '),
+      contains(
+        'RenderBox.size accessed in '
+        'InvalidSizeAccessInDryLayoutBox.computeDryLayout. '
+        "The computeDryLayout method must not access the RenderBox's own size, or the size of its child, "
+        "because it's established in performLayout or performResize using different BoxConstraints.",
+      ),
+    );
+  });
+
  test('Flex and padding', () {
    final RenderBox size = RenderConstrainedBox(
      additionalConstraints: const BoxConstraints().tighten(height: 100.0),
--- a/packages/flutter/test/widgets/selectable_region_context_menu_test.dart
+++ b/packages/flutter/test/widgets/selectable_region_context_menu_test.dart
@ -186,21 +186,18 @@ class RenderSelectionSpy extends RenderProxyBox with Selectable, SelectionRegist
  final Set<VoidCallback> listeners = <VoidCallback>{};
  List<SelectionEvent> events = <SelectionEvent>[];

-  @override
-  Size get size => _size;
-  Size _size = Size.zero;
-
  @override
  List<Rect> get boundingBoxes => _boundingBoxes;
  final List<Rect> _boundingBoxes = <Rect>[];

  @override
-  Size computeDryLayout(BoxConstraints constraints) {
-    _size = Size(constraints.maxWidth, constraints.maxHeight);
-    _boundingBoxes.add(Rect.fromLTWH(0.0, 0.0, constraints.maxWidth, constraints.maxHeight));
-    return _size;
+  void performLayout() {
+    _boundingBoxes.add(Offset.zero & (size = computeDryLayout(constraints)));
  }

+  @override
+  Size computeDryLayout(BoxConstraints constraints) => constraints.biggest;
+
  @override
  void addListener(VoidCallback listener) => listeners.add(listener);

--- a/packages/flutter/test/widgets/selectable_region_test.dart
+++ b/packages/flutter/test/widgets/selectable_region_test.dart
@ -6358,18 +6358,14 @@ class RenderSelectionSpy extends RenderProxyBox with Selectable, SelectionRegist
  final Set<VoidCallback> listeners = <VoidCallback>{};
  List<SelectionEvent> events = <SelectionEvent>[];

-  @override
-  Size get size => _size;
-  Size _size = Size.zero;
-
  @override
  List<Rect> get boundingBoxes => <Rect>[paintBounds];

  @override
-  Size computeDryLayout(BoxConstraints constraints) {
-    _size = Size(constraints.maxWidth, constraints.maxHeight);
-    return _size;
-  }
+  Size computeDryLayout(BoxConstraints constraints) => constraints.biggest;
+
+  @override
+  void performLayout() => size = computeDryLayout(constraints);

  @override
  void addListener(VoidCallback listener) => listeners.add(listener);