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

Take J.Case into account in JavaTemplate #4744

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

Laurens-W
Copy link
Contributor

What's changed?

J.Case is now handled as a block of code that needs method invocations to be ended with a semicolon

What's your motivation?

Anyone you would like to review specifically?

@timtebeek

Any additional context

Relevant test case

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

@Laurens-W Laurens-W added the bug Something isn't working label Dec 4, 2024
@Laurens-W Laurens-W requested a review from timtebeek December 4, 2024 19:07
@Laurens-W Laurens-W self-assigned this Dec 4, 2024
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great to see the quick work here! Approved already assuming you've tested this, or plan to shortly after a merge.

@timtebeek timtebeek merged commit efc38e3 into main Dec 4, 2024
2 checks passed
@timtebeek timtebeek deleted the log-statement-in-switch branch December 4, 2024 19:33
@SunghwanJang
Copy link

SunghwanJang commented Dec 5, 2024

Thank you for this quick fix.
I confirmed that this fix works well when the method invocation is at 'Lambda' or 'For each loop'.

but still fails this case.

	@Test
	void switchCaseStatements() {
        rewriteRun(
                spec -> spec.recipe(new SystemErrToLogging2()),
                //language=java
                java(
                  """
                    class A {
                        org.slf4j.Logger logger = null;

                        void m(int cnt) {
                            switch (cnt) {
                                case 1:
                                    System.err.println("Oh no");
                                    break;
                                case 2:
                                default:
                                    break;
                            }
                        }
                        
                        String m2(){
                            return null;
                        }
                    }
                    """,
                  """
                    class A {
                        org.slf4j.Logger logger = null;

                        void m(int cnt) {
                            switch (cnt) {
                                case 1:
                                    logger.error("Oh no");
                                    break;
                                case 2:
                                default:
                                    break;
                            }
                        }
                        
                        String void m2(){
                            return null;
                        }
                    }
                    """
                )
              );
		
	}

SystemErrToLogging recipe only works for method invocation inside lambda.
so I copied the recipe and I deleted the condition that 'if (getCursor().getParentOrThrow().getValue() instanceof J.Lambda)'

import java.util.Set;

import org.openrewrite.Cursor;
import org.openrewrite.ExecutionContext;
import org.openrewrite.NlsRewrite.Description;
import org.openrewrite.NlsRewrite.DisplayName;
import org.openrewrite.Preconditions;
import org.openrewrite.Recipe;
import org.openrewrite.Repeat;
import org.openrewrite.TreeVisitor;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.JavaParser;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.logging.LoggingFramework;
import org.openrewrite.java.search.FindFieldsOfType;
import org.openrewrite.java.search.UsesMethod;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypeUtils;

public class SystemErrToLogging2 extends Recipe {

	private static final MethodMatcher systemErrPrint = new MethodMatcher("java.io.PrintStream print*(String)");
	
	@Override
	public @DisplayName String getDisplayName() {
		return "test";
	}

	@Override
	public @Description String getDescription() {
		return "test.";
	}

	@Override
	public TreeVisitor<?, ExecutionContext> getVisitor() {
		LoggingFramework framework = LoggingFramework.fromOption("SLF4J");

		return Preconditions.check(new UsesMethod<>(systemErrPrint),
				Repeat.repeatUntilStable(new JavaIsoVisitor<ExecutionContext>() {
					@Override
					public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
						J.MethodInvocation m = super.visitMethodInvocation(method, ctx);
						if (systemErrPrint.matches((Expression) method)) {
							if (m.getSelect() != null && m.getSelect() instanceof J.FieldAccess) {
								JavaType.Variable field = ((J.FieldAccess) m.getSelect()).getName().getFieldType();
								if (field != null && "err".equals(field.getName())
										&& TypeUtils.isOfClassType(field.getOwner(), "java.lang.System")) {
									Cursor printCursor = new Cursor(getCursor().getParent(), m);
									return logInsteadOfPrint(printCursor, ctx);
								}
							}
						}
						return m;
					}

					private J.MethodInvocation logInsteadOfPrint(Cursor printCursor, ExecutionContext ctx) {
						J.MethodInvocation print = printCursor.getValue();
						Cursor classCursor = getCursor().dropParentUntil(J.ClassDeclaration.class::isInstance);
						Set<J.VariableDeclarations> loggers = FindFieldsOfType.find(classCursor.getValue(),
								framework.getLoggerType());
						if (!loggers.isEmpty()) {
							J.Identifier computedLoggerName = loggers.iterator().next().getVariables().get(0).getName();
							print = replaceMethodInvocation(printCursor, ctx, print,
									computedLoggerName);
						}
						return print;
					}

					private J.MethodInvocation replaceMethodInvocation(Cursor printCursor, ExecutionContext ctx,
							J.MethodInvocation print,
							J.Identifier computedLoggerName) {

						print = getErrorTemplateNoException(ctx).apply(printCursor,
								print.getCoordinates().replace(), computedLoggerName, print.getArguments().get(0));

						return print;
					}

					private JavaTemplate getErrorTemplateNoException(ExecutionContext ctx) {
						return JavaTemplate
                                .builder("#{any(org.slf4j.Logger)}.error(#{any(String)})")
                                .contextSensitive()
                                .javaParser(JavaParser.fromJavaVersion()
                                        .classpathFromResources(ctx, "slf4j-api-2.1"))
                                .build();
					}
				}));
	}
}

stub:

import org.openrewrite.java.internal.template.__M__;
import org.openrewrite.java.internal.template.__P__;
class A{org.slf4j.Logger logger;
void m(int cnt){{
/*__TEMPLATE__*/__P__.<org.slf4j.Logger>/*__p0__*/p().error(__P__.<java.lang.String>/*__p1__*/p())/*__TEMPLATE_STOP__*/
}}
String m2(){
return null;
}
}

'Could not parse as Java' exception only occurs when the second method exists with a return statement.

@timtebeek
Copy link
Contributor

Oh wow, that's unexpected. I had indeed stripped that out thinking it had no effect. @Laurens-W could you have another look?

@SunghwanJang
Copy link

I can confirm that the latest snapshot fixes the issue.

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants