Fix expression evaluation problem

This CL fixes a bug in expression evaluator where we would
elevate a conditional dependency by mistake when it is not
read completely. This was caused by marking bit paths as
read in the same cycle.

There is still a potential bug in the code path where we
detect a variable would've already been calculated for a
dependency to be calculated (e.g. a ? a : b) but this
change is a fairly safe compared to making a big change
in that logic.

Bug: 895468
Change-Id: I60a704a59c3b5b8e4f833f060c6233d356dab6c8

4 files changed, 103 insertions, 26 deletions

diff --git a/compiler/src/main/java/android/databinding/tool/expr/Expr.java b/compiler/src/main/java/android/databinding/tool/expr/Expr.java
index 768a59a5..42d98a19 100644
--- a/compiler/src/main/java/android/databinding/tool/expr/Expr.java
+++ b/compiler/src/main/java/android/databinding/tool/expr/Expr.java
@@ -553,7 +553,8 @@ abstract public class Expr implements VersionProvider, LocationScopeProvider {
                     allCovered = false;
                     break;
                 }
-                final BitSet readForConditional = (BitSet) expr.findConditionalFlags().clone();
+                final BitSet readForConditional = (BitSet) expr
+                        .getShouldReadFlagsWithConditionals().clone();
 
                 // FIXME: this does not do full traversal so misses some cases
                 // to calculate that conditional, i should've read /readForConditional/ flags
diff --git a/compiler/src/main/java/android/databinding/tool/expr/ExprModel.java b/compiler/src/main/java/android/databinding/tool/expr/ExprModel.java
index e9cdfab2..87169cee 100644
--- a/compiler/src/main/java/android/databinding/tool/expr/ExprModel.java
+++ b/compiler/src/main/java/android/databinding/tool/expr/ExprModel.java
@@ -589,15 +589,17 @@ public class ExprModel {
                 }
             }
         }
-        for (Expr partialRead : markedSomeFlagsAsRead) {
-            // even if all paths are not satisfied, we can elevate certain conditional dependencies
-            // if all of their paths are satisfied.
-            for (Dependency dependency : partialRead.getDependants()) {
-                Expr dependant = dependency.getDependant();
-                if (dependant.isConditional() && dependant.getAllCalculationPaths()
-                        .areAllPathsSatisfied(partialRead.mReadSoFar)) {
-                    if (dependant.considerElevatingConditionals(partialRead)) {
-                        elevated = true;
+        if (!elevated) {
+            for (Expr partialRead : markedSomeFlagsAsRead) {
+                // even if all paths are not satisfied, we can elevate certain conditional
+                // dependencies if all of their paths are satisfied.
+                for (Dependency dependency : partialRead.getDependants()) {
+                    Expr dependant = dependency.getDependant();
+                    if (dependant.isConditional() && dependant.getAllCalculationPaths()
+                            .areAllPathsSatisfied(partialRead.mReadSoFar)) {
+                        if (dependant.considerElevatingConditionals(partialRead)) {
+                            elevated = true;
+                        }
                     }
                 }
             }
diff --git a/compiler/src/main/kotlin/android/databinding/tool/writer/LayoutBinderWriter.kt b/compiler/src/main/kotlin/android/databinding/tool/writer/LayoutBinderWriter.kt
index 6147519b..30b2583d 100644
--- a/compiler/src/main/kotlin/android/databinding/tool/writer/LayoutBinderWriter.kt
+++ b/compiler/src/main/kotlin/android/databinding/tool/writer/LayoutBinderWriter.kt
@@ -207,7 +207,7 @@ val Expr.callbackLocalName by lazyProp { expr : Expr ->
 }
 
 val Expr.executePendingLocalName by lazyProp { expr : Expr ->
-    if(expr.needsLocalField) "${expr.model.ext.getUniqueName(expr.readableName, Scope.EXECUTE_PENDING_METHOD, false)}"
+    if(expr.isDynamic || expr.needsLocalField) "${expr.model.ext.getUniqueName(expr.readableName, Scope.EXECUTE_PENDING_METHOD, false)}"
     else expr.toCode().generate()
 }
 
@@ -898,7 +898,7 @@ class LayoutBinderWriter(val layoutBinder : LayoutBinder) {
         if (model.flagMapping != null) {
             val mapping = model.flagMapping
             for (i in mapping.indices) {
-                tab("flag $i: ${mapping[i]}")
+                tab("flag $i (${longToBinary(1L + i)}): ${model.findFlagExpression(i)}")
             }
         }
         nl("flag mapping end*/")
diff --git a/compiler/src/test/java/android/databinding/tool/expr/ExprModelTest.java b/compiler/src/test/java/android/databinding/tool/expr/ExprModelTest.java
index 20d523f6..e20dea74 100644
--- a/compiler/src/test/java/android/databinding/tool/expr/ExprModelTest.java
+++ b/compiler/src/test/java/android/databinding/tool/expr/ExprModelTest.java
@@ -432,13 +432,10 @@ public class ExprModelTest {
         assertTrue(mExprModel.markBitsRead());
 
         shouldRead = getShouldRead();
-        // FIXME: there is no real case to read u1 anymore because if b>c was not true,
-        // u1.getCond(d) will never be set. Right now, we don't have mechanism to figure this out
-        // and also it does not affect correctness (just an unnecessary if stmt)
-        assertExactMatch(shouldRead, u1, u2, u1LastName, u2LastName, bcTernary.getIfTrue(), bcTernary);
+
+        assertExactMatch(shouldRead, u2, u1LastName, u2LastName, bcTernary.getIfTrue(), bcTernary);
         firstRead = getReadFirst(shouldRead);
-        assertExactMatch(firstRead, u1, u2);
-        assertFlags(u1, bcTernary.getIfTrue().getRequirementFlagIndex(true));
+        assertExactMatch(firstRead, u1LastName, u2);
         assertFlags(u2, bcTernary.getIfTrue().getRequirementFlagIndex(false));
         assertFlags(u1LastName, bcTernary.getIfTrue().getRequirementFlagIndex(true));
         assertFlags(u2LastName, bcTernary.getIfTrue().getRequirementFlagIndex(false));
@@ -604,12 +601,13 @@ public class ExprModelTest {
         Collections.addAll(justRead, a, b, c);
         assertEquals(0, filterOut(getReadFirst(shouldRead, justRead), justRead).size());
         assertTrue(mExprModel.markBitsRead());
+
         shouldRead = getShouldRead();
         // if a and b are not invalid, a won't be read in the first step. But if c's expression
         // is invalid and c == true, a must be read. Depending on a, d might be read as well.
         // don't need to read b anymore because `a ? b : true` and `b ? a : false` has the same
         // invalidation flags.
-        assertExactMatch(shouldRead, a, abTernary, baTernary);
+        assertExactMatch(shouldRead, a, baTernary);
         justRead.clear();
 
         readFirst = getReadFirst(shouldRead);
@@ -618,20 +616,23 @@ public class ExprModelTest {
         Collections.addAll(justRead, a);
 
         readFirst = filterOut(getReadFirst(shouldRead, justRead), justRead);
-        assertExactMatch(readFirst, abTernary, baTernary);
-        Collections.addAll(justRead, abTernary, baTernary);
+        assertExactMatch(readFirst, baTernary);
+        Collections.addAll(justRead, baTernary);
 
         readFirst = filterOut(getReadFirst(shouldRead, justRead), justRead);
         assertEquals(0, filterOut(getReadFirst(shouldRead, justRead), justRead).size());
         assertTrue(mExprModel.markBitsRead());
 
         shouldRead = getShouldRead();
+        // now we can read abTernary as well which had a conditional dependency on a but we did not
+        // elevate its dependency since we had other expressions elevated already
+
         // now we can read adf ternary and c ternary
         justRead.clear();
-        assertExactMatch(shouldRead, d, cTernary.getIfTrue(), cTernary);
+        assertExactMatch(shouldRead, abTernary, d, cTernary.getIfTrue(), cTernary);
         readFirst = getReadFirst(shouldRead);
-        assertExactMatch(readFirst, d);
-        Collections.addAll(justRead, d);
+        assertExactMatch(readFirst, d, abTernary);
+        Collections.addAll(justRead, d, abTernary);
         readFirst = filterOut(getReadFirst(shouldRead, justRead), justRead);
         assertExactMatch(readFirst, cTernary.getIfTrue());
         Collections.addAll(justRead, cTernary.getIfTrue());
@@ -695,6 +696,76 @@ public class ExprModelTest {
     }
 
     @Test
+    public void testInvalidateConditional() {
+        MockLayoutBinder lb = new MockLayoutBinder();
+        mExprModel = lb.getModel();
+        final IdentifierExpr foo = lb.addVariable("foo", Foo.class.getCanonicalName(), null);
+        final IdentifierExpr c = lb.addVariable("c", "int", null);
+        final TernaryExpr ternary = parse(lb, "foo.a > 0 && (foo.b > 0 || c > 0)",
+                TernaryExpr.class);
+        final ComparisonExpr fooB0 = parse(lb, "foo.b > 0", ComparisonExpr.class);
+        final FieldAccessExpr fooB = (FieldAccessExpr) fooB0.getLeft();
+        mExprModel.seal();
+        final ComparisonExpr fooA0 = (ComparisonExpr) ternary.getPred();
+        final FieldAccessExpr fooA = (FieldAccessExpr) fooA0.getLeft();
+
+        // foo.b > 0 || c > 0
+        final TernaryExpr ternaryIfTrue = (TernaryExpr) ternary.getIfTrue();
+        final ComparisonExpr c0 = (ComparisonExpr) ternaryIfTrue.getIfFalse();
+
+        List<Expr> toRead = getShouldRead();
+        assertExactMatch(toRead, foo, fooA, fooA0, fooB, fooB0);
+        List<Expr> justRead = new ArrayList<Expr>();
+        List<Expr> readNow = getReadFirst(toRead, justRead);
+        assertExactMatch(readNow, foo);
+        justRead.addAll(readNow);
+
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, fooA, fooB);
+        justRead.addAll(readNow);
+
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, fooA0, fooB0);
+        justRead.addAll(readNow);
+
+        assertTrue(mExprModel.markBitsRead());
+        justRead.clear();
+        toRead = getShouldRead();
+
+        // there is a second path to calculate fooB, fooB0 where fooB is not invalid but fooA or
+        // c is invalid.
+        assertExactMatch(toRead, fooB, fooB0);
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, fooB);
+        justRead.add(fooB);
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, fooB0);
+
+        assertTrue(mExprModel.markBitsRead());
+        justRead.clear();
+        toRead = getShouldRead();
+
+        assertExactMatch(toRead, c, c0, ternary.getIfTrue(), ternary);
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, c);
+        justRead.addAll(readNow);
+
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, c0);
+        justRead.addAll(readNow);
+
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, ternary.getIfTrue());
+        justRead.addAll(readNow);
+
+        readNow = filterOut(getReadFirst(toRead, justRead), justRead);
+        assertExactMatch(readNow, ternary);
+        justRead.addAll(readNow);
+
+        assertFalse(mExprModel.markBitsRead());
+    }
+
+    @Test
     public void testNoFlagsForNonBindingStatic() {
         MockLayoutBinder lb = new MockLayoutBinder();
         mExprModel = lb.getModel();
@@ -913,8 +984,6 @@ public class ExprModelTest {
             assertFalse("should not have " + s, hasCallbackIdentifier(mExprModel, index, s));
             index ++;
         }
-
-
     }
 
     private boolean hasIdentifier(ExprModel model, String name) {
@@ -1095,6 +1164,11 @@ public class ExprModelTest {
         return ExprModel.filterShouldRead(mExprModel.getPendingExpressions());
     }
 
+    public static class Foo {
+        public final int a = 1;
+        public final int b = 1;
+    }
+
     public static class User implements Observable {
 
         String name;