Fix Inherited bugs (#3657)

Fixes https://github.com/flutter/flutter/issues/3493

 - rebuild stateless widgets that have dependencies when their ancestors change but they don't

Fixes https://github.com/flutter/flutter/issues/3120

 - rebuild widgets that tried to inherit from a widget that didn't exist, when the widget is added

This adds a pointer and a bool to Element, which isn't great. It also adds a more or less complete tree walk when you add a new Inherited widget at the top of your tree, which isn't cheap.

packages/flutter/lib/src/material/ink_well.dart

@@ -196,15 +196,10 @@ class _InkResponseState<T extends InkResponse> extends State<T> {
     super.deactivate();
   }
 
-  @override
-  void dependenciesChanged(Type affectedWidgetType) {
-    if (affectedWidgetType == Theme && _lastHighlight != null)
-      _lastHighlight.color = Theme.of(context).highlightColor;
-  }
-
   @override
   Widget build(BuildContext context) {
     assert(config.debugCheckContext(context));
+    _lastHighlight?.color = Theme.of(context).highlightColor;
     final bool enabled = config.onTap != null || config.onDoubleTap != null || config.onLongPress != null;
     return new GestureDetector(
       onTapDown: enabled ? _handleTapDown : null,

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

@@ -451,7 +451,7 @@ abstract class State<T extends StatefulWidget> {
   /// Called when an Inherited widget in the ancestor chain has changed. Usually
   /// there is nothing to do here; whenever this is called, build() is also
   /// called.
-  void dependenciesChanged(Type affectedWidgetType) { }
+  void dependenciesChanged() { }
 
   @override
   String toString() {
@@ -1120,37 +1120,50 @@ abstract class Element implements BuildContext {
     element.visitChildren(_activateRecursively);
   }
 
+  /// Called when a previously de-activated widget (see [deactivate]) is reused
+  /// instead of being unmounted (see [unmount]).
   void activate() {
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
     assert(widget != null);
     assert(depth != null);
     assert(!_active);
     _active = true;
+    // We unregistered our dependencies in deactivate, but never cleared the list.
+    // Since we're going to be reused, let's clear our list now.
+    _dependencies?.clear();
     _updateInheritance();
     assert(() { _debugLifecycleState = _ElementLifecycle.active; return true; });
   }
 
+  // TODO(ianh): Define activation/deactivation thoroughly (other methods point
+  // here for details).
   void deactivate() {
     assert(_debugLifecycleState == _ElementLifecycle.active);
     assert(widget != null);
     assert(depth != null);
     assert(_active);
-    if (_dependencies != null) {
+    if (_dependencies != null && _dependencies.length > 0) {
       for (InheritedElement dependency in _dependencies)
         dependency._dependents.remove(this);
-      _dependencies.clear();
+      // For expediency, we don't actually clear the list here, even though it's
+      // no longer representative of what we are registered with. If we never
+      // get re-used, it doesn't matter. If we do, then we'll clear the list in
+      // activate(). The benefit of this is that it allows BuildableElement's
+      // activate() implementation to decide whether to rebuild based on whether
+      // we had dependencies here.
     }
     _inheritedWidgets = null;
     _active = false;
     assert(() { _debugLifecycleState = _ElementLifecycle.inactive; return true; });
   }
 
-  /// Called after children have been deactivated.
+  /// Called after children have been deactivated (see [deactivate]).
   void debugDeactivated() {
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
   }
 
-  /// Called when an Element is removed from the tree permanently.
+  /// Called when an Element is removed from the tree permanently after having
+  /// been deactivated (see [deactivate]).
   void unmount() {
     assert(_debugLifecycleState == _ElementLifecycle.inactive);
     assert(widget != null);
@@ -1168,6 +1181,7 @@ abstract class Element implements BuildContext {
 
   Map<Type, InheritedElement> _inheritedWidgets;
   Set<InheritedElement> _dependencies;
+  bool _hadUnsatisfiedDependencies = false;
 
   @override
   InheritedWidget inheritFromWidgetOfExactType(Type targetType) {
@@ -1179,6 +1193,7 @@ abstract class Element implements BuildContext {
       ancestor._dependents.add(this);
       return ancestor.widget;
     }
+    _hadUnsatisfiedDependencies = true;
     return null;
   }
 
@@ -1229,9 +1244,7 @@ abstract class Element implements BuildContext {
       ancestor = ancestor._parent;
   }
 
-  void dependenciesChanged(Type affectedWidgetType) {
-    assert(false);
-  }
+  void dependenciesChanged();
 
   String debugGetCreatorChain(int limit) {
     List<String> chain = <String>[];
@@ -1402,6 +1415,19 @@ abstract class BuildableElement extends Element {
       owner._debugCurrentBuildTarget = this;
      return true;
     });
+    _hadUnsatisfiedDependencies = false;
+    // In theory, we would also clear our actual _dependencies here. However, to
+    // clear it we'd have to notify each of them, unregister from them, and then
+    // reregister as soon as the build function re-dependended on it. So to
+    // avoid faffing around we just never unregister our dependencies except
+    // when we're deactivated. In principle this means we might be getting
+    // notified about widget types we once inherited from but no longer do, but
+    // in practice this is so rare that the extra cost when it does happen is
+    // far outweighed by the avoided work in the common case.
+    // We _do_ clear the list properly any time our ancestor chain changes in a
+    // way that might result in us getting a different Element's Widget for a
+    // particular Type. This avoids the potential of being registered to
+    // multiple identically-typed Widgets' Elements at the same time.
     performRebuild();
     assert(() {
       assert(owner._debugCurrentBuildTarget == this);
@@ -1411,22 +1437,28 @@ abstract class BuildableElement extends Element {
     assert(!_dirty);
   }
 
-  /// Called by [rebuild] after [rebuild] has checked that this element indeed
-  /// needs to build and is still active.
-  ///
-  /// This function is called if this element is marked as needing to be built
-  /// (see [markNeedsBuild]). For example, if [State.setState] is called on its
-  /// associated [State] object or if this element depends on an
-  /// [InheritedElement] that has itself changed.
+  /// Called by rebuild() after the appropriate checks have been made.
   void performRebuild();
 
   @override
-  void dependenciesChanged(Type affectedWidgetType) {
+  void dependenciesChanged() {
+    assert(_active);
+    markNeedsBuild();
+  }
+
+  @override
+  void activate() {
+    final bool shouldRebuild = ((_dependencies != null && _dependencies.length > 0) || _hadUnsatisfiedDependencies);
+    super.activate(); // clears _dependencies, and sets active to true
+    if (shouldRebuild) {
+      assert(_active); // otherwise markNeedsBuild is a no-op
       markNeedsBuild();
     }
+  }
 
   @override
   void _reassemble() {
+    assert(_active); // otherwise markNeedsBuild is a no-op
     markNeedsBuild();
     super._reassemble();
   }
@@ -1619,6 +1651,10 @@ class StatefulElement extends ComponentElement {
   @override
   void activate() {
     super.activate();
+    // Since the State could have observed the deactivate() and thus disposed of
+    // resources allocated in the build function, we have to rebuild the widget
+    // so that its State can reallocate its resources.
+    assert(_active); // otherwise markNeedsBuild is a no-op
     markNeedsBuild();
   }
 
@@ -1647,9 +1683,9 @@ class StatefulElement extends ComponentElement {
   }
 
   @override
-  void dependenciesChanged(Type affectedWidgetType) {
-    super.dependenciesChanged(affectedWidgetType);
-    _state.dependenciesChanged(affectedWidgetType);
+  void dependenciesChanged() {
+    super.dependenciesChanged();
+    _state.dependenciesChanged();
   }
 
   @override
@@ -1683,12 +1719,12 @@ abstract class _ProxyElement extends ComponentElement {
     assert(widget != newWidget);
     super.update(newWidget);
     assert(widget == newWidget);
-    notifyDescendants(oldWidget);
+    notifyClients(oldWidget);
     _dirty = true;
     rebuild();
   }
 
-  void notifyDescendants(_ProxyWidget oldWidget);
+  void notifyClients(_ProxyWidget oldWidget);
 }
 
 class ParentDataElement<T extends RenderObjectWidget> extends _ProxyElement {
@@ -1728,7 +1764,7 @@ class ParentDataElement<T extends RenderObjectWidget> extends _ProxyElement {
   }
 
   @override
-  void notifyDescendants(ParentDataWidget<T> oldWidget) {
+  void notifyClients(ParentDataWidget<T> oldWidget) {
     void notifyChildren(Element child) {
       if (child is RenderObjectElement) {
         child.updateParentData(widget);
@@ -1771,7 +1807,7 @@ class InheritedElement extends _ProxyElement {
   }
 
   @override
-  void notifyDescendants(InheritedWidget oldWidget) {
+  void notifyClients(InheritedWidget oldWidget) {
     if (!widget.updateShouldNotify(oldWidget))
       return;
     dispatchDependenciesChanged();
@@ -1784,16 +1820,17 @@ class InheritedElement extends _ProxyElement {
   /// function at other times if their inherited information changes outside of
   /// the build phase.
   void dispatchDependenciesChanged() {
-    final Type ourRuntimeType = widget.runtimeType;
-    for (Element dependant in _dependents) {
-      dependant.dependenciesChanged(ourRuntimeType);
+    for (Element dependent in _dependents) {
+      dependent.dependenciesChanged();
       assert(() {
         // check that it really is our descendant
-        Element ancestor = dependant._parent;
+        Element ancestor = dependent._parent;
         while (ancestor != this && ancestor != null)
           ancestor = ancestor._parent;
         return ancestor == this;
       });
+      // check that it really deepends on us
+      assert(dependent._dependencies.contains(this));
     }
   }
 }

packages/flutter/test/widget/inherited_test.dart

@@ -6,6 +6,8 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/material.dart';
 import 'package:test/test.dart';
 
+import 'test_widgets.dart';
+
 class TestInherited extends InheritedWidget {
   TestInherited({ Key key, Widget child, this.shouldNotify: true })
     : super(key: key, child: child);
@@ -18,6 +20,16 @@ class TestInherited extends InheritedWidget {
   }
 }
 
+class ValueInherited extends InheritedWidget {
+  ValueInherited({ Key key, Widget child, this.value })
+    : super(key: key, child: child);
+
+  final int value;
+
+  @override
+  bool updateShouldNotify(ValueInherited oldWidget) => value != oldWidget.value;
+}
+
 void main() {
   testWidgets('Inherited notifies dependents', (WidgetTester tester) {
     List<TestInherited> log = <TestInherited>[];
@@ -74,4 +86,356 @@ void main() {
 
     expect(log, equals([first, second]));
   });
+
+  testWidgets('Update inherited when removing node', (WidgetTester tester) {
+    final List<String> log = <String>[];
+
+    tester.pumpWidget(
+      new Container(
+        child: new ValueInherited(
+          value: 1,
+          child: new Container(
+            child: new FlipWidget(
+              left: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new ValueInherited(
+                      value: 3,
+                      child: new Container(
+                        child: new Builder(
+                          builder: (BuildContext context) {
+                            ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+                            log.add('a: ${v.value}');
+                            return new Text('');
+                          }
+                        )
+                      )
+                    )
+                  )
+                )
+              ),
+              right: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new Container(
+                      child: new Builder(
+                        builder: (BuildContext context) {
+                          ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+                          log.add('b: ${v.value}');
+                          return new Text('');
+                        }
+                      )
+                    )
+                  )
+                )
+              )
+            )
+          )
+        )
+      )
+    );
+
+    expect(log, equals(<String>['a: 3']));
+    log.clear();
+
+    tester.pump();
+
+    expect(log, equals(<String>[]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<String>['b: 2']));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<String>['a: 3']));
+    log.clear();
+  });
+
+  testWidgets('Update inherited when removing node and child has global key', (WidgetTester tester) {
+
+    final List<String> log = <String>[];
+
+    Key key = new GlobalKey();
+
+    tester.pumpWidget(
+      new Container(
+        child: new ValueInherited(
+          value: 1,
+          child: new Container(
+            child: new FlipWidget(
+              left: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new ValueInherited(
+                      value: 3,
+                      child: new Container(
+                        key: key,
+                        child: new Builder(
+                          builder: (BuildContext context) {
+                            ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+                            log.add('a: ${v.value}');
+                            return new Text('');
+                          }
+                        )
+                      )
+                    )
+                  )
+                )
+              ),
+              right: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new Container(
+                      key: key,
+                      child: new Builder(
+                        builder: (BuildContext context) {
+                          ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+                          log.add('b: ${v.value}');
+                          return new Text('');
+                        }
+                      )
+                    )
+                  )
+                )
+              )
+            )
+          )
+        )
+      )
+    );
+
+    expect(log, equals(<String>['a: 3']));
+    log.clear();
+
+    tester.pump();
+
+    expect(log, equals(<String>[]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<String>['b: 2']));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<String>['a: 3']));
+    log.clear();
+  });
+
+  testWidgets('Update inherited when removing node and child has global key with constant child', (WidgetTester tester) {
+    final List<int> log = <int>[];
+
+    Key key = new GlobalKey();
+
+    Widget child = new Builder(
+      builder: (BuildContext context) {
+        ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+        log.add(v.value);
+        return new Text('');
+      }
+    );
+
+    tester.pumpWidget(
+      new Container(
+        child: new ValueInherited(
+          value: 1,
+          child: new Container(
+            child: new FlipWidget(
+              left: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new ValueInherited(
+                      value: 3,
+                      child: new Container(
+                        key: key,
+                        child: child
+                      )
+                    )
+                  )
+                )
+              ),
+              right: new Container(
+                child: new ValueInherited(
+                  value: 2,
+                  child: new Container(
+                    child: new Container(
+                      key: key,
+                      child: child
+                    )
+                  )
+                )
+              )
+            )
+          )
+        )
+      )
+    );
+
+    expect(log, equals(<int>[3]));
+    log.clear();
+
+    tester.pump();
+
+    expect(log, equals(<int>[]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<int>[2]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<int>[3]));
+    log.clear();
+  });
+
+  testWidgets('Update inherited when removing node and child has global key with constant child, minimised', (WidgetTester tester) {
+
+    final List<int> log = <int>[];
+
+    Widget child = new Builder(
+      key: new GlobalKey(),
+      builder: (BuildContext context) {
+        ValueInherited v = context.inheritFromWidgetOfExactType(ValueInherited);
+        log.add(v.value);
+        return new Text('');
+      }
+    );
+
+    tester.pumpWidget(
+      new ValueInherited(
+        value: 2,
+        child: new FlipWidget(
+          left: new ValueInherited(
+            value: 3,
+            child: child
+          ),
+          right: child
+        )
+      )
+    );
+
+    expect(log, equals(<int>[3]));
+    log.clear();
+
+    tester.pump();
+
+    expect(log, equals(<int>[]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<int>[2]));
+    log.clear();
+
+    flipStatefulWidget(tester);
+    tester.pump();
+
+    expect(log, equals(<int>[3]));
+    log.clear();
+  });
+
+  testWidgets('Inherited widget notifies descendants when descendant previously failed to find a match', (WidgetTester tester) {
+    int inheritedValue = -1;
+
+    final Widget inner = new Container(
+      key: new GlobalKey(),
+      child: new Builder(
+        builder: (BuildContext context) {
+          ValueInherited widget = context.inheritFromWidgetOfExactType(ValueInherited);
+          inheritedValue = widget?.value;
+          return new Container();
+        }
+      )
+    );
+
+    tester.pumpWidget(
+      inner
+    );
+    expect(inheritedValue, isNull);
+
+    inheritedValue = -2;
+    tester.pumpWidget(
+      new ValueInherited(
+        value: 3,
+        child: inner
+      )
+    );
+    expect(inheritedValue, equals(3));
+  });
+
+  testWidgets('Inherited widget doesn\'t notify descendants when descendant did not previously fail to find a match and had no dependencies', (WidgetTester tester) {
+    int buildCount = 0;
+
+    final Widget inner = new Container(
+      key: new GlobalKey(),
+      child: new Builder(
+        builder: (BuildContext context) {
+          buildCount += 1;
+          return new Container();
+        }
+      )
+    );
+
+    tester.pumpWidget(
+      inner
+    );
+    expect(buildCount, equals(1));
+
+    tester.pumpWidget(
+      new ValueInherited(
+        value: 3,
+        child: inner
+      )
+    );
+    expect(buildCount, equals(1));
+  });
+
+  testWidgets('Inherited widget does notify descendants when descendant did not previously fail to find a match but did have other dependencies', (WidgetTester tester) {
+    int buildCount = 0;
+
+    final Widget inner = new Container(
+      key: new GlobalKey(),
+      child: new TestInherited(
+        shouldNotify: false,
+        child: new Builder(
+          builder: (BuildContext context) {
+            context.inheritFromWidgetOfExactType(TestInherited);
+            buildCount += 1;
+            return new Container();
+          }
+        )
+      )
+    );
+
+    tester.pumpWidget(
+      inner
+    );
+    expect(buildCount, equals(1));
+
+    tester.pumpWidget(
+      new ValueInherited(
+        value: 3,
+        child: inner
+      )
+    );
+    expect(buildCount, equals(2));
+  });
 }

travis/test.sh

@@ -6,14 +6,6 @@ export PATH="$PWD/bin:$PWD/bin/cache/dart-sdk/bin:$PATH"
 # analyze all the Dart code in the repo
 flutter analyze --flutter-repo
 
-# generate and analyze our large sample app
-dart dev/tools/mega_gallery.dart
-(cd dev/benchmarks/mega_gallery; flutter analyze --watch --benchmark)
-
-# keep the rest of this file in sync with
-# //chrome_infra/build/scripts/slave/recipes/flutter/flutter.py
-# see https://github.com/flutter/flutter/blob/master/infra/README.md
-
 # run tests
 (cd packages/flutter; flutter test)
 (cd packages/flutter_driver; dart -c test/all.dart)
@@ -27,3 +19,7 @@ dart dev/tools/mega_gallery.dart
 (cd examples/layers; flutter test)
 (cd examples/material_gallery; flutter test)
 (cd examples/stocks; flutter test)
+
+# generate and analyze our large sample app
+dart dev/tools/mega_gallery.dart
+(cd dev/benchmarks/mega_gallery; flutter analyze --watch --benchmark)