From c522be58a699bfd5db824db56286716dd5daaa25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Bertolo=20Fafi=C3=A1n?= Date: Mon, 2 Oct 2023 09:25:08 +0200 Subject: [PATCH 1/5] fix: FinalizeMethodArguments considers unary and += expressions --- .../FinalizeMethodArguments.java | 33 +++++++++ .../FinalizeMethodArgumentsTest.java | 69 +++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java index b6070fb51..7daeb179a 100644 --- a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java +++ b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java @@ -12,6 +12,7 @@ import lombok.EqualsAndHashCode; import lombok.Value; +import org.jetbrains.annotations.NotNull; import org.openrewrite.*; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; @@ -145,6 +146,38 @@ public J.Assignment visitAssignment(J.Assignment a, AtomicBoolean hasAssignment) return assignment; } + + @Override + public J.@NotNull Unary visitUnary(final J.@NotNull Unary unary, final AtomicBoolean hasAssignment) { + if (hasAssignment.get()) { + return unary; + } + + final J.Unary u = super.visitUnary(unary, hasAssignment); + if (u.getOperator().isModifying() && u.getExpression() instanceof J.Identifier) { + final J.Identifier i = (J.Identifier) u.getExpression(); + if (i.getSimpleName().equals(this.variable.getSimpleName())) { + hasAssignment.set(true); + } + } + return u; + } + + @Override + public J.AssignmentOperation visitAssignmentOperation(final J.AssignmentOperation assignOp, final AtomicBoolean hasAssignment) { + if (hasAssignment.get()) { + return assignOp; + } + + final J.AssignmentOperation a = super.visitAssignmentOperation(assignOp, hasAssignment); + if (a.getVariable() instanceof J.Identifier) { + final J.Identifier i = (J.Identifier) a.getVariable(); + if (i.getSimpleName().equals(this.variable.getSimpleName())) { + hasAssignment.set(true); + } + } + return a; + } } private static Statement updateParam(final Statement p) { diff --git a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java index b790bf752..8204a6c17 100644 --- a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java @@ -239,4 +239,73 @@ public String getSubeventNameForCategoryGroupId(final String subeventName, final ) ); } + + @Test + void doNotReplaceIfAssignedThroughUnaryOrAccumulator(){ + rewriteRun(java(""" + package Test; + + class Test { + + protected int addFinalToThisVar(int unalteredVariable) { + return unalteredVariable; + } + + protected int increment(int variableToIncrement) { + variableToIncrement++; + return variableToIncrement; + } + + protected int preIncrement(int variableToPreIncrement) { + return ++variableToPreIncrement; + } + + protected int decrement(int variableToDecrement) { + variableToDecrement--; + return variableToDecrement; + } + + protected int preDecrement(int variableToPreDecrement) { + return --variableToPreDecrement; + } + + protected int accumulate(int add, int accumulator) { + accumulator += add; + return accumulator; + } + } + """, """ + package Test; + + class Test { + + protected int addFinalToThisVar(final int unalteredVariable) { + return unalteredVariable; + } + + protected int increment(int variableToIncrement) { + variableToIncrement++; + return variableToIncrement; + } + + protected int preIncrement(int variableToPreIncrement) { + return ++variableToPreIncrement; + } + + protected int decrement(int variableToDecrement) { + variableToDecrement--; + return variableToDecrement; + } + + protected int preDecrement(int variableToPreDecrement) { + return --variableToPreDecrement; + } + + protected int accumulate(int add, int accumulator) { + accumulator += add; + return accumulator; + } + } + """)); + } } From bf5c6222a2ddf26afa53ebe61005b015b7ead8d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Bertolo=20Fafi=C3=A1n?= Date: Mon, 2 Oct 2023 10:10:04 +0200 Subject: [PATCH 2/5] chore: add language tag --- .../openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java index 8204a6c17..5c8c2a06b 100644 --- a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java @@ -242,6 +242,7 @@ public String getSubeventNameForCategoryGroupId(final String subeventName, final @Test void doNotReplaceIfAssignedThroughUnaryOrAccumulator(){ + //language=java rewriteRun(java(""" package Test; From a4debfa4e1409b03456fe9ca9afa6b8cc3d5a1a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Bertolo=20Fafi=C3=A1n?= Date: Mon, 2 Oct 2023 10:12:53 +0200 Subject: [PATCH 3/5] chore: mark contributed code --- .../org/openrewrite/staticanalysis/FinalizeMethodArguments.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java index 7daeb179a..3db64a8d4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java +++ b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java @@ -147,6 +147,7 @@ public J.Assignment visitAssignment(J.Assignment a, AtomicBoolean hasAssignment) return assignment; } + // contributed as a fix for issue #176 @Override public J.@NotNull Unary visitUnary(final J.@NotNull Unary unary, final AtomicBoolean hasAssignment) { if (hasAssignment.get()) { @@ -163,6 +164,7 @@ public J.Assignment visitAssignment(J.Assignment a, AtomicBoolean hasAssignment) return u; } + // contributed as a fix for issue #176 @Override public J.AssignmentOperation visitAssignmentOperation(final J.AssignmentOperation assignOp, final AtomicBoolean hasAssignment) { if (hasAssignment.get()) { From a4575ae21e4c2357b0c6c042cf395b03e0cd1531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Bertolo=20Fafi=C3=A1n?= <60966552+AlejandroBertolo@users.noreply.github.com> Date: Mon, 2 Oct 2023 10:49:42 +0200 Subject: [PATCH 4/5] Update src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java Co-authored-by: Tim te Beek --- .../openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java index 5c8c2a06b..3ff19a5a1 100644 --- a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java @@ -241,6 +241,7 @@ public String getSubeventNameForCategoryGroupId(final String subeventName, final } @Test + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/176") void doNotReplaceIfAssignedThroughUnaryOrAccumulator(){ //language=java rewriteRun(java(""" From ca9c0a61a1d11e85a7b7203c180ccb5c8c092d0f Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 2 Oct 2023 11:17:20 +0200 Subject: [PATCH 5/5] Polish --- .../FinalizeMethodArguments.java | 5 +- .../FinalizeMethodArgumentsTest.java | 329 ++++++++---------- 2 files changed, 150 insertions(+), 184 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java index 3db64a8d4..d7191cdb2 100644 --- a/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java +++ b/src/main/java/org/openrewrite/staticanalysis/FinalizeMethodArguments.java @@ -12,7 +12,6 @@ import lombok.EqualsAndHashCode; import lombok.Value; -import org.jetbrains.annotations.NotNull; import org.openrewrite.*; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; @@ -147,9 +146,8 @@ public J.Assignment visitAssignment(J.Assignment a, AtomicBoolean hasAssignment) return assignment; } - // contributed as a fix for issue #176 @Override - public J.@NotNull Unary visitUnary(final J.@NotNull Unary unary, final AtomicBoolean hasAssignment) { + public J.Unary visitUnary(final J.Unary unary, final AtomicBoolean hasAssignment) { if (hasAssignment.get()) { return unary; } @@ -164,7 +162,6 @@ public J.Assignment visitAssignment(J.Assignment a, AtomicBoolean hasAssignment) return u; } - // contributed as a fix for issue #176 @Override public J.AssignmentOperation visitAssignmentOperation(final J.AssignmentOperation assignOp, final AtomicBoolean hasAssignment) { if (hasAssignment.get()) { diff --git a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java index 3ff19a5a1..33982ae33 100644 --- a/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/FinalizeMethodArgumentsTest.java @@ -76,13 +76,12 @@ void doNotAddFinalIfAssigned() { //language=java java( """ - package a; - class A { - void SubeventUtils(String a) { - a = "abc"; - } - } - """ + class A { + void SubeventUtils(String a) { + a = "abc"; + } + } + """ ) ); } @@ -93,12 +92,10 @@ void doNotAddFinalIfInterface() { //language=java java( """ - package a; - public interface MarketDeleteService { - - void deleteMarket(Long marketId, String deletionTimestamp); - } - """ + public interface MarketDeleteService { + void deleteMarket(Long marketId, String deletionTimestamp); + } + """ ) ); } @@ -110,25 +107,17 @@ void replaceWithFinalModifier() { //language=java java( """ - package com.test; - - class TestClass { - - private void getAccaCouponData(String responsiveRequestConfig, String card) { - - } - } - """, + class TestClass { + private void getAccaCouponData(String responsiveRequestConfig, String card) { + } + } + """, """ - package com.test; - - class TestClass { - - private void getAccaCouponData(final String responsiveRequestConfig, final String card) { - - } - } - """ + class TestClass { + private void getAccaCouponData(final String responsiveRequestConfig, final String card) { + } + } + """ ) ); } @@ -139,175 +128,155 @@ void replaceWithFinalModifierWhenAnnotated() { //language=java java( """ - public class Test { - public void test(@Override Integer test) {} - } - """, + class Test { + public void test(@Override Integer test) {} + } + """, """ - public class Test { - public void test(@Override final Integer test) {} - } - """ + class Test { + public void test(@Override final Integer test) {} + } + """ ) ); } @Test - void replaceWithFinalModifierNoArguments() { + void doNotReplaceWithFinalModifier() { rewriteRun( + spec -> spec.typeValidationOptions(TypeValidation.none()), //language=java java( """ - package com.test; - - class TestClass { - - private void getAccaCouponData() { - - } - } - """ + package responsive.utils.subevent; + + import responsive.enums.subevent.SubeventTypes; + import responsive.model.dto.card.SubEvent; + import java.util.List; + import org.springframework.beans.factory.annotation.Value; + import org.springframework.stereotype.Component; + + import static responsive.enums.matchdata.MatchDataTitleSeparator.AT; + import static responsive.enums.matchdata.MatchDataTitleSeparator.VS; + import static java.lang.String.format; + import static org.apache.commons.lang3.StringUtils.splitByWholeSeparator; + + /** + * Created by mza05 on 13/10/2017. + */ + @Component + class SubeventUtils { + + private static final String SUBEVENT_FORMAT = "%s%s%s"; + private final List categoryGroupIdForChangeSubeventName; + + public SubeventUtils( + @Value("#{'${responsive.category.group.id.change.subevent.name}'.split(',')}") final List categoryGroupIdForChangeSubeventName) { + this.categoryGroupIdForChangeSubeventName = categoryGroupIdForChangeSubeventName; + } + + public static boolean isSubeventOfSpecifiedType(final SubEvent subEvent, final List requiredTypes) { + + if (subEvent.getType() == null) { + return false; + } + return requiredTypes.stream() + .anyMatch(requiredType -> requiredType.getType().equalsIgnoreCase(subEvent.getType())); + + } + + /** + * Change SubeventName by CategoryGroupId and rebub + * @param subeventName + * @param categoryGroupId + * @return + */ + public String getSubeventNameForCategoryGroupId(final String subeventName, final Integer categoryGroupId) { + + if (subeventName != null && categoryGroupId != null + && subeventName.contains(AT.getSeparator()) + && categoryGroupIdForChangeSubeventName.contains(categoryGroupId)) { + final var subeventTeamSplit = splitByWholeSeparator(subeventName, AT.getSeparator()); + if (subeventTeamSplit.length > 0) { + return format(SUBEVENT_FORMAT, subeventTeamSplit[0], VS.getSeparator(), subeventTeamSplit[1]); + } + } + + return subeventName; + } + } + """ ) ); } @Test - void doNotReplaceWithFinalModifier() { + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/176") + void doNotReplaceIfAssignedThroughUnaryOrAccumulator() { rewriteRun( - spec -> spec.typeValidationOptions(TypeValidation.none()), //language=java java( """ - package responsive.utils.subevent; - - import responsive.enums.subevent.SubeventTypes; - import responsive.model.dto.card.SubEvent; - import java.util.List; - import org.springframework.beans.factory.annotation.Value; - import org.springframework.stereotype.Component; - - import static responsive.enums.matchdata.MatchDataTitleSeparator.AT; - import static responsive.enums.matchdata.MatchDataTitleSeparator.VS; - import static java.lang.String.format; - import static org.apache.commons.lang3.StringUtils.splitByWholeSeparator; - - /** - * Created by mza05 on 13/10/2017. - */ - @Component - public class SubeventUtils { - - private static final String SUBEVENT_FORMAT = "%s%s%s"; - private final List categoryGroupIdForChangeSubeventName; - - public SubeventUtils( - @Value("#{'${responsive.category.group.id.change.subevent.name}'.split(',')}") final List categoryGroupIdForChangeSubeventName) { - this.categoryGroupIdForChangeSubeventName = categoryGroupIdForChangeSubeventName; - } - - public static boolean isSubeventOfSpecifiedType(final SubEvent subEvent, final List requiredTypes) { - - if (subEvent.getType() == null) { - return false; - } - return requiredTypes.stream() - .anyMatch(requiredType -> requiredType.getType().equalsIgnoreCase(subEvent.getType())); - - } - - /** - * Change SubeventName by CategoryGroupId and rebub - * @param subeventName - * @param categoryGroupId - * @return - */ - public String getSubeventNameForCategoryGroupId(final String subeventName, final Integer categoryGroupId) { - - if (subeventName != null && categoryGroupId != null - && subeventName.contains(AT.getSeparator()) - && categoryGroupIdForChangeSubeventName.contains(categoryGroupId)) { - final var subeventTeamSplit = splitByWholeSeparator(subeventName, AT.getSeparator()); - if (subeventTeamSplit.length > 0) { - return format(SUBEVENT_FORMAT, subeventTeamSplit[0], VS.getSeparator(), subeventTeamSplit[1]); - } - } - - return subeventName; - } - } - """ + class Test { + protected int addFinalToThisVar(int unalteredVariable) { + return unalteredVariable; + } + + protected int increment(int variableToIncrement) { + variableToIncrement++; + return variableToIncrement; + } + + protected int preIncrement(int variableToPreIncrement) { + return ++variableToPreIncrement; + } + + protected int decrement(int variableToDecrement) { + variableToDecrement--; + return variableToDecrement; + } + + protected int preDecrement(int variableToPreDecrement) { + return --variableToPreDecrement; + } + + protected int accumulate(int add, int accumulator) { + accumulator += add; + return accumulator; + } + } + """, + """ + class Test { + protected int addFinalToThisVar(final int unalteredVariable) { + return unalteredVariable; + } + + protected int increment(int variableToIncrement) { + variableToIncrement++; + return variableToIncrement; + } + + protected int preIncrement(int variableToPreIncrement) { + return ++variableToPreIncrement; + } + + protected int decrement(int variableToDecrement) { + variableToDecrement--; + return variableToDecrement; + } + + protected int preDecrement(int variableToPreDecrement) { + return --variableToPreDecrement; + } + + protected int accumulate(int add, int accumulator) { + accumulator += add; + return accumulator; + } + } + """ ) ); } - - @Test - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/176") - void doNotReplaceIfAssignedThroughUnaryOrAccumulator(){ - //language=java - rewriteRun(java(""" - package Test; - - class Test { - - protected int addFinalToThisVar(int unalteredVariable) { - return unalteredVariable; - } - - protected int increment(int variableToIncrement) { - variableToIncrement++; - return variableToIncrement; - } - - protected int preIncrement(int variableToPreIncrement) { - return ++variableToPreIncrement; - } - - protected int decrement(int variableToDecrement) { - variableToDecrement--; - return variableToDecrement; - } - - protected int preDecrement(int variableToPreDecrement) { - return --variableToPreDecrement; - } - - protected int accumulate(int add, int accumulator) { - accumulator += add; - return accumulator; - } - } - """, """ - package Test; - - class Test { - - protected int addFinalToThisVar(final int unalteredVariable) { - return unalteredVariable; - } - - protected int increment(int variableToIncrement) { - variableToIncrement++; - return variableToIncrement; - } - - protected int preIncrement(int variableToPreIncrement) { - return ++variableToPreIncrement; - } - - protected int decrement(int variableToDecrement) { - variableToDecrement--; - return variableToDecrement; - } - - protected int preDecrement(int variableToPreDecrement) { - return --variableToPreDecrement; - } - - protected int accumulate(int add, int accumulator) { - accumulator += add; - return accumulator; - } - } - """)); - } }