diff --git a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java index b66d08bef..af572815c 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java +++ b/src/main/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java @@ -67,6 +67,15 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu if (shouldRetainOnStaticMethod(methodType)) { return m; } + // If the enclosing method has dependent type parameters (e.g., ), + // removing the explicit type argument from this call can prevent the compiler from + // correctly inferring the dependent type parameters. For example: + // lexicographical(Comparator.naturalOrder()) + // where lexicographical is declared as , the explicit + // is required so the compiler can bind both T and S correctly. + if (hasDependentTypeParameters(enclosingMethod.getMethodType())) { + return m; + } // Cannot remove type parameters if it would introduce ambiguity about which method should be called if (enclosingMethod.getMethodType() == null) { return m; @@ -173,6 +182,73 @@ private boolean canInferTypeArgumentsFromArguments(JavaType.Method methodType) { .forEach(it -> formalTypeNames.remove(((JavaType.GenericTypeVariable) it).getName())); return formalTypeNames.isEmpty(); } + + /** + * Returns true if the given method has dependent type parameters — that is, at least one + * type parameter has a bound that references another type parameter declared by the same + * method (e.g., {@code }). + *

+ * When such dependencies exist, removing explicit type arguments from an argument expression + * can prevent the compiler from resolving the dependent type parameters correctly. + */ + private boolean hasDependentTypeParameters(JavaType.@Nullable Method enclosingMethodType) { + if (enclosingMethodType == null) { + return false; + } + List formalNames = enclosingMethodType.getDeclaredFormalTypeNames(); + if (formalNames.size() < 2) { + // Need at least two type parameters for one to bound another + return false; + } + List typesToSearch = new ArrayList<>(enclosingMethodType.getParameterTypes()); + if (enclosingMethodType.getReturnType() != null) { + typesToSearch.add(enclosingMethodType.getReturnType()); + } + return typesToSearch.stream() + .anyMatch(t -> hasGenericWithFormalParamBound(t, formalNames)); + } + + /** + * Recursively searches {@code type} for a {@link JavaType.GenericTypeVariable} whose name + * is among {@code formalNames} AND whose bounds directly reference another name in + * {@code formalNames}. Such a variable represents a dependent type parameter. + */ + private boolean hasGenericWithFormalParamBound(@Nullable JavaType type, List formalNames) { + if (type instanceof JavaType.GenericTypeVariable) { + JavaType.GenericTypeVariable gtv = (JavaType.GenericTypeVariable) type; + if (formalNames.contains(gtv.getName())) { + for (JavaType bound : gtv.getBounds()) { + if (referencesAnyFormalName(bound, formalNames)) { + return true; + } + } + } + return false; + } + if (type instanceof JavaType.Parameterized) { + return ((JavaType.Parameterized) type).getTypeParameters().stream() + .anyMatch(t -> hasGenericWithFormalParamBound(t, formalNames)); + } + if (type instanceof JavaType.Array) { + return hasGenericWithFormalParamBound(((JavaType.Array) type).getElemType(), formalNames); + } + return false; + } + + /** + * Returns true if {@code type} is, or transitively contains, a + * {@link JavaType.GenericTypeVariable} whose name is in {@code formalNames}. + */ + private boolean referencesAnyFormalName(@Nullable JavaType type, List formalNames) { + if (type instanceof JavaType.GenericTypeVariable) { + return formalNames.contains(((JavaType.GenericTypeVariable) type).getName()); + } + if (type instanceof JavaType.Parameterized) { + return ((JavaType.Parameterized) type).getTypeParameters().stream() + .anyMatch(t -> referencesAnyFormalName(t, formalNames)); + } + return false; + } }); } } diff --git a/src/main/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversions.java b/src/main/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversions.java new file mode 100644 index 000000000..191b1077e --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversions.java @@ -0,0 +1,93 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * 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.staticanalysis; + +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.jspecify.annotations.Nullable; +import org.openrewrite.*; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesMethod; +import org.openrewrite.java.tree.J; + +import java.util.Set; + +import static java.util.Collections.singleton; + +@EqualsAndHashCode(callSuper = false) +@Value +public class UseLocaleWithCaseConversions extends Recipe { + + private static final MethodMatcher TO_LOWER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toLowerCase()"); + private static final MethodMatcher TO_UPPER_CASE_METHOD_MATCHER = new MethodMatcher("java.lang.String toUpperCase()"); + + @Option(displayName = "Locale", + description = "The name of the constant field on `java.util.Locale` to use, such as `ROOT` or `US`. " + + "Defaults to `ROOT` when not provided, which is recommended for locale-independent, " + + "algorithmic case conversions.", + example = "ROOT", + required = false) + @Nullable + String locale; + + @Override + public String getDisplayName() { + return "Specify a `Locale` when comparing locale-dependent data"; + } + + @Override + public String getDescription() { + return "Explicitly specify a `Locale` when calling `String#toLowerCase()` or `String#toUpperCase()`, " + + "as the result of case conversion can vary between locales. For example, converting the letter " + + "`I` to lower case yields a dotless `ı` under a Turkish locale instead of the expected `i`. " + + "Relying on the platform default locale can therefore lead to unexpected and hard-to-reproduce " + + "bugs. See [STR02-J](https://wiki.sei.cmu.edu/confluence/display/java/STR02-J.+Specify+an+appropriate+locale+when+comparing+locale-dependent+data) " + + "for more information."; + } + + @Override + public Set getTags() { + return singleton("STR02-J"); + } + + @Override + public TreeVisitor getVisitor() { + String localeConstant = (locale == null || locale.isEmpty()) ? "ROOT" : locale; + return Preconditions.check( + Preconditions.or( + new UsesMethod<>(TO_LOWER_CASE_METHOD_MATCHER), + new UsesMethod<>(TO_UPPER_CASE_METHOD_MATCHER) + ), + new JavaIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + J.MethodInvocation m = super.visitMethodInvocation(method, ctx); + if (TO_LOWER_CASE_METHOD_MATCHER.matches(m) || TO_UPPER_CASE_METHOD_MATCHER.matches(m)) { + m = JavaTemplate.builder("java.util.Locale." + localeConstant) + .contextSensitive() + .imports("java.util.Locale") + .build() + .apply(updateCursor(m), m.getCoordinates().replaceArguments()); + maybeAddImport("java.util.Locale"); + } + return m; + } + } + ); + } +} diff --git a/src/main/resources/META-INF/rewrite/common-static-analysis.yml b/src/main/resources/META-INF/rewrite/common-static-analysis.yml index d49c0e70f..dbd489e47 100644 --- a/src/main/resources/META-INF/rewrite/common-static-analysis.yml +++ b/src/main/resources/META-INF/rewrite/common-static-analysis.yml @@ -102,6 +102,9 @@ recipeList: # - org.openrewrite.staticanalysis.UseCollectionInterfaces - org.openrewrite.staticanalysis.UseDiamondOperator - org.openrewrite.staticanalysis.UseJavaStyleArrayDeclarations +# Changes case-conversion semantics for locale-sensitive strings; opt-in only. +# https://github.com/openrewrite/rewrite-static-analysis/issues/78 +# - org.openrewrite.staticanalysis.UseLocaleWithCaseConversions - org.openrewrite.staticanalysis.UsePortableNewlines # https://github.com/openrewrite/rewrite-static-analysis/issues/10 # - org.openrewrite.staticanalysis.UseLambdaForFunctionalInterface diff --git a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java new file mode 100644 index 000000000..3c4f53573 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArguments.java @@ -0,0 +1,126 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * 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.staticanalysis; + +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; + +class UseLocaleWithCaseConversionsTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new UseLocaleWithCaseConversions(null)); + } + + @DocumentExample + @Test + void toLowerCaseWithoutLocale() { + rewriteRun( + //language=java + java( + """ + class Test { + String lower(String str) { + return str.toLowerCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.ROOT); + } + } + """ + ) + ); + } + + @Test + void toUpperCaseWithoutLocale() { + rewriteRun( + //language=java + java( + """ + class Test { + String upper(String str) { + return str.toUpperCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String upper(String str) { + return str.toUpperCase(Locale.ROOT); + } + } + """ + ) + ); + } + + @Test + void doNotChangeWhenLocaleAlreadySpecified() { + rewriteRun( + //language=java + java( + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.US); + } + } + """ + ) + ); + } + + @Test + void useConfiguredLocale() { + rewriteRun( + spec -> spec.recipe(new UseLocaleWithCaseConversions("US")), + //language=java + java( + """ + class Test { + String lower(String str) { + return str.toLowerCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.US); + } + } + """ + ) + ); + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArgumentsTest.java b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArgumentsTest.java index 19bd9cfd5..d88d5b037 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArgumentsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UnnecessaryExplicitTypeArgumentsTest.java @@ -346,6 +346,33 @@ void test(Class clazz) { } } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/783") + @Test + void retainExplicitTypeArgumentForDependentTypeParameters() { + rewriteRun( + //language=java + java( + """ + import java.util.Comparator; + + class Test { + /** + * A method with dependent type parameters where S extends T. + * The explicit type argument on the Comparator is required so the compiler + * can bind both T and S (S extends T) correctly. + */ + static Comparator> lexicographical(Comparator comparator) { + return null; + } + + static final Comparator> COMPARATOR = + lexicographical(Comparator.naturalOrder()); + } + """ + ) + ); + } + @Nested class kotlinTest { @Test diff --git a/src/test/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversionsTest.java b/src/test/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversionsTest.java new file mode 100644 index 000000000..3c4f53573 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/UseLocaleWithCaseConversionsTest.java @@ -0,0 +1,126 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * 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.staticanalysis; + +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; + +class UseLocaleWithCaseConversionsTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new UseLocaleWithCaseConversions(null)); + } + + @DocumentExample + @Test + void toLowerCaseWithoutLocale() { + rewriteRun( + //language=java + java( + """ + class Test { + String lower(String str) { + return str.toLowerCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.ROOT); + } + } + """ + ) + ); + } + + @Test + void toUpperCaseWithoutLocale() { + rewriteRun( + //language=java + java( + """ + class Test { + String upper(String str) { + return str.toUpperCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String upper(String str) { + return str.toUpperCase(Locale.ROOT); + } + } + """ + ) + ); + } + + @Test + void doNotChangeWhenLocaleAlreadySpecified() { + rewriteRun( + //language=java + java( + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.US); + } + } + """ + ) + ); + } + + @Test + void useConfiguredLocale() { + rewriteRun( + spec -> spec.recipe(new UseLocaleWithCaseConversions("US")), + //language=java + java( + """ + class Test { + String lower(String str) { + return str.toLowerCase(); + } + } + """, + """ + import java.util.Locale; + + class Test { + String lower(String str) { + return str.toLowerCase(Locale.US); + } + } + """ + ) + ); + } +}