Detect method overrides in lambda expressions

Fixes: 192562926 Test: RequiresOptInDetectorTest Change-Id: I6f28cb80bd87a2de4066c522d75c72586983f595
ggfan · Jul 15, 2021 · 2996dc0 · 2996dc0
1 parent e737b22
commit 2996dc0
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 38 deletions.
diff --git a/annotation/annotation-experimental-lint/integration-tests/lint-baseline.xml b/annotation/annotation-experimental-lint/integration-tests/lint-baseline.xml
@@ -67,14 +67,25 @@
             column="25"/>
     </issue>
 
+    <issue
+        id="UnsafeOptInUsageError"
+        message="This declaration is opt-in and its usage should be marked with `@sample.optin.ExperimentalJavaAnnotation` or `@OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class)`"
+        errorLine1="        ExperimentalInterface lambda = () -> {}; // unsafe"
+        errorLine2="                                       ~~~~~~~~">
+        <location
+            file="src/main/java/sample/optin/RegressionTestJava192562469.java"
+            line="67"
+            column="40"/>
+    </issue>
+
     <issue
         id="UnsafeOptInUsageError"
         message="This declaration is opt-in and its usage should be marked with `@sample.optin.ExperimentalJavaAnnotation` or `@OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class)`"
         errorLine1="        public void experimentalMethod() {} // unsafe override"
         errorLine2="                    ~~~~~~~~~~~~~~~~~~">
         <location
             file="src/main/java/sample/optin/RegressionTestJava192562926.java"
-            line="35"
+            line="39"
             column="21"/>
     </issue>
 
@@ -85,10 +96,21 @@
         errorLine2="                        ~~~~~~~~~~~~~~~~~~">
         <location
             file="src/main/java/sample/optin/RegressionTestJava192562926.java"
-            line="45"
+            line="49"
             column="25"/>
     </issue>
 
+    <issue
+        id="UnsafeOptInUsageError"
+        message="This declaration is opt-in and its usage should be marked with `@sample.optin.ExperimentalJavaAnnotation` or `@OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class)`"
+        errorLine1="        StableInterface lambda = () -> {}; // unsafe override"
+        errorLine2="                                 ~~~~~~~~">
+        <location
+            file="src/main/java/sample/optin/RegressionTestJava192562926.java"
+            line="52"
+            column="34"/>
+    </issue>
+
     <issue
         id="UnsafeOptInUsageError"
         message="This declaration is opt-in and its usage should be marked with `@sample.optin.ExperimentalJavaAnnotation` or `@OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class)`"

diff --git a/...mental-lint/integration-tests/src/main/java/sample/optin/RegressionTestJava192562926.java b/...mental-lint/integration-tests/src/main/java/sample/optin/RegressionTestJava192562926.java
@@ -23,6 +23,10 @@
 @SuppressWarnings("unused")
 public class RegressionTestJava192562926 {
     interface StableInterface {
+        // This method will show up first in the list provided by PsiClass.allMethods, but it's
+        // not the method that we want to inspect since it has a concrete implementation.
+        default void abstractMethodWithDefault() {}
+
         @ExperimentalJavaAnnotation
         void experimentalMethod();
     }
@@ -45,6 +49,6 @@ void regressionTestOverrides() {
             public void experimentalMethod() {} // unsafe override
         };
 
-        StableInterface lambda = () -> {}; // safe (due to bug in Kotlin compiler)
+        StableInterface lambda = () -> {}; // unsafe override
     }
 }
diff --git a/...mental-lint/integration-tests/src/main/java/sample/optin/RegressionTestKotlin192562926.kt b/...mental-lint/integration-tests/src/main/java/sample/optin/RegressionTestKotlin192562926.kt
@@ -23,6 +23,10 @@ package sample.optin
  */
 class RegressionTestKotlin192562926 {
     internal fun interface StableInterface {
+        // This method will show up first in the list provided by PsiClass.allMethods, but it's
+        // not the method that we want to inspect since it has a concrete implementation.
+        fun abstractMethodWithDefault() {}
+
         @ExperimentalKotlinAnnotation
         fun experimentalMethod()
     }
@@ -41,6 +45,6 @@ class RegressionTestKotlin192562926 {
         val anonymous: StableInterface = object : StableInterface {
             override fun experimentalMethod() {} // unsafe override
         }
-        val lambda = StableInterface {} // safe (due to bug in Kotlin compiler)
+        val lambda = StableInterface {} // unsafe override, but Kotlin compiler does not detect
     }
 }
diff --git a/...rimental-lint/src/main/java/androidx/annotation/experimental/lint/ExperimentalDetector.kt b/...rimental-lint/src/main/java/androidx/annotation/experimental/lint/ExperimentalDetector.kt
@@ -37,7 +37,9 @@ import com.intellij.psi.PsiClassType
 import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiField
 import com.intellij.psi.PsiMethod
+import com.intellij.psi.PsiModifier
 import com.intellij.psi.PsiModifierListOwner
+import com.intellij.psi.impl.source.PsiClassReferenceType
 import com.intellij.psi.search.GlobalSearchScope
 import com.intellij.psi.util.PsiTreeUtil
 import com.intellij.psi.util.PsiTypesUtil
@@ -54,16 +56,15 @@ import org.jetbrains.uast.UClassLiteralExpression
 import org.jetbrains.uast.UElement
 import org.jetbrains.uast.UEnumConstant
 import org.jetbrains.uast.UExpression
+import org.jetbrains.uast.ULambdaExpression
 import org.jetbrains.uast.UMethod
 import org.jetbrains.uast.UReferenceExpression
 import org.jetbrains.uast.USimpleNameReferenceExpression
 import org.jetbrains.uast.UVariable
-import org.jetbrains.uast.getParentOfType
 import org.jetbrains.uast.java.JavaUAnnotation
 import org.jetbrains.uast.toUElement
 import org.jetbrains.uast.tryResolve
 import java.util.ArrayList
-import java.util.Locale
 
 class ExperimentalDetector : Detector(), SourceCodeScanner {
     private val visitedUsages: MutableMap<UElement, MutableSet<String>> = mutableMapOf()
@@ -79,14 +80,32 @@ class ExperimentalDetector : Detector(), SourceCodeScanner {
         "java.lang.Object"
     )
 
+    override fun visitClass(
+        context: JavaContext,
+        lambda: ULambdaExpression,
+    ) {
+        // Infer the overridden method by taking the first (and only) abstract method from the
+        // functional interface being implemented.
+        val superClass = (lambda.functionalInterfaceType as? PsiClassReferenceType)?.resolve()
+        val superMethod = superClass?.allMethods
+            ?.first { method -> method.isAbstract() }
+            ?.toUElement()
+
+        if (superMethod is UMethod) {
+            checkMethodOverride(context, lambda, superMethod)
+        }
+    }
+
     override fun visitClass(
         context: JavaContext,
         declaration: UClass,
     ) {
         declaration.methods.forEach { method ->
             val eval = context.evaluator
             if (eval.isOverride(method, true)) {
-                checkMethodOverride(context, method)
+                method.findSuperMethods().forEach { superMethod ->
+                    checkMethodOverride(context, method, superMethod)
+                }
             }
         }
     }
@@ -99,32 +118,31 @@ class ExperimentalDetector : Detector(), SourceCodeScanner {
      */
     private fun checkMethodOverride(
         context: JavaContext,
-        methodDeclaration: UMethod,
+        usage: UElement,
+        superMethod: PsiMethod,
     ) {
-        methodDeclaration.findSuperMethods().forEach { superMethod ->
-            val evaluator = context.evaluator
-            val allAnnotations = evaluator.getAllAnnotations(superMethod, inHierarchy = true)
-            val methodAnnotations = filterRelevantAnnotations(
-                evaluator, allAnnotations, methodDeclaration,
-            )
+        val evaluator = context.evaluator
+        val allAnnotations = evaluator.getAllAnnotations(superMethod, inHierarchy = true)
+        val methodAnnotations = filterRelevantAnnotations(
+            evaluator, allAnnotations, usage,
+        )
 
-            // Look for annotations on the class as well: these trickle
-            // down to all the methods in the class
-            val containingClass: PsiClass? = superMethod.containingClass
-            val (classAnnotations, pkgAnnotations) = getClassAndPkgAnnotations(
-                containingClass, evaluator, methodDeclaration
-            )
+        // Look for annotations on the class as well: these trickle
+        // down to all the methods in the class
+        val containingClass: PsiClass? = superMethod.containingClass
+        val (classAnnotations, pkgAnnotations) = getClassAndPkgAnnotations(
+            containingClass, evaluator, usage
+        )
 
-            doCheckMethodOverride(
-                context,
-                superMethod,
-                methodAnnotations,
-                classAnnotations,
-                pkgAnnotations,
-                methodDeclaration,
-                containingClass,
-            )
-        }
+        doCheckMethodOverride(
+            context,
+            superMethod,
+            methodAnnotations,
+            classAnnotations,
+            pkgAnnotations,
+            usage,
+            containingClass,
+        )
     }
 
     /**
@@ -139,13 +157,13 @@ class ExperimentalDetector : Detector(), SourceCodeScanner {
         methodAnnotations: List<UAnnotation>,
         classAnnotations: List<UAnnotation>,
         pkgAnnotations: List<UAnnotation>,
-        method: UMethod,
+        usage: UElement,
         containingClass: PsiClass?,
     ) {
         if (methodAnnotations.isNotEmpty()) {
             checkAnnotations(
                 context,
-                argument = method,
+                argument = usage,
                 type = AnnotationUsageType.METHOD_CALL,
                 method = superMethod,
                 referenced = superMethod,
@@ -160,7 +178,7 @@ class ExperimentalDetector : Detector(), SourceCodeScanner {
         if (containingClass != null && classAnnotations.isNotEmpty()) {
             checkAnnotations(
                 context,
-                argument = method,
+                argument = usage,
                 type = AnnotationUsageType.METHOD_CALL_CLASS,
                 method = superMethod,
                 referenced = superMethod,
@@ -175,7 +193,7 @@ class ExperimentalDetector : Detector(), SourceCodeScanner {
         if (pkgAnnotations.isNotEmpty()) {
             checkAnnotations(
                 context,
-                argument = method,
+                argument = usage,
                 type = AnnotationUsageType.METHOD_CALL_PACKAGE,
                 method = superMethod,
                 referenced = superMethod,
@@ -771,3 +789,6 @@ private fun UElement.isDeclarationAnnotatedWithOptInOf(
         ) == true
     }
 } == true
+
+private fun PsiModifierListOwner.isAbstract(): Boolean =
+    modifierList?.hasModifierProperty(PsiModifier.ABSTRACT) == true
diff --git a/...l-lint/src/test/kotlin/androidx/annotation/experimental/lint/RequiresOptInDetectorTest.kt b/...l-lint/src/test/kotlin/androidx/annotation/experimental/lint/RequiresOptInDetectorTest.kt
@@ -313,7 +313,10 @@ src/sample/optin/RegressionTestJava192562469.java:62: Error: This declaration is
 src/sample/optin/RegressionTestJava192562469.java:64: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
             public void experimentalMethod() {} // unsafe override
                         ~~~~~~~~~~~~~~~~~~
-4 errors, 0 warnings
+src/sample/optin/RegressionTestJava192562469.java:67: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
+        ExperimentalInterface lambda = () -> {}; // unsafe
+                                       ~~~~~~~~
+5 errors, 0 warnings
         """.trimIndent()
         /* ktlint-enable max-line-length */
 
@@ -329,13 +332,16 @@ src/sample/optin/RegressionTestJava192562469.java:64: Error: This declaration is
 
         /* ktlint-disable max-line-length */
         val expected = """
-src/sample/optin/RegressionTestJava192562926.java:35: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
+src/sample/optin/RegressionTestJava192562926.java:39: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
         public void experimentalMethod() {} // unsafe override
                     ~~~~~~~~~~~~~~~~~~
-src/sample/optin/RegressionTestJava192562926.java:45: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
+src/sample/optin/RegressionTestJava192562926.java:49: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
             public void experimentalMethod() {} // unsafe override
                         ~~~~~~~~~~~~~~~~~~
-2 errors, 0 warnings
+src/sample/optin/RegressionTestJava192562926.java:52: Error: This declaration is opt-in and its usage should be marked with @sample.optin.ExperimentalJavaAnnotation or @OptIn(markerClass = sample.optin.ExperimentalJavaAnnotation.class) [UnsafeOptInUsageError]
+        StableInterface lambda = () -> {}; // unsafe override
+                                 ~~~~~~~~
+3 errors, 0 warnings
         """.trimIndent()
         /* ktlint-enable max-line-length */