Actually notify GlobalKey listeners in fn3

This patch makes a number of changes:

1) buildDirtyComponents now prevents all calls to setState, not just those
   higher in the tree. This change is necessary for consistency with
   MixedViewport and HomogeneousViewport because those widgets already build
   subwidgets with that restriction. If the "normal" build didn't enforce that
   rule, then some widgets would break when put inside a mixed or homogeneous
   viewport.

2) We now notify global key listeners in a microtask after beginFrame. That
   means setState is legal in these callbacks and that we'll produce another
   frame if someone calls setState in such a callback.

packages/flutter/lib/src/fn3/binding.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:async';
+
 import 'package:sky/animation.dart';
 import 'package:sky/rendering.dart';
 import 'package:sky/src/fn3/framework.dart';
@@ -15,7 +17,7 @@ class WidgetFlutterBinding extends FlutterBinding {
   }
 
   /// Ensures that there is a FlutterBinding object instantiated.
-  static void initBinding() {
+  static void ensureInitialized() {
     if (FlutterBinding.instance == null)
       new WidgetFlutterBinding();
     assert(FlutterBinding.instance is WidgetFlutterBinding);
@@ -38,16 +40,14 @@ class WidgetFlutterBinding extends FlutterBinding {
   void beginFrame(double timeStamp) {
     buildDirtyElements();
     super.beginFrame(timeStamp);
+    scheduleMicrotask(GlobalKey.checkForDuplicatesAndNotifyListeners);
   }
 
-  final List<BuildableElement> _dirtyElements = new List<BuildableElement>();
-
-  int _debugBuildingAtDepth;
+  List<BuildableElement> _dirtyElements = new List<BuildableElement>();
 
   /// Adds an element to the dirty elements list so that it will be rebuilt
   /// when buildDirtyElements is called.
   void scheduleBuildFor(BuildableElement element) {
-    assert(_debugBuildingAtDepth == null || element.depth > _debugBuildingAtDepth);
     assert(!_dirtyElements.contains(element));
     assert(element.dirty);
     if (_dirtyElements.isEmpty)
@@ -55,46 +55,30 @@ class WidgetFlutterBinding extends FlutterBinding {
     _dirtyElements.add(element);
   }
 
-  void _absorbDirtyElements(List<BuildableElement> list) {
-    assert(_debugBuildingAtDepth != null);
-    assert(!_dirtyElements.any((element) => element.depth <= _debugBuildingAtDepth));
-    _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth);
-    list.addAll(_dirtyElements);
-    _dirtyElements.clear();
-  }
-
   /// Builds all the elements that were marked as dirty using schedule(), in depth order.
   /// If elements are marked as dirty while this runs, they must be deeper than the algorithm
   /// has yet reached.
   /// This is called by beginFrame().
   void buildDirtyElements() {
-    assert(_debugBuildingAtDepth == null);
     if (_dirtyElements.isEmpty)
       return;
-    assert(() { _debugBuildingAtDepth = 0; return true; });
-    List<BuildableElement> sortedDirtyElements = new List<BuildableElement>();
-    int index = 0;
-    do {
-      _absorbDirtyElements(sortedDirtyElements);
-      for (; index < sortedDirtyElements.length; index += 1) {
-        BuildableElement element = sortedDirtyElements[index];
-        assert(() {
-          if (element.depth > _debugBuildingAtDepth)
-            _debugBuildingAtDepth = element.depth;
-          return element.depth == _debugBuildingAtDepth;
-        });
+    BuildableElement.lockState(() {
+      _dirtyElements.sort((BuildableElement a, BuildableElement b) => a.depth - b.depth);
+      for (BuildableElement element in _dirtyElements)
         element.rebuild();
-      }
-    } while (_dirtyElements.isNotEmpty);
-    assert(() { _debugBuildingAtDepth = null; return true; });
+      _dirtyElements.clear();
+    });
+    assert(_dirtyElements.isEmpty);
   }
 }
 
 void runApp(Widget app) {
-  WidgetFlutterBinding.initBinding();
+  WidgetFlutterBinding.ensureInitialized();
+  BuildableElement.lockState(() {
     WidgetFlutterBinding.instance.renderViewElement.update(
       WidgetFlutterBinding.instance.describeApp(app)
     );
+  });
 }
 
 /// This class provides a bridge from a RenderObject to an Element tree. The

packages/flutter/lib/src/fn3/framework.dart

@@ -118,8 +118,7 @@ abstract class GlobalKey extends Key {
     assert(removed);
   }
 
-  // TODO(ianh): call this
-  static void _notifyListeners() {
+  static void checkForDuplicatesAndNotifyListeners() {
     assert(() {
       String message = '';
       for (GlobalKey key in _debugDuplicates.keys) {
@@ -326,13 +325,8 @@ abstract class State<T extends StatefulComponent> {
   void setState(void fn()) {
     assert(_debugLifecycleState != _StateLifecycle.defunct);
     fn();
-    if (_element._builder != null) {
-      // _element._builder is set after initState(). We verify that we're past
-      // that before calling markNeedsBuild() so that setState()s triggered
-      // during initState() during lockState() don't cause any trouble.
     _element.markNeedsBuild();
   }
-  }
 
   /// Called when this object is removed from the tree. Override this to clean
   /// up any resources allocated by this object.
@@ -645,6 +639,17 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
   bool get dirty => _dirty;
   bool _dirty = true;
 
+  // We to let component authors call setState from initState, didUpdateConfig,
+  // and build even when state is locked because its convenient and a no-op
+  // anyway. This flag ensures that this convenience is only allowed on the
+  // element currently undergoing initState, didUpdateConfig, or build.
+  bool _debugAllowIgnoredCallsToMarkNeedsBuild = false;
+  bool _debugSetAllowIgnoredCallsToMarkNeedsBuild(bool value) {
+    assert(_debugAllowIgnoredCallsToMarkNeedsBuild == !value);
+    _debugAllowIgnoredCallsToMarkNeedsBuild = value;
+    return true;
+  }
+
   void mount(Element parent, dynamic newSlot) {
     super.mount(parent, newSlot);
     assert(_child == null);
@@ -664,21 +669,27 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
     if (!_dirty)
       return;
     assert(_debugLifecycleState == _ElementLifecycle.mounted);
-    _dirty = false;
+    assert(_debugStateLocked);
+    assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(true));
     Widget built;
     try {
       built = _builder(this);
       assert(built != null);
     } catch (e, stack) {
-      _debugReportException('building $this', e, stack);
+      _debugReportException('building ${_widget}', e, stack);
       built = new ErrorWidget();
+    } finally {
+      // We delay marking the element as clean until after calling _builder so
+      // that attempts to markNeedsBuild() during build() will be ignored.
+      _dirty = false;
+      assert(_debugSetAllowIgnoredCallsToMarkNeedsBuild(false));
     }
 
     try {
       _child = updateChild(_child, built, slot);
       assert(_child != null);
     } catch (e, stack) {
-      _debugReportException('building $this', e, stack);
+      _debugReportException('building ${_widget}', e, stack);
       built = new ErrorWidget();
       _child = updateChild(null, built, slot);
     }
@@ -689,12 +700,16 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
   static int _debugStateLockLevel = 0;
   static bool get _debugStateLocked => _debugStateLockLevel > 0;
 
-  /// Calls the callback argument synchronously, but in a context where calls to
-  /// State.setState() will fail. Use this when it is possible that you will
-  /// trigger code in components but want to make sure that there is no
-  /// possibility that any components will be marked dirty, for example because
-  /// you are in the middle of layout and you are not going to be flushing the
-  /// build queue (since that could mutate the layout tree).
+  /// Establishes a scope in which component build functions can run.
+  ///
+  /// Inside a build scope, component build functions are allowed to run, but
+  /// State.setState() is forbidden. This mechanism prevents build functions
+  /// from transitively requiring other build functions to run, potentially
+  /// causing infinite loops.
+  ///
+  /// After unwinding the last build scope on the stack, the framework verifies
+  /// that each global key is used at most once and notifies listeners about
+  /// changes to global keys.
   static void lockState(void callback()) {
     _debugStateLockLevel += 1;
     try {
@@ -712,10 +727,12 @@ abstract class BuildableElement<T extends Widget> extends Element<T> {
   /// components dirty during event handlers before the frame begins, not during
   /// the build itself.
   void markNeedsBuild() {
-    assert(!_debugStateLocked);
-    assert(_debugLifecycleState == _ElementLifecycle.mounted);
+    assert(_debugLifecycleState != _ElementLifecycle.defunct);
+    assert(!_debugStateLocked || (_debugAllowIgnoredCallsToMarkNeedsBuild && _dirty));
     if (_dirty)
       return;
+    assert(_debugLifecycleState == _ElementLifecycle.mounted);
+    assert(!_debugStateLocked);
     _dirty = true;
     assert(scheduleBuildFor != null);
     scheduleBuildFor(this);
@@ -757,10 +774,17 @@ class StatefulComponentElement extends BuildableElement<StatefulComponent> {
     : _state = widget.createState(), super(widget) {
     assert(_state._element == null);
     _state._element = this;
+    assert(_builder == null);
+    _builder = _state.build;
     assert(_state._config == null);
     _state._config = widget;
     assert(_state._debugLifecycleState == _StateLifecycle.created);
+    try {
+      _debugSetAllowIgnoredCallsToMarkNeedsBuild(true);
       _state.initState(this);
+    } finally {
+      _debugSetAllowIgnoredCallsToMarkNeedsBuild(false);
+    }
     assert(() {
       if (_state._debugLifecycleState == _StateLifecycle.initialized)
         return true;
@@ -768,10 +792,6 @@ class StatefulComponentElement extends BuildableElement<StatefulComponent> {
       return false;
     });
     assert(() { _state._debugLifecycleState = _StateLifecycle.ready; return true; });
-    assert(_builder == null);
-    // see State.setState() for why it's important that _builder be set after
-    // initState() is called.
-    _builder = _state.build;
   }
 
   State get state => _state;
@@ -781,16 +801,30 @@ class StatefulComponentElement extends BuildableElement<StatefulComponent> {
     super.update(newWidget);
     assert(widget == newWidget);
     StatefulComponent oldConfig = _state._config;
+    // Notice that we mark ourselves as dirty before calling didUpdateConfig to
+    // let authors call setState from within didUpdateConfig without triggering
+    // asserts.
+    _dirty = true;
     _state._config = widget;
+    try {
+      _debugSetAllowIgnoredCallsToMarkNeedsBuild(true);
       _state.didUpdateConfig(oldConfig);
-    _dirty = true;
+    } finally {
+      _debugSetAllowIgnoredCallsToMarkNeedsBuild(false);
+    }
     rebuild();
   }
 
   void unmount() {
     super.unmount();
     _state.dispose();
-    assert(_state._debugLifecycleState == _StateLifecycle.defunct);
+    assert(() {
+      if (_state._debugLifecycleState == _StateLifecycle.defunct)
+        return true;
+      print('${_state.runtimeType}.dispose failed to call super.dispose');
+      return false;
+    });
+    assert(!_dirty); // See BuildableElement.unmount for why this is important.
     _state._element = null;
     _state = null;
   }

packages/flutter/lib/src/fn3/homogeneous_viewport.dart

@@ -90,12 +90,14 @@ class HomogeneousViewportElement extends RenderObjectElement<HomogeneousViewport
   }
 
   void layout(BoxConstraints constraints) {
-    // we lock the framework state (meaning that no elements can call markNeedsBuild()) because we are
-    // in the middle of layout and if we allowed people to set state, they'd expect to have that state
-    // reflected immediately, which, if we were to try to honour it, would potentially result in
-    // assertions since you can't normally mutate the render object tree during layout. (If there was
-    // a way to limit this to only descendants of this, it'd be ok, since we are exempt from that
-    // assert since we are actively doing our own layout still.)
+    // We enter a build scope (meaning that markNeedsBuild() is forbidden)
+    // because we are in the middle of layout and if we allowed people to set
+    // state, they'd expect to have that state reflected immediately, which, if
+    // we were to try to honour it, would potentially result in assertions
+    // because you can't normally mutate the render object tree during layout.
+    // (If there were a way to limit these writes to descendants of this, it'd
+    // be ok because we are exempt from that assert since we are still actively
+    // doing our own layout.)
     BuildableElement.lockState(() {
       double mainAxisExtent = widget.direction == ScrollDirection.vertical ? constraints.maxHeight : constraints.maxWidth;
       double offset;

packages/unit/test/widget/build_scope_test.dart

+import 'package:sky/src/fn3.dart';
+import 'package:test/test.dart';
+
+import '../fn3/widget_tester.dart';
+import '../fn3/test_widgets.dart';
+
+class ProbeWidget extends StatefulComponent {
+  ProbeWidgetState createState() => new ProbeWidgetState();
+}
+
+class ProbeWidgetState extends State<ProbeWidget> {
+  static int buildCount = 0;
+
+  void initState(BuildContext context) {
+    super.initState(context);
+    setState(() {});
+  }
+
+  void didUpdateConfig(ProbeWidget oldConfig) {
+    setState(() {});
+  }
+
+  Widget build(BuildContext context) {
+    setState(() {});
+    buildCount++;
+    return new Container();
+  }
+}
+
+class BadWidget extends StatelessComponent {
+  BadWidget(this.parentState);
+
+  final State parentState;
+
+  Widget build(BuildContext context) {
+    parentState.setState(() {});
+    return new Container();
+  }
+}
+
+class BadWidgetParent extends StatefulComponent {
+  BadWidgetParentState createState() => new BadWidgetParentState();
+}
+
+class BadWidgetParentState extends State<BadWidgetParent> {
+  Widget build(BuildContext context) {
+    return new BadWidget(this);
+  }
+}
+
+class BadDisposeWidget extends StatefulComponent {
+  BadDisposeWidgetState createState() => new BadDisposeWidgetState();
+}
+
+class BadDisposeWidgetState extends State<BadDisposeWidget> {
+  Widget build(BuildContext context) {
+    return new Container();
+  }
+
+  void dispose() {
+    setState(() {});
+    super.dispose();
+  }
+}
+
+void main() {
+  dynamic cachedException;
+
+  setUp(() {
+    assert(cachedException == null);
+    debugWidgetsExceptionHandler = (String context, dynamic exception, StackTrace stack) {
+      cachedException = exception;
+    };
+  });
+
+  tearDown(() {
+    assert(cachedException == null);
+    cachedException = null;
+    debugWidgetsExceptionHandler = null;
+  });
+
+  test('Legal times for setState', () {
+    WidgetTester tester = new WidgetTester();
+
+    GlobalKey flipKey = new GlobalKey();
+    expect(ProbeWidgetState.buildCount, equals(0));
+    tester.pumpFrame(new ProbeWidget());
+    expect(ProbeWidgetState.buildCount, equals(1));
+    tester.pumpFrame(new ProbeWidget());
+    expect(ProbeWidgetState.buildCount, equals(2));
+    tester.pumpFrame(new FlipComponent(
+      key: flipKey,
+      left: new Container(),
+      right: new ProbeWidget()
+    ));
+    expect(ProbeWidgetState.buildCount, equals(2));
+    (flipKey.currentState as FlipComponentState).flip();
+    tester.pumpFrameWithoutChange();
+    expect(ProbeWidgetState.buildCount, equals(3));
+    (flipKey.currentState as FlipComponentState).flip();
+    tester.pumpFrameWithoutChange();
+    expect(ProbeWidgetState.buildCount, equals(3));
+    tester.pumpFrame(new Container());
+    expect(ProbeWidgetState.buildCount, equals(3));
+  });
+
+  test('Setting parent state during build is forbidden', () {
+    WidgetTester tester = new WidgetTester();
+
+    expect(cachedException, isNull);
+    tester.pumpFrame(new BadWidgetParent());
+    expect(cachedException, isNotNull);
+    cachedException = null;
+    tester.pumpFrame(new Container());
+    expect(cachedException, isNull);
+  });
+
+  test('Setting state during dispose is forbidden', () {
+    WidgetTester tester = new WidgetTester();
+
+    tester.pumpFrame(new BadDisposeWidget());
+    expect(() {
+      tester.pumpFrame(new Container());
+    }, throws);
+  });
+}