242305422: CheckResultDetector allows unused results in Java synchronized expressions

Fixes: 242305422
Test: Unit test included
Change-Id: I60162490345ef0b25ccb05d6c5ead354f52ae7b7

3 files changed, 63 insertions, 6 deletions

diff --git a/lint/libs/lint-api/src/main/java/com/android/tools/lint/detector/api/UastLintUtils.kt b/lint/libs/lint-api/src/main/java/com/android/tools/lint/detector/api/UastLintUtils.kt
index b87a702441..c65691c8a7 100644
--- a/lint/libs/lint-api/src/main/java/com/android/tools/lint/detector/api/UastLintUtils.kt
+++ b/lint/libs/lint-api/src/main/java/com/android/tools/lint/detector/api/UastLintUtils.kt
@@ -29,6 +29,7 @@ import com.intellij.psi.PsiMethod
 import com.intellij.psi.PsiModifier
 import com.intellij.psi.PsiParameter
 import com.intellij.psi.PsiVariable
+import com.intellij.psi.util.PsiTreeUtil
 import org.jetbrains.kotlin.asJava.elements.FakeFileForLightClass
 import org.jetbrains.kotlin.psi.KtFile
 import org.jetbrains.uast.UAnnotation
@@ -493,6 +494,16 @@ fun UElement.isBelow(parent: UElement, strict: Boolean = false): Boolean {
 }
 
 /**
+ * Returns true if [this] element is a child or indirect child of the
+ * given [parent]. If [strict] is false, this method will return true
+ * when [parent] is the same as [this].
+ */
+fun PsiElement?.isBelow(parent: PsiElement, strict: Boolean = false): Boolean {
+    this ?: return false
+    return PsiTreeUtil.isAncestor(parent, this, strict)
+}
+
+/**
  * Like [UFile.accept], but in the case of multi-file classes (where
  * multiple source files containing top level declarations are annotated
  * with `@JvmMultifileClass`, all naming the same target class) the
diff --git a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/CheckResultDetector.kt b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/CheckResultDetector.kt
index ad72c0bcbd..4173191ee1 100644
--- a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/CheckResultDetector.kt
+++ b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/CheckResultDetector.kt
@@ -30,6 +30,7 @@ import com.android.tools.lint.detector.api.SourceCodeScanner
 import com.android.tools.lint.detector.api.UImplicitCallExpression
 import com.android.tools.lint.detector.api.UastLintUtils.Companion.getAnnotationStringValue
 import com.android.tools.lint.detector.api.findSelector
+import com.android.tools.lint.detector.api.isBelow
 import com.android.tools.lint.detector.api.isJava
 import com.android.tools.lint.detector.api.isKotlin
 import com.android.tools.lint.detector.api.nextStatement
@@ -313,11 +314,15 @@ class CheckResultDetector : AbstractAnnotationDetector(), SourceCodeScanner {
                 curr = curr.uastParent ?: return true
             }
 
-            @Suppress("RedundantIf")
             if (curr is UBlockExpression) {
-                if (curr.sourcePsi is PsiSynchronizedStatement) {
-                    return false
+                val sourcePsi = curr.sourcePsi
+                if (sourcePsi is PsiSynchronizedStatement) {
+                    val lock = sourcePsi.lockExpression
+                    if (lock != null && element.sourcePsi.isBelow(lock)) {
+                        return false
+                    }
                 }
+
                 // In Java, it's apparent when an expression is unused:
                 // the parent is a block expression. However, in Kotlin it's
                 // much trickier: values can flow through blocks and up through
@@ -342,7 +347,7 @@ class CheckResultDetector : AbstractAnnotationDetector(), SourceCodeScanner {
 
                 // It's the last child: see if the parent is unused
                 val parent = skipParenthesizedExprUp(curr.uastParent)
-                if (parent is ULambdaExpression && isKotlin(curr.sourcePsi)) {
+                if (parent is ULambdaExpression && isKotlin(sourcePsi)) {
                     val expressionType = parent.getExpressionType()?.canonicalText
                     if (expressionType != null &&
                         expressionType.startsWith("kotlin.jvm.functions.Function") &&
@@ -357,7 +362,7 @@ class CheckResultDetector : AbstractAnnotationDetector(), SourceCodeScanner {
                     return false
                 }
 
-                if (isJava(curr.sourcePsi)) {
+                if (isJava(sourcePsi)) {
                     // In Java there's no implicit passing to the parent
                     return true
                 }
diff --git a/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/CheckResultDetectorTest.kt b/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/CheckResultDetectorTest.kt
index 8dc665c58f..314c3be162 100644
--- a/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/CheckResultDetectorTest.kt
+++ b/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/CheckResultDetectorTest.kt
@@ -1656,7 +1656,7 @@ class CheckResultDetectorTest : AbstractCheckTest() {
                     public Object getLock() { return Api.class; }
                     public void test() {
                         synchronized (getLock()) {
-                            println("Test");
+                            System.out.println("Test");
                         }
                     }
                 }
@@ -1675,6 +1675,47 @@ class CheckResultDetectorTest : AbstractCheckTest() {
         ).run().expectClean()
     }
 
+    fun testSynchronizedUnused() {
+        // 242305422: CheckResultDetector allows unused results in Java synchronized expressions.
+        lint().files(
+            java(
+                """
+                import androidx.annotation.CheckResult;
+                public class Api {
+                    @CheckResult
+                    public Object getLock() { return Api.class; }
+                    public void test() {
+                        synchronized (getLock()) {
+                            getLock(); // ERROR
+                            System.out.println("test");
+                        }
+                    }
+                }
+                """
+            ).indented(),
+            kotlin(
+                """
+                class Api2 : Api() {
+                    fun test2() {
+                        synchronized(getLock()) { getLock(); println("test") } // ERROR
+                    }
+                }
+                """
+            ).indented(),
+            SUPPORT_ANNOTATIONS_JAR
+        ).run().expect(
+            """
+            src/Api.java:7: Warning: The result of getLock is not used [CheckResult]
+                        getLock(); // ERROR
+                        ~~~~~~~~~
+            src/Api2.kt:3: Warning: The result of getLock is not used [CheckResult]
+                    synchronized(getLock()) { getLock(); println("test") } // ERROR
+                                              ~~~~~~~~~
+            0 errors, 2 warnings
+            """
+        )
+    }
+
     fun testKotlinTest() {
         lint().files(
             kotlin(