Skip to content

Fix ClassCastException on J.Ternary visitor#3861

Closed
ammachado wants to merge 1 commit intoopenrewrite:mainfrom
ammachado:ternary-visitor
Closed

Fix ClassCastException on J.Ternary visitor#3861
ammachado wants to merge 1 commit intoopenrewrite:mainfrom
ammachado:ternary-visitor

Conversation

@ammachado
Copy link
Copy Markdown
Contributor

What's changed?

Changed JavaVisitor#visitTernary to allow simplification of ternary expressions.

What's your motivation?

I tried to implement this issue to explore rewrite-templating, and used the following implementation:

import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import org.openrewrite.java.template.RecipeDescriptor;

public class SimplifyTernary {

    @RecipeDescriptor(
            name = "Simplify ternaries",
            description = "Simplify `expr ? true : false` to `expr`."
    )
    public static class SimplifyTernaryTrueFalse {

        @BeforeTemplate
        boolean before(boolean expr) {
            return expr ? true : false;
        }

        @AfterTemplate
        boolean after(boolean expr) {
            return expr;
        }
    }
}

With the following test case:

@Test
void simplifyTrueFalse() {
    rewriteRun(
      spec -> spec.recipe(new SimplifyTernaryRecipes()),
      java(
        """
          class Test {
              boolean trueCondition = true ? true : false;
          }
          """,
        """
          class Test {
              boolean trueCondition = true;
          }
          """
      )
    );
}

Anything in particular you'd like reviewers to focus on?

Test passes, but the change might break something else.

Anyone you would like to review specifically?

No one.

Have you considered any alternatives or workarounds?

Implement the rule as a regular visitor, instead of a Refaster template.

Any additional context

Here's the exception trace for the test:

Caused by: org.openrewrite.internal.RecipeRunException: java.lang.ClassCastException: class org.openrewrite.java.tree.J$Literal cannot be cast to class org.openrewrite.java.tree.Statement (org.openrewrite.java.tree.J$Literal and org.openrewrite.java.tree.Statement are in unnamed module of loader 'app')
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:329)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:184)
    at org.openrewrite.java.JavaTemplate.apply(JavaTemplate.java:67)
    at org.openrewrite.java.migrate.lang.SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1.visitExpression(SimplifyTernaryRecipes.java:81)
    at org.openrewrite.java.migrate.lang.SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1.visitExpression(SimplifyTernaryRecipes.java:72)
    at org.openrewrite.java.JavaVisitor.visitTernary(JavaVisitor.java:1153)
    at org.openrewrite.java.tree.J$Ternary.acceptJava(J.java:5181)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    at org.openrewrite.java.JavaVisitor.visitLeftPadded(JavaVisitor.java:1401)
    at org.openrewrite.java.JavaVisitor.visitVariable(JavaVisitor.java:1301)
    at org.openrewrite.java.tree.J$VariableDeclarations$NamedVariable.acceptJava(J.java:5932)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    at org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1373)
    at org.openrewrite.java.JavaVisitor.lambda$visitVariableDeclarations$29(JavaVisitor.java:961)
    at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
    at org.openrewrite.java.JavaVisitor.visitVariableDeclarations(JavaVisitor.java:961)
    at org.openrewrite.java.tree.J$VariableDeclarations.acceptJava(J.java:5818)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    at org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1373)
    at org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:399)
    at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
    at org.openrewrite.java.JavaVisitor.visitBlock(JavaVisitor.java:398)
    at org.openrewrite.java.tree.J$Block.acceptJava(J.java:834)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    at org.openrewrite.java.JavaVisitor.visitClassDeclaration(JavaVisitor.java:486)
    at org.openrewrite.java.tree.J$ClassDeclaration.acceptJava(J.java:1286)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    at org.openrewrite.java.JavaVisitor.lambda$visitCompilationUnit$10(JavaVisitor.java:499)
    at org.openrewrite.internal.ListUtils.map(ListUtils.java:176)
    at org.openrewrite.java.JavaVisitor.visitCompilationUnit(JavaVisitor.java:499)
    at org.openrewrite.java.tree.J$CompilationUnit.acceptJava(J.java:1588)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:184)
    at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$5(RecipeRunCycle.java:164)
    at io.micrometer.core.instrument.AbstractTimer.recordCallable(AbstractTimer.java:147)
    at org.openrewrite.table.RecipeRunStats.recordEdit(RecipeRunStats.java:68)
    at org.openrewrite.scheduling.RecipeRunCycle.lambda$editSources$6(RecipeRunCycle.java:161)
    ... 15 more
Caused by: java.lang.ClassCastException: class org.openrewrite.java.tree.J$Literal cannot be cast to class org.openrewrite.java.tree.Statement (org.openrewrite.java.tree.J$Literal and org.openrewrite.java.tree.Statement are in unnamed module of loader 'app')
    at org.openrewrite.java.JavaVisitor.visitTernary(JavaVisitor.java:1159)
    at org.openrewrite.java.tree.J$Ternary.acceptJava(J.java:5181)
    at org.openrewrite.java.tree.J.accept(J.java:59)
    at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    ... 62 more

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@knutwannheden
Copy link
Copy Markdown
Contributor

Interesting. I don't think we should have to change the JavaVisitor. It seems like the visitor may be doing something it should not be doing. Why does visitStatement() here do anything rather than visitExpression()?

@timtebeek
Copy link
Copy Markdown
Member

Figured quickly share what recipe classes are generated for

SimplifyTernaryRecipes
import org.openrewrite.ExecutionContext;
import org.openrewrite.Preconditions;
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.internal.lang.NonNullApi;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.search.*;
import org.openrewrite.java.template.Primitive;
import org.openrewrite.java.template.Semantics;
import org.openrewrite.java.template.function.*;
import org.openrewrite.java.template.internal.AbstractRefasterJavaVisitor;
import org.openrewrite.java.tree.*;

import java.util.*;

import static org.openrewrite.java.template.internal.AbstractRefasterJavaVisitor.EmbeddingOption.*;

/**
 * OpenRewrite recipes created for Refaster template {@code org.openrewrite.java.migrate.SimplifyTernary}.
 */
@SuppressWarnings("all")
public class SimplifyTernaryRecipes extends Recipe {
    /**
     * Instantiates a new instance.
     */
    public SimplifyTernaryRecipes() {}

    @Override
    public String getDisplayName() {
        return "`SimplifyTernary` Refaster recipes";
    }

    @Override
    public String getDescription() {
        return "Refaster template recipes for `org.openrewrite.java.migrate.SimplifyTernary`.";
    }

    @Override
    public List<Recipe> getRecipeList() {
        return Arrays.asList(
                new SimplifyTernaryTrueFalseRecipe()
        );
    }

    /**
     * OpenRewrite recipe created for Refaster template {@code SimplifyTernary.SimplifyTernaryTrueFalse}.
     */
    @SuppressWarnings("all")
    @NonNullApi
    public static class SimplifyTernaryTrueFalseRecipe extends Recipe {

        /**
         * Instantiates a new instance.
         */
        public SimplifyTernaryTrueFalseRecipe() {}

        @Override
        public String getDisplayName() {
            return "Simplify ternaries";
        }

        @Override
        public String getDescription() {
            return "Simplify `expr ? true : false` to `expr`.";
        }

        @Override
        public TreeVisitor<?, ExecutionContext> getVisitor() {
            JavaVisitor<ExecutionContext> javaVisitor = new AbstractRefasterJavaVisitor() {
                final JavaTemplate before = Semantics.expression(this, "before", (@Primitive Boolean expr) -> expr ? true : false).build();
                final JavaTemplate after = Semantics.expression(this, "after", (@Primitive Boolean expr) -> expr).build();

                @Override
                public J visitExpression(Expression elem, ExecutionContext ctx) {
                    JavaTemplate.Matcher matcher;
                    if ((matcher = before.matcher(getCursor())).find()) {
                        return embed(
                                after.apply(getCursor(), elem.getCoordinates().replace(), matcher.parameter(0)),
                                getCursor(),
                                ctx,
                                SHORTEN_NAMES, SIMPLIFY_BOOLEANS
                        );
                    }
                    return super.visitExpression(elem, ctx);
                }

            };
            return javaVisitor;
        }
    }

}
SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_before
import org.openrewrite.java.*;

/**
 * OpenRewrite `before` template created for {@code org.openrewrite.java.migrate.SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1}.
 */
@SuppressWarnings("all")
public class SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_before {
    /**
     * Instantiates a new instance.
     */
    public SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_before() {}

    /**
     * Get the {@code JavaTemplate.Builder} to match or replace.
     * @return the JavaTemplate builder.
     */
    public static JavaTemplate.Builder getTemplate() {
        return JavaTemplate
                .builder("#{expr:any(boolean)} ? true : false");
    }
}
SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_after
import org.openrewrite.java.*;

/**
 * OpenRewrite `after` template created for {@code org.openrewrite.java.migrate.SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1}.
 */
@SuppressWarnings("all")
public class SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_after {
    /**
     * Instantiates a new instance.
     */
    public SimplifyTernaryRecipes$SimplifyTernaryTrueFalseRecipe$1_after() {}

    /**
     * Get the {@code JavaTemplate.Builder} to match or replace.
     * @return the JavaTemplate builder.
     */
    public static JavaTemplate.Builder getTemplate() {
        return JavaTemplate
                .builder("#{expr:any(boolean)}");
    }
}

Hadn't expected any of this to trigger a class cast exception indeed, as I still think this issue is a good candidate for Refaster.

t = t.withPrefix(visitSpace(t.getPrefix(), Space.Location.TERNARY_PREFIX, p));
t = t.withMarkers(visitMarkers(t.getMarkers(), p));
Expression temp = (Expression) visitExpression(t, p);
J temp = visitExpression(t, p);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required, since J.Literal implements Expression

@knutwannheden
Copy link
Copy Markdown
Contributor

This should help: #3867

@knutwannheden
Copy link
Copy Markdown
Contributor

knutwannheden commented Dec 30, 2023

@ammachado Maybe you can check if #3867 fixes the problem for you and then we could close this PR.

@ammachado
Copy link
Copy Markdown
Contributor Author

This fixes it. Thanks.

@ammachado ammachado closed this Dec 30, 2023
@timtebeek
Copy link
Copy Markdown
Member

Thanks for confirming! Guess we now can indeed add rewrite-templating to rewrite-static-analysis to implement this recipe.

@ammachado ammachado deleted the ternary-visitor branch December 30, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants