VariableNameUtils#generateVariableName() should consider following statements

[knutwannheden]

When using VariableNameUtils#generateVariableName() to generate variable names to be inserted at a given position in the code (like it is being used in the ZipSlip recipe of rewrite-java-security, which is the only use I found), then the logic should also consider the statements that follow the given cursor (which will roughly be the insertion point for the variable), otherwise when a new variable is inserted it could conflict with following variables.

In the following example the utility would for a base name of s calculate the name s1 even if that conflicts with a variable that follows. Thus, inserting a variable would render the class uncompilable.

class A {
  void test() {
    String s = null;
    ; // <-- insert new variable with base name "s" here
    String s1 = null;
  }
}

I am currently facing this issue in the InstanceOfPatternMatch recipe.

[knutwannheden]

Also note that VariableNameUtils is currently unaware of the flow scope introduced by the instanceof pattern matching. See the following test, where the commented out test cases should actually also succeed. At least the probe1 case should be supported. I think the others are probably quite rare.

    @ParameterizedTest
    @CsvSource(value = {
//      "probe1:probe1,s1,o",
      "probe2:probe2,o",
      "probe3:probe3,probe2,o",
//      "probe4:probe4,n,probe2,o",
//      "probe5:probe5,n,probe2,o",
    }, delimiter = ':')
    void instanceOfPatternFlowScope(String scope, String result) {
        rewriteRun(
          baseTest(scope, result),
          java(
            """
              class Test {
                  void m(Object o) {
                      if (o instanceof String s1) {
                          Object probe1 = s1;
                      }
                      Object probe2;
                      if (!(o instanceof Number n)) {
                          Object probe3;
                          throw new IllegalStateException();
                      } else {
                          Object probe4 = n;
                      }
                      Object probe5 = n;
                  }
              }
              """
          )
        );
    }

@traceyyoshima I should probably report a separate issue for this. What do you think?

[traceyyoshima]

Also note that VariableNameUtils is currently unaware of the flow scope introduced by the instanceof pattern matching. See the following test, where the commented out test cases should actually also succeed. At least the probe1 case should be supported. I think the others are probably quite rare.

Hi @knutwannheden, yes, I think these should be two separate issues to make the work clearer.

[timtebeek]

So the specific case has been fixed here in

• Fix clashing variable names generated by InstanceOfPatternMatch rewrite-static-analysis#538

We could still make improvements to VariableNameUtils itself, but with enough items on the backlog already I propose we do so when we find it's needed downstream.