Fix commit transation detector

First, fix the error message; it was sharing code with the
recycle detector and accidentally mentioned recycle rather
than commit in the error message.

Second, fix a bug where it would fail to find committed
transactions if the commit() was reached through setter
chaining (where each setter returns this).

Also, remove checking for recycling of Message objects;
according to the following thread it looks unnecessary:
https://groups.google.com/forum/?fromgroups=#!\
topic/android-developers/9pHuc7lGunY

Finally, the unit test class had the wrong name (was not
moved when the recycle detector was generalized to handle
other types of cleanup.)

Change-Id: I80f4bf65acf304660c8a7e7a5b9e1b4f932e25c0

4 files changed, 81 insertions, 49 deletions

diff --git a/lint/cli/src/test/java/com/android/tools/lint/checks/RecycleDetectorTest.java b/lint/cli/src/test/java/com/android/tools/lint/checks/CleanupDetectorTest.java
index df6f451..01d462a 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/checks/RecycleDetectorTest.java
+++ b/lint/cli/src/test/java/com/android/tools/lint/checks/CleanupDetectorTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 The Android Open Source Project
+ * Copyright (C) 2013 The Android Open Source Project
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,7 +19,7 @@ package com.android.tools.lint.checks;
 import com.android.tools.lint.detector.api.Detector;
 
 @SuppressWarnings("javadoc")
-public class RecycleDetectorTest extends AbstractCheckTest {
+public class CleanupDetectorTest extends AbstractCheckTest {
     @Override
     protected Detector getDetector() {
         return new CleanupDetector();
@@ -36,12 +36,6 @@ public class RecycleDetectorTest extends AbstractCheckTest {
             "src/test/pkg/RecycleTest.java:79: Warning: This VelocityTracker should be recycled after use with #recycle() [Recycle]\n" +
             "  VelocityTracker tracker = VelocityTracker.obtain();\n" +
             "                                            ~~~~~~\n" +
-            "src/test/pkg/RecycleTest.java:85: Warning: This Message should be recycled after use with #recycle() [Recycle]\n" +
-            "  Message message1 = getHandler().obtainMessage();\n" +
-            "                                  ~~~~~~~~~~~~~\n" +
-            "src/test/pkg/RecycleTest.java:86: Warning: This Message should be recycled after use with #recycle() [Recycle]\n" +
-            "  Message message2 = Message.obtain();\n" +
-            "                             ~~~~~~\n" +
             "src/test/pkg/RecycleTest.java:92: Warning: This MotionEvent should be recycled after use with #recycle() [Recycle]\n" +
             "  MotionEvent event1 = MotionEvent.obtain(null);\n" +
             "                                   ~~~~~~\n" +
@@ -63,7 +57,7 @@ public class RecycleDetectorTest extends AbstractCheckTest {
             "src/test/pkg/RecycleTest.java:129: Warning: This Parcel should be recycled after use with #recycle() [Recycle]\n" +
             "  Parcel myparcel = Parcel.obtain();\n" +
             "                           ~~~~~~\n" +
-            "0 errors, 12 warnings\n",
+            "0 errors, 10 warnings\n",
 
             lintProject(
                 "apicheck/classpath=>.classpath",
@@ -75,20 +69,20 @@ public class RecycleDetectorTest extends AbstractCheckTest {
     }
 
     public void testCommit() throws Exception {
-        assertEquals(""
-                + "src/test/pkg/CommitTest.java:24: Warning: This FragmentTransaction should be recycled after use with #recycle() [CommitTransaction]\n"
-                + "        getFragmentManager().beginTransaction(); // Missing commit\n"
-                + "                             ~~~~~~~~~~~~~~~~\n"
-                + "src/test/pkg/CommitTest.java:29: Warning: This FragmentTransaction should be recycled after use with #recycle() [CommitTransaction]\n"
-                + "        FragmentTransaction transaction2 = getFragmentManager().beginTransaction(); // Missing commit\n"
-                + "                                                                ~~~~~~~~~~~~~~~~\n"
-                + "src/test/pkg/CommitTest.java:38: Warning: This FragmentTransaction should be recycled after use with #recycle() [CommitTransaction]\n"
-                + "        getFragmentManager().beginTransaction(); // Missing commit\n"
-                + "                             ~~~~~~~~~~~~~~~~\n"
-                + "src/test/pkg/CommitTest.java:64: Warning: This FragmentTransaction should be recycled after use with #recycle() [Recycle]\n"
-                + "        getSupportFragmentManager().beginTransaction();\n"
-                + "                                    ~~~~~~~~~~~~~~~~\n"
-                + "0 errors, 4 warnings\n",
+        assertEquals("" +
+            "src/test/pkg/CommitTest.java:25: Warning: This transaction should be completed with a commit() call [CommitTransaction]\n" +
+            "        getFragmentManager().beginTransaction(); // Missing commit\n" +
+            "                             ~~~~~~~~~~~~~~~~\n" +
+            "src/test/pkg/CommitTest.java:30: Warning: This transaction should be completed with a commit() call [CommitTransaction]\n" +
+            "        FragmentTransaction transaction2 = getFragmentManager().beginTransaction(); // Missing commit\n" +
+            "                                                                ~~~~~~~~~~~~~~~~\n" +
+            "src/test/pkg/CommitTest.java:39: Warning: This transaction should be completed with a commit() call [CommitTransaction]\n" +
+            "        getFragmentManager().beginTransaction(); // Missing commit\n" +
+            "                             ~~~~~~~~~~~~~~~~\n" +
+            "src/test/pkg/CommitTest.java:65: Warning: This transaction should be completed with a commit() call [Recycle]\n" +
+            "        getSupportFragmentManager().beginTransaction();\n" +
+            "                                    ~~~~~~~~~~~~~~~~\n" +
+            "0 errors, 4 warnings\n",
 
             lintProject(
                     "apicheck/classpath=>.classpath",
@@ -110,4 +104,17 @@ public class RecycleDetectorTest extends AbstractCheckTest {
                         "bytecode/DialogFragment.class.data=>bin/classes/test/pkg/DialogFragment.class"
                 ));
     }
+
+    public void testHasReturnType() throws Exception {
+        assertTrue(CleanupDetector.hasReturnType("android/app/FragmentTransaction",
+                "(Landroid/app/Fragment;)Landroid/app/FragmentTransaction;"));
+        assertTrue(CleanupDetector.hasReturnType("android/app/FragmentTransaction",
+                "()Landroid/app/FragmentTransaction;"));
+        assertFalse(CleanupDetector.hasReturnType("android/app/FragmentTransaction",
+                "()Landroid/app/FragmentTransactions;"));
+        assertFalse(CleanupDetector.hasReturnType("android/app/FragmentTransaction",
+                "()Landroid/app/FragmentTransactions"));
+        assertFalse(CleanupDetector.hasReturnType("android/app/FragmentTransaction",
+                "()android/app/FragmentTransaction;"));
+    }
 }
diff --git a/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.class.data b/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.class.data
index 76f35c0..842fb21 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.class.data
+++ b/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.class.data
diff --git a/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.java.txt b/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.java.txt
index 49395d7..24edaf3 100644
--- a/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.java.txt
+++ b/lint/cli/src/test/java/com/android/tools/lint/checks/data/bytecode/CommitTest.java.txt
@@ -1,6 +1,7 @@
 package test.pkg;
 
 import android.app.Activity;
+import android.app.Fragment;
 import android.app.FragmentManager;
 import android.app.FragmentTransaction;
 
@@ -63,4 +64,28 @@ public class CommitTest extends Activity {
     public void error4() {
         getSupportFragmentManager().beginTransaction();
     }
+
+    android.support.v4.app.Fragment mFragment1 = null;
+    Fragment mFragment2 = null;
+
+    public void ok7() {
+        getSupportFragmentManager().beginTransaction().add(android.R.id.content, mFragment1).commit();
+    }
+
+    public void ok8() {
+        getFragmentManager().beginTransaction().add(android.R.id.content, mFragment2).commit();
+    }
+
+    public void ok10() {
+        // Test chaining
+        FragmentManager fragmentManager = getFragmentManager();
+        fragmentManager.beginTransaction().addToBackStack("test").attach(mFragment2).detach(mFragment2)
+        .disallowAddToBackStack().hide(mFragment2).setBreadCrumbShortTitle("test")
+        .show(mFragment2).setCustomAnimations(0, 0).commit();
+    }
+
+    public void ok9() {
+        FragmentManager fragmentManager = getFragmentManager();
+        fragmentManager.beginTransaction().commit();
+    }
 }
diff --git a/lint/libs/lint_checks/src/main/java/com/android/tools/lint/checks/CleanupDetector.java b/lint/libs/lint_checks/src/main/java/com/android/tools/lint/checks/CleanupDetector.java
index 11365e2..1541d23 100644
--- a/lint/libs/lint_checks/src/main/java/com/android/tools/lint/checks/CleanupDetector.java
+++ b/lint/libs/lint_checks/src/main/java/com/android/tools/lint/checks/CleanupDetector.java
@@ -18,6 +18,7 @@ package com.android.tools.lint.checks;
 
 import com.android.annotations.NonNull;
 import com.android.annotations.Nullable;
+import com.android.annotations.VisibleForTesting;
 import com.android.tools.lint.detector.api.Category;
 import com.android.tools.lint.detector.api.ClassContext;
 import com.android.tools.lint.detector.api.Context;
@@ -81,7 +82,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
     private static final String RECYCLE = "recycle";                                  //$NON-NLS-1$
     private static final String OBTAIN = "obtain";                                    //$NON-NLS-1$
     private static final String OBTAIN_NO_HISTORY = "obtainNoHistory";                //$NON-NLS-1$
-    private static final String OBTAIN_MESSAGE = "obtainMessage";                     //$NON-NLS-1$
     private static final String OBTAIN_ATTRIBUTES = "obtainAttributes";               //$NON-NLS-1$
     private static final String OBTAIN_TYPED_ARRAY = "obtainTypedArray";              //$NON-NLS-1$
     private static final String OBTAIN_STYLED_ATTRIBUTES = "obtainStyledAttributes";  //$NON-NLS-1$
@@ -94,7 +94,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
     private static final String TYPED_ARRAY_CLS = "android/content/res/TypedArray";   //$NON-NLS-1$
     private static final String CONTEXT_CLS = "android/content/Context";              //$NON-NLS-1$
     private static final String MOTION_EVENT_CLS = "android/view/MotionEvent";        //$NON-NLS-1$
-    private static final String MESSAGE_CLS = "android/os/Message";                   //$NON-NLS-1$
     private static final String HANDLER_CLS = "android/os/Handler";                   //$NON-NLS-1$
     private static final String RESOURCES_CLS = "android/content/res/Resources";      //$NON-NLS-1$
     private static final String PARCEL_CLS = "android/os/Parcel";                     //$NON-NLS-1$
@@ -105,14 +104,11 @@ public class CleanupDetector extends Detector implements ClassScanner {
 
     // Target description signatures
     private static final String TYPED_ARRAY_SIG = "Landroid/content/res/TypedArray;"; //$NON-NLS-1$
-    private static final String MESSAGE_SIG = "Landroid/os/Message;";                 //$NON-NLS-1$
 
     private boolean mObtainsTypedArray;
     private boolean mRecyclesTypedArray;
     private boolean mObtainsTracker;
     private boolean mRecyclesTracker;
-    private boolean mObtainsMessage;
-    private boolean mRecyclesMessage;
     private boolean mObtainsMotionEvent;
     private boolean mRecyclesMotionEvent;
     private boolean mObtainsParcel;
@@ -130,7 +126,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
         if (phase == 1) {
             if (mObtainsTypedArray && !mRecyclesTypedArray
                     || mObtainsTracker && !mRecyclesTracker
-                    || mObtainsMessage && !mRecyclesMessage
                     || mObtainsParcel && !mRecyclesParcel
                     || mObtainsMotionEvent && !mRecyclesMotionEvent
                     || mObtainsTransaction && !mCommitsTransaction) {
@@ -150,7 +145,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
                 OBTAIN,
                 OBTAIN_ATTRIBUTES,
                 OBTAIN_TYPED_ARRAY,
-                OBTAIN_MESSAGE,
                 OBTAIN_NO_HISTORY,
                 BEGIN_TRANSACTION,
                 COMMIT,
@@ -173,8 +167,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
                 mRecyclesTypedArray = true;
             } else if (owner.equals(VELOCITY_TRACKER_CLS)) {
                 mRecyclesTracker = true;
-            } else if (owner.equals(MESSAGE_CLS)) {
-                mRecyclesMessage = true;
             } else if (owner.equals(MOTION_EVENT_CLS)) {
                 mRecyclesMotionEvent = true;
             } else if (owner.equals(PARCEL_CLS)) {
@@ -199,14 +191,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
                     mRecyclesMotionEvent = true;
                 }
             }
-        } else if (OBTAIN_MESSAGE.equals(name)) {
-            if (owner.equals(HANDLER_CLS) && desc.endsWith(MESSAGE_SIG)) {
-                mObtainsMessage = true;
-                if (phase == 2 && !mRecyclesMessage) {
-                    context.report(RECYCLE_RESOURCE, method, call, context.getLocation(call),
-                            getErrorMessage(MESSAGE_CLS), null);
-                }
-            }
         } else if (OBTAIN.equals(name)) {
             if (owner.equals(VELOCITY_TRACKER_CLS)) {
                 mObtainsTracker = true;
@@ -215,14 +199,6 @@ public class CleanupDetector extends Detector implements ClassScanner {
                             getErrorMessage(VELOCITY_TRACKER_CLS),
                             null);
                 }
-            } else if (owner.equals(MESSAGE_CLS) && desc.endsWith(MESSAGE_SIG)) {
-                // TODO: Handle Message constructor?
-                mObtainsMessage = true;
-                if (phase == 2 && !mRecyclesMessage) {
-                    context.report(RECYCLE_RESOURCE, method, call, context.getLocation(call),
-                            getErrorMessage(MESSAGE_CLS),
-                            null);
-                }
             } else if (owner.equals(PARCEL_CLS)) {
                 mObtainsParcel = true;
                 if (phase == 2 && !mRecyclesParcel) {
@@ -269,7 +245,7 @@ public class CleanupDetector extends Detector implements ClassScanner {
 
     /** Computes an error message for a missing recycle of the given type */
     private static String getErrorMessage(String owner) {
-        if (FRAGMENT_MANAGER_CLS.equals(owner)) {
+        if (FRAGMENT_TRANSACTION_CLS.equals(owner) || FRAGMENT_TRANSACTION_V4_CLS.equals(owner)) {
             return "This transaction should be completed with a commit() call";
         }
         String className = owner.substring(owner.lastIndexOf('/') + 1);
@@ -308,6 +284,23 @@ public class CleanupDetector extends Detector implements ClassScanner {
         return false;
     }
 
+    @VisibleForTesting
+    static boolean hasReturnType(String owner, String desc) {
+        int descLen = desc.length();
+        int ownerLen = owner.length();
+        if (descLen < ownerLen + 3) {
+            return false;
+        }
+        if (desc.charAt(descLen - 1) != ';') {
+            return false;
+        }
+        int typeBegin = descLen - 2 - ownerLen;
+        if (desc.charAt(typeBegin - 1) != ')' || desc.charAt(typeBegin) != 'L') {
+            return false;
+        }
+        return desc.regionMatches(typeBegin + 1, owner, 0, ownerLen);
+    }
+
     /**
      * ASM interpreter which tracks the instances of the allocated resource, and
      * checks whether it is eventually passed to a {@code recycle()} call. If the
@@ -434,6 +427,13 @@ public class CleanupDetector extends Detector implements ClassScanner {
                             mIsCleanedUp = true;
                             return INSTANCE;
                         }
+                    } else if (call.owner.equals(mRecycleOwner)
+                            && hasReturnType(mRecycleOwner, call.desc)) {
+                        // Called method which returns self. This helps handle cases where you call
+                        //   createTransaction().method1().method2().method3().commit() -- if
+                        // method1, 2 and 3 all return "this" then the commit call is really
+                        // called on the createTransaction instance
+                        return INSTANCE;
                     }
                 }
             }