From 0b994292372fce1f911ea7369a4aaf9afa2cb275 Mon Sep 17 00:00:00 2001 From: Jonathan Schneider Date: Tue, 10 Jan 2023 22:25:28 -0500 Subject: [PATCH] DeclarationSiteTypeVariance doesn't operate on overridden methods / issue #2621 --- .../DeclarationSiteTypeVarianceTest.java | 41 +++++++++++++++++-- .../cleanup/DeclarationSiteTypeVariance.java | 11 ++++- .../org/openrewrite/java/tree/JavaType.java | 31 ++++++++++++++ .../resources/META-INF/rewrite/cleanup.yml | 2 +- 4 files changed, 80 insertions(+), 5 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVarianceTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVarianceTest.java index b157577f983..3eb64252970 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVarianceTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVarianceTest.java @@ -19,6 +19,7 @@ import org.openrewrite.config.Environment; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; +import org.openrewrite.test.SourceSpec; import java.util.List; @@ -31,7 +32,8 @@ class DeclarationSiteTypeVarianceTest implements RewriteTest { public void defaults(RecipeSpec spec) { spec.recipe(new DeclarationSiteTypeVariance( List.of("java.util.function.Function"), - List.of("java.lang.*") + List.of("java.lang.*"), + true )); } @@ -39,7 +41,8 @@ public void defaults(RecipeSpec spec) { void validation() { assertThat(new DeclarationSiteTypeVariance( List.of("java.util.function.Function"), - List.of("java.lang.*") + List.of("java.lang.*"), + null ).validate().isInvalid()).isTrue(); } @@ -108,7 +111,8 @@ void invariance() { rewriteRun( spec -> spec.recipe(new DeclarationSiteTypeVariance( List.of("java.util.function.Function"), - List.of("java.lang.*") + List.of("java.lang.*"), + null )), java( """ @@ -177,4 +181,35 @@ void test(Function f) { ) ); } + + @Test + void overriddenMethods() { + rewriteRun( + java( + """ + interface In {} + interface Out {} + """ + ), + java( + """ + import java.util.function.Function; + interface TestInterface { + void test(Function f); + } + """, + SourceSpec::skip + ), + java( + """ + import java.util.function.Function; + class TestImpl implements TestInterface { + @Override + public void test(Function f) { + } + } + """ + ) + ); + } } diff --git a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVariance.java b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVariance.java index 469d2e09fe9..bc6aaf367d6 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVariance.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVariance.java @@ -48,6 +48,12 @@ public class DeclarationSiteTypeVariance extends Recipe { @Nullable List excludedBounds; + @Option(displayName = "Exclude final classes", + description = "If true, do not add variance to final classes.", + required = false) + @Nullable + Boolean excludeFinalClasses; + @Override public String getDisplayName() { return "Properly use declaration-site type variance"; @@ -82,6 +88,9 @@ protected TreeVisitor getVisitor() { @Override public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); + if (m.getMethodType() != null && m.getMethodType().isOverride()) { + return m; + } return m.withParameters(ListUtils.map(m.getParameters(), param -> { if (param instanceof J.VariableDeclarations) { J.VariableDeclarations varParam = (J.VariableDeclarations) param; @@ -117,7 +126,7 @@ private J.ParameterizedType useDeclarationSiteVariance(J.ParameterizedType pt, V } } } - if (fq.getFlags().contains(Flag.Final)) { + if (Boolean.TRUE.equals(excludeFinalClasses) && fq.getFlags().contains(Flag.Final)) { return tp; } } diff --git a/rewrite-java/src/main/java/org/openrewrite/java/tree/JavaType.java b/rewrite-java/src/main/java/org/openrewrite/java/tree/JavaType.java index c1792fcb650..3e3e3ca2a65 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/tree/JavaType.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/tree/JavaType.java @@ -965,6 +965,37 @@ public FullyQualified getDeclaringType() { return declaringType; } + public boolean isOverride() { + if (declaringType instanceof JavaType.Unknown) { + return false; + } + + Stack interfaces = new Stack<>(); + interfaces.addAll(declaringType.getInterfaces()); + + while (!interfaces.isEmpty()) { + FullyQualified declaring = interfaces.pop(); + interfaces.addAll(declaring.getInterfaces()); + + nextMethod: + for (Method method : declaring.getMethods()) { + if (method.getName().equals(name)) { + List params = method.getParameterTypes(); + if (getParameterTypes().size() != method.getParameterTypes().size()) { + continue; + } + for (int i = 0; i < params.size(); i++) { + if (!TypeUtils.isOfType(getParameterTypes().get(i), params.get(i))) { + continue nextMethod; + } + } + return true; + } + } + } + return false; + } + public boolean isInheritedFrom(String fullyQualifiedTypeName) { if (declaringType instanceof JavaType.Unknown) { return false; diff --git a/rewrite-java/src/main/resources/META-INF/rewrite/cleanup.yml b/rewrite-java/src/main/resources/META-INF/rewrite/cleanup.yml index fbc6d295f4e..4e6605cacd3 100644 --- a/rewrite-java/src/main/resources/META-INF/rewrite/cleanup.yml +++ b/rewrite-java/src/main/resources/META-INF/rewrite/cleanup.yml @@ -51,7 +51,7 @@ tags: --- type: specs.openrewrite.org/v1beta/recipe name: org.openrewrite.java.cleanup.CommonDeclarationSiteTypeVariances -displayName: "Properly use declaration-site type variance for well-known types." +displayName: "Properly use declaration-site type variance for well-known types" description: "When using a method parameter like `Function`, it should rather be `Function`. This recipe checks for method parameters of well-known types." recipeList: - org.openrewrite.java.cleanup.DeclarationSiteTypeVariance: