Don't if-wrap logs when arguments are getters

src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java

@@ -29,6 +29,7 @@
 import java.util.*;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static java.util.Collections.emptyList;
 import static org.openrewrite.Preconditions.or;
@@ -72,24 +73,25 @@ private static class AddIfEnabledVisitor extends JavaVisitor<ExecutionContext> {
         @Override
         public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
             J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx);
-            if ((infoMatcher.matches(m) || debugMatcher.matches(m) || traceMatcher.matches(m)) &&
-                    !isInIfStatementWithLogLevelCheck(getCursor(), m)) {
-                List<Expression> arguments = ListUtils.filter(m.getArguments(), a -> a instanceof J.MethodInvocation);
-                if (m.getSelect() != null && !arguments.isEmpty()) {
-                    J container = getCursor().getParentTreeCursor().getValue();
-                    if (container instanceof J.Block) {
-                        UUID id = container.getId();
-                        J.If if_ = ((J.If) JavaTemplate
-                                .builder("if(#{logger:any(org.slf4j.Logger)}.is#{}Enabled()) {}")
-                                .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+"))
-                                .build()
-                                .apply(getCursor(), m.getCoordinates().replace(),
-                                        m.getSelect(), StringUtils.capitalize(m.getSimpleName())))
-                                .withThenPart(m.withPrefix(m.getPrefix().withWhitespace("\n" + m.getPrefix().getWhitespace().replace("\n", ""))))
-                                .withPrefix(m.getPrefix().withComments(emptyList()));
-                        visitedBlocks.add(id);
-                        return if_;
-                    }
+            if (
+                    m.getSelect() != null
+                    && (infoMatcher.matches(m) || debugMatcher.matches(m) || traceMatcher.matches(m))
+                    && !isInIfStatementWithLogLevelCheck(getCursor(), m)
+                    && isAnyArgumentExpensive(m)
+            ) {
+                J container = getCursor().getParentTreeCursor().getValue();
+                if (container instanceof J.Block) {
+                    UUID id = container.getId();
+                    J.If if_ = ((J.If) JavaTemplate
+                            .builder("if(#{logger:any(org.slf4j.Logger)}.is#{}Enabled()) {}")
+                            .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+"))
+                            .build()
+                            .apply(getCursor(), m.getCoordinates().replace(),
+                                    m.getSelect(), StringUtils.capitalize(m.getSimpleName())))
+                            .withThenPart(m.withPrefix(m.getPrefix().withWhitespace("\n" + m.getPrefix().getWhitespace().replace("\n", ""))))
+                            .withPrefix(m.getPrefix().withComments(emptyList()));
+                    visitedBlocks.add(id);
+                    return if_;
                 }
             }
             return m;
@@ -114,6 +116,21 @@ private boolean isInIfStatementWithLogLevelCheck(Cursor cursor, J.MethodInvocati
                     (debugMatcher.matches(m) && sideEffects.stream().allMatch(e -> e instanceof J.MethodInvocation && isDebugEnabledMatcher.matches((J.MethodInvocation) e))) ||
                     (traceMatcher.matches(m) && sideEffects.stream().allMatch(e -> e instanceof J.MethodInvocation && isTraceEnabledMatcher.matches((J.MethodInvocation) e)));
         }
+
+        private boolean isAnyArgumentExpensive(J.MethodInvocation m) {
+            return !m
+                    .getArguments()
+                    .stream()
+                    .allMatch(
+                            arg ->
+                                    (arg instanceof J.MethodInvocation && isSimpleGetter(arg))
+                                    || arg instanceof J.Literal
+                            );
+        }
+
+        private boolean isSimpleGetter(Expression arg) {
+            return false;
+        }
     }
 
     @EqualsAndHashCode(callSuper = false)

src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java

@@ -17,13 +17,17 @@
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.openrewrite.DocumentExample;
 import org.openrewrite.InMemoryExecutionContext;
 import org.openrewrite.java.JavaParser;
 import org.openrewrite.test.RecipeSpec;
 import org.openrewrite.test.RewriteTest;
 
+import java.util.stream.Stream;
+
 import static org.openrewrite.java.Assertions.java;
 
 class WrapExpensiveLogStatementsInConditionalsTest implements RewriteTest {
@@ -580,4 +584,100 @@ String expensiveOp() {
           )
         );
     }
+
+    private static Stream<Arguments> wrapWhenExpensiveArgument() {
+        return Stream.of(
+          Arguments.of("notAGetter()"), // not a getter
+          Arguments.of("new A()"), // allocating a new object
+          Arguments.of("new A().getClass()"), // allocating a new object first
+          Arguments.of("\"foo\".getBytes()"), // allocating a string first
+          Arguments.of("input.getBytes(StandardCharsets.UTF_16)"), // getter with an argument
+          Arguments.of("getClass().getName()"), // getter on a method invocation expression
+          Arguments.of("optional.get()"), // not a getter
+          Arguments.of("A.getExpensive()"), // static getter likely to use external resources or allocate things
+          Arguments.of("getExpensive()") // static getter likely to use external resources or allocate things
+        );
+    }
+
+    @MethodSource
+    @ParameterizedTest
+    void wrapWhenExpensiveArgument(String logArgument) {
+        //language=java
+        rewriteRun(
+          java(
+            String.format("""
+              import java.nio.charset.StandardCharsets;
+              import java.util.Optional;
+              import org.slf4j.Logger;
+
+              class A {
+                  void method(Logger log, String input, Optional<String> optional) {
+                      log.info("{}", %s);
+                  }
+
+                  String notAGetter() {
+                      return "property";
+                  }
+
+                  static String getExpensive() {
+                      return "expensive";
+                  }
+              }
+              """, logArgument),
+            String.format("""
+              import java.nio.charset.StandardCharsets;
+              import java.util.Optional;
+              import org.slf4j.Logger;
+
+              class A {
+                  void method(Logger log, String input, Optional<String> optional) {
+                      if (log.isInfoEnabled()) {
+                          log.info("{}", %s);
+                      }
+                  }
+
+                  String notAGetter() {
+                      return "property";
+                  }
+
+                  static String getExpensive() {
+                      return "expensive";
+                  }
+              }
+              """, logArgument)
+          )
+        );
+    }
+
+    private static Stream<Arguments> dontWrapWhenCheapArgument() {
+        // We don't allocate stuff or very unlikely:
+        return Stream.of(
+          Arguments.of("input"), // identifier alone
+          Arguments.of("getClass()"), // a getter
+          Arguments.of("log.getName()"), // a getter
+          Arguments.of("34 + 78"), // literal
+          Arguments.of("8344"), // literal
+          Arguments.of("\"like, literally!\"") // literal
+          // add boolean expression composed of variables/contants only (and a negative test of it too)
+        );
+    }
+
+    @MethodSource
+    @ParameterizedTest
+    void dontWrapWhenCheapArgument(String logArgument) {
+        //language=java
+        rewriteRun(
+          java(
+            String.format("""
+              import org.slf4j.Logger;
+
+              class A {
+                  void method(Logger log, String input) {
+                      log.info("{}", %s);
+                  }
+              }
+              """, logArgument)
+          )
+        );
+    }
 }