Fix NPE caused by bad scan() in TreeDiffer.java

cgruber · cgruber · commit 353db303733d · 2014-07-17T13:46:01.000-07:00
Old scan() ignored the possibility of an expected node having a null value when an actual node existed. The new scan() accounts for this possibility and also throws a more relevant NPE should another similar situation arise. ------------- Created by MOE: http://code.google.com/p/moe-java MOE_MIGRATED_REVID=68173457
diff --git a/src/main/java/com/google/testing/compile/TreeDiffer.java b/src/main/java/com/google/testing/compile/TreeDiffer.java
@@ -181,10 +181,12 @@ private void checkForDiff(boolean p, String message, Object... formatArgs) {
     }
 
     private TreePath actualPathPlus(Tree actual) {
+      checkNotNull(actual, "Tried to push null actual tree onto path.");
       return new TreePath(actualPath, actual);
     }
 
     private TreePath expectedPathPlus(Tree expected) {
+      checkNotNull(expected, "Tried to push null expected tree onto path.");
       return new TreePath(expectedPath, expected);
     }
 
@@ -209,13 +211,14 @@ private Void pushPathAndAccept(Tree expected, Tree actual) {
     }
 
     public Void scan(@Nullable Tree expected, @Nullable Tree actual) {
-      if (expected == null) {
-        if (actual != null) {
-          diffBuilder.addExtraActualNode(actualPathPlus(actual));
-        }
-        return null;
+      if (expected == null && actual != null) {
+        diffBuilder.addExtraActualNode(actualPathPlus(actual));
+      } else if (expected != null && actual == null) {
+        diffBuilder.addExtraExpectedNode(expectedPathPlus(expected));
+      } else if (actual != null && expected != null) {
+        pushPathAndAccept(expected, actual);
       }
-      return pushPathAndAccept(expected, actual);
+      return null;
     }
 
     private Void parallelScan(Iterable<? extends Tree> expecteds,
diff --git a/src/test/java/com/google/testing/compile/TreeDifferTest.java b/src/test/java/com/google/testing/compile/TreeDifferTest.java
@@ -56,7 +56,6 @@ public class TreeDifferTest {
       MoreTrees.parseLinesToTree("package test;",
           "import java.util.List;",
           "",
-          "",
           "final class TestClass {",
           "    public String toString() {",
           "        Object variable = new Object();",
@@ -71,11 +70,24 @@ public class TreeDifferTest {
           "            }",
           "        }",
           "    }",
-          "",
           "    public int extraNonsense() {",
           "      return 0;",
           "    }",
           "}");
+  private static final CompilationUnitTree ASSERT_TREE_WITH_MESSAGE =
+      MoreTrees.parseLinesToTree("package test;",
+          "final class TestClass {",
+          "   private void fail() {",
+          "      assert false : \"details\";",
+          "   }",
+          "}");
+  private static final CompilationUnitTree ASSERT_TREE_WITHOUT_MESSAGE =
+      MoreTrees.parseLinesToTree("package test;",
+          "final class TestClass {",
+          "   private void fail() {",
+          "      assert false;",
+          "   }",
+          "}");
 
   @Test
   public void scan_differingCompilationUnits() {
@@ -108,7 +120,15 @@ public void scan_differingCompilationUnits() {
       differingNodesFound.add(SimplifiedDiff.create(differingNode));
     }
     ASSERT.that(differingNodesExpected).iteratesAs(differingNodesFound.build());
+  }
 
+  @Test
+  public void scan_testExtraFields() {
+    TreeDifference diff =
+        TreeDiffer.diffCompilationUnits(ASSERT_TREE_WITH_MESSAGE, ASSERT_TREE_WITHOUT_MESSAGE);
+    ASSERT.that(diff.isEmpty()).isFalse();
+    diff = TreeDiffer.diffCompilationUnits(ASSERT_TREE_WITHOUT_MESSAGE, ASSERT_TREE_WITH_MESSAGE);
+    ASSERT.that(diff.isEmpty()).isFalse();
   }
 
   @Test

Original file line number	Diff line number	Diff line change
`@@ -181,10 +181,12 @@ private void checkForDiff(boolean p, String message, Object... formatArgs) {`
`181`	`181`	`}`
`182`	`182`
`183`	`183`	`private TreePath actualPathPlus(Tree actual) {`
	`184`	`+ checkNotNull(actual, "Tried to push null actual tree onto path.");`
`184`	`185`	`return new TreePath(actualPath, actual);`
`185`	`186`	`}`
`186`	`187`
`187`	`188`	`private TreePath expectedPathPlus(Tree expected) {`
	`189`	`+ checkNotNull(expected, "Tried to push null expected tree onto path.");`
`188`	`190`	`return new TreePath(expectedPath, expected);`
`189`	`191`	`}`
`190`	`192`
`@@ -209,13 +211,14 @@ private Void pushPathAndAccept(Tree expected, Tree actual) {`
`209`	`211`	`}`
`210`	`212`
`211`	`213`	`public Void scan(@Nullable Tree expected, @Nullable Tree actual) {`
`212`		`- if (expected == null) {`
`213`		`- if (actual != null) {`
`214`		`- diffBuilder.addExtraActualNode(actualPathPlus(actual));`
`215`		`- }`
`216`		`- return null;`
	`214`	`+ if (expected == null && actual != null) {`
	`215`	`+ diffBuilder.addExtraActualNode(actualPathPlus(actual));`
	`216`	`+ } else if (expected != null && actual == null) {`
	`217`	`+ diffBuilder.addExtraExpectedNode(expectedPathPlus(expected));`
	`218`	`+ } else if (actual != null && expected != null) {`
	`219`	`+ pushPathAndAccept(expected, actual);`
`217`	`220`	`}`
`218`		`- return pushPathAndAccept(expected, actual);`
	`221`	`+ return null;`
`219`	`222`	`}`
`220`	`223`
`221`	`224`	`private Void parallelScan(Iterable<? extends Tree> expecteds,`