Analyse escaping for local vars #4658

MBoegers · 2024-11-08T22:46:03Z

What's changed?

I added two visitors that help decide if variables escape and where.

What's your motivation?

When migrating existing code, the question "Is this instance ever leaking its defining scope?" raises from time to time.
Take the migration from StringBuffer to StringBuilder.
StringBuilder is generally in favor, if no synchronization is needed (see JavaDoc StringBuilder).
Synchronization is not needed if the instance never escapes its defining scope.
These helping visitors enable such migration by answering the question of escaping vars.

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

Additional ways to leak an instance.

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

I considered the Dataflow API, but I am not fluent enough to archive the same..

Any additional context

This visitor and its helper methods will help @mkarg to realize his migrations for the OpenJDK project.

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

…ir defining scope

…defining scope or which variables will escape its defining scope

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java

rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java

shanman190 · 2024-11-09T00:05:32Z

Not specifically related to this PR, but thought that I'd share a bit simpler data flow API example given I too found rewrite-security-frameworks a bit difficult to follow originally. I ended up using the data flow API over in rewrite-feature-flags to great effect.

https://github.com/openrewrite/rewrite-feature-flags/blob/main/src/main/java/org/openrewrite/featureflags/search/FindFeatureFlag.java#L99-L112

MBoegers · 2024-11-09T07:50:29Z

Thanks @shanman190 for the reference, I'll definitely have a look. I think the Dataflow API needs some introduction material. After I had a breakthrough I'll share some ;)

…iableEscapeLocationsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

Some suggestions could not be made:

rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateTest.java
- lines 44-44

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions

Some suggestions could not be made:

rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateTest.java
- lines 44-44

mkarg · 2024-12-01T18:46:30Z

Thanks @shanman190 for the reference, I'll definitely have a look. I think the Dataflow API needs some introduction material. After I had a breakthrough I'll share some ;)

Does that imply that this PR will change or that it will get revoked? I am actively waiting for this visitors to be finally usable. Thanks for updating.

MBoegers · 2024-12-02T07:33:06Z

Hi @mkarg ATM I have not the resources to dig deep into the FlowAPI to implement the alternative solution. I would suggest to postpone this reimplementation but stick with the API fur a further versions. Would that work for you?

mkarg · 2024-12-02T10:03:51Z

I personally have no strong feelings how the visitors are implemented as long as there is a stable API to call them.

mkarg · 2024-12-04T14:34:42Z

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java

+                    package com.sample;
+                    public class Foo{
+                        void test() {
+                            StringBuilder /*~~>*/sb = new StringBuilder();


As other is itself non-escaping, sb essentially is non-escpaing, too.

mkarg · 2024-12-04T14:35:07Z

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java

+                    package com.sample;
+                    public class Foo{
+                        void test() {
+                            StringBuilder /*~~>*/sb = new StringBuilder();


As other is itself non-escaping, sb essentially is non-escpaing, too.

…penrewrite#4659) * Keep multiple whitelines and same comments from the gitignore file * Keep multiple whitelines and same comments from the gitignore file

…penrewrite#4657) * working draft * IntelliJ formatting * Update rewrite-yaml/src/main/java/org/openrewrite/yaml/search/FindProperty.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: Sam Snyder <sam@moderne.io> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update for rewrite 8.0 * Cosmetics * Fix a few more issues * Fix a few more test failures * Fix a few more test failures * Upgrade recipes to new `JavaTemplate` API * Replace `getRecipeList()` overrides with `@Getter(lazy = true)` property * Consistently use `latest.release` or `latest.integration` * Revert "Replace `getRecipeList()` overrides with `@Getter(lazy = true)` property" This reverts commit 897d59e79c9515564da45911ce2246934c5b1ae7. * Migrate to new `Recipe#getInitialState(ExecutionContext)` API * Polish * Fix a broken test Due to reversal of logic after inlining `doAfterVisit()` logic. * Fix another broken test We should change the `UpgradeDependencyVersion` in `rewrite-java-dependencies` so that workarounds like this are not necessary. * Fix last failing test (use `doAfterVisit()` again) --------- Co-authored-by: Sam Snyder <sam@moderne.io>

…ingAnnotation

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.staticanalysis.NullableOnMethodReturnType?organizationId=T3BlblJld3JpdGU%3D Co-authored-by: Moderne <team@moderne.io>

* Add test * Added compilation unit test * Swap expected and actual * Restore previous behavior --------- Co-authored-by: Laurens Westerlaken <laurens.westerlaken@jdriven.com>

…te#4673) * Polish tests * Polish --------- Co-authored-by: Tim te Beek <tim@moderne.io>

…erpolation is used on a version number

…eter is specified

…penrewrite#4677) * Remove redundant space for DeleteMethodArgument when argumentIndex=0 Signed-off-by: kuchang <kuchang@ebay.com> * Update FindPluginsTest.java * Adopt prefix of removed element --------- Signed-off-by: kuchang <kuchang@ebay.com> Co-authored-by: Tim te Beek <tim@moderne.io>

This is especially important for the YAML implementation, which applies the `ReplaceAliasWithAnchorValueVisitor` visitor to all elements.

Only apply `ReplaceAliasWithAnchorValueVisitor` to `Yaml.Document` elements. See: openrewrite#4650

…rewrite#4680)

* Add some initial tests * Add append functionality * Add markers * Fix constructor after merge * Polish code * Verify default behavior retained for declarative recipes through use of `null` in tests * Add test that fails due to unexpected second cycle * Adopt Markers.findFirst * Add newline to indicate Marker is on assignment * Fix test and polish * Update rewrite-java/src/main/java/org/openrewrite/java/AddOrUpdateAnnotationAttribute.java Co-authored-by: Tim te Beek <tim@moderne.io> --------- Co-authored-by: Tim te Beek <tim@moderne.io>

…4688) * Make `FindMissingTypes` stricter for method invocations Validate argument count with respect to parameter count. * Try to fix linking in `ReloadableJava17JavadocVisitor` * Try to fix `TypeUtils#isAssignableTo()` * Also update Java 11 and Java 21 Javadoc visitors * Also update Java 11 and Java 21 Javadoc visitors * More updates and simplifications * Corner case in `TypeUtils`

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.gradle.UpdateGradleWrapper?organizationId=T3BlblJld3JpdGU%3D#defaults=W3sibmFtZSI6ImFkZElmTWlzc2luZyIsInZhbHVlIjoiRmFsc2UifV0= Co-authored-by: Moderne <team@moderne.io>

I didn't even realize the recipe had this feature.

* Add `TypeUtils` tests for primitives * Correct `TypeUtils#isAssignableTo()` for primitive array types * New `TypeUtils#isAssignableTo()` API We need to consider the "position" when performing the assignability check. I.e. check if a parameterized type compared against a generic type variable is used in a context where the generic type parameter is bound by the parameterized type or not. * Implement new API * Remove one more `stream()` call * Update Javadoc visitors to use `TypeUtils#isAssignableTo()` correctly * Correction * Corrections * Corrections * More Javadoc fixes * Add missing `@MinimumJava11` annotation

MBoegers added 4 commits November 8, 2024 12:46

Visitor that can be used to find local variables that are leaking the…

7f55c3a

…ir defining scope

add licenses

5a03ded

add support for ternary operators to FindVariablesEscapeLocation

1088ccc

Add visitor to easily determine, if a named variable will escape its …

a38d727

…defining scope or which variables will escape its defining scope

MBoegers force-pushed the escape-analysis branch from 1c83352 to a38d727 Compare November 8, 2024 22:48

github-actions bot reviewed Nov 8, 2024

View reviewed changes

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java Outdated Show resolved Hide resolved

rewrite-java/src/test/java/org/openrewrite/java/search/FindVariableEscapeLocationsTest.java Outdated Show resolved Hide resolved

timtebeek marked this pull request as draft November 11, 2024 23:07

timtebeek assigned MBoegers Nov 11, 2024

timtebeek and others added 2 commits November 12, 2024 00:08

Update rewrite-java/src/test/java/org/openrewrite/java/search/FindVar…

ea3bf6c

…iableEscapeLocationsTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into escape-analysis

018669f

timtebeek added the enhancement New feature or request label Nov 11, 2024

github-actions bot reviewed Nov 11, 2024

View reviewed changes

rewrite-java/src/test/java/org/openrewrite/java/search/FindEscapingVariablesTest.java Outdated Show resolved Hide resolved

FindEscapingVariablesTe

fd1a30c

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Nov 12, 2024

View reviewed changes

mkarg reviewed Dec 4, 2024

View reviewed changes

Jenson3210 and others added 10 commits December 18, 2024 17:13

Keep multiple whitelines and same comments from the gitignore file (o…

9966a1b

…penrewrite#4659) * Keep multiple whitelines and same comments from the gitignore file * Keep multiple whitelines and same comments from the gitignore file

Format JavaTemplateTest.addAnnotation to minimize review bot spam

cb0b4a7

Merge remote-tracking branch 'origin/main'

d01b739

Update cursor-droping logic in RemoveMethodInvocationsVisitor

fd170d7

update RemoveMethodInvocationsVisitor to work on java files only

749063f

Remove private modifiers from marker subtypes

f4967ef

Fix possible ClassCastException and cleanup warnings in NoRequestMapp…

43c2e02

…ingAnnotation

refactor: Move @Nullable method annotations to the return type

2969c43

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.staticanalysis.NullableOnMethodReturnType?organizationId=T3BlblJld3JpdGU%3D Co-authored-by: Moderne <team@moderne.io>

rlsanders4 and others added 30 commits December 18, 2024 17:13

ChangeType does not work on J.ClassDeclaration (openrewrite#4670)

5b1d46c

* Add test * Added compilation unit test * Swap expected and actual * Restore previous behavior --------- Co-authored-by: Laurens Westerlaken <laurens.westerlaken@jdriven.com>

Refactor ChangeType tests to adhere to testing framework (openrewri…

74f45a7

…te#4673) * Polish tests * Polish --------- Co-authored-by: Tim te Beek <tim@moderne.io>

Add JavaDoc to ListUtils class

ae16af8

Move project matcher to MavenVisitor (openrewrite#4675)

1247d42

Fix FindPlugins.find() throwing a class-cast exception if GString int…

618d2cf

…erpolation is used on a version number

Fix Gradle AddDependency not doing anything when no onlyIfUsing param…

aae6492

…eter is specified

Also report DeserializationErrors in FindParseFailures recipe

b174dcd

Trim RecipeDescriptors

dde7882

Slightly improve performance of JsonPathMatchers

64086ef

This is especially important for the YAML implementation, which applies the `ReplaceAliasWithAnchorValueVisitor` visitor to all elements.

Improve performance of YAML JsonPathMatcher

5201917

Only apply `ReplaceAliasWithAnchorValueVisitor` to `Yaml.Document` elements. See: openrewrite#4650

Improve performance of JsonPathMatchers by only parsing once

cf3f27b

JsonPatchMatcher: Replace Stream API with for-loops

37d24b3

JsonPatchMatcher: Short-circuit matching when possible

221ea31

Fix accidental regression

95e9712

Polish YAML JsonPatchMatcher

9623d4a

Yet another YAML JsonPatchMatcher performance tweak

0b20b3b

Yet another YAML JsonPatchMatcher performance tweak

5e5ae55

Allow YAML JsonPatchMatcher to match document-level sequences (open…

ec692b3

…rewrite#4680)

Polish UpdateGradleWrapper

e30edf7

Call isTrue on Reference asserts in XmlParserTest (openrewrite#4689)

8627e4c

Fix NPE in SpringReference (openrewrite#4691)

88678ca

Make UpdateGradleWrapper.GradleWrapperState directly configurable

832c12c

Find Refaster style recipes as well (openrewrite#4693)

a29d6d5

refactor: Update Gradle wrapper

8c15566

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.gradle.UpdateGradleWrapper?organizationId=T3BlblJld3JpdGU%3D#defaults=W3sibmFtZSI6ImFkZElmTWlzc2luZyIsInZhbHVlIjoiRmFsc2UifV0= Co-authored-by: Moderne <team@moderne.io>

Find Yaml recipes as well (openrewrite#4695)

5821fbf

Add find source files negation test.

0bf6212

I didn't even realize the recipe had this feature.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Analyse escaping for local vars #4658

Analyse escaping for local vars #4658

MBoegers commented Nov 8, 2024 •

edited

Loading

shanman190 commented Nov 9, 2024

MBoegers commented Nov 9, 2024

github-actions bot left a comment

github-actions bot left a comment

mkarg commented Dec 1, 2024

MBoegers commented Dec 2, 2024

mkarg commented Dec 2, 2024

mkarg Dec 4, 2024

mkarg Dec 4, 2024

Analyse escaping for local vars #4658

Are you sure you want to change the base?

Analyse escaping for local vars #4658

Conversation

MBoegers commented Nov 8, 2024 • edited Loading

What's changed?

What's your motivation?

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

shanman190 commented Nov 9, 2024

MBoegers commented Nov 9, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

mkarg commented Dec 1, 2024

MBoegers commented Dec 2, 2024

mkarg commented Dec 2, 2024

mkarg Dec 4, 2024

Choose a reason for hiding this comment

mkarg Dec 4, 2024

Choose a reason for hiding this comment

MBoegers commented Nov 8, 2024 •

edited

Loading