DeclarationSiteTypeVariance doesn't operate on overridden methods / i…

…ssue #2621
openrewrite · Jan 11, 2023 · 0b99429 · 0b99429
1 parent 9ab9f10
commit 0b99429
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 5 deletions.
diff --git a/...java-test/src/test/java/org/openrewrite/java/cleanup/DeclarationSiteTypeVarianceTest.java b/...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,15 +32,17 @@ class DeclarationSiteTypeVarianceTest implements RewriteTest {
     public void defaults(RecipeSpec spec) {
         spec.recipe(new DeclarationSiteTypeVariance(
           List.of("java.util.function.Function<IN, OUT>"),
-          List.of("java.lang.*")
+          List.of("java.lang.*"),
+          true
         ));
     }
 
     @Test
     void validation() {
         assertThat(new DeclarationSiteTypeVariance(
           List.of("java.util.function.Function<INVALID, OUT>"),
-          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<INVARIANT, OUT>"),
-            List.of("java.lang.*")
+            List.of("java.lang.*"),
+            null
           )),
           java(
             """
@@ -177,4 +181,35 @@ void test(Function<In, ? extends Out> f) {
           )
         );
     }
+
+    @Test
+    void overriddenMethods() {
+        rewriteRun(
+          java(
+            """
+              interface In {}
+              interface Out {}
+              """
+          ),
+          java(
+            """
+              import java.util.function.Function;
+              interface TestInterface {
+                  void test(Function<In, Out> f);
+              }
+              """,
+            SourceSpec::skip
+          ),
+          java(
+            """
+              import java.util.function.Function;
+              class TestImpl implements TestInterface {
+                  @Override
+                  public void test(Function<In, Out> 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
@@ -48,6 +48,12 @@ public class DeclarationSiteTypeVariance extends Recipe {
     @Nullable
     List<String> 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<?, ExecutionContext> 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
@@ -965,6 +965,37 @@ public FullyQualified getDeclaringType() {
             return declaringType;
         }
 
+        public boolean isOverride() {
+            if (declaringType instanceof JavaType.Unknown) {
+                return false;
+            }
+
+            Stack<FullyQualified> 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<JavaType> 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
@@ -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<IN, OUT>`, it should rather be `Function<? super IN, ? extends OUT>`. This recipe checks for method parameters of well-known types."
 recipeList:
   - org.openrewrite.java.cleanup.DeclarationSiteTypeVariance: