Alek/remove to string calls from array instances #126

AlekSimpson · 2023-06-19T22:22:51Z

What's changed?

toString should not be called on array instances. RSPEC-2116

What's your motivation?

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

I think the recipe is pretty much finished, only two things is there is an edge case with String.format() calls I still haven't quite worked out. It gives an error when I try and run the tests on them. Second thing is that I'm not sure how to get the preconditions to check for if the code is using the array type before it runs.

Anyone you would like to review specifically?

@timtebeek

Checklist

I've added unit tests to cover both positive and negative cases
I've added the license header to any new files through ./gradlew licenseFormat
I've used the IntelliJ auto-formatter on affected files
I've updated the documentation (if applicable)

…a string with a StringBuilder

…ualsTest.java Co-authored-by: Tim te Beek <[email protected]>

…uals.java Co-authored-by: Tim te Beek <[email protected]>

…ualsTest.java Co-authored-by: Tim te Beek <[email protected]>

…e not just raw string variables but also things like method calls

…for two tests which are testing an edge cases related to String.format()

timtebeek · 2023-06-20T04:33:45Z

There's some conflicts here; could you resolve those? Did you perhaps start your branch from your previous branch?

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

AlekSimpson · 2023-06-20T23:26:55Z

I figured out the edge case and also the precondition check. I think this recipe is functionally complete, but maybe there could be last minute polishes I don't see.

sambsnyd

Good work! This will be a useful recipe. 👍
After you address my feedback this looks good to check in, no need to seek another round of review from me.

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

src/test/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstancesTest.java

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

…so reworked recipe so that it uses cursor messaging

AlekSimpson · 2023-06-22T18:59:54Z

Ok, so I've tried to incorporate all the feedback from this thread. I still need to add more tests for a bunch of the other methods mentioned above. However, I think I should wait to do that till I have the cursor messaging working. I've updated the branch with my implementation of the cursor messaging but it doesn't work. The message never seems to be found in the visitExpression() visitor and I'm not sure why. @knutwannheden

knutwannheden · 2023-06-23T10:41:49Z

Ok, so I've tried to incorporate all the feedback from this thread. I still need to add more tests for a bunch of the other methods mentioned above. However, I think I should wait to do that till I have the cursor messaging working. I've updated the branch with my implementation of the cursor messaging but it doesn't work. The message never seems to be found in the visitExpression() visitor and I'm not sure why. @knutwannheden

@AlekSimpson You got it almost all right! The important thing here is that the super.visitMethodInvocation() call comes after the message is added to the cursor. Otherwise visitExpression() will end up getting called before the cursor message is added. I made some changes and will let you take it from here.

…ipe also covers arrays concatenated with strings now

AlekSimpson · 2023-06-23T23:02:24Z

All changes have been made and tests have been updated. All tests pass.

knutwannheden

Almost there! I added a few more comments which should be addressed.

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

timtebeek · 2023-06-26T14:44:14Z

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

        public Expression visitExpression(Expression exp, ExecutionContext ctx) {
            Expression e = (Expression) super.visitExpression(exp, ctx);
-            if (e.getType() instanceof JavaType.Array) {
+            if (e instanceof TypedTree && e.getType() instanceof JavaType.Array) {


Maybe this is more a question for Knut of Kun, but I had expected a call out to TypeUtils here rather than a direct use of instanceof. When would we use each?

We could replace e.getType() instanceof JavaType.Array with TypeUtils.asArray(e.getType()) != null but I don't think it is really much better. IMHO the instanceof here is fine.

knutwannheden

Apart from one last minor comment I think this looks good.

knutwannheden · 2023-06-27T07:45:01Z

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

        public Expression visitExpression(Expression exp, ExecutionContext ctx) {
            Expression e = (Expression) super.visitExpression(exp, ctx);
-            if (e.getType() instanceof JavaType.Array) {
+            if (e instanceof TypedTree && e.getType() instanceof JavaType.Array) {


We could replace e.getType() instanceof JavaType.Array with TypeUtils.asArray(e.getType()) != null but I don't think it is really much better. IMHO the instanceof here is fine.

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java

…lsFromArrayInstances.java Co-authored-by: Knut Wannheden <[email protected]>

src/test/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstancesTest.java

…lsFromArrayInstancesTest.java

AlekSimpson and others added 14 commits June 16, 2023 13:58

new recipe that changes .equals() to .contentEquals() when comparing …

3f4e109

…a string with a StringBuilder

polished somethings up and added license headers

b4877a3

Update src/test/java/org/openrewrite/staticanalysis/EqualsToContentEq…

ae41eb5

…ualsTest.java Co-authored-by: Tim te Beek <[email protected]>

Update src/main/java/org/openrewrite/staticanalysis/EqualsToContentEq…

90338a6

…uals.java Co-authored-by: Tim te Beek <[email protected]>

Update src/main/java/org/openrewrite/staticanalysis/EqualsToContentEq…

b17a46b

…uals.java Co-authored-by: Tim te Beek <[email protected]>

Update src/main/java/org/openrewrite/staticanalysis/EqualsToContentEq…

05753ca

…uals.java Co-authored-by: Tim te Beek <[email protected]>

Update src/main/java/org/openrewrite/staticanalysis/EqualsToContentEq…

74ed6f4

…uals.java Co-authored-by: Tim te Beek <[email protected]>

Update src/main/java/org/openrewrite/staticanalysis/EqualsToContentEq…

24c02a3

…uals.java Co-authored-by: Tim te Beek <[email protected]>

Update src/test/java/org/openrewrite/staticanalysis/EqualsToContentEq…

ac3ccee

…ualsTest.java Co-authored-by: Tim te Beek <[email protected]>

added missing import

15089c1

updated license header year

86e3d79

added test to check that the recipe runs correctly on selects that ar…

9423d0e

…e not just raw string variables but also things like method calls

added first test, need to test recipe

a69675b

the recipe for the toString() replacement is mostly finished, except …

dbc8624

…for two tests which are testing an edge cases related to String.format()

AlekSimpson added the recipe label Jun 19, 2023

AlekSimpson requested a review from timtebeek June 19, 2023 22:22

AlekSimpson self-assigned this Jun 19, 2023

AlekSimpson marked this pull request as draft June 19, 2023 22:22

timtebeek requested a review from joanvr June 20, 2023 04:34

Merge branch 'main' into alek/RemoveToStringCallsFromArrayInstances

83890a9

timtebeek reviewed Jun 20, 2023

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java Outdated Show resolved Hide resolved

AlekSimpson added 2 commits June 20, 2023 13:14

fixed edge case, all tests pass now

a574b69

added preconidition check

3c4ed65

AlekSimpson marked this pull request as ready for review June 20, 2023 23:31

sambsnyd approved these changes Jun 21, 2023

View reviewed changes

knutwannheden reviewed Jun 21, 2023

View reviewed changes

added support for more methods that implicitly call toString() and al…

bd586b7

…so reworked recipe so that it uses cursor messaging

AlekSimpson force-pushed the alek/RemoveToStringCallsFromArrayInstances branch from 8077b60 to bd586b7 Compare June 22, 2023 18:53

added RSPEC tags and updated import statements

18e5c8c

AlekSimpson mentioned this pull request Jun 22, 2023

Remove hashCode calls on arrays #128

Merged

4 tasks

The cursor message must be added before the super call

40167dd

rpau linked an issue Jun 23, 2023 that may be closed by this pull request

hashCode and toString should not be called on array instances. RSPEC-2116 #44

Closed

fixed cursor messaging, updated tests for more edge case methods, rec…

5ca3b34

…ipe also covers arrays concatenated with strings now

knutwannheden reviewed Jun 25, 2023

View reviewed changes

timtebeek mentioned this pull request Jun 26, 2023

hashCode and toString should not be called on array instances. RSPEC-2116 #44

Closed

timtebeek reviewed Jun 26, 2023

View reviewed changes

more changes from PR feedback

ffadf48

AlekSimpson force-pushed the alek/RemoveToStringCallsFromArrayInstances branch from 5963274 to ffadf48 Compare June 26, 2023 16:32

added support for static methods

f488a15

knutwannheden reviewed Jun 27, 2023

View reviewed changes

Update src/main/java/org/openrewrite/staticanalysis/RemoveToStringCal…

522afbe

…lsFromArrayInstances.java Co-authored-by: Knut Wannheden <[email protected]>

timtebeek approved these changes Jun 27, 2023

View reviewed changes

src/test/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstancesTest.java Outdated Show resolved Hide resolved

Update src/test/java/org/openrewrite/staticanalysis/RemoveToStringCal…

81f193a

…lsFromArrayInstancesTest.java

timtebeek merged commit 2127a09 into main Jun 27, 2023

timtebeek deleted the alek/RemoveToStringCallsFromArrayInstances branch June 27, 2023 15:22

Alek/remove to string calls from array instances #126

Alek/remove to string calls from array instances #126

Uh oh!

Conversation

AlekSimpson commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's changed?

What's your motivation?

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

Anyone you would like to review specifically?

Checklist

Uh oh!

timtebeek commented Jun 20, 2023

Uh oh!

Uh oh!

AlekSimpson commented Jun 20, 2023

Uh oh!

sambsnyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlekSimpson commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knutwannheden commented Jun 23, 2023

Uh oh!

AlekSimpson commented Jun 23, 2023

Uh oh!

knutwannheden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timtebeek Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

knutwannheden Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

knutwannheden left a comment

Choose a reason for hiding this comment

Uh oh!

knutwannheden Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AlekSimpson commented Jun 19, 2023 •

edited

Loading

AlekSimpson commented Jun 22, 2023 •

edited

Loading