Warn about potentially accidentally overriding methods

Change-Id: Ic8d20a002f6225cb15f609b3b7582716904e9278

12 files changed, 401 insertions, 5 deletions

diff --git a/lint/libs/lint_api/src/com/android/tools/lint/client/api/LintDriver.java b/lint/libs/lint_api/src/com/android/tools/lint/client/api/LintDriver.java
index 415c305..7f6c0ee 100644
--- a/lint/libs/lint_api/src/com/android/tools/lint/client/api/LintDriver.java
+++ b/lint/libs/lint_api/src/com/android/tools/lint/client/api/LintDriver.java
@@ -523,6 +523,13 @@ public class LintDriver {
                     assert detector instanceof Detector.ClassScanner : detector;
                 }
             }
+
+            List<Detector> classCodeDetectors = mScopeDetectors.get(Scope.ALL_CLASS_FILES);
+            if (classCodeDetectors != null) {
+                for (Detector detector : classCodeDetectors) {
+                    assert detector instanceof Detector.ClassScanner : detector;
+                }
+            }
         }
     }
 
@@ -850,7 +857,9 @@ public class LintDriver {
             return;
         }
 
-        if (mScope.contains(Scope.CLASS_FILE) || mScope.contains(Scope.JAVA_LIBRARIES)) {
+        if (mScope.contains(Scope.CLASS_FILE)
+                || mScope.contains(Scope.ALL_CLASS_FILES)
+                || mScope.contains(Scope.JAVA_LIBRARIES)) {
             checkClasses(project, main);
         }
 
@@ -1054,6 +1063,7 @@ public class LintDriver {
         }
 
         runClassDetectors(Scope.CLASS_FILE, classEntries, project, main);
+        runClassDetectors(Scope.ALL_CLASS_FILES, classEntries, project, main);
     }
 
     private void checkIndividualClassFiles(
diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/Scope.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/Scope.java
index a917c11..fcafb64 100644
--- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/Scope.java
+++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/Scope.java
@@ -70,6 +70,13 @@ public enum Scope {
      */
     CLASS_FILE,
 
+    /**
+     * The analysis considers <b>all</b> the Java class files together.
+     * <p>
+     * This flag is mutually exclusive with {@link #CLASS_FILE}.
+     */
+    ALL_CLASS_FILES,
+
     /** The analysis considers the manifest file */
     MANIFEST,
 
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 d1c8f67..aafcd5c 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
@@ -55,7 +55,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
     private static final List<Issue> sIssues;
 
     static {
-        final int initialCapacity = 119;
+        final int initialCapacity = 120;
         List<Issue> issues = new ArrayList<Issue>(initialCapacity);
 
         issues.add(AccessibilityDetector.ISSUE);
@@ -86,6 +86,7 @@ public class BuiltinIssueRegistry extends IssueRegistry {
         issues.add(TooManyViewsDetector.TOO_MANY);
         issues.add(TooManyViewsDetector.TOO_DEEP);
         issues.add(GridLayoutDetector.ISSUE);
+        issues.add(OverrideDetector.ISSUE);
         issues.add(OnClickDetector.ISSUE);
         issues.add(ViewTagDetector.ISSUE);
         issues.add(LocaleDetector.STRING_LOCALE);
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/OverrideDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/OverrideDetector.java
new file mode 100644
index 0000000..137bc21
--- /dev/null
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/OverrideDetector.java
@@ -0,0 +1,271 @@
+/*
+ * 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.SdkConstants.CONSTRUCTOR_NAME;
+import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
+import static org.objectweb.asm.Opcodes.ACC_PROTECTED;
+import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
+import static org.objectweb.asm.Opcodes.ACC_STATIC;
+
+import com.android.annotations.NonNull;
+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.Location;
+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 com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
+import com.google.common.collect.Sets.SetView;
+
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.MethodNode;
+
+import java.util.EnumSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+
+/**
+ * Checks for accidental overrides
+ */
+public class OverrideDetector extends Detector implements ClassScanner {
+    /** Accidental overrides */
+    public static final Issue ISSUE = Issue.create(
+            "DalvikOverride", //$NON-NLS-1$
+            "Looks for methods treated as overrides by Dalvik",
+
+            "The Android virtual machine will treat a package private method in one " +
+            "class as overriding a package private method in its super class, even if " +
+            "they are in separate packages. This may be surprising, but for compatibility " +
+            "reasons the behavior has not been changed (yet).\n" +
+            "\n" +
+            "If you really did intend for this method to override the other, make the " +
+            "method `protected` instead.\n" +
+            "\n" +
+            "If you did *not* intend the override, consider making the method private, or " +
+            "changing its name or signature.",
+
+            Category.CORRECTNESS,
+            7,
+            Severity.ERROR,
+            OverrideDetector.class,
+            EnumSet.of(Scope.ALL_CLASS_FILES));
+
+    /** map from owner class name to JVM signatures for its package private methods  */
+    private Map<String, Set<String>> mPackagePrivateMethods = Maps.newHashMap();
+
+    /** Map from owner to signature to super class being overridden */
+    private Map<String, Map<String, String>> mErrors;
+
+    /**
+     * Map from owner to signature to corresponding location. When there are
+     * errors a single error can have locations for both the overriding and
+     * overridden methods.
+     */
+    private Map<String, Map<String, Location>> mLocations;
+
+    /** Constructs a new {@link OverrideDetector} */
+    public OverrideDetector() {
+    }
+
+    @Override
+    public @NonNull Speed getSpeed() {
+        return Speed.NORMAL;
+    }
+
+    @Override
+    public void afterCheckProject(@NonNull Context context) {
+        // Process the check in two passes:
+        //
+        // In the first pass, gather the full set of package private methods for
+        // each class.
+        // When all classes have been processed at the end of the first pass,
+        // find out whether any of the methods are potentially overriding those
+        // in its super classes.
+        //
+        // If so, request a second pass. In the second pass, we gather full locations
+        // for both the base and overridden method calls, and store these.
+        // If the location is found to be in a suppressed context, remove that error
+        // entry.
+        //
+        // At the end of the second pass, we generate the errors, combining locations
+        // from both the overridden and overriding methods.
+        if (context.getPhase() == 1) {
+            Set<String> classes = mPackagePrivateMethods.keySet();
+            LintDriver driver = context.getDriver();
+            for (String owner : classes) {
+                Set<String> methods = mPackagePrivateMethods.get(owner);
+                String superClass = driver.getSuperClass(owner);
+                int packageIndex = owner.lastIndexOf('/');
+                while (superClass != null) {
+                    int superPackageIndex = superClass.lastIndexOf('/');
+
+                    // Only compare methods that differ in packages
+                    if (packageIndex == -1 || superPackageIndex != packageIndex ||
+                            !owner.regionMatches(0, superClass, 0, packageIndex)) {
+                        Set<String> superMethods = mPackagePrivateMethods.get(superClass);
+                        if (superMethods != null) {
+                            SetView<String> intersection = Sets.intersection(methods,
+                                    superMethods);
+                            if (!intersection.isEmpty()) {
+                                if (mLocations == null) {
+                                    mLocations = Maps.newHashMap();
+                                }
+                                // We need a separate data structure to keep track of which
+                                // signatures are in error,
+                                if (mErrors == null) {
+                                    mErrors = Maps.newHashMap();
+                                }
+
+                                for (String signature : intersection) {
+                                    Map<String, Location> locations = mLocations.get(owner);
+                                    if (locations == null) {
+                                        locations = Maps.newHashMap();
+                                        mLocations.put(owner, locations);
+                                    }
+                                    locations.put(signature, null);
+
+                                    locations = mLocations.get(superClass);
+                                    if (locations == null) {
+                                        locations = Maps.newHashMap();
+                                        mLocations.put(superClass, locations);
+                                    }
+                                    locations.put(signature, null);
+
+
+                                    Map<String, String> errors = mErrors.get(owner);
+                                    if (errors == null) {
+                                        errors = Maps.newHashMap();
+                                        mErrors.put(owner, errors);
+                                    }
+                                    errors.put(signature, superClass);
+                                }
+                            }
+                        }
+                    }
+                    superClass = driver.getSuperClass(superClass);
+                }
+            }
+
+            if (mErrors != null) {
+                context.requestRepeat(this, ISSUE.getScope());
+            }
+        } else {
+            assert context.getPhase() == 2;
+
+            for (Entry<String, Map<String, String>> ownerEntry : mErrors.entrySet()) {
+                String owner = ownerEntry.getKey();
+                Map<String, String> methodToSuper = ownerEntry.getValue();
+                for (Entry<String, String> entry : methodToSuper.entrySet()) {
+                    String signature = entry.getKey();
+                    String superClass = entry.getValue();
+
+                    Map<String, Location> ownerLocations = mLocations.get(owner);
+                    if (ownerLocations != null) {
+                        Location location = ownerLocations.get(signature);
+                        if (location != null) {
+                            Map<String, Location> superLocations = mLocations.get(superClass);
+                            if (superLocations != null) {
+                                Location superLocation = superLocations.get(signature);
+                                if (superLocation != null) {
+                                    location.setSecondary(superLocation);
+                                    superLocation.setMessage(
+                                            "This method is treated as overridden");
+                                }
+                            }
+                            String methodName = signature;
+                            int index = methodName.indexOf('(');
+                            if (index != -1) {
+                                methodName = methodName.substring(0, index);
+                            }
+                            String message = String.format(
+                                    "This package private method may be unintentionally " +
+                                    "overriding %1$s in %2$s", methodName,
+                                    ClassContext.getFqcn(superClass));
+                            context.report(ISSUE, location, message, null);
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    @SuppressWarnings("rawtypes") // ASM4 API
+    @Override
+    public void checkClass(@NonNull ClassContext context, @NonNull ClassNode classNode) {
+        List methodList = classNode.methods;
+        if (context.getPhase() == 1) {
+            for (Object m : methodList) {
+                MethodNode method = (MethodNode) m;
+                int access = method.access;
+                // Only record non-static package private methods
+                if ((access & (ACC_STATIC|ACC_PRIVATE|ACC_PROTECTED|ACC_PUBLIC)) != 0) {
+                    continue;
+                }
+
+                // Ignore constructors too
+                if (CONSTRUCTOR_NAME.equals(method.name)) {
+                    continue;
+                }
+
+                String owner = classNode.name;
+                Set<String> methods = mPackagePrivateMethods.get(owner);
+                if (methods == null) {
+                    methods = Sets.newHashSetWithExpectedSize(methodList.size());
+                    mPackagePrivateMethods.put(owner, methods);
+                }
+                methods.add(method.name + method.desc);
+            }
+        } else {
+            assert context.getPhase() == 2;
+            Map<String, Location> methods = mLocations.get(classNode.name);
+            if (methods == null) {
+                // No locations needed from this class
+                return;
+            }
+
+            for (Object m : methodList) {
+                MethodNode method = (MethodNode) m;
+
+                String signature = method.name + method.desc;
+                if (methods.containsKey(signature)){
+                    if (method != null && context.getDriver().isSuppressed(ISSUE, method)) {
+                        Map<String, String> errors = mErrors.get(classNode.name);
+                        if (errors != null) {
+                            errors.remove(signature);
+                        }
+                        continue;
+                    }
+
+                    Location location = context.getLocation(method, classNode);
+                    methods.put(signature, location);
+                    String description = ClassContext.createSignature(classNode.name,
+                            method.name, method.desc);
+                    location.setClientData(description);
+                }
+            }
+        }
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java
index e9f70d4..e4d8bb5 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java
@@ -54,8 +54,7 @@ public class OnClickDetectorTest extends AbstractCheckTest {
             "res/layout/onclick.xml:58: Error: Corresponding method handler 'public void simple_typo(android.view.View)' not found (did you mean void test.pkg.OnClickActivity#simple_tyop(android.view.View) ?) [OnClick]\n" +
             "        android:onClick=\"simple_typo\"\n" +
             "        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" +
-            "9 errors, 0 warnings\n" +
-            "",
+            "9 errors, 0 warnings\n",
 
             lintProject(
                 "bytecode/.classpath=>.classpath",
@@ -73,5 +72,4 @@ public class OnClickDetectorTest extends AbstractCheckTest {
 
                 lintProject("res/layout/accessibility.xml"));
     }
-
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OverrideDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OverrideDetectorTest.java
new file mode 100644
index 0000000..68f80f9
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OverrideDetectorTest.java
@@ -0,0 +1,47 @@
+/*
+ * 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 com.android.tools.lint.detector.api.Detector;
+
+@SuppressWarnings("javadoc")
+public class OverrideDetectorTest extends AbstractCheckTest {
+    @Override
+    protected Detector getDetector() {
+        return new OverrideDetector();
+    }
+
+    public void test() throws Exception {
+        assertEquals(
+            "src/pkg2/Class2.java:7: Error: This package private method may be unintentionally overriding method in pkg1.Class1 [DalvikOverride]\n" +
+            "    void method() { // Flag this as an accidental override\n" +
+            "         ~~~~~~\n" +
+            "    src/pkg1/Class1.java:4: This method is treated as overridden\n" +
+            "1 errors, 0 warnings\n",
+
+            lintProject(
+                "bytecode/.classpath=>.classpath",
+                "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+                "src/pkg1/Class1.java.txt=>src/pkg1/Class1.java",
+                "src/pkg2/Class2.java.txt=>src/pkg2/Class2.java",
+                "bytecode/Class1.class.data=>bin/classes/pkg1/Class1.class",
+                "bytecode/Class1$Class4.class.data=>bin/classes/pkg1/Class1$Class4.class",
+                "bytecode/Class2.class.data=>bin/classes/pkg2/Class2.class",
+                "bytecode/Class2$Class3.class.data=>bin/classes/pkg2/Class2$Class3.class"
+                ));
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1$Class4.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1$Class4.class.data
new file mode 100644
index 0000000..bfd88db
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1$Class4.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1.class.data
new file mode 100644
index 0000000..cee3058
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class1.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2$Class3.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2$Class3.class.data
new file mode 100644
index 0000000..2a58332
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2$Class3.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2.class.data
new file mode 100644
index 0000000..1b72f03
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/Class2.class.data
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg1/Class1.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg1/Class1.java.txt
new file mode 100644
index 0000000..193ff3c
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg1/Class1.java.txt
@@ -0,0 +1,29 @@
+package pkg1;
+
+public class Class1 {
+    void method() {
+    }
+
+    void method2(int foo) {
+    }
+
+    void method3() {
+    }
+
+    void method4() {
+    }
+
+    void method5() {
+    }
+
+    void method6() {
+    }
+
+    void method7() {
+    }
+
+    public static class Class4 extends Class1 {
+        void method() { // Not an error: same package
+        }
+    }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg2/Class2.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg2/Class2.java.txt
new file mode 100644
index 0000000..651c021
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/pkg2/Class2.java.txt
@@ -0,0 +1,33 @@
+package pkg2;
+
+import android.annotation.SuppressLint;
+import pkg1.Class1;
+
+public class Class2 extends Class1 {
+    void method() { // Flag this as an accidental override
+    }
+
+    void method2(String foo) { // not an override: different signature
+    }
+
+    static void method4() { // not an override: static
+    }
+
+    private void method3() { // not an override: private
+    }
+
+    protected void method5() { // not an override: protected
+    }
+
+    public void method6() { // not an override: public
+    }
+
+    @SuppressLint("DalvikOverride")
+    public void method7() { // suppressed: no warning
+    }
+
+    public class Class3 extends Object {
+        void method() { // Not an override: not a subclass
+        }
+    }
+}