From 10b84a1ae8c6fd87c1b4f8d496f777907699135a Mon Sep 17 00:00:00 2001 From: Tor Norbye Date: Tue, 29 May 2012 12:04:13 -0700 Subject: Add new lint check to ensure that Fragments are instantiatable Fragments should provide a default constructor, and only a default constructor, such that they can be recreated by the system on configuration changes. This changeset adds a lint check to catch cases where this is not the case -- such as fragments that are non-static innerclasses, or where the the class or constructor is not public. It also warns if the fragment contains any *other* constructors, since the fragment documentation strongly advises against it. Change-Id: I8cdd00fd7c74259f84977804e36ace7c43864026 --- .../tools/lint/detector/api/LintConstants.java | 2 + .../android/tools/lint/detector/api/LintUtils.java | 2 - .../tools/lint/checks/BuiltinIssueRegistry.java | 3 +- .../tools/lint/checks/FragmentDetector.java | 168 +++++++++++++++++++++ .../tools/lint/checks/FragmentDetectorTest.java | 46 ++++++ .../bytecode/FragmentTest$Fragment1.class.data | Bin 0 -> 373 bytes .../bytecode/FragmentTest$Fragment2.class.data | Bin 0 -> 460 bytes .../bytecode/FragmentTest$Fragment3.class.data | Bin 0 -> 377 bytes .../bytecode/FragmentTest$Fragment4.class.data | Bin 0 -> 406 bytes .../bytecode/FragmentTest$Fragment5.class.data | Bin 0 -> 471 bytes .../bytecode/FragmentTest$Fragment6.class.data | Bin 0 -> 487 bytes .../bytecode/FragmentTest$NotAFragment.class.data | Bin 0 -> 465 bytes .../FragmentTest$ValidFragment1.class.data | Bin 0 -> 392 bytes .../checks/data/bytecode/FragmentTest.class.data | Bin 0 -> 774 bytes .../checks/data/bytecode/FragmentTest.java.txt | 55 +++++++ 15 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 lint/libs/lint_checks/src/com/android/tools/lint/checks/FragmentDetector.java create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/FragmentDetectorTest.java create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment1.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment2.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment3.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment4.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment5.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment6.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$NotAFragment.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$ValidFragment1.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.java.txt 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 = ""; //$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 sIssues; static { - final int initialCapacity = 91; + final int initialCapacity = 92; List issues = new ArrayList(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. + *

+ * 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment1.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment2.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment3.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment4.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment5.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$Fragment6.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$NotAFragment.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest$ValidFragment1.class.data differ 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 Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/FragmentTest.class.data differ 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 { + } +} -- cgit v1.1