From 832fcdec0a37d73c365095ed0e067eb399ba10af Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Fri, 1 Mar 2019 09:58:04 -0800
Subject: [PATCH] Evaluate computed property dependencies first. Fixes #5143

---
 lib/mixins/property-effects.js         | 135 ++++++++++++++++++++++---
 lib/utils/settings.js                  |  17 ++++
 test/unit/property-effects-elements.js |  41 ++++++++
 test/unit/property-effects.html        |  93 ++++++++++++++++-
 4 files changed, 271 insertions(+), 15 deletions(-)

diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js
index 65a2e31f80..c60a0341b0 100644
--- a/lib/mixins/property-effects.js
+++ b/lib/mixins/property-effects.js
@@ -19,7 +19,7 @@ import { camelToDashCase, dashToCamelCase } from '../utils/case-map.js';
 import { PropertyAccessors } from './property-accessors.js';
 /* for annotated effects */
 import { TemplateStamp } from './template-stamp.js';
-import { sanitizeDOMValue, legacyUndefined, legacyNoBatch, legacyNotifyOrder } from '../utils/settings.js';
+import { sanitizeDOMValue, legacyUndefined, legacyNoBatch, legacyNotifyOrder, orderedComputed } from '../utils/settings.js';
 
 // Monotonically increasing unique ID used for de-duping effects triggered
 // from multiple properties in the same turn
@@ -40,6 +40,8 @@ const TYPES = {
   READ_ONLY: '__readOnly'
 };
 
+const COMPUTE_INFO = '__computeInfo';
+
 /** @const {!RegExp} */
 const capitalAttributeRegex = /[A-Z]/;
 
@@ -80,20 +82,23 @@ let DataEffect; //eslint-disable-line no-unused-vars
  *
  * @param {Object} model Prototype or instance
  * @param {string} type Property effect type
+ * @param {boolean=} cloneArrays Clone any arrays mapped in the map
  * @return {Object} The own-property map of effects for the given type
  * @private
  */
-function ensureOwnEffectMap(model, type) {
+function ensureOwnEffectMap(model, type, cloneArrays) {
   let effects = model[type];
   if (!effects) {
     effects = model[type] = {};
   } else if (!model.hasOwnProperty(type)) {
     effects = model[type] = Object.create(model[type]);
-    for (let p in effects) {
-      let protoFx = effects[p];
-      let instFx = effects[p] = Array(protoFx.length);
-      for (let i=0; i<protoFx.length; i++) {
-        instFx[i] = protoFx[i];
+    if (cloneArrays) {
+      for (let p in effects) {
+        let protoFx = effects[p];
+        let instFx = effects[p] = Array(protoFx.length);
+        for (let i=0; i<protoFx.length; i++) {
+          instFx[i] = protoFx[i];
+        }
       }
     }
   }
@@ -415,10 +420,14 @@ function runComputedEffects(inst, changedProps, oldProps, hasPaths) {
       Object.assign(/** @type {!Object} */ (changedProps), inst.__dataPending);
       inputProps = inst.__dataPending;
       inst.__dataPending = null;
+      // Ensure all computed properties are de-duped against the same turn
+      dedupeId--;
     }
   }
 }
 
+const TRANSITIVE_DEPENDENCY = '~transitive~dependency~';
+
 /**
  * Implements the "computed property" effect by running the method with the
  * values of the arguments specified in the `info` object and setting the
@@ -426,14 +435,28 @@ function runComputedEffects(inst, changedProps, oldProps, hasPaths) {
  *
  * @param {!Polymer_PropertyEffects} inst The instance the effect will be run on
  * @param {string} property Name of property
- * @param {?Object} props Bag of current property changes
+ * @param {?Object} changedProps Bag of current property changes
  * @param {?Object} oldProps Bag of previous values for changed properties
  * @param {?} info Effect metadata
  * @return {void}
  * @private
  */
-function runComputedEffect(inst, property, props, oldProps, info) {
-  let result = runMethodEffect(inst, property, props, oldProps, info);
+function runComputedEffect(inst, property, changedProps, oldProps, info) {
+  if (orderedComputed) {
+    // Compute any computed dependencies first; this recurses through the
+    // dependency graph to ensure computed properties are never computed with
+    // dependencies that may become invalidated later in this turn
+    computeDependencies(inst, info, changedProps, oldProps);
+    // If the dependency was not transitive, it's definitely dirty and needs to
+    // be computed; if it is transitive, check its arguments against the current
+    // changed props and only re-compute if it is dirty
+    if (property === TRANSITIVE_DEPENDENCY && 
+        !computedPropertyIsDirty(inst, info, changedProps)) {
+      return;
+    }
+  }
+  // Dirty check dependencies and run if any invalid
+  let result = runMethodEffect(inst, property, changedProps, oldProps, info);
   // Abort if method returns a no-op result
   if (result === NOOP) {
     return;
@@ -446,6 +469,86 @@ function runComputedEffect(inst, property, props, oldProps, info) {
   }
 }
 
+/**
+ * Returns any dependencies of a computed property that are themselves
+ * also computed.
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to test
+ * @param {?} info Computed effect metadata
+ * @return {Array<?>} Array of computed effect metadata for depenencies
+ * @private
+ */
+ function computedDependenciesFor(inst, info) {
+  let deps = info.computedDependencies;
+  if (!deps) {
+    deps = info.computedDependencies = [];
+    info.args.forEach(arg => {
+      const dep = arg.rootProperty || arg.name;
+      const depInfo = inst[COMPUTE_INFO][dep];
+      if (depInfo) {
+        deps.push(depInfo);
+      }
+    });
+  }
+  return deps;
+}
+
+/**
+ * Runs computed effects for dependencies of the computed property effect
+ * described by `info`.
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to test
+ * @param {Object} info Effect metadata
+ * @param {Object} changedProps Bag of current property changes
+ * @param {Object} oldProps Bag of previous values for changed properties
+ * @return {void}
+ * @private
+ */
+ function computeDependencies(inst, info, changedProps, oldProps) {
+  let deps = computedDependenciesFor(inst, info);
+  if (deps.length) {
+    deps.forEach(depInfo => {
+      if (depInfo.lastRun !== info.lastRun) {
+        depInfo.lastRun = info.lastRun;
+        runComputedEffect(inst, TRANSITIVE_DEPENDENCY, changedProps, oldProps, depInfo);
+      }
+    });
+  }
+}
+
+/**
+ * Returns whether the computed effect has any changed dependencies.
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to test
+ * @param {?} info Effect metadata
+ * @param {Object} changedProps Bag of current property changes
+ * @return {boolean} true when the computed effect should be re-calculated
+ * @private
+ */
+function computedPropertyIsDirty(inst, info, changedProps) {
+  return info.dynamicFn && propertyIsDirty(inst, {name: info.methodName}, changedProps) ||
+    info.args.some(arg => propertyIsDirty(inst, arg, changedProps));
+}
+
+/**
+ * Returns whether any changed props match the effect trigger
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to test
+ * @param {DataTrigger} trigger Descriptor
+ * @param {Object} changedProps Bag of current property changes
+ * @return {boolean} true when the property is dirty
+ * @private
+ */
+function propertyIsDirty(inst, trigger, changedProps) {
+  return [changedProps, inst.__dataPending].some(props => {
+    for (let p in props) {
+      if (pathMatchesTrigger(p, trigger)) {
+        return true;
+      }
+    }
+  });
+}
+
 /**
  * Computes path changes based on path links set up using the `linkPaths`
  * API.
@@ -778,7 +881,7 @@ function addNotifyListener(node, inst, binding) {
  * @param {boolean|Object=} dynamicFn Boolean or object map indicating whether
  *   method names should be included as a dependency to the effect. Note,
  *   defaults to true if the signature is static (sig.static is true).
- * @return {void}
+ * @return {Object} Effect metadata for this method effect
  * @private
  */
 function createMethodEffect(model, sig, type, effectFn, methodInfo, dynamicFn) {
@@ -802,6 +905,7 @@ function createMethodEffect(model, sig, type, effectFn, methodInfo, dynamicFn) {
       fn: effectFn, info: info
     });
   }
+  return info;
 }
 
 /**
@@ -1152,6 +1256,8 @@ export const PropertyEffects = dedupingMixin(superClass => {
       /** @type {Object} */
       this.__computeEffects;
       /** @type {Object} */
+      this.__computeInfo;
+      /** @type {Object} */
       this.__reflectEffects;
       /** @type {Object} */
       this.__notifyEffects;
@@ -1239,7 +1345,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
     _addPropertyEffect(property, type, effect) {
       this._createPropertyAccessor(property, type == TYPES.READ_ONLY);
       // effects are accumulated into arrays per property based on type
-      let effects = ensureOwnEffectMap(this, type)[property];
+      let effects = ensureOwnEffectMap(this, type, true)[property];
       if (!effects) {
         effects = this[type][property] = [];
       }
@@ -1256,7 +1362,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
      * @return {void}
      */
     _removePropertyEffect(property, type, effect) {
-      let effects = ensureOwnEffectMap(this, type)[property];
+      let effects = ensureOwnEffectMap(this, type, true)[property];
       let idx = effects.indexOf(effect);
       if (idx >= 0) {
         effects.splice(idx, 1);
@@ -2249,7 +2355,8 @@ export const PropertyEffects = dedupingMixin(superClass => {
       if (!sig) {
         throw new Error("Malformed computed expression '" + expression + "'");
       }
-      createMethodEffect(this, sig, TYPES.COMPUTE, runComputedEffect, property, dynamicFn);
+      const info = createMethodEffect(this, sig, TYPES.COMPUTE, runComputedEffect, property, dynamicFn);
+      ensureOwnEffectMap(this, COMPUTE_INFO)[property] = info;
     }
 
     /**
diff --git a/lib/utils/settings.js b/lib/utils/settings.js
index dbfc58f018..271bdf9547 100644
--- a/lib/utils/settings.js
+++ b/lib/utils/settings.js
@@ -228,3 +228,20 @@ export let legacyNotifyOrder = false;
 export const setLegacyNotifyOrder = function(useLegacyNotifyOrder) {
   legacyNotifyOrder = useLegacyNotifyOrder;
 };
+
+/**
+ * Setting to ensure computed properties are computed in order to ensure
+ * re-computation never occurs in a given turn.
+ */
+export let orderedComputed = false;
+
+/**
+ * Sets `orderedComputed` globally for all elements to enable ordered computed
+ * property computation.
+ *
+ * @param {boolean} useOrderedComputed enable or disable ordered computed effects
+ * @return {void}
+ */
+export const setOrderedComputed = function(useOrderedComputed) {
+  orderedComputed = useOrderedComputed;
+};
diff --git a/test/unit/property-effects-elements.js b/test/unit/property-effects-elements.js
index 0e0f6094d6..7d5c16375b 100644
--- a/test/unit/property-effects-elements.js
+++ b/test/unit/property-effects-elements.js
@@ -1011,6 +1011,47 @@ class SubObserverElement extends SuperObserverElement {
 }
 customElements.define(SubObserverElement.is, SubObserverElement);
 
+customElements.define('x-computed-ordering', class extends PolymerElement {
+  static get properties() {
+    return {
+      a: {type: Number},
+      b: {type: Number},
+      c: {type: Number},
+      d: {type: Number},
+      abbcd: {computed: 'computeABBCD(a, b, bcd)', observer: 'abbcdChanged'},
+      bcd: {computed: 'computeBCD(bc, d)', observer: 'bcdChanged'},
+      bc: {computed: 'computeBC(b, c)', observer: 'bcChanged'},
+      circIn: {type: Number},
+      circA: {computed: 'computeCircA(circIn, circB)'},
+      circB: {computed: 'computeCircA(circIn, circA)'},
+    };
+  }
+  constructor() {
+    super();
+    sinon.spy(this, 'computeABBCD');
+    sinon.spy(this, 'computeBCD');
+    sinon.spy(this, 'computeBC');
+    this.abbcdChanged = sinon.spy();
+    this.bcdChanged = sinon.spy();
+    this.bcChanged = sinon.spy();
+  }
+  computeABBCD(a, b, bcd) {
+    return a + b + bcd;
+  }
+  computeBCD(bc, d) {
+    return bc + d;
+  }
+  computeBC(b, c) {
+    return b + c;
+  }
+  computeCircA(circIn, circB) {
+    return circIn + (circB || 0);
+  }
+  computeCircB(circIn, circA) {
+    return circIn + (circA || 0);
+  }
+});
+  
 class SVGElement extends PolymerElement {
   static get template() {
     return html`
diff --git a/test/unit/property-effects.html b/test/unit/property-effects.html
index b44344df4a..bd64d8bfd6 100644
--- a/test/unit/property-effects.html
+++ b/test/unit/property-effects.html
@@ -26,7 +26,7 @@
 <script type="module">
 import './property-effects-elements.js';
 import { Polymer, html } from '../../polymer-legacy.js';
-import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyWarnings, legacyUndefined, setLegacyUndefined, legacyNoBatch, setLegacyNoBatch, legacyNotifyOrder, setLegacyNotifyOrder } from '../../lib/utils/settings.js';
+import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyWarnings, legacyUndefined, setLegacyUndefined, legacyNoBatch, setLegacyNoBatch, legacyNotifyOrder, setLegacyNotifyOrder, setOrderedComputed } from '../../lib/utils/settings.js';
 import { PropertyEffects } from '../../lib/mixins/property-effects.js';
 import { flush } from '../../lib/utils/flush.js';
 
@@ -2087,6 +2087,97 @@
   });
 
 });
+
+suite('computed property ordering', function() {
+  var el;
+
+  setup(function() {
+    setOrderedComputed(true);
+    el = document.createElement('x-computed-ordering');
+    document.body.appendChild(el);
+  });
+
+  teardown(function() {
+    setOrderedComputed(false);
+    document.body.removeChild(el);
+  });
+
+  test('computed property ordering', function() {
+
+    el.setProperties({
+      a: 1000,
+      b: 100,
+      c: 10,
+      d: 1
+    });
+    assert.equal(el.bc, 110);
+    assert.equal(el.bcd, 111);
+    assert.equal(el.abbcd, 1211);
+    assert.equal(el.computeBC.callCount, 1);
+    assert.equal(el.computeBCD.callCount, 1);
+    assert.equal(el.computeABBCD.callCount, 1);
+
+    el.setProperties({
+      b: 200,
+      c: 20
+    });
+    assert.equal(el.bc, 220);
+    assert.equal(el.bcd, 221);
+    assert.equal(el.abbcd, 1421);
+    assert.equal(el.computeBC.callCount, 2);
+    assert.equal(el.computeBCD.callCount, 2);
+    assert.equal(el.computeABBCD.callCount, 2);
+
+    el.a = 2000;
+    assert.equal(el.bc, 220);
+    assert.equal(el.bcd, 221);
+    assert.equal(el.abbcd, 2421);
+    assert.equal(el.computeBC.callCount, 2);
+    assert.equal(el.computeBCD.callCount, 2);
+    assert.equal(el.computeABBCD.callCount, 3);
+
+    el.c = 30;
+    assert.equal(el.bc, 230);
+    assert.equal(el.bcd, 231);
+    assert.equal(el.abbcd, 2431);
+    assert.equal(el.computeBC.callCount, 3);
+    assert.equal(el.computeBCD.callCount, 3);
+    assert.equal(el.computeABBCD.callCount, 4);
+
+    el.d = 2;
+    assert.equal(el.bc, 230);
+    assert.equal(el.bcd, 232);
+    assert.equal(el.abbcd, 2432);
+    assert.equal(el.computeBC.callCount, 3);
+    assert.equal(el.computeBCD.callCount, 4);
+    assert.equal(el.computeABBCD.callCount, 5);
+
+    el.setProperties({
+      a: 1000,
+      b: 100,
+      c: 10,
+      d: 1
+    });
+    assert.equal(el.bc, 110);
+    assert.equal(el.bcd, 111);
+    assert.equal(el.abbcd, 1211);
+    assert.equal(el.computeBC.callCount, 4);
+    assert.equal(el.computeBCD.callCount, 5);
+    assert.equal(el.computeABBCD.callCount, 6);
+
+  });  
+
+  test('ensure circular dependencies do not iloop', function() {
+    el.circIn = 1;
+    // The answers here don't _really_ matter; it just shouldn't iloop
+    // circA will go first (defined first), which will pull circB, which
+    // has no circA yet, and thus returns 1 for this turn
+    assert.ok(el.circA); // should be equal to 2, based on impl
+    assert.ok(el.circB); // should be equal to 1, based on impl
+  });
+
+});
+
 </script>
 
 </body>