From 7f55c3af51bff8d4a2d48e8679e97f920e2cb0e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 8 Nov 2024 12:46:37 +0100 Subject: [PATCH 1/6] Visitor that can be used to find local variables that are leaking their defining scope --- .../search/FindVariablesEscapeLocation.java | 154 +++++++++ .../FindVariablesEscapeLocationTest.java | 319 ++++++++++++++++++ 2 files changed, 473 insertions(+) create mode 100644 rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java create mode 100644 rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java new file mode 100644 index 00000000000..5d9fc67ddba --- /dev/null +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java @@ -0,0 +1,154 @@ +package org.openrewrite.java.search; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.Statement; +import org.openrewrite.marker.SearchResult; + +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * Find escaping points of local variables. + * An escaping point is the statement where a reference to a local variable leafs it scope an is now accessible from outside the method. + * In some situation such escaping is wanted, think of getters, but everytime its importent to rethink. + * Is mutability, synchronization or information hiding a problem here? + */ +//todo ternary operator support needed +public class FindVariablesEscapeLocation extends JavaIsoVisitor { + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { + method = super.visitMethodInvocation(method, executionContext); + + boolean noLeaks = findLocalArguments(method.getArguments()).isEmpty(); + + return noLeaks ? method : SearchResult.found(method); + } + + @Override + public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext executionContext) { + assignment = super.visitAssignment(assignment, executionContext); + + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + if (isPrimitive(identifier)) { + return assignment; + } + + boolean targetIsOutsider = isNotLocalVar(identifier); + boolean assignedByLocalVar = assignment.getAssignment() instanceof J.Identifier && isLocalVar(((J.Identifier) assignment.getAssignment())); + boolean leaksVariable = targetIsOutsider && assignedByLocalVar; + + return leaksVariable ? SearchResult.found(assignment) : assignment; + } + + @Override + public J.Return visitReturn(J.Return _return, ExecutionContext executionContext) { + _return = super.visitReturn(_return, executionContext); + + boolean returnsLocalVar = _return.getExpression() instanceof J.Identifier + && !isPrimitive(((J.Identifier) _return.getExpression())) + && isLocalVar(((J.Identifier) _return.getExpression())); + + return returnsLocalVar ? SearchResult.found(_return) : _return; + } + + @Override + public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext executionContext) { + newClass = super.visitNewClass(newClass, executionContext); + + boolean noLeaks = findLocalArguments(newClass.getArguments()).isEmpty(); + + return noLeaks ? newClass : SearchResult.found(newClass); + } + + /** + * Finds statements that enable escaping of local variables from the given subtree + */ + public static Set find(J subtree) { + return TreeVisitor.collect(new FindVariablesEscapeLocation(), subtree, new HashSet<>()) + .stream() + .filter(Statement.class::isInstance) + .map(Statement.class::cast) + .collect(Collectors.toSet()); + } + + /** + * Finds identifier of local variables that escape from the given subtree + */ + public static Set findLeakingVars(J subtree) { + Function> extractIdentifiers = t -> { + Set identifiers = new HashSet<>(); + if (t instanceof J.Return) { + Expression expr = ((J.Return) t).getExpression(); + if (expr instanceof J.Identifier) { + identifiers.add((J.Identifier) expr); + } + } else if (t instanceof J.Assignment) { + Expression expr = ((J.Assignment) t).getAssignment(); + if (expr instanceof J.Identifier) { + identifiers.add((J.Identifier) expr); + } + } else if (t instanceof J.NewClass) { + identifiers.addAll(findLocalArguments(((J.NewClass) t).getArguments())); + } else if(t instanceof J.MethodInvocation) { + identifiers.addAll(findLocalArguments(((J.MethodInvocation) t).getArguments())); + } + + return identifiers; + }; + + return TreeVisitor.collect(new FindVariablesEscapeLocation(), subtree, new HashSet<>()) + .stream() + .map(extractIdentifiers) + .flatMap(Collection::stream) + .collect(Collectors.toSet()); + } + + private static boolean isLocalVar(J.Identifier identifier) { + JavaType.Variable fieldType = identifier.getFieldType(); + + if (fieldType != null) { + JavaType owner = fieldType.getOwner(); + + boolean isOwnedByMethod = owner instanceof JavaType.Method; + if (isOwnedByMethod) { + boolean isMethodParameter = ((JavaType.Method) owner).getParameterNames().contains(identifier.getSimpleName()); + return !isMethodParameter; + } + } + + return false; + } + + private static boolean isNotLocalVar(J.Identifier identifier) { + return !isLocalVar(identifier); + } + + private static boolean isPrimitive(J.Identifier identifier) { + JavaType.Variable fieldType = identifier.getFieldType(); + return fieldType != null && fieldType.getType() instanceof JavaType.Primitive; + } + + private static boolean isNotPrimitive(J.Identifier identifier) { + return !isPrimitive(identifier); + } + + private static List findLocalArguments(List arguments) { + return arguments.stream() + .filter(J.Identifier.class::isInstance) + .map(J.Identifier.class::cast) + .filter(FindVariablesEscapeLocation::isNotPrimitive) + .filter(FindVariablesEscapeLocation::isLocalVar) + .collect(Collectors.toList()); + } +} diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java new file mode 100644 index 00000000000..1fdb69b3e8c --- /dev/null +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java @@ -0,0 +1,319 @@ +package org.openrewrite.java.search; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +public class FindVariablesEscapeLocationTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(RewriteTest.toRecipe(FindVariablesEscapeLocation::new)); + } + + @Nested + class Escaping { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return o; + } + } + """, """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + /*~~>*/return o; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = o; + } + } + """, """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + /*~~>*/someField = o; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(o); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + /*~~>*/System.out.print(o); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(o); + } + } + """, """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> /*~~>*/System.out.print(o); + } + } + """)); + } + + @Test + void viaNew() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = /*~~>*/new StringBuilder(sb); + } + } + """)); + } + } + + @Nested + class Secure { + @Nested + class OtherObject { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return new Object(); + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = new Object(); + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(new Object()); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(new Object()); + } + } + """)); + } + } + + @Nested + class Primitives { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + int test() { + int o = 1; + return o; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + int someField; + void test() { + int o = 1; + someField = o; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + int o = 1; + System.out.print(o); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + int o = 1; + Runnable r = () -> System.out.print(o); + } + } + """)); + } + } + + @Nested + class Parameter { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test(Object other) { + Object o = new Object(); + return other; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test(Object other) { + Object o = new Object(); + someField = other; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test(Object other) { + Object o = new Object(); + System.out.print(other); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test(Object other) { + Object o = new Object(); + Runnable r = () -> System.out.print(other); + } + } + """)); + } + } + } +} From 5a03dedd92cf6a03344008ab9d2353b70725f073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 8 Nov 2024 21:27:36 +0100 Subject: [PATCH 2/6] add licenses --- .../java/search/FindVariablesEscapeLocation.java | 15 +++++++++++++++ .../search/FindVariablesEscapeLocationTest.java | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java index 5d9fc67ddba..ed4fde37392 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java @@ -1,3 +1,18 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.java.search; import org.openrewrite.ExecutionContext; diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java index 1fdb69b3e8c..636a69106d2 100644 --- a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.openrewrite.java.search; import org.junit.jupiter.api.Nested; From 1088cccd9262eb84d288cd7934d3b043a6d5b788 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 8 Nov 2024 21:55:09 +0100 Subject: [PATCH 3/6] add support for ternary operators to FindVariablesEscapeLocation --- ....java => FindVariableEscapeLocations.java} | 62 +++-- ...a => FindVariableEscapeLocationsTest.java} | 221 ++++++++++++++---- 2 files changed, 217 insertions(+), 66 deletions(-) rename rewrite-java/src/main/java/org/openrewrite/java/search/{FindVariablesEscapeLocation.java => FindVariableEscapeLocations.java} (69%) rename rewrite-java/src/test/java/org/openrewrite/java/search/{FindVariablesEscapeLocationTest.java => FindVariableEscapeLocationsTest.java} (62%) diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java similarity index 69% rename from rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java rename to rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java index ed4fde37392..082c941c2a1 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariablesEscapeLocation.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java @@ -38,8 +38,7 @@ * In some situation such escaping is wanted, think of getters, but everytime its importent to rethink. * Is mutability, synchronization or information hiding a problem here? */ -//todo ternary operator support needed -public class FindVariablesEscapeLocation extends JavaIsoVisitor { +public class FindVariableEscapeLocations extends JavaIsoVisitor { @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { @@ -54,13 +53,15 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext executionContext) { assignment = super.visitAssignment(assignment, executionContext); - J.Identifier identifier = (J.Identifier) assignment.getVariable(); - if (isPrimitive(identifier)) { + J.Identifier assigmentTarget = (J.Identifier) assignment.getVariable(); + if (isPrimitive(assigmentTarget)) { return assignment; } - boolean targetIsOutsider = isNotLocalVar(identifier); - boolean assignedByLocalVar = assignment.getAssignment() instanceof J.Identifier && isLocalVar(((J.Identifier) assignment.getAssignment())); + boolean targetIsOutsider = isNotLocalVar(assigmentTarget); + boolean assignedByLocalVar = extractIdentifiers(assignment.getAssignment()).stream() + .map(FindVariableEscapeLocations::isLocalVar) + .reduce(false, Boolean::logicalOr); boolean leaksVariable = targetIsOutsider && assignedByLocalVar; return leaksVariable ? SearchResult.found(assignment) : assignment; @@ -70,9 +71,16 @@ public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ex public J.Return visitReturn(J.Return _return, ExecutionContext executionContext) { _return = super.visitReturn(_return, executionContext); - boolean returnsLocalVar = _return.getExpression() instanceof J.Identifier - && !isPrimitive(((J.Identifier) _return.getExpression())) - && isLocalVar(((J.Identifier) _return.getExpression())); + if (_return.getExpression() == null) { + return _return; + } + + boolean returnsLocalVar = extractIdentifiers(_return.getExpression()).stream() + .map(FindVariableEscapeLocations::extractIdentifiers) + .flatMap(Collection::stream) + .filter(i -> !isPrimitive(i)) + .map(FindVariableEscapeLocations::isLocalVar) + .reduce(false, Boolean::logicalOr); return returnsLocalVar ? SearchResult.found(_return) : _return; } @@ -90,7 +98,7 @@ public J.NewClass visitNewClass(J.NewClass newClass, ExecutionContext executionC * Finds statements that enable escaping of local variables from the given subtree */ public static Set find(J subtree) { - return TreeVisitor.collect(new FindVariablesEscapeLocation(), subtree, new HashSet<>()) + return TreeVisitor.collect(new FindVariableEscapeLocations(), subtree, new HashSet<>()) .stream() .filter(Statement.class::isInstance) .map(Statement.class::cast) @@ -122,13 +130,37 @@ public static Set findLeakingVars(J subtree) { return identifiers; }; - return TreeVisitor.collect(new FindVariablesEscapeLocation(), subtree, new HashSet<>()) + return TreeVisitor.collect(new FindVariableEscapeLocations(), subtree, new HashSet<>()) .stream() .map(extractIdentifiers) .flatMap(Collection::stream) .collect(Collectors.toSet()); } + /** + * Extracts Set of Identifiers from J.Identifiers and Ternary operators. + * These two potentially participate in very case but will not lead to escaping themselves. + */ + private static Set extractIdentifiers(Expression assignment) { + Set identifiers = new HashSet<>(); + // fast return if already an J.Identifier + if (assignment instanceof J.Identifier) { + identifiers.add((J.Identifier) assignment); + } + if (assignment instanceof J.Ternary) { + // if ternary we only need to care about direct assigment. + // the other possibilities (J.MethodInvocation, J.NewClass) are handled by the visitor itself. + if (((J.Ternary) assignment).getFalsePart() instanceof J.Identifier) { + identifiers.add((J.Identifier) ((J.Ternary) assignment).getFalsePart()); + } + if (((J.Ternary) assignment).getTruePart() instanceof J.Identifier) { + identifiers.add((J.Identifier) ((J.Ternary) assignment).getTruePart()); + } + } + + return identifiers; + } + private static boolean isLocalVar(J.Identifier identifier) { JavaType.Variable fieldType = identifier.getFieldType(); @@ -160,10 +192,10 @@ private static boolean isNotPrimitive(J.Identifier identifier) { private static List findLocalArguments(List arguments) { return arguments.stream() - .filter(J.Identifier.class::isInstance) - .map(J.Identifier.class::cast) - .filter(FindVariablesEscapeLocation::isNotPrimitive) - .filter(FindVariablesEscapeLocation::isLocalVar) + .map(FindVariableEscapeLocations::extractIdentifiers) + .flatMap(Collection::stream) + .filter(FindVariableEscapeLocations::isNotPrimitive) + .filter(FindVariableEscapeLocations::isLocalVar) .collect(Collectors.toList()); } } diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java similarity index 62% rename from rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java rename to rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java index 636a69106d2..bf44242f15e 100644 --- a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariablesEscapeLocationTest.java +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java @@ -23,28 +23,30 @@ import static org.openrewrite.java.Assertions.java; -public class FindVariablesEscapeLocationTest implements RewriteTest { +public class FindVariableEscapeLocationsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(RewriteTest.toRecipe(FindVariablesEscapeLocation::new)); + spec.recipe(RewriteTest.toRecipe(FindVariableEscapeLocations::new)); } @Nested class Escaping { - @Test - @DocumentExample - void viaReturnValue() { - rewriteRun(java( - """ - package com.sample; - public class Foo{ - Object test() { - Object o = new Object(); - return o; + @Nested + class Directly { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return o; + } } - } - """, """ + """, """ package com.sample; public class Foo{ Object test() { @@ -53,96 +55,213 @@ Object test() { } } """)); - } + } - @Test - void viaField() { - rewriteRun(java( - """ + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = o; + } + } + """, """ package com.sample; public class Foo{ Object someField; void test() { Object o = new Object(); - someField = o; + /*~~>*/someField = o; } } - """, """ + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(o); + } + } + """, """ package com.sample; public class Foo{ - Object someField; void test() { Object o = new Object(); - /*~~>*/someField = o; + /*~~>*/System.out.print(o); } } """)); - } + } - @Test - void viaMethodCall() { - rewriteRun(java( - """ + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(o); + } + } + """, """ package com.sample; public class Foo{ - void test() { + Runnable test() { Object o = new Object(); - System.out.print(o); + Runnable r = () -> /*~~>*/System.out.print(o); } } - """, """ + """)); + } + + @Test + void viaNew() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb); + } + } + """, """ package com.sample; public class Foo{ void test() { - Object o = new Object(); - /*~~>*/System.out.print(o); + StringBuilder sb = new StringBuilder(); + StringBuilder other = /*~~>*/new StringBuilder(sb); } } """)); + } } - @Test - void viaLambdaBody() { - rewriteRun(java( - """ + @Nested + class WrappedInTernary { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return o != null ? o : "null"; + } + } + """, """ package com.sample; public class Foo{ - Runnable test() { + Object test() { Object o = new Object(); - Runnable r = () -> System.out.print(o); + /*~~>*/return o != null ? o : "null"; } } - """, """ + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = o != null ? o : "null"; + } + } + """, """ package com.sample; public class Foo{ - Runnable test() { + Object someField; + void test() { Object o = new Object(); - Runnable r = () -> /*~~>*/System.out.print(o); + /*~~>*/someField = o != null ? o : "null"; } } """)); - } + } - @Test - void viaNew() { - rewriteRun(java( - """ + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(o != null ? o : "null"); + } + } + """, """ package com.sample; public class Foo{ void test() { - StringBuilder sb = new StringBuilder(); - StringBuilder other = new StringBuilder(sb); + Object o = new Object(); + /*~~>*/System.out.print(o != null ? o : "null"); } } - """, """ + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(o != null ? o : "null"); + } + } + """, """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> /*~~>*/System.out.print(o != null ? o : "null"); + } + } + """)); + } + + @Test + void viaNew() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb != null ? sb : new StringBuilder()); + } + } + """, """ package com.sample; public class Foo{ void test() { StringBuilder sb = new StringBuilder(); - StringBuilder other = /*~~>*/new StringBuilder(sb); + StringBuilder other = /*~~>*/new StringBuilder(sb != null ? sb : new StringBuilder()); } } """)); + } } } From a38d72779ed20ae07eeb59a607b2318854eb82b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 8 Nov 2024 23:30:47 +0100 Subject: [PATCH 4/6] Add visitor to easily determine, if a named variable will escape its defining scope or which variables will escape its defining scope --- .../java/search/FindEscapingVariables.java | 72 +++ .../search/FindVariableEscapeLocations.java | 13 +- .../search/FindEscapingVariablesTest.java | 453 ++++++++++++++++++ 3 files changed, 529 insertions(+), 9 deletions(-) create mode 100644 rewrite-java/src/main/java/org/openrewrite/java/search/FindEscapingVariables.java create mode 100644 rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/FindEscapingVariables.java b/rewrite-java/src/main/java/org/openrewrite/java/search/FindEscapingVariables.java new file mode 100644 index 00000000000..7ad603d3e7d --- /dev/null +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/FindEscapingVariables.java @@ -0,0 +1,72 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.search; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.J; +import org.openrewrite.marker.SearchResult; + +import java.util.HashSet; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Identify local variables that escape there defining scope. + * + * @see FindVariableEscapeLocations + */ +public class FindEscapingVariables extends JavaIsoVisitor { + /** + * Finds named local variables from the given subtree that will escape their defining scope + */ + public static Set find(J subtree) { + return TreeVisitor.collect(new FindEscapingVariables(), subtree, new HashSet<>()) + .stream() + .filter(J.VariableDeclarations.NamedVariable.class::isInstance) + .map(J.VariableDeclarations.NamedVariable.class::cast) + .collect(Collectors.toSet()); + } + + /** + * Determine if a local named variable from the given subtree will escape its defining scope + */ + public static boolean willLeakFrom(J.VariableDeclarations.NamedVariable variable, J subtree) { + return find(subtree).contains(variable); + } + + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext executionContext) { + variable = super.visitVariable(variable, executionContext); + + if (variable.isField(getCursor())) { + return variable; + } + + J.Block definingScope = getCursor().firstEnclosing(J.Block.class); + if (definingScope != null && variable.getName().getFieldType() != null) { + boolean willLeak = FindVariableEscapeLocations.findLeakingVars(definingScope).stream() + .map(J.Identifier::getFieldType) + .anyMatch(variable.getName().getFieldType()::equals); + if (willLeak) { + variable = SearchResult.found(variable); + } + } + + return variable; + } +} diff --git a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java index 082c941c2a1..74344a7e773 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/search/FindVariableEscapeLocations.java @@ -25,6 +25,7 @@ import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.SearchResult; +import javax.annotation.Nullable; import java.util.Collection; import java.util.HashSet; import java.util.List; @@ -112,15 +113,9 @@ public static Set findLeakingVars(J subtree) { Function> extractIdentifiers = t -> { Set identifiers = new HashSet<>(); if (t instanceof J.Return) { - Expression expr = ((J.Return) t).getExpression(); - if (expr instanceof J.Identifier) { - identifiers.add((J.Identifier) expr); - } + identifiers.addAll(extractIdentifiers(((J.Return) t).getExpression())); } else if (t instanceof J.Assignment) { - Expression expr = ((J.Assignment) t).getAssignment(); - if (expr instanceof J.Identifier) { - identifiers.add((J.Identifier) expr); - } + identifiers.addAll(extractIdentifiers(((J.Assignment) t).getAssignment())); } else if (t instanceof J.NewClass) { identifiers.addAll(findLocalArguments(((J.NewClass) t).getArguments())); } else if(t instanceof J.MethodInvocation) { @@ -141,7 +136,7 @@ public static Set findLeakingVars(J subtree) { * Extracts Set of Identifiers from J.Identifiers and Ternary operators. * These two potentially participate in very case but will not lead to escaping themselves. */ - private static Set extractIdentifiers(Expression assignment) { + private static Set extractIdentifiers(@Nullable Expression assignment) { Set identifiers = new HashSet<>(); // fast return if already an J.Identifier if (assignment instanceof J.Identifier) { diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java new file mode 100644 index 00000000000..dec70d2d09b --- /dev/null +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java @@ -0,0 +1,453 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.search; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +public class FindEscapingVariablesTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(RewriteTest.toRecipe(FindEscapingVariables::new)); + } + + @Nested + class Escaping { + @Nested + class Directly { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return o; + } + } + """, """ + package com.sample; + public class Foo{ + Object test() { + Object /*~~>*/o = new Object(); + return o; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = o; + } + } + """, """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object /*~~>*/o = new Object(); + someField = o; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(o); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + Object /*~~>*/o = new Object(); + System.out.print(o); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(o); + } + } + """, """ + package com.sample; + public class Foo{ + Runnable test() { + Object /*~~>*/o = new Object(); + Runnable r = () -> System.out.print(o); + } + } + """)); + } + + @Test + void viaNew() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + StringBuilder /*~~>*/sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb); + } + } + """)); + } + } + + @Nested + class WrappedInTernary { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return o != null ? o : "null"; + } + } + """, """ + package com.sample; + public class Foo{ + Object test() { + Object /*~~>*/o = new Object(); + return o != null ? o : "null"; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = o != null ? o : "null"; + } + } + """, """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object /*~~>*/o = new Object(); + someField = o != null ? o : "null"; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(o != null ? o : "null"); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + Object /*~~>*/o = new Object(); + System.out.print(o != null ? o : "null"); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(o != null ? o : "null"); + } + } + """, """ + package com.sample; + public class Foo{ + Runnable test() { + Object /*~~>*/o = new Object(); + Runnable r = () -> System.out.print(o != null ? o : "null"); + } + } + """)); + } + + @Test + void viaNew() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + StringBuilder sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb != null ? sb : new StringBuilder()); + } + } + """, """ + package com.sample; + public class Foo{ + void test() { + StringBuilder /*~~>*/sb = new StringBuilder(); + StringBuilder other = new StringBuilder(sb != null ? sb : new StringBuilder()); + } + } + """)); + } + } + } + + @Nested + class Secure { + @Nested + class OtherObject { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test() { + Object o = new Object(); + return new Object(); + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test() { + Object o = new Object(); + someField = new Object(); + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + Object o = new Object(); + System.out.print(new Object()); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + Object o = new Object(); + Runnable r = () -> System.out.print(new Object()); + } + } + """)); + } + } + + @Nested + class Primitives { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + int test() { + int o = 1; + return o; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + int someField; + void test() { + int o = 1; + someField = o; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test() { + int o = 1; + System.out.print(o); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test() { + int o = 1; + Runnable r = () -> System.out.print(o); + } + } + """)); + } + } + + @Nested + class Parameter { + @Test + @DocumentExample + void viaReturnValue() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object test(Object other) { + Object o = new Object(); + return other; + } + } + """)); + } + + @Test + void viaField() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Object someField; + void test(Object other) { + Object o = new Object(); + someField = other; + } + } + """)); + } + + @Test + void viaMethodCall() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + void test(Object other) { + Object o = new Object(); + System.out.print(other); + } + } + """)); + } + + @Test + void viaLambdaBody() { + rewriteRun(java( + """ + package com.sample; + public class Foo{ + Runnable test(Object other) { + Object o = new Object(); + Runnable r = () -> System.out.print(other); + } + } + """)); + } + } + } +} From ea3bf6c3ee08a14707d70b6e676bff2c8e07e25a Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 12 Nov 2024 00:08:15 +0100 Subject: [PATCH 5/6] Update rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../java/search/FindVariableEscapeLocationsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java index bf44242f15e..1bd466a55dc 100644 --- a/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java @@ -23,7 +23,7 @@ import static org.openrewrite.java.Assertions.java; -public class FindVariableEscapeLocationsTest implements RewriteTest { +class FindVariableEscapeLocationsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { From fd1a30cb064b57105b74810c8209c25862337e1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Tue, 12 Nov 2024 12:44:23 +0100 Subject: [PATCH 6/6] FindEscapingVariablesTe Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../org/openrewrite/java/search/FindEscapingVariablesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java b/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java index dec70d2d09b..e43bc1520a1 100644 --- a/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java +++ b/rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java @@ -23,7 +23,7 @@ import static org.openrewrite.java.Assertions.java; -public class FindEscapingVariablesTest implements RewriteTest { +class FindEscapingVariablesTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) {