[PatternMatchingInstanceof] Make negated matches optional

This introduces a configuration option that allows one to disable
the replacement of casts that are implied by a negated instanceof
expression and are not within the same expression as the instanceof.
This affects casts in else branches and those below an if statement
where the then branch always returns from the method.
Casts where the readability benefit of a pattern-maching instanceof
is clear, i.e. those within the then branch of an if
and those inside the same expression, are still replaced.

The default of the option is true, which keeps the current behavior.

Fixes #4925

Author: PhilippWendler

core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java

@@ -29,6 +29,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.InstanceOfTreeMatcher;
 import com.google.errorprone.bugpatterns.threadsafety.ConstantExpressions;
@@ -64,9 +65,13 @@
 public final class PatternMatchingInstanceof extends BugChecker implements InstanceOfTreeMatcher {
   private final ConstantExpressions constantExpressions;
 
+  private final boolean enableNegatedMatches;
+
   @Inject
-  PatternMatchingInstanceof(ConstantExpressions constantExpressions) {
+  PatternMatchingInstanceof(ConstantExpressions constantExpressions, ErrorProneFlags flags) {
     this.constantExpressions = constantExpressions;
+    this.enableNegatedMatches =
+        flags.getBoolean("PatternMatchingInstanceof:EnableNegatedMatches").orElse(true);
   }
 
   @Override
@@ -169,7 +174,7 @@ private static String generateVariableName(Type targetType, VisitorState state)
   }
 
   /** Finds trees which are implied by the {@code instanceOfTree}. */
-  private static ImmutableList<Tree> findImpliedStatements(
+  private ImmutableList<Tree> findImpliedStatements(
       InstanceOfTree tree, VisitorState state) {
     Tree last = tree;
     boolean negated = false;
@@ -204,19 +209,23 @@ private static ImmutableList<Tree> findImpliedStatements(
           if (ifTree.getCondition() != last) {
             return impliedStatements.build();
           }
-          StatementTree positiveBranch =
-              negated ? ifTree.getElseStatement() : ifTree.getThenStatement();
-          if (positiveBranch != null) {
-            impliedStatements.add(positiveBranch);
-          }
-          StatementTree negativeBranch =
-              negated ? ifTree.getThenStatement() : ifTree.getElseStatement();
-          if (negativeBranch != null && !Reachability.canCompleteNormally(negativeBranch)) {
-            if (parentPath.getParentPath().getLeaf() instanceof BlockTree blockTree) {
-              var index = blockTree.getStatements().indexOf(ifTree);
-              impliedStatements.addAll(
-                  blockTree.getStatements().subList(index + 1, blockTree.getStatements().size()));
+          if (enableNegatedMatches) {
+            StatementTree positiveBranch =
+                negated ? ifTree.getElseStatement() : ifTree.getThenStatement();
+            if (positiveBranch != null) {
+              impliedStatements.add(positiveBranch);
+            }
+            StatementTree negativeBranch =
+                negated ? ifTree.getThenStatement() : ifTree.getElseStatement();
+            if (negativeBranch != null && !Reachability.canCompleteNormally(negativeBranch)) {
+              if (parentPath.getParentPath().getLeaf() instanceof BlockTree blockTree) {
+                var index = blockTree.getStatements().indexOf(ifTree);
+                impliedStatements.addAll(
+                    blockTree.getStatements().subList(index + 1, blockTree.getStatements().size()));
+              }
             }
+          } else if (!negated) {
+            impliedStatements.add(ifTree.getThenStatement());
           }
           return impliedStatements.build();
         }

core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java

@@ -57,6 +57,36 @@ void test(Object o) {
         .doTest();
   }
 
+  @Test
+  public void positive_disabledNegation() {
+    helper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Object o) {
+                if (o instanceof Test) {
+                  Test test = (Test) o;
+                  test(test);
+                }
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Object o) {
+                if (o instanceof Test test) {
+                  test(test);
+                }
+              }
+            }
+            """)
+        .setArgs("-XepOpt:PatternMatchingInstanceof:EnableNegatedMatches=false")
+        .doTest();
+  }
+
   @Test
   public void negatedIf() {
     helper
@@ -88,6 +118,27 @@ void test(Object o) {
         .doTest();
   }
 
+  @Test
+  public void negatedIf_disabledNegation() {
+    helper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Object o) {
+                if (!(o instanceof Test)) {
+                } else {
+                  Test test = (Test) o;
+                  test(test);
+                }
+              }
+            }
+            """)
+        .expectUnchanged()
+        .setArgs("-XepOpt:PatternMatchingInstanceof:EnableNegatedMatches=false")
+        .doTest();
+  }
+
   @Test
   public void withinIf_elseCannotCompleteNormally_variableInScopeForStatementsAfter() {
     helper
@@ -187,6 +238,27 @@ void test(Object o) {
         .doTest();
   }
 
+  @Test
+  public void negatedIfWithReturn_disabledNegation() {
+    helper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              void test(Object o) {
+                if (!(o instanceof Test)) {
+                  return;
+                }
+                Test test = (Test) o;
+                test(test);
+              }
+            }
+            """)
+        .expectUnchanged()
+        .setArgs("-XepOpt:PatternMatchingInstanceof:EnableNegatedMatches=false")
+        .doTest();
+  }
+
   @Test
   public void negatedIf_butNoDefiniteReturn_noFinding() {
     helper
@@ -618,6 +690,47 @@ public boolean equals(Object o) {
         .doTest();
   }
 
+  @Test
+  public void withinIfCondition_andUsedAfter_disabledNegation() {
+    helper
+        .addInputLines(
+            "Test.java",
+            """
+            class Test {
+              private final int x = 0;
+              private final int y = 1;
+
+              @Override
+              public boolean equals(Object o) {
+                if (!(o instanceof Test) || ((Test) o).x != this.x) {
+                  return false;
+                }
+                Test other = (Test) o;
+                return other.y == this.y;
+              }
+            }
+            """)
+        .addOutputLines(
+            "Test.java",
+            """
+            class Test {
+              private final int x = 0;
+              private final int y = 1;
+
+              @Override
+              public boolean equals(Object o) {
+                if (!(o instanceof Test test) || test.x != this.x) {
+                  return false;
+                }
+                Test other = (Test) o;
+                return other.y == this.y;
+              }
+            }
+            """)
+        .setArgs("-XepOpt:PatternMatchingInstanceof:EnableNegatedMatches=false")
+        .doTest();
+  }
+
   @Test
   public void conditionalExpression() {
     helper

docs/bugpattern/PatternMatchingInstanceof.md

@@ -24,5 +24,11 @@ void handle(Object o) {
 }
 ```
 
+If the flag `-XepOpt:PatternMatchingInstanceof:EnableNegatedMatches=false` is used,
+only casts that are in the same expression as the instanceof
+or in the block that starts with the instanceof are rewritten,
+but casts inside else branches or below an if statement with a negated instanceof
+are left unchanged.
+
 For more information on pattern matching and `instanceof`, see
 [Pattern Matching for the instanceof Operator](https://docs.oracle.com/en/java/javase/21/language/pattern-matching-instanceof.html)