From 1bca434cdde07f9fa03fe349cd6ee4144dd42588 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 13 Dec 2019 14:39:07 -0800 Subject: [PATCH] Check for NaN in sliver constraints, improve error messaging (#46995) --- .../flutter/lib/src/rendering/sliver.dart | 56 +++++++++---- .../flutter/test/rendering/slivers_test.dart | 79 +++++++++++++++++++ 2 files changed, 118 insertions(+), 17 deletions(-) diff --git a/packages/flutter/lib/src/rendering/sliver.dart b/packages/flutter/lib/src/rendering/sliver.dart index c336a6f8537..d92a7c131fb 100644 --- a/packages/flutter/lib/src/rendering/sliver.dart +++ b/packages/flutter/lib/src/rendering/sliver.dart @@ -416,32 +416,54 @@ class SliverConstraints extends Constraints { InformationCollector informationCollector, }) { assert(() { + bool hasErrors = false; + final StringBuffer errorMessage = StringBuffer('\n'); void verify(bool check, String message) { if (check) return; + hasErrors = true; + errorMessage.writeln(' $message'); + } + void verifyDouble(double property, String name, {bool mustBePositive = false, bool mustBeNegative = false}) { + verify(property != null, 'The "$name" is null.'); + if (property.isNaN) { + String additional = '.'; + if (mustBePositive) { + additional = ', expected greater than or equal to zero.'; + } else if (mustBeNegative) { + additional = ', expected less than or equal to zero.'; + } + verify(false, 'The "$name" is NaN$additional'); + } else if (mustBePositive) { + verify(property >= 0.0, 'The "$name" is negative.'); + } else if (mustBeNegative) { + verify(property <= 0.0, 'The "$name" is positive.'); + } + } + verify(axis != null, 'The "axis" is null.'); + verify(growthDirection != null, 'The "growthDirection" is null.'); + verifyDouble(scrollOffset, 'scrollOffset'); + verifyDouble(overlap, 'overlap'); + verifyDouble(remainingPaintExtent, 'remainingPaintExtent'); + verifyDouble(crossAxisExtent, 'crossAxisExtent'); + verifyDouble(viewportMainAxisExtent, 'viewportMainAxisExtent'); + verifyDouble(scrollOffset, 'scrollOffset', mustBePositive: true); + verify(crossAxisDirection != null, 'The "crossAxisDirection" is null.'); + verify(axisDirectionToAxis(axisDirection) != axisDirectionToAxis(crossAxisDirection), 'The "axisDirection" and the "crossAxisDirection" are along the same axis.'); + verifyDouble(viewportMainAxisExtent, 'viewportMainAxisExtent', mustBePositive: true); + verifyDouble(remainingPaintExtent, 'remainingPaintExtent', mustBePositive: true); + verifyDouble(remainingCacheExtent, 'remainingCacheExtent', mustBePositive: true); + verifyDouble(cacheOrigin, 'cacheOrigin', mustBeNegative: true); + verifyDouble(precedingScrollExtent, 'precedingScrollExtent', mustBePositive: true); + verify(isNormalized, 'The constraints are not normalized.'); // should be redundant with earlier checks + if (hasErrors) { throw FlutterError.fromParts([ - ErrorSummary('$runtimeType is not valid: $message'), + ErrorSummary('$runtimeType is not valid: $errorMessage'), if (informationCollector != null) ...informationCollector(), DiagnosticsProperty('The offending constraints were', this, style: DiagnosticsTreeStyle.errorProperty), ]); } - verify(axis != null, 'The "axis" is null.'); - verify(growthDirection != null, 'The "growthDirection" is null.'); - verify(scrollOffset != null, 'The "scrollOffset" is null.'); - verify(overlap != null, 'The "overlap" is null.'); - verify(remainingPaintExtent != null, 'The "remainingPaintExtent" is null.'); - verify(crossAxisExtent != null, 'The "crossAxisExtent" is null.'); - verify(viewportMainAxisExtent != null, 'The "viewportMainAxisExtent" is null.'); - verify(scrollOffset >= 0.0, 'The "scrollOffset" is negative.'); - verify(crossAxisExtent >= 0.0, 'The "crossAxisExtent" is negative.'); - verify(crossAxisDirection != null, 'The "crossAxisDirection" is null.'); - verify(axisDirectionToAxis(axisDirection) != axisDirectionToAxis(crossAxisDirection), 'The "axisDirection" and the "crossAxisDirection" are along the same axis.'); - verify(viewportMainAxisExtent >= 0.0, 'The "viewportMainAxisExtent" is negative.'); - verify(remainingPaintExtent >= 0.0, 'The "remainingPaintExtent" is negative.'); - verify(remainingCacheExtent >= 0.0, 'The "remainingCacheExtent" is negative.'); - verify(cacheOrigin <= 0.0, 'The "cacheOrigin" is positive.'); - verify(isNormalized, 'The constraints are not normalized.'); // should be redundant with earlier checks return true; }()); return true; diff --git a/packages/flutter/test/rendering/slivers_test.dart b/packages/flutter/test/rendering/slivers_test.dart index e426efedcf6..2dbcb412466 100644 --- a/packages/flutter/test/rendering/slivers_test.dart +++ b/packages/flutter/test/rendering/slivers_test.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/widgets.dart'; @@ -921,6 +922,84 @@ void main() { crossAxisPositions.clear(); }); }); + + test('SliverConstraints check for NaN on all double properties', () { + const SliverConstraints constraints = SliverConstraints( + axisDirection: AxisDirection.down, + cacheOrigin: double.nan, + crossAxisDirection: AxisDirection.left, + crossAxisExtent: double.nan, + growthDirection: GrowthDirection.forward, + overlap: double.nan, + precedingScrollExtent: double.nan, + remainingCacheExtent: double.nan, + remainingPaintExtent: double.nan, + scrollOffset: double.nan, + userScrollDirection: ScrollDirection.idle, + viewportMainAxisExtent: double.nan, + ); + bool threw = false; + try { + constraints.debugAssertIsValid(); + } on FlutterError catch (error) { + expect( + error.message, + 'SliverConstraints is not valid:\n' + ' The "scrollOffset" is NaN.\n' + ' The "overlap" is NaN.\n' + ' The "remainingPaintExtent" is NaN.\n' + ' The "crossAxisExtent" is NaN.\n' + ' The "viewportMainAxisExtent" is NaN.\n' + ' The "scrollOffset" is NaN, expected greater than or equal to zero.\n' + ' The "viewportMainAxisExtent" is NaN, expected greater than or equal to zero.\n' + ' The "remainingPaintExtent" is NaN, expected greater than or equal to zero.\n' + ' The "remainingCacheExtent" is NaN, expected greater than or equal to zero.\n' + ' The "cacheOrigin" is NaN, expected less than or equal to zero.\n' + ' The "precedingScrollExtent" is NaN, expected greater than or equal to zero.\n' + ' The constraints are not normalized.\n' + 'The offending constraints were:\n' + ' SliverConstraints(AxisDirection.down, GrowthDirection.forward, ScrollDirection.idle, scrollOffset: NaN, remainingPaintExtent: NaN, overlap: NaN, crossAxisExtent: NaN, crossAxisDirection: AxisDirection.left, viewportMainAxisExtent: NaN, remainingCacheExtent: NaN cacheOrigin: NaN )', + ); + threw = true; + } + expect(threw, true); + }); + + test('SliverConstraints check for sign on relevant double properties', () { + const SliverConstraints constraints = SliverConstraints( + axisDirection: AxisDirection.down, + cacheOrigin: 1.0, + crossAxisDirection: AxisDirection.left, + crossAxisExtent: 0.0, + growthDirection: GrowthDirection.forward, + overlap: 0.0, + precedingScrollExtent: -1.0, + remainingCacheExtent: -1.0, + remainingPaintExtent: -1.0, + scrollOffset: -1.0, + userScrollDirection: ScrollDirection.idle, + viewportMainAxisExtent: 0.0, + ); + bool threw = false; + try { + constraints.debugAssertIsValid(); + } on FlutterError catch (error) { + expect( + error.message, + 'SliverConstraints is not valid:\n' + ' The "scrollOffset" is negative.\n' + ' The "remainingPaintExtent" is negative.\n' + ' The "remainingCacheExtent" is negative.\n' + ' The "cacheOrigin" is positive.\n' + ' The "precedingScrollExtent" is negative.\n' + ' The constraints are not normalized.\n' + 'The offending constraints were:\n' + ' SliverConstraints(AxisDirection.down, GrowthDirection.forward, ScrollDirection.idle, scrollOffset: -1.0, remainingPaintExtent: -1.0, crossAxisExtent: 0.0, crossAxisDirection: AxisDirection.left, viewportMainAxisExtent: 0.0, remainingCacheExtent: -1.0 cacheOrigin: 1.0 )', + ); + threw = true; + } + expect(threw, true); + }); } class _DummyHitTestTarget implements HitTestTarget {