Fixed bugs in Kotlin Android Lint CleanupDetector

#KT-14780 Fixed
#KT-14677 Fixed

(cherry picked from commit f591b4958e4a836d37d70c2ad8ff15e626c6342a)

idea/idea-android/tests/org/jetbrains/kotlin/android/lint/KotlinLintTestGenerated.java

@@ -54,6 +54,12 @@ public class KotlinLintTestGenerated extends AbstractKotlinLintTest {
         doTest(fileName);
     }
 
+    @TestMetadata("closeCursor.kt")
+    public void testCloseCursor() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/android/lint/closeCursor.kt");
+        doTest(fileName);
+    }
+
     @TestMetadata("commitFragment.kt")
     public void testCommitFragment() throws Exception {
         String fileName = KotlinTestUtils.navigationMetadata("idea/testData/android/lint/commitFragment.kt");
@@ -150,6 +156,12 @@ public class KotlinLintTestGenerated extends AbstractKotlinLintTest {
         doTest(fileName);
     }
 
+    @TestMetadata("velocityTrackerRecycle.kt")
+    public void testVelocityTrackerRecycle() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/android/lint/velocityTrackerRecycle.kt");
+        doTest(fileName);
+    }
+
     @TestMetadata("viewConstructor.kt")
     public void testViewConstructor() throws Exception {
         String fileName = KotlinTestUtils.navigationMetadata("idea/testData/android/lint/viewConstructor.kt");

idea/testData/android/lint/closeCursor.kt

@@ -0,0 +1,36 @@
+// INSPECTION_CLASS: org.jetbrains.android.inspections.klint.AndroidLintInspectionToolProvider$AndroidKLintRecycleInspection
+
+@file:Suppress("UNUSED_VARIABLE")
+
+import android.app.Activity
+import android.os.Bundle
+
+class MainActivity : Activity() {
+
+    override fun onCreate(savedInstanceState: Bundle?) {
+        super.onCreate(savedInstanceState)
+
+        val cursor = contentResolver.<warning descr="This `Cursor` should be freed up after use with `#close()`">query</warning>(null, null, null, null, null)
+
+        // WARNING
+        contentResolver.<warning descr="This `Cursor` should be freed up after use with `#close()`">query</warning>(null, null, null, null, null)
+
+        // OK, closed in chained call
+        contentResolver.query(null, null, null, null, null).close()
+
+        // KT-14677: Kotlin Lint: "Missing recycle() calls" report cursor with `use()` call
+        val cursorUsed = contentResolver.query(null, null, null, null, null)
+        cursorUsed.use {  }
+
+        // OK, used in chained call
+        contentResolver.query(null, null, null, null, null).use {
+
+        }
+
+        // KT-13372: Android Lint for Kotlin: false positive "Cursor should be freed" inside 'if' expression
+        if (true) {
+            val c = contentResolver.query(null, null, null, null, null)
+            c.close()
+        }
+    }
+}

idea/testData/android/lint/commitFragment.kt

@@ -1,7 +1,10 @@
 // INSPECTION_CLASS: org.jetbrains.android.inspections.klint.AndroidLintInspectionToolProvider$AndroidKLintCommitTransactionInspection
 
+@file:Suppress("UNUSED_VARIABLE")
+
 import android.app.Activity
 import android.app.FragmentTransaction
+import android.app.FragmentManager
 import android.os.Bundle
 
 class MainActivity : Activity() {
@@ -17,8 +20,7 @@ class MainActivity : Activity() {
         transaction2.commit()
 
         //WARNING
-        @Suppress("UNUSED_VARIABLE")
+        val transaction3 = fragmentManager.<warning descr="This transaction should be completed with a `commit()` call">beginTransaction</warning>()
-        val transaction3 = fragmentManager.<warning>beginTransaction</warning>()
 
         //OK
         fragmentManager.beginTransaction().commit()
@@ -30,4 +32,10 @@ class MainActivity : Activity() {
             a.commit()
         }
     }
+
+    // KT-14780: Kotlin Lint: "Missing commit() calls" false positive when the result of `commit()` is assigned or used as receiver
+    fun testResultOfCommit(fm: FragmentManager) {
+        val r1 = fm.beginTransaction().hide(fm.findFragmentByTag("aTag")).commit()
+        val r2 = fm.beginTransaction().hide(fm.findFragmentByTag("aTag")).commit().toString()
+    }
 }

idea/testData/android/lint/sharedPrefs.kt

@@ -66,4 +66,9 @@ class SharedPrefsText(context: Context) : Activity() {
         val editor = preferences.<warning descr="`SharedPreferences.edit()` without a corresponding `commit()` or `apply()` call"><warning descr="`SharedPreferences.edit()` without a corresponding `commit()` or `apply()` call">edit()</warning></warning>
         editor.putString("foo", "bar")
     }
+
+    fun testResultOfCommit() {
+        val r1 = PreferenceManager.getDefaultSharedPreferences(this).edit().putString("wat", "wat").commit()
+        val r2 = PreferenceManager.getDefaultSharedPreferences(this).edit().putString("wat", "wat").commit().toString()
+    }
 }

idea/testData/android/lint/velocityTrackerRecycle.kt

@@ -0,0 +1,23 @@
+// INSPECTION_CLASS: org.jetbrains.android.inspections.klint.AndroidLintInspectionToolProvider$AndroidKLintRecycleInspection
+
+@file:Suppress("UNUSED_VARIABLE")
+
+import android.app.Activity
+import android.os.Bundle
+import android.view.VelocityTracker
+
+class MainActivity : Activity() {
+
+    override fun onCreate(savedInstanceState: Bundle?) {
+        super.onCreate(savedInstanceState)
+
+        VelocityTracker.<warning descr="This `VelocityTracker` should be recycled after use with `#recycle()`">obtain</warning>()
+
+        VelocityTracker.obtain().recycle()
+
+        val v1 = VelocityTracker.<warning descr="This `VelocityTracker` should be recycled after use with `#recycle()`">obtain</warning>()
+
+        val v2 = VelocityTracker.obtain()
+        v2.recycle()
+    }
+}

plugins/lint/lint-checks/src/com/android/tools/klint/checks/CleanupDetector.java

@@ -19,7 +19,6 @@ package com.android.tools.klint.checks;
 import static com.android.SdkConstants.CLASS_CONTENTPROVIDER;
 import static com.android.SdkConstants.CLASS_CONTEXT;
 import static com.android.tools.klint.detector.api.LintUtils.skipParentheses;
-import static com.intellij.psi.util.PsiTreeUtil.getParentOfType;
 import static org.jetbrains.uast.UastUtils.getOutermostQualified;
 import static org.jetbrains.uast.UastUtils.getParentOfType;
 import static org.jetbrains.uast.UastUtils.getQualifiedChain;
@@ -47,6 +46,7 @@ import com.intellij.psi.PsiType;
 import com.intellij.psi.PsiVariable;
 
 import com.intellij.psi.util.InheritanceUtil;
+import com.intellij.util.containers.Predicate;
 import org.jetbrains.uast.UBinaryExpression;
 import org.jetbrains.uast.UCallExpression;
 import org.jetbrains.uast.UDoWhileExpression;
@@ -141,6 +141,7 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
     private static final String QUERY_WITH_FACTORY = "queryWithFactory";              //$NON-NLS-1$
     private static final String RAW_QUERY_WITH_FACTORY = "rawQueryWithFactory";       //$NON-NLS-1$
     private static final String CLOSE = "close";                                      //$NON-NLS-1$
+    private static final String USE = "use";                                          //$NON-NLS-1$
     private static final String EDIT = "edit";                                        //$NON-NLS-1$
 
     private static final String MOTION_EVENT_CLS = "android.view.MotionEvent";        //$NON-NLS-1$
@@ -173,6 +174,8 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
             "android.content.SharedPreferences"; //$NON-NLS-1$
     private static final String ANDROID_CONTENT_SHARED_PREFERENCES_EDITOR =
             "android.content.SharedPreferences.Editor"; //$NON-NLS-1$
+    private static final String CLOSABLE = "java.io.Closeable"; //$NON-NLS-1$
+
 
     /** Constructs a new {@link CleanupDetector} */
     public CleanupDetector() {
@@ -289,53 +292,11 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
             //    android.provider.DocumentsProvider#querySearchDocuments
             //    android.provider.MediaStore$Images$Media#query
             //    android.widget.FilterQueryProvider#runQuery
-            checkRecycled(context, node, CURSOR_CLS, CLOSE);
+            checkClosedOrUsed(context, node, CURSOR_CLS);
         }
     }
 
-    private static void checkRecycled(@NonNull final JavaContext context, @NonNull UCallExpression node,
+    private static void reportRecycleResource(JavaContext context, String recycleType, String recycleName, @NonNull UCallExpression node) {
-            @NonNull final String recycleType, @NonNull final String recycleName) {
-        PsiVariable boundVariable = getVariableElement(node);
-        if (boundVariable == null) {
-            return;
-        }
-
-        UMethod method = getParentOfType(node, UMethod.class, true);
-        if (method == null) {
-            return;
-        }
-        FinishVisitor visitor = new FinishVisitor(context, boundVariable) {
-            @Override
-            protected boolean isCleanupCall(@NonNull UCallExpression call) {
-                String methodName = call.getMethodName();
-                if (!recycleName.equals(methodName)) {
-                    return false;
-                }
-                PsiMethod method = call.resolve();
-                if (method != null) {
-                    PsiClass containingClass = method.getContainingClass();
-                    if (InheritanceUtil.isInheritor(containingClass, false, recycleType)) {
-                        // Yes, called the right recycle() method; now make sure
-                        // we're calling it on the right variable
-                        UExpression operand = call.getReceiver();
-                        if (operand instanceof UReferenceExpression) {
-                            PsiElement resolved = ((UReferenceExpression) operand).resolve();
-                            //noinspection SuspiciousMethodCalls
-                            if (resolved != null && mVariables.contains(resolved)) {
-                                return true;
-                            }
-                        }
-                    }
-                }
-                return false;
-            }
-        };
-
-        method.accept(visitor);
-        if (visitor.isCleanedUp() || visitor.variableEscapes()) {
-            return;
-        }
-
         String className = recycleType.substring(recycleType.lastIndexOf('.') + 1);
         String message;
         if (RECYCLE.equals(recycleName)) {
@@ -355,14 +316,156 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
         context.report(RECYCLE_RESOURCE, node, location, message);
     }
 
-    private static void checkTransactionCommits(@NonNull JavaContext context,
+    private static void checkClosedOrUsed(@NonNull final JavaContext context, @NonNull UCallExpression node,
-            @NonNull UCallExpression node, @NonNull PsiMethod calledMethod) {
+            @NonNull final String recycleType) {
-        if (isBeginTransaction(context, calledMethod)) {
+
-            PsiVariable boundVariable = getVariableElement(node, true);
+        if (isCleanedUpInChain(node, new Predicate<UCallExpression>() {
-            if (boundVariable == null && isCommittedInChainedCalls(context, node)) {
+            @Override
+            public boolean apply(@org.jetbrains.annotations.Nullable UCallExpression call) {
+                return isCloseMethodCall(call) || isUseMethodCall(call);
+            }
+        })) {
             return;
         }
 
+        PsiVariable boundVariable = getVariableElement(node);
+        if (boundVariable == null) {
+            reportRecycleResource(context, recycleType, CLOSE, node);
+            return;
+        }
+
+        UMethod method = getParentOfType(node, UMethod.class, true);
+        if (method == null) {
+            return;
+        }
+
+        FinishVisitor visitor = new FinishVisitor(context, boundVariable) {
+            @Override
+            protected boolean isCleanupCall(@NonNull UCallExpression call) {
+                if (isUseMethodCall(call) || isCloseMethodCall(call)) {
+                    UExpression receiver = call.getReceiver();
+                    if (receiver instanceof UReferenceExpression) {
+                        PsiElement resolved = ((UReferenceExpression) receiver).resolve();
+                        //noinspection SuspiciousMethodCalls
+                        if (resolved != null && mVariables.contains(resolved)) {
+                            return true;
+                        }
+                    }
+                }
+                return false;
+            }
+        };
+
+        method.accept(visitor);
+        if (visitor.isCleanedUp() || visitor.variableEscapes()) {
+            return;
+        }
+
+        reportRecycleResource(context, recycleType, CLOSE, node);
+    }
+
+    private static boolean isCloseMethodCall(UCallExpression call) {
+        return isValidCleanupMethodCall(call, CLOSE, CLOSABLE);
+    }
+
+    private static boolean isUseMethodCall(UCallExpression call) {
+        return USE.equals(call.getMethodName());
+    }
+
+    private static boolean isValidCleanupMethodCall(@NonNull UCallExpression call, @NonNull String methodName, @NonNull String className) {
+        if (!methodName.equals(call.getMethodName())) {
+            return false;
+        }
+
+        PsiMethod method = call.resolve();
+        if (method == null) {
+            return false;
+        }
+
+        return InheritanceUtil.isInheritor(method.getContainingClass(), false, className);
+    }
+
+    private static boolean isCleanedUpInChain(UExpression expression, Predicate<UCallExpression> isCleanupCallPredicate) {
+        List<UExpression> chain = getQualifiedChain(getOutermostQualified(expression));
+        boolean skip = true;
+        for (UExpression e : chain) {
+            if (e == expression) {
+                skip = false;
+                continue;
+            }
+
+            if (skip) {
+                continue;
+            }
+
+            if (e instanceof UCallExpression) {
+                UCallExpression call = (UCallExpression) e;
+                if (isCleanupCallPredicate.apply(call)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    private static void checkRecycled(@NonNull final JavaContext context, @NonNull UCallExpression node,
+            @NonNull final String recycleType, @NonNull final String recycleName) {
+
+        if (isCleanedUpInChain(node, new Predicate<UCallExpression>() {
+            @Override
+            public boolean apply(@org.jetbrains.annotations.Nullable UCallExpression call) {
+                return isValidCleanupMethodCall(call, recycleName, recycleType);
+            }
+        })) {
+            return;
+        }
+
+        PsiVariable boundVariable = getVariableElement(node);
+        if (boundVariable == null) {
+            reportRecycleResource(context, recycleType, recycleName, node);
+            return;
+        }
+
+        UMethod method = getParentOfType(node, UMethod.class, true);
+        if (method == null) {
+            return;
+        }
+
+        FinishVisitor visitor = new FinishVisitor(context, boundVariable) {
+            @Override
+            protected boolean isCleanupCall(@NonNull UCallExpression call) {
+                if (isValidCleanupMethodCall(call, recycleName, recycleType)) {
+                    // Yes, called the right recycle() method; now make sure
+                    // we're calling it on the right variable
+                    UExpression operand = call.getReceiver();
+                    if (operand instanceof UReferenceExpression) {
+                        PsiElement resolved = ((UReferenceExpression) operand).resolve();
+                        //noinspection SuspiciousMethodCalls
+                        if (resolved != null && mVariables.contains(resolved)) {
+                            return true;
+                        }
+                    }
+                }
+                return false;
+            }
+        };
+
+        method.accept(visitor);
+        if (visitor.isCleanedUp() || visitor.variableEscapes()) {
+            return;
+        }
+
+        reportRecycleResource(context, recycleType, recycleName, node);
+    }
+
+    private static void checkTransactionCommits(@NonNull JavaContext context,
+            @NonNull UCallExpression node, @NonNull PsiMethod calledMethod) {
+        if (isBeginTransaction(context, calledMethod)) {
+            if (isCommittedInChainedCalls(context, node)) {
+                return;
+            }
+
+            PsiVariable boundVariable = getVariableElement(node, true);
             if (boundVariable != null) {
                 UMethod method = getParentOfType(node, UMethod.class, true);
                 if (method == null) {
@@ -428,26 +531,20 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
         }
     }
 
-    private static boolean isCommittedInChainedCalls(@NonNull JavaContext context,
+    private static boolean isCommittedInChainedCalls(@NonNull final JavaContext context,
             @NonNull UCallExpression node) {
         // Look for chained calls since the FragmentManager methods all return "this"
         // to allow constructor chaining, e.g.
         //    getFragmentManager().beginTransaction().addToBackStack("test")
         //            .disallowAddToBackStack().hide(mFragment2).setBreadCrumbShortTitle("test")
         //            .show(mFragment2).setCustomAnimations(0, 0).commit();
-        List<UExpression> chain = getQualifiedChain(getOutermostQualified(node));
-        if (!chain.isEmpty()) {
-            UExpression lastExpression = chain.get(chain.size() - 1);
-            if (lastExpression instanceof UCallExpression) {
-                UCallExpression methodInvocation = (UCallExpression) lastExpression;
-                if (isTransactionCommitMethodCall(context, methodInvocation)
-                        || isShowFragmentMethodCall(context, methodInvocation)) {
-                    return true;
-                }
-            }
-        }
 
-        return false;
+        return isCleanedUpInChain(node, new Predicate<UCallExpression>() {
+            @Override
+            public boolean apply(@org.jetbrains.annotations.Nullable UCallExpression call) {
+                return isTransactionCommitMethodCall(context, call) || isShowFragmentMethodCall(context, call);
+            }
+        });
     }
 
     private static boolean isTransactionCommitMethodCall(@NonNull JavaContext context,
@@ -569,21 +666,14 @@ public class CleanupDetector extends Detector implements Detector.UastScanner {
         return false;
     }
 
-    private static boolean isEditorCommittedInChainedCalls(@NonNull JavaContext context,
+    private static boolean isEditorCommittedInChainedCalls(@NonNull final JavaContext context,
             @NonNull UCallExpression node) {
-        List<UExpression> chain = getQualifiedChain(getOutermostQualified(node));
+        return isCleanedUpInChain(node, new Predicate<UCallExpression>() {
-        if (!chain.isEmpty()) {
+            @Override
-            UExpression lastExpression = chain.get(chain.size() - 1);
+            public boolean apply(@org.jetbrains.annotations.Nullable UCallExpression call) {
-            if (lastExpression instanceof UCallExpression) {
+                return isEditorCommitMethodCall(context, call) || isEditorApplyMethodCall(context, call);
-                UCallExpression methodInvocation = (UCallExpression) lastExpression;
-                if (isEditorCommitMethodCall(context, methodInvocation)
-                        || isEditorApplyMethodCall(context, methodInvocation)) {
-                    return true;
             }
-            }
+        });
-        }
-        
-        return false;
     }
 
     private static boolean isEditorCommitMethodCall(@NonNull JavaContext context,

plugins/uast-kotlin/src/org/jetbrains/uast/kotlin/KotlinUastLanguagePlugin.kt

@@ -22,6 +22,7 @@ import com.intellij.psi.PsiElement
 import com.intellij.psi.PsiFile
 import com.intellij.psi.PsiVariable
 import com.intellij.psi.impl.source.tree.LeafPsiElement
+import com.intellij.psi.util.PsiTreeUtil
 import org.jetbrains.kotlin.asJava.LightClassUtil
 import org.jetbrains.kotlin.asJava.classes.KtLightClass
 import org.jetbrains.kotlin.asJava.elements.FakeFileForLightClass
@@ -187,6 +188,9 @@ internal object KotlinConverter {
             is KtCatchClause -> el<UCatchClause> { KotlinUCatchClause(element, parent) }
             is KtExpression -> KotlinConverter.convertExpression(element, parent, requiredType)
             is KtLambdaArgument -> KotlinConverter.convertExpression(element.getLambdaExpression(), parent, requiredType)
+            is KtContainerNode -> element.getExpression()?.let {
+                KotlinConverter.convertExpression(it, parent, requiredType)
+            } ?: UastEmptyExpression
             else -> {
                 if (element is LeafPsiElement && element.elementType == KtTokens.IDENTIFIER) {
                     el<UIdentifier> { UIdentifier(element, parent) }
@@ -313,4 +317,7 @@ internal object KotlinConverter {
         val result = declarations.first() as TDeclaration
         return result
     }
+
+    internal fun KtContainerNode.getExpression(): KtExpression? =
+            PsiTreeUtil.getChildOfType(this, KtExpression::class.java)
 }