Report GlobalKey duplicates (#8593)

packages/flutter/lib/src/foundation/assertions.dart

@@ -102,6 +102,42 @@ class FlutterErrorDetails {
   /// error to the console on end-user devices, while still allowing a custom
   /// error handler to see the errors even in release builds.
   final bool silent;
+
+  /// Converts the [exception] to a string.
+  ///
+  /// This applies some additional logic to 
+  String exceptionAsString() {
+    String longMessage;
+    if (exception is AssertionError) {
+      // Regular _AssertionErrors thrown by assert() put the message last, after
+      // some code snippets. This leads to ugly messages. To avoid this, we move
+      // the assertion message up to before the code snippets, separated by a
+      // newline, if we recognise that format is being used.
+      final String message = exception.message;
+      final String fullMessage = exception.toString();
+      if (message is String && message != fullMessage) {
+        if (fullMessage.length > message.length) {
+          final int position = fullMessage.lastIndexOf(message);
+          if (position == fullMessage.length - message.length &&
+              position > 2 &&
+              fullMessage.substring(position - 2, position) == ': ') {
+            longMessage = '${message.trimRight()}\n${fullMessage.substring(0, position - 2)}';
+          }
+        }
+      }
+      longMessage ??= fullMessage;
+    } else if (exception is String) {
+      longMessage = exception;
+    } else if (exception is Error || exception is Exception) {
+      longMessage = exception.toString();
+    } else {
+      longMessage = '  ${exception.toString()}';
+    }
+    longMessage = longMessage.trimRight();
+    if (longMessage.isEmpty)
+      longMessage = '  <no message available>';
+    return longMessage;
+  }
 }
 
 /// Error class used to report Flutter-specific assertion failures and
@@ -207,8 +243,14 @@ class FlutterError extends AssertionError {
         } else {
           errorName = '${details.exception.runtimeType} object';
         }
-        debugPrint('The following $errorName was $verb:', wrapWidth: _kWrapWidth);
-        debugPrint('${details.exception}', wrapWidth: _kWrapWidth);
+        // Many exception classes put their type at the head of their message.
+        // This is redundant with the way we display exceptions, so attempt to
+        // strip out that header when we see it.
+        final String prefix = '${details.exception.runtimeType}: ';
+        String message = details.exceptionAsString();
+        if (message.startsWith(prefix))
+          message = message.substring(prefix.length);
+        debugPrint('The following $errorName was $verb:\n$message', wrapWidth: _kWrapWidth);
       }
       Iterable<String> stackLines = (details.stack != null) ? details.stack.toString().trimRight().split('\n') : null;
       if ((details.exception is AssertionError) && (details.exception is! FlutterError)) {
@@ -216,6 +258,7 @@ class FlutterError extends AssertionError {
         if (stackLines != null) {
           final List<String> stackList = stackLines.take(2).toList();
           if (stackList.length >= 2) {
+            // TODO(ianh): This has bitrotted and is no longer matching. https://github.com/flutter/flutter/issues/4021
             final RegExp throwPattern = new RegExp(r'^#0 +_AssertionError._throwNew \(dart:.+\)$');
             final RegExp assertPattern = new RegExp(r'^#1 +[^(]+ \((.+?):([0-9]+)(?::[0-9]+)?\)$');
             if (throwPattern.hasMatch(stackList[0])) {
@@ -249,11 +292,11 @@ class FlutterError extends AssertionError {
       if (details.informationCollector != null) {
         final StringBuffer information = new StringBuffer();
         details.informationCollector(information);
-        debugPrint('\n$information', wrapWidth: _kWrapWidth);
+        debugPrint('\n${information.toString().trimRight()}', wrapWidth: _kWrapWidth);
       }
       debugPrint(footer);
     } else {
-      debugPrint('Another exception was thrown: ${details.exception.toString().split("\n")[0]}');
+      debugPrint('Another exception was thrown: ${details.exceptionAsString().split("\n")[0].trimLeft()}');
     }
     _errorCount += 1;
   }

packages/flutter/lib/src/rendering/object.dart

@@ -2668,9 +2668,9 @@ abstract class ContainerRenderObjectMixin<ChildType extends RenderObject, Parent
   /// If `after` is null, then this inserts the child at the start of the list,
   /// and the child becomes the new [firstChild].
   void insert(ChildType child, { ChildType after }) {
-    assert(child != this);
-    assert(after != this);
-    assert(child != after);
+    assert(child != this, 'A RenderObject cannot be inserted into itself.');
+    assert(after != this, 'A RenderObject cannot simultaneously be both the parent and the sibling of another RenderObject.');
+    assert(child != after, 'A RenderObject cannot be inserted after itself.');
     assert(child != _firstChild);
     assert(child != _lastChild);
     adoptChild(child);

packages/flutter/test/widgets/global_keys_duplicated_test.dart

+// Copyright 2015 The Chromium 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:flutter_test/flutter_test.dart';
+import 'package:flutter/widgets.dart';
+
+// There's also some duplicate GlobalKey tests in the framework_test.dart file.
+
+void main() {
+  testWidgets('GlobalKey children of one node', (WidgetTester tester) async {
+    // This is actually a test of the regular duplicate key logic, which
+    // happens before the duplicate GlobalKey logic.
+    await tester.pumpWidget(new Row(children: <Widget>[
+      new Container(key: new GlobalObjectKey(0)),
+      new Container(key: new GlobalObjectKey(0)),
+    ]));
+    final dynamic error = tester.takeException();
+    expect(error, isFlutterError);
+    expect(error.toString(), startsWith('Duplicate keys found.\n'));
+    expect(error.toString(), contains('Row'));
+    expect(error.toString(), contains('[GlobalObjectKey int#${0.hashCode}]'));
+  });
+
+  testWidgets('GlobalKey children of two nodes', (WidgetTester tester) async {
+    await tester.pumpWidget(new Row(children: <Widget>[
+      new Container(child: new Container(key: new GlobalObjectKey(0))),
+      new Container(child: new Container(key: new GlobalObjectKey(0))),
+    ]));
+    final dynamic error = tester.takeException();
+    expect(error, isFlutterError);
+    expect(error.toString(), startsWith('Multiple widgets used the same GlobalKey.\n'));
+    expect(error.toString(), contains('different widgets that both had the following description'));
+    expect(error.toString(), contains('Container'));
+    expect(error.toString(), contains('[GlobalObjectKey int#${0.hashCode}]'));
+    expect(error.toString(), endsWith('\nA GlobalKey can only be specified on one widget at a time in the widget tree.'));
+  });
+
+  testWidgets('GlobalKey children of two different nodes', (WidgetTester tester) async {
+    await tester.pumpWidget(new Row(children: <Widget>[
+      new Container(child: new Container(key: new GlobalObjectKey(0))),
+      new Container(key: new Key('x'), child: new Container(key: new GlobalObjectKey(0))),
+    ]));
+    final dynamic error = tester.takeException();
+    expect(error, isFlutterError);
+    expect(error.toString(), startsWith('Multiple widgets used the same GlobalKey.\n'));
+    expect(error.toString(), isNot(contains('different widgets that both had the following description')));
+    expect(error.toString(), contains('Container()'));
+    expect(error.toString(), contains('Container([<\'x\'>])'));
+    expect(error.toString(), contains('[GlobalObjectKey int#${0.hashCode}]'));
+    expect(error.toString(), endsWith('\nA GlobalKey can only be specified on one widget at a time in the widget tree.'));
+  });
+
+  testWidgets('GlobalKey children of two nodes', (WidgetTester tester) async {
+    StateSetter nestedSetState;
+    bool flag = false;
+    await tester.pumpWidget(new Row(children: <Widget>[
+      new Container(child: new Container(key: new GlobalObjectKey(0))),
+      new Container(child: new StatefulBuilder(
+        builder: (BuildContext context, StateSetter setState) {
+          nestedSetState = setState;
+          if (flag)
+            return new Container(key: new GlobalObjectKey(0));
+          return new Container();
+        },
+      )),
+    ]));
+    nestedSetState(() { flag = true; });
+    await tester.pump();
+    final dynamic error = tester.takeException();
+    expect(error.toString(), startsWith('Duplicate GlobalKey detected in widget tree.\n'));
+    expect(error.toString(), contains('The following GlobalKey was specified multiple times'));
+    // The following line is verifying the grammar is correct in this common case.
+    // We should probably also verify the three other combinations that can be generated...
+    expect(error.toString(), contains('This was determined by noticing that after the widget with the above global key was moved out of its previous parent, that previous parent never updated during this frame, meaning that it either did not update at all or updated before the widget was moved, in either case implying that it still thinks that it should have a child with that global key.'));
+    expect(error.toString(), contains('[GlobalObjectKey int#0]'));
+    expect(error.toString(), contains('Container()'));
+    expect(error.toString(), endsWith('\nA GlobalKey can only be specified on one widget at a time in the widget tree.'));
+    expect(error, isFlutterError);
+  });
+}

packages/flutter/test/widgets/duplicate_key_test.dart → packages/flutter/test/widgets/global_keys_moving_test.dart

@@ -22,7 +22,7 @@ class StatefulLeaf extends StatefulWidget {
 }
 
 class StatefulLeafState extends State<StatefulLeaf> {
-  void test() { setState(() { }); }
+  void markNeedsBuild() { setState(() { }); }
 
   @override
   Widget build(BuildContext context) => new Text('leaf');
@@ -38,7 +38,7 @@ class KeyedWrapper extends StatelessWidget {
     return new Container(
       key: key1,
       child: new StatefulLeaf(
-        key: key2
+        key: key2,
       )
     );
   }
@@ -48,16 +48,16 @@ Widget builder() {
   return new Column(
     children: <Widget>[
       new KeyedWrapper(items[1].key1, items[1].key2),
-      new KeyedWrapper(items[0].key1, items[0].key2)
-    ]
+      new KeyedWrapper(items[0].key1, items[0].key2),
+    ],
   );
 }
 
 void main() {
-  testWidgets('duplicate key smoke test', (WidgetTester tester) async {
+  testWidgets('moving subtrees with global keys - smoketest', (WidgetTester tester) async {
     await tester.pumpWidget(builder());
     final StatefulLeafState leaf = tester.firstState(find.byType(StatefulLeaf));
-    leaf.test();
+    leaf.markNeedsBuild();
     await tester.pump();
     final Item lastItem = items[1];
     items.remove(lastItem);

packages/flutter/test/widgets/reparent_state_with_layout_builder_test.dart

@@ -79,10 +79,14 @@ void main() {
 
   testWidgets('Clean then reparent with dependencies',
       (WidgetTester tester) async {
-    final GlobalKey key = new GlobalKey();
+
+    int layoutBuilderBuildCount = 0;
 
     StateSetter keyedSetState;
+    StateSetter layoutBuilderSetState;
+    StateSetter childSetState;
 
+    final GlobalKey key = new GlobalKey();
     final Widget keyedWidget = new StatefulBuilder(
       key: key,
       builder: (BuildContext context, StateSetter setState) {
@@ -93,13 +97,8 @@ void main() {
     );
 
     Widget layoutBuilderChild = keyedWidget;
-    StateSetter layoutBuilderSetState;
-
-    StateSetter childSetState;
     Widget deepChild = new Container();
 
-    int layoutBuilderBuildCount = 0;
-
     await tester.pumpWidget(new MediaQuery(
       data: new MediaQueryData.fromWindow(ui.window),
       child: new Column(
@@ -109,8 +108,8 @@ void main() {
             layoutBuilderSetState = setState;
             return new LayoutBuilder(
               builder: (BuildContext context, BoxConstraints constraints) {
-                ++layoutBuilderBuildCount;
-                return layoutBuilderChild;
+                layoutBuilderBuildCount += 1;
+                return layoutBuilderChild; // initially keyedWidget above, but then a new Container
               },
             );
           }),
@@ -123,7 +122,7 @@ void main() {
                       child: new StatefulBuilder(builder:
                           (BuildContext context, StateSetter setState) {
                         childSetState = setState;
-                        return deepChild;
+                        return deepChild; // initially a Container, but then the keyedWidget above
                       }),
                     ),
                   ),
@@ -134,25 +133,24 @@ void main() {
         ],
       ),
     ));
-
     expect(layoutBuilderBuildCount, 1);
 
-    // This call adds the element ot the dirty list.
-    keyedSetState(() {});
+    keyedSetState(() { /* Change nothing but add the element to the dirty list. */ });
 
     childSetState(() {
+      // The deep child builds in the initial build phase. It takes the child
+      // from the LayoutBuilder before the LayoutBuilder has a chance to build.
       deepChild = keyedWidget;
     });
 
-    // The layout builder will build in a separate build scope. This delays the
-    // removal of the keyed child until this build scope.
     layoutBuilderSetState(() {
+      // The layout builder will build in a separate build scope. This delays
+      // the removal of the keyed child until this build scope.
       layoutBuilderChild = new Container();
     });
 
     // The essential part of this test is that this call to pump doesn't throw.
     await tester.pump();
-
     expect(layoutBuilderBuildCount, 2);
   });
 }

packages/flutter_test/lib/src/binding.dart

@@ -512,26 +512,25 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding {
     try {
       debugBuildingDirtyElements = true;
       buildOwner.buildScope(renderViewElement);
-      if (_phase == EnginePhase.build)
-        return;
-      assert(renderView != null);
-      pipelineOwner.flushLayout();
-      if (_phase == EnginePhase.layout)
-        return;
-      pipelineOwner.flushCompositingBits();
-      if (_phase == EnginePhase.compositingBits)
-        return;
-      pipelineOwner.flushPaint();
-      if (_phase == EnginePhase.paint)
-        return;
-      renderView.compositeFrame(); // this sends the bits to the GPU
-      if (_phase == EnginePhase.composite)
-        return;
-      pipelineOwner.flushSemantics();
-      if (_phase == EnginePhase.flushSemantics)
-        return;
-    } finally {
+      if (_phase != EnginePhase.build) {
+        assert(renderView != null);
+        pipelineOwner.flushLayout();
+        if (_phase != EnginePhase.layout) {
+          pipelineOwner.flushCompositingBits();
+          if (_phase != EnginePhase.compositingBits) {
+            pipelineOwner.flushPaint();
+            if (_phase != EnginePhase.paint) {
+              renderView.compositeFrame(); // this sends the bits to the GPU
+              if (_phase != EnginePhase.composite) {
+                pipelineOwner.flushSemantics();
+                assert(_phase == EnginePhase.flushSemantics || _phase == EnginePhase.sendSemanticsTree);
+              }
+            }
+          }
+        }
+      }
       buildOwner.finalizeTree();
+    } finally {
       debugBuildingDirtyElements = false;
     }
   }