Added delete statement logic for specific patterns menntioned in issue …

…uber#104
ketkarameya · Dec 1, 2021 · 2ef2671 · 2ef2671
1 parent 4d2330b
commit 2ef2671
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 0 deletions.
diff --git a/java/piranha/build.gradle b/java/piranha/build.gradle
@@ -37,6 +37,7 @@ dependencies {
 
     testCompile deps.test.junit4
     testCompile deps.test.daggerAnnotations
+    testImplementation group: 'org.mockito', name: 'mockito-core', version: '2.1.0'
     testCompile(deps.build.errorProneTestHelpers) {
         exclude group: "junit", module: "junit"
     }

diff --git a/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java b/java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
@@ -14,6 +14,16 @@
 package com.uber.piranha;
 
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.booleanConstant;
+import static com.google.errorprone.matchers.Matchers.booleanLiteral;
+import static com.google.errorprone.matchers.Matchers.contains;
+import static com.google.errorprone.matchers.Matchers.enclosingNode;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.methodInvocation;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
 
 import com.facebook.infer.annotation.Initializer;
 import com.google.auto.service.AutoService;
@@ -27,6 +37,7 @@
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.util.ASTHelpers;
 import com.google.errorprone.util.FindIdentifiers;
 import com.sun.source.tree.AnnotationTree;
@@ -820,6 +831,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     }
 
     Value x = evalExpr(tree, state);
+    if (x == Value.TRUE || x == Value.FALSE) {
+      if (enclosingNode(MATCH_SPECIFIC_PATTERN).matches(tree, state)) {
+        StatementTree stmt = ASTHelpers.findEnclosingNode(state.getPath(), StatementTree.class);
+        if (stmt != null) {
+          return buildDescription(stmt).addFix(SuggestedFix.delete(stmt)).build();
+        }
+      }
+    }
     return updateCode(x, tree, tree, state);
   }
 
@@ -1353,6 +1372,36 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
     return builder.build();
   }
 
+  private final Matcher<ExpressionTree> MATCH_XP_API =
+      (a, vs) -> {
+        Value value = evalExpr(a, vs);
+        return value == Value.TRUE || value == Value.FALSE;
+      };
+
+  private final Matcher<ExpressionTree> MATCH_BOOLEANS =
+      anyOf(
+          booleanLiteral(true),
+          booleanLiteral(false),
+          booleanConstant(true),
+          booleanConstant(false));
+
+  // Had to wrap everything in `contains` to satisfy the type checker.
+  // I think we don't need to have the `thenReturn` clause, do we?
+  private final Matcher<Tree> MATCH_SPECIFIC_PATTERN =
+      contains(
+          ExpressionTree.class,
+          methodInvocation(
+              allOf(
+                  instanceMethod().anyClass().named("thenReturn"),
+                  contains(
+                      ExpressionTree.class,
+                      methodInvocation(
+                          staticMethod().onClass("org.mockito.Mockito").named("when"),
+                          ALL,
+                          MATCH_XP_API))),
+              ALL,
+              MATCH_BOOLEANS));
+
   @Override
   public Description matchConditionalExpression(
       ConditionalExpressionTree tree, VisitorState state) {

diff --git a/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java b/java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java
@@ -499,4 +499,46 @@ public void testIgnoresPrefixMatchFlag() throws IOException {
             "class TestClassPartialMatch { }")
         .doTest();
   }
+
+  @Test
+  public void testRemoveSpecificAPIpatterns() throws IOException {
+
+    ErrorProneFlags.Builder b = ErrorProneFlags.builder();
+    b.putFlag("Piranha:FlagName", "STALE_FLAG");
+    b.putFlag("Piranha:IsTreated", "false");
+    b.putFlag("Piranha:Config", "config/properties.json");
+
+    BugCheckerRefactoringTestHelper bcr =
+        BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass());
+
+    bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath());
+    bcr = PiranhaTestingHelpers.addHelperClasses(bcr);
+    bcr.addInputLines(
+            "XPFlagCleanerSinglePositiveCase.java",
+            "package com.uber.piranha;",
+            "import static org.mockito.Mockito.when;",
+            "class MockitoTest {",
+            " private XPTest experimentation;",
+            " public void evaluate() {",
+            "  when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenReturn(false);",
+            "  when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenReturn(\"Should not Delete\");",
+            "  when(foobar()).thenReturn(false);",
+            " }",
+            " public boolean foobar() { return true;}",
+            "}")
+        .addOutputLines(
+            "XPFlagCleanerSinglePositiveCase.java",
+            "package com.uber.piranha;",
+            "import static org.mockito.Mockito.when;",
+            "class MockitoTest {",
+            " private XPTest experimentation;",
+            " public void evaluate() {",
+            "",
+            "  when(foobar()).thenReturn(false);",
+            " }",
+            " public boolean foobar() { return true;}",
+            "}")
+        .doTest();
+    //    Mockito.when()
+  }
 }