Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FinalizeMethodArguments should consider unary increments/decrements #176

Closed
AlejandroBertolo opened this issue Sep 28, 2023 · 1 comment · Fixed by #180
Closed

FinalizeMethodArguments should consider unary increments/decrements #176

AlejandroBertolo opened this issue Sep 28, 2023 · 1 comment · Fixed by #180
Assignees
Labels
bug Something isn't working test provided

Comments

@AlejandroBertolo
Copy link
Contributor

AlejandroBertolo commented Sep 28, 2023

Greetings. Thanks for your work in this useful library!.

We've found a issue with the FinalizeMethodArguments recipe, by which unary operators are not considered when deciding if a parameter has been reassigned. This leads to compilation errors in processed source code.

What version of OpenRewrite are you using?

staticAnalysis 1.0.7
RecipeBom 2.3.0
RewriteMavenPlugin 5.7.1

How are you running OpenRewrite?

Through Maven plugin, in a multi-module project.

What is the smallest, simplest way to reproduce the problem?

The following test generates changes that won't compile.
edit: edited the test for less complexity

package Test;

class Test {

  protected int addFinalToThisVar(int unalteredVariable) {
    return unalteredVariable;
  }

  protected int increment(int variableToIncrement) {
    variableToIncrement++;
    return variableToIncrement;
  }

  protected int preIncrement(int variableToPreIncrement) {
    return ++variableToPreIncrement;
  }

  protected int decrement(int variableToDecrement) {
    variableToDecrement--;
    return variableToDecrement;
  }

  protected int preDecrement(int variableToPreDecrement) {
    return --variableToPreDecrement;
  }

  protected int accumulate(int add, int accumulator) {
    accumulator += add;
    return accumulator;
  }
}

What did you expect to see?

package Test;

class Test {

  protected int addFinalToThisVar(final int unalteredVariable) {
    return unalteredVariable;
  }

  protected int increment(int variableToIncrement) {
    variableToIncrement++;
    return variableToIncrement;
  }

  protected int preIncrement(int variableToPreIncrement) {
    return ++variableToPreIncrement;
  }

  protected int decrement(int variableToDecrement) {
    variableToDecrement--;
    return variableToDecrement;
  }

  protected int preDecrement(int variableToPreDecrement) {
    return --variableToPreDecrement;
  }

  protected int accumulate(final int add, int accumulator) {
    accumulator += add;
    return accumulator;
  }
}

What did you see instead?

package Test;

class Test {

  protected int addFinalToThisVar(final int unalteredVariable) {
    return unalteredVariable;
  }

  protected int increment(final int variableToIncrement) {
    variableToIncrement++;
    return variableToIncrement;
  }

  protected int preIncrement(final int variableToPreIncrement) {
    return ++variableToPreIncrement;
  }

  protected int decrement(final int variableToDecrement) {
    variableToDecrement--;
    return variableToDecrement;
  }

  protected int preDecrement(final int variableToPreDecrement) {
    return --variableToPreDecrement;
  }

  protected int accumulate(final int add, final int accumulator) {
    accumulator += add;
    return accumulator;
  }

Are you interested in contributing a fix to OpenRewrite?

Yes. The fix seems easy as it is already implemented in FinalizeLocalVariables. Bringing the VisitUnary and VisitAssignmentOperator logic from that recipe seems to fix the issues (altho the "add" variable in the last test method is not finalized, though that would be a minor concern).

@AlejandroBertolo AlejandroBertolo added the bug Something isn't working label Sep 28, 2023
@timtebeek timtebeek moved this to Backlog in OpenRewrite Oct 1, 2023
@timtebeek
Copy link
Contributor

Thanks for the detailed report @AlejandroBertolo ! Seems like an oversight indeed, and would be good to see fixed. Thanks for offering to help out; let me know if you're able to fit that in, otherwise I can try to fit this in.

Roughly speaking the steps should be:

  1. Fork & checkout the project
  2. Add a unit test using your above example similar to this one
    @Issue("https://github.com/openrewrite/rewrite/issues/3133")
    @Test
    void twoParameters() {
    rewriteRun(
    //language=java
    java(
    """
    class Foo {
    int test(final int a, int b) {
    return a + b;
    }
    }
    """,
    """
    class Foo {
    int test(final int a, final int b) {
    return a + b;
    }
    }
    """
    )
    );
    }
  3. Update the recipe visitor to take increments into consideration
    public TreeVisitor<?, ExecutionContext> getVisitor() {
    return new JavaIsoVisitor<ExecutionContext>() {
    @Override
    public MethodDeclaration visitMethodDeclaration(MethodDeclaration methodDeclaration, ExecutionContext executionContext) {
    MethodDeclaration declarations = super.visitMethodDeclaration(methodDeclaration, executionContext);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test provided
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants