Patch suggestion of PatternMatchingInstanceOf introduces duplicate variable names

[PhilippWendler]

In Error Prone 2.37.0, the check PatternMatchingInstanceOf produces suggestions that use variable names that are already used in the existing code, or even use the same variable name twice. This fails to compile.

Example:

public class Test2 {

  int test_two_variables() {
    Object o1 = 1;
    Object o2 = 1;
    // Here Error Prone uses the variable name "i" twice.
    if (o1 instanceof Integer && o2 instanceof Integer) {
      return ((Integer) o1) + ((Integer) o2);
    }
    return 0;
  }

  void test_local_variable() {
    Object o = 1;
    // Here Error Prone uses the variable name "i" that is already used.
    Integer i = o instanceof Integer ? (Integer) o : 0;
  }
}

Generated suggestion:

--- Test.java
+++ Test.java
@@ -5,6 +5,6 @@
     Object o2 = 1;
     // Here Error Prone uses the variable name "i" twice.
-    if (o1 instanceof Integer && o2 instanceof Integer) {
-      return ((Integer) o1) + ((Integer) o2);
+    if (o1 instanceof Integer i && o2 instanceof Integer i) {
+      return i + i;
     }
     return 0;
@@ -14,5 +14,5 @@
     Object o = 1;
     // Here Error Prone uses the variable name "i" that is already used.
-    Integer i = o instanceof Integer ? (Integer) o : 0;
+    Integer i = o instanceof Integer i ? i : 0;
   }
 }

[cushon]

Thanks for the report, I think this is a known issue related to this TODO:

https://github.com/google/error-prone/blob/1966d6d30224d0e33d90271aa2f5e8d9bea4453f/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java#L112

[graememorgan]

It's actually not quite that TODO, it's more insidious. That TODO would be relevant if we clobbered another variable name in scope before we applied any fixes. What's happening in Philipp's case is that we're generating two totally separate fixes and then applying them together.

The problem is that the refactoring operates on one instanceof at a time, so in both cases i is a valid variable name, but not when you apply both. Fixing that would be pretty awkward; we'd have to scan a broader scope at once and keep track of a set of sensible names.

[PhilippWendler]

I have seen both: reuse of an existing variable name by Error Prone, and Error Prone introducing the same variable name twice. The former is probably more common, so if it is easier to fix, a partial fix for this case alone might be worth it.