Merge "Add new lint check to ensure that Fragments are instantiatable"

15 files changed, 273 insertions, 3 deletions

diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
index 12dffc5..3b7d63f 100644
--- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
+++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
@@ -327,4 +327,6 @@ public class LintConstants {
 
     // Class Names
     public static final String CONSTRUCTOR_NAME = "<init>";                          //$NON-NLS-1$
+    public static final String FRAGMENT = "android/app/Fragment";                    //$NON-NLS-1$
+    public static final String FRAGMENT_V4 = "android/support/v4/app/Fragment";      //$NON-NLS-1$
 }
diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintUtils.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintUtils.java
index 154ba67..ef987f2 100644
--- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintUtils.java
+++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintUtils.java
@@ -16,7 +16,6 @@
 
 package com.android.tools.lint.detector.api;
 
-import static com.android.tools.lint.detector.api.LintConstants.CONSTRUCTOR_NAME;
 import static com.android.tools.lint.detector.api.LintConstants.DOT_XML;
 import static com.android.tools.lint.detector.api.LintConstants.ID_RESOURCE_PREFIX;
 import static com.android.tools.lint.detector.api.LintConstants.NEW_ID_RESOURCE_PREFIX;
@@ -33,7 +32,6 @@ import com.google.common.io.Files;
 import org.objectweb.asm.Opcodes;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.FieldNode;
-import org.objectweb.asm.tree.MethodNode;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
index 720c0da..0645d56 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
@@ -53,7 +53,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
     private static final List<Issue> sIssues;
 
     static {
-        final int initialCapacity = 91;
+        final int initialCapacity = 92;
         List<Issue> issues = new ArrayList<Issue>(initialCapacity);
 
         issues.add(AccessibilityDetector.ISSUE);
@@ -86,6 +86,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
         issues.add(OnClickDetector.ISSUE);
         issues.add(RegistrationDetector.ISSUE);
         issues.add(HandlerDetector.ISSUE);
+        issues.add(FragmentDetector.ISSUE);
         issues.add(TranslationDetector.EXTRA);
         issues.add(TranslationDetector.MISSING);
         issues.add(HardcodedValuesDetector.ISSUE);
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/FragmentDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/FragmentDetector.java
new file mode 100644
index 0000000..37b22ea
--- /dev/null
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/FragmentDetector.java
@@ -0,0 +1,168 @@
+/*
+ * Copyright (C) 2012 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.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.tools.lint.checks;
+
+import static com.android.tools.lint.detector.api.LintConstants.CONSTRUCTOR_NAME;
+import static com.android.tools.lint.detector.api.LintConstants.FRAGMENT;
+import static com.android.tools.lint.detector.api.LintConstants.FRAGMENT_V4;
+
+import com.android.tools.lint.client.api.LintDriver;
+import com.android.tools.lint.detector.api.Category;
+import com.android.tools.lint.detector.api.ClassContext;
+import com.android.tools.lint.detector.api.Context;
+import com.android.tools.lint.detector.api.Detector;
+import com.android.tools.lint.detector.api.Detector.ClassScanner;
+import com.android.tools.lint.detector.api.Issue;
+import com.android.tools.lint.detector.api.LintUtils;
+import com.android.tools.lint.detector.api.Scope;
+import com.android.tools.lint.detector.api.Severity;
+import com.android.tools.lint.detector.api.Speed;
+
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.MethodNode;
+
+import java.io.File;
+import java.util.List;
+
+/**
+ * Checks that Fragment subclasses can be instantiated via
+ * {link {@link Class#newInstance()}}: the class is public, static, and has
+ * a public null constructor.
+ * <p>
+ * This helps track down issues like
+ *   http://stackoverflow.com/questions/8058809/fragment-activity-crashes-on-screen-rotate
+ * (and countless duplicates)
+ */
+public class FragmentDetector extends Detector implements ClassScanner {
+    private static final String FRAGMENT_NAME_SUFFIX = "Fragment";              //$NON-NLS-1$
+
+    /** Are fragment subclasses instantiatable? */
+    public static final Issue ISSUE = Issue.create(
+        "ValidFragment", //$NON-NLS-1$
+        "Ensures that Fragment subclasses can be instantiated",
+
+        "From the Fragment documentation:\n" +
+        "*Every* fragment must have an empty constructor, so it can be instantiated when " +
+        "restoring its activity's state. It is strongly recommended that subclasses do not " +
+        "have other constructors with parameters, since these constructors will not be " +
+        "called when the fragment is re-instantiated; instead, arguments can be supplied " +
+        "by the caller with setArguments(Bundle) and later retrieved by the Fragment " +
+        "with getArguments().",
+
+        Category.CORRECTNESS,
+        6,
+        Severity.WARNING,
+        FragmentDetector.class,
+        Scope.CLASS_FILE_SCOPE).setMoreInfo(
+        "http://developer.android.com/reference/android/app/Fragment.html#Fragment()"); //$NON-NLS-1$
+
+
+    /** Constructs a new {@link FragmentDetector} */
+    public FragmentDetector() {
+    }
+
+    @Override
+    public Speed getSpeed() {
+        return Speed.FAST;
+    }
+
+    @Override
+    public boolean appliesTo(Context context, File file) {
+        return true;
+    }
+
+    // ---- Implements ClassScanner ----
+
+    @Override
+    public void checkClass(ClassContext context, ClassNode classNode) {
+        if ((classNode.access & Opcodes.ACC_ABSTRACT) != 0) {
+            // Ignore abstract classes since they are clearly (and by definition) not intended to
+            // be instantiated. We're looking for accidental non-static or missing constructor
+            // scenarios here.
+            return;
+        }
+
+        LintDriver driver = context.getDriver();
+
+        if (!(driver.isSubclassOf(classNode, FRAGMENT)
+                || driver.isSubclassOf(classNode, FRAGMENT_V4))) {
+            if (!context.getScope().contains(Scope.ALL_JAVA_FILES)) {
+                // Single file checking: Just check that it looks like a fragment class
+                // (since we don't have a full superclass map)
+                if (!classNode.name.endsWith(FRAGMENT_NAME_SUFFIX) ||
+                        classNode.superName == null) {
+                    return;
+                }
+            } else {
+                return;
+            }
+        }
+
+        if ((classNode.access & Opcodes.ACC_PUBLIC) == 0) {
+            context.report(ISSUE, context.getLocation(classNode), String.format(
+                    "This fragment class should be public (%1$s)",
+                        ClassContext.createSignature(classNode.name, null, null)),
+                    null);
+            return;
+        }
+
+        if (classNode.name.indexOf('$') != -1 && !LintUtils.isStaticInnerClass(classNode)) {
+            context.report(ISSUE, context.getLocation(classNode), String.format(
+                    "This fragment inner class should be static (%1$s)",
+                        ClassContext.createSignature(classNode.name, null, null)),
+                    null);
+            return;
+        }
+
+        boolean hasDefaultConstructor = false;
+        @SuppressWarnings("rawtypes") // ASM API
+        List methodList = classNode.methods;
+        for (Object m : methodList) {
+            MethodNode method = (MethodNode) m;
+            if (method.name.equals(CONSTRUCTOR_NAME)) {
+                if (method.desc.equals("()V")) { //$NON-NLS-1$
+                    // The constructor must be public
+                    if ((method.access & Opcodes.ACC_PUBLIC) != 0) {
+                        hasDefaultConstructor = true;
+                    } else {
+                        context.report(ISSUE, context.getLocation(method, classNode),
+                                "The default constructor must be public",
+                                null);
+                        // Also mark that we have a constructor so we don't complain again
+                        // below since we've already emitted a more specific error related
+                        // to the default constructor
+                        hasDefaultConstructor = true;
+                    }
+                } else if (!method.desc.contains("()")) { //$NON-NLS-1$
+                    context.report(ISSUE, context.getLocation(method, classNode),
+                        "Avoid non-default constructors in fragments: use a default constructor " +
+                        "plus Fragment#setArguments(Bundle) instead",
+                        null);
+                }
+            }
+        }
+
+        if (!hasDefaultConstructor) {
+            context.report(ISSUE, context.getLocation(classNode), String.format(
+                    "This fragment should provide a default constructor (a public " +
+                    "constructor with no arguments) (%1$s)",
+                        ClassContext.createSignature(classNode.name, null, null)),
+                    null);
+        }
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/FragmentDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/FragmentDetectorTest.java
new file mode 100644
index 0000000..e25bbbc
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/FragmentDetectorTest.java
@@ -0,0 +1,46 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Eclipse Public License, Version 1.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.eclipse.org/org/documents/epl-v10.php
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.android.tools.lint.checks;
+
+import com.android.tools.lint.detector.api.Detector;
+
+@SuppressWarnings("javadoc")
+public class FragmentDetectorTest extends AbstractCheckTest {
+    @Override
+    protected Detector getDetector() {
+        return new FragmentDetector();
+    }
+
+    public void test() throws Exception {
+        assertEquals(
+            "FragmentTest.java:10: Warning: This fragment class should be public (test.pkg.FragmentTest.Fragment1)\n" +
+            "FragmentTest.java:15: Warning: This fragment inner class should be static (test.pkg.FragmentTest.Fragment2)\n" +
+            "FragmentTest.java:21: Warning: The default constructor must be public\n" +
+            "FragmentTest.java:27: Warning: Avoid non-default constructors in fragments: use a default constructor plus Fragment#setArguments(Bundle) instead\n" +
+            "FragmentTest.java:27: Warning: This fragment should provide a default constructor (a public constructor with no arguments) (test.pkg.FragmentTest.Fragment4)\n" +
+            "FragmentTest.java:36: Warning: Avoid non-default constructors in fragments: use a default constructor plus Fragment#setArguments(Bundle) instead",
+
+            lintProject(
+                "bytecode/FragmentTest$Fragment1.class.data=>bin/classes/test/pkg/FragmentTest$Fragment1.class",
+                "bytecode/FragmentTest$Fragment2.class.data=>bin/classes/test/pkg/FragmentTest$Fragment2.class",
+                "bytecode/FragmentTest$Fragment3.class.data=>bin/classes/test/pkg/FragmentTest$Fragment3.class",
+                "bytecode/FragmentTest$Fragment4.class.data=>bin/classes/test/pkg/FragmentTest$Fragment4.class",
+                "bytecode/FragmentTest$Fragment5.class.data=>bin/classes/test/pkg/FragmentTest$Fragment5.class",
+                "bytecode/FragmentTest$Fragment6.class.data=>bin/classes/test/pkg/FragmentTest$Fragment6.class",
+                "bytecode/FragmentTest$NotAFragment.class.data=>bin/classes/test/pkg/FragmentTest$NotAFragment.class",
+                "bytecode/FragmentTest.java.txt=>src/test/pkg/FragmentTest.java"));
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment1.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment1.class.data
new file mode 100644
index 0000000..11a98dd
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment1.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment2.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment2.class.data
new file mode 100644
index 0000000..d77579a
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment2.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment3.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment3.class.data
new file mode 100644
index 0000000..b1ec17d
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment3.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment4.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment4.class.data
new file mode 100644
index 0000000..f89f8ed
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment4.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment5.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment5.class.data
new file mode 100644
index 0000000..4a23e03
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment5.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment6.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment6.class.data
new file mode 100644
index 0000000..2e10e12
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment6.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$NotAFragment.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$NotAFragment.class.data
new file mode 100644
index 0000000..8f5b827
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$NotAFragment.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$ValidFragment1.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$ValidFragment1.class.data
new file mode 100644
index 0000000..a3354a7
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$ValidFragment1.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.class.data
new file mode 100644
index 0000000..6cc8387
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.java.txt
new file mode 100644
index 0000000..d27c04a
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.java.txt
@@ -0,0 +1,55 @@
+package test.pkg;
+
+import android.annotation.SuppressLint;
+import android.app.Fragment;
+
+@SuppressWarnings("unused")
+public class FragmentTest {
+
+	// Should be public
+	private static class Fragment1 extends Fragment {
+
+	}
+
+	// Should be static
+	public class Fragment2 extends Fragment {
+
+	}
+
+	// Should have a public constructor
+	public static class Fragment3 extends Fragment {
+		private Fragment3() {
+		}
+	}
+
+	// Should have a public constructor with no arguments
+	public static class Fragment4 extends Fragment {
+		private Fragment4(int dummy) {
+		}
+	}
+
+	// Should *only* have the default constructor, not the
+	// multi-argument one
+	public static class Fragment5 extends Fragment {
+		public Fragment5() {
+		}
+		public Fragment5(int dummy) {
+		}
+	}
+
+	// Suppressed
+	@SuppressLint("ValidFragment")
+	public static class Fragment6 extends Fragment {
+		private Fragment6() {
+		}
+	}
+
+	public static class ValidFragment1 extends Fragment {
+		public ValidFragment1() {
+		}
+	}
+
+	// (Not a fragment)
+	private class NotAFragment {
+	}
+}