Merge "Limit <FrameLayout> to <merge> lint check to included layouts"

8 files changed, 236 insertions, 38 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 d2a5a72..1d7f0ec 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
@@ -243,4 +243,11 @@ public class LintConstants {
     public static final String ANDROID_LIBRARY = "android.library";     //$NON-NLS-1$
     public static final String ANDROID_LIBRARY_REFERENCE_FORMAT = "android.library.reference.%1$d";//$NON-NLS-1$
     public static final String PROJECT_PROPERTIES = "project.properties";//$NON-NLS-1$
+
+    // Java References
+    public static final String ATTR_REF_PREFIX = "?attr/";               //$NON-NLS-1$
+    public static final String R_PREFIX = "R.";                          //$NON-NLS-1$
+    public static final String R_ID_PREFIX = "R.id.";                    //$NON-NLS-1$
+    public static final String R_LAYOUT_PREFIX = "R.layout.";            //$NON-NLS-1$
+    public static final String R_ATTR_PREFIX = "R.attr.";                //$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 69cb591..214ce96 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
@@ -172,6 +172,16 @@ public class LintUtils {
     }
 
     /**
+     * Returns true if the given element is the root element of its document
+     *
+     * @param element the element to test
+     * @return true if the element is the root element
+     */
+    public static boolean isRootElement(Element element) {
+        return element == element.getOwnerDocument().getDocumentElement();
+    }
+
+    /**
      * Returns the given id without an {@code @id/} or {@code @+id} prefix
      *
      * @param id the id to strip
@@ -238,4 +248,20 @@ public class LintUtils {
         assert assertionsEnabled = true; // Intentional side-effect
         return assertionsEnabled;
     }
+
+    /**
+     * Returns the layout resource name for the given layout file
+     *
+     * @param layoutFile the file pointing to the layout
+     * @return the layout resource name, not including the {@code @layout}
+     *         prefix
+     */
+    public static String getLayoutName(File layoutFile) {
+        String name = layoutFile.getName();
+        int dotIndex = name.indexOf('.');
+        if (dotIndex != -1) {
+            name = name.substring(0, dotIndex);
+        }
+        return name;
+    }
 }
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/DuplicateIdDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/DuplicateIdDetector.java
index 1f7726a..18e8b79 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/DuplicateIdDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/DuplicateIdDetector.java
@@ -28,6 +28,7 @@ import com.android.tools.lint.detector.api.Category;
 import com.android.tools.lint.detector.api.Context;
 import com.android.tools.lint.detector.api.Issue;
 import com.android.tools.lint.detector.api.LayoutDetector;
+import com.android.tools.lint.detector.api.LintUtils;
 import com.android.tools.lint.detector.api.Location;
 import com.android.tools.lint.detector.api.Scope;
 import com.android.tools.lint.detector.api.Severity;
@@ -185,7 +186,7 @@ public class DuplicateIdDetector extends LayoutDetector {
 
         for (Entry<File, List<String>> entry : mIncludes.entrySet()) {
             File file = entry.getKey();
-            String from = getLayoutName(file);
+            String from = LintUtils.getLayoutName(file);
 
             // Merge include lists
             List<String> layouts = entry.getValue();
@@ -200,7 +201,7 @@ public class DuplicateIdDetector extends LayoutDetector {
         // Merge id maps
         for (Entry<File, Set<String>> entry : mFileToIds.entrySet()) {
             File file = entry.getKey();
-            String from = getLayoutName(file);
+            String from = LintUtils.getLayoutName(file);
             Set<String> ids = entry.getValue();
             if (ids != null) {
                 Set<String> set = resourceToIds.get(from);
@@ -235,15 +236,6 @@ public class DuplicateIdDetector extends LayoutDetector {
         }
     }
 
-    private String getLayoutName(File file) {
-        String name = file.getName();
-        int dotIndex = name.indexOf('.');
-        if (dotIndex != -1) {
-            name = name.substring(0, dotIndex);
-        }
-        return name;
-    }
-
     /**
      * Computes the complete set of ids in a layout (including included layouts,
      * transitively) and emits warnings when it detects that there is a
@@ -282,7 +274,7 @@ public class DuplicateIdDetector extends LayoutDetector {
                                 File second = null;
                                 for (Map.Entry<File, Set<String>> entry : mFileToIds.entrySet()) {
                                     File file = entry.getKey();
-                                    String name = getLayoutName(file);
+                                    String name = LintUtils.getLayoutName(file);
                                     if (name.equals(from)) {
                                         Set<String> fileIds = entry.getValue();
                                         if (fileIds.contains(id)) {
@@ -300,7 +292,7 @@ public class DuplicateIdDetector extends LayoutDetector {
                                     for (Map.Entry<File, List<String>> entry
                                             : mIncludes.entrySet()) {
                                         File file = entry.getKey();
-                                        String name = getLayoutName(file);
+                                        String name = LintUtils.getLayoutName(file);
                                         if (name.equals(from)) {
                                             first = file;
                                         }
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetector.java
index d433a92..2a67a9d 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetector.java
@@ -19,24 +19,68 @@ package com.android.tools.lint.checks;
 import static com.android.tools.lint.detector.api.LintConstants.ANDROID_URI;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_BACKGROUND;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_FOREGROUND;
+import static com.android.tools.lint.detector.api.LintConstants.ATTR_LAYOUT;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_LAYOUT_GRAVITY;
+import static com.android.tools.lint.detector.api.LintConstants.DOT_JAVA;
 import static com.android.tools.lint.detector.api.LintConstants.FRAME_LAYOUT;
+import static com.android.tools.lint.detector.api.LintConstants.INCLUDE;
+import static com.android.tools.lint.detector.api.LintConstants.LAYOUT_RESOURCE_PREFIX;
+import static com.android.tools.lint.detector.api.LintConstants.R_LAYOUT_PREFIX;
 
+import com.android.annotations.NonNull;
+import com.android.annotations.Nullable;
 import com.android.tools.lint.detector.api.Category;
+import com.android.tools.lint.detector.api.Context;
+import com.android.tools.lint.detector.api.Detector;
 import com.android.tools.lint.detector.api.Issue;
+import com.android.tools.lint.detector.api.JavaContext;
 import com.android.tools.lint.detector.api.LayoutDetector;
+import com.android.tools.lint.detector.api.LintUtils;
+import com.android.tools.lint.detector.api.Location;
+import com.android.tools.lint.detector.api.Location.Handle;
 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.android.tools.lint.detector.api.XmlContext;
+import com.android.util.Pair;
 
-import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import lombok.ast.AstVisitor;
+import lombok.ast.Expression;
+import lombok.ast.MethodInvocation;
+import lombok.ast.Select;
+import lombok.ast.StrictListAccessor;
+
 /**
  * Checks whether a root FrameLayout can be replaced with a {@code <merge>} tag.
  */
-public class MergeRootFrameLayoutDetector extends LayoutDetector {
+public class MergeRootFrameLayoutDetector extends LayoutDetector implements Detector.JavaScanner {
+    /**
+     * Set of layouts that we want to enable the warning for. We only warn for
+     * {@code <FrameLayout>}'s that are the root of a layout included from
+     * another layout, or directly referenced via a {@code setContentView} call.
+     */
+    private Set<String> mWhitelistedLayouts;
+
+    /**
+     * Set of pending [layout, location] pairs where the given layout is a
+     * FrameLayout that perhaps should be replaced by a {@code <merge>} tag (if
+     * the layout is included or set as the content view. This must be processed
+     * after the whole project has been scanned since the set of includes etc
+     * can be encountered after the included layout.
+     */
+    private List<Pair<String, Location.Handle>> mPending;
 
     /** The main issue discovered by this detector */
     public static final Issue ISSUE = Issue.create(
@@ -50,7 +94,7 @@ public class MergeRootFrameLayoutDetector extends LayoutDetector {
             4,
             Severity.WARNING,
             MergeRootFrameLayoutDetector.class,
-            Scope.RESOURCE_FILE_SCOPE).setMoreInfo(
+            EnumSet.of(Scope.ALL_RESOURCE_FILES, Scope.JAVA_FILE)).setMoreInfo(
             "http://android-developers.blogspot.com/2009/03/android-layout-tricks-3-optimize-by.html"); //$NON-NLS-1$
 
     /** Constructs a new {@link MergeRootFrameLayoutDetector} */
@@ -63,16 +107,90 @@ public class MergeRootFrameLayoutDetector extends LayoutDetector {
     }
 
     @Override
-    public void visitDocument(XmlContext context, Document document) {
-        Element root = document.getDocumentElement();
-        if (root.getTagName().equals(FRAME_LAYOUT) &&
-            ((isWidthFillParent(root) && isHeightFillParent(root)) ||
-                    !root.hasAttributeNS(ANDROID_URI, ATTR_LAYOUT_GRAVITY))
-                && !root.hasAttributeNS(ANDROID_URI, ATTR_BACKGROUND)
-                && !root.hasAttributeNS(ANDROID_URI, ATTR_FOREGROUND)
-                && !hasPadding(root)) {
-            context.report(ISSUE, context.getLocation(root),
-                    "This <FrameLayout> can be replaced with a <merge> tag", null);
+    public boolean appliesTo(Context context, File file) {
+        return LintUtils.isXmlFile(file) || LintUtils.endsWith(file.getName(), DOT_JAVA);
+    }
+
+    @Override
+    public void afterCheckProject(Context context) {
+        if (mPending != null && mWhitelistedLayouts != null) {
+            // Process all the root FrameLayouts that are eligible, and generate
+            // suggestions for <merge> replacements for any layouts that are included
+            // from other layouts
+            for (Pair<String, Handle> pair : mPending) {
+                String layout = pair.getFirst();
+                if (mWhitelistedLayouts.contains(layout)) {
+                    Handle handle = pair.getSecond();
+                    Location location = handle.resolve();
+                    context.report(ISSUE, location,
+                            "This <FrameLayout> can be replaced with a <merge> tag", null);
+                }
+            }
+        }
+    }
+
+    // Implements XmlScanner
+
+    @Override
+    public Collection<String> getApplicableElements() {
+        return Arrays.asList(INCLUDE, FRAME_LAYOUT);
+    }
+
+    @Override
+    public void visitElement(XmlContext context, Element element) {
+        String tag = element.getTagName();
+        if (tag.equals(INCLUDE)) {
+            String layout = element.getAttribute(ATTR_LAYOUT); // NOTE: Not in android: namespace
+            if (layout.startsWith(LAYOUT_RESOURCE_PREFIX)) { // Ignore @android:layout/ layouts
+                layout = layout.substring(LAYOUT_RESOURCE_PREFIX.length());
+                whiteListLayout(layout);
+            }
+        } else {
+            assert tag.equals(FRAME_LAYOUT);
+            if (LintUtils.isRootElement(element) &&
+                ((isWidthFillParent(element) && isHeightFillParent(element)) ||
+                        !element.hasAttributeNS(ANDROID_URI, ATTR_LAYOUT_GRAVITY))
+                    && !element.hasAttributeNS(ANDROID_URI, ATTR_BACKGROUND)
+                    && !element.hasAttributeNS(ANDROID_URI, ATTR_FOREGROUND)
+                    && !hasPadding(element)) {
+                String layout = LintUtils.getLayoutName(context.file);
+                Handle handle = context.parser.createLocationHandle(context, element);
+                if (mPending == null) {
+                    mPending = new ArrayList<Pair<String,Handle>>();
+                }
+                mPending.add(Pair.of(layout, handle));
+            }
+        }
+    }
+
+    private void whiteListLayout(String layout) {
+        if (mWhitelistedLayouts == null) {
+            mWhitelistedLayouts = new HashSet<String>();
+        }
+        mWhitelistedLayouts.add(layout);
+    }
+
+    // Implements JavaScanner
+
+    @Override
+    public List<String> getApplicableMethodNames() {
+        return Collections.singletonList("setContentView"); //$NON-NLS-1$
+    }
+
+    @Override
+    public void visitMethod(
+            @NonNull JavaContext context,
+            @Nullable AstVisitor visitor,
+            @NonNull MethodInvocation node) {
+        StrictListAccessor<Expression, MethodInvocation> argumentList = node.astArguments();
+        if (argumentList != null && argumentList.size() == 1) {
+            Expression argument = argumentList.first();
+            if (argument instanceof Select) {
+                String expression = argument.toString();
+                if (expression.startsWith(R_LAYOUT_PREFIX)) {
+                    whiteListLayout(expression.substring(R_LAYOUT_PREFIX.length()));
+                }
+            }
         }
     }
 }
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/UnusedResourceDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/UnusedResourceDetector.java
index 35e11d8..0f378d3 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/UnusedResourceDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/UnusedResourceDetector.java
@@ -18,13 +18,16 @@ package com.android.tools.lint.checks;
 
 import static com.android.tools.lint.detector.api.LintConstants.ANDROID_URI;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_NAME;
+import static com.android.tools.lint.detector.api.LintConstants.ATTR_REF_PREFIX;
 import static com.android.tools.lint.detector.api.LintConstants.DOT_JAVA;
 import static com.android.tools.lint.detector.api.LintConstants.DOT_PNG;
 import static com.android.tools.lint.detector.api.LintConstants.DOT_XML;
 import static com.android.tools.lint.detector.api.LintConstants.RESOURCE_CLR_STYLEABLE;
 import static com.android.tools.lint.detector.api.LintConstants.RESOURCE_CLZ_ARRAY;
-import static com.android.tools.lint.detector.api.LintConstants.RESOURCE_CLZ_ATTR;
 import static com.android.tools.lint.detector.api.LintConstants.RESOURCE_CLZ_ID;
+import static com.android.tools.lint.detector.api.LintConstants.R_ATTR_PREFIX;
+import static com.android.tools.lint.detector.api.LintConstants.R_ID_PREFIX;
+import static com.android.tools.lint.detector.api.LintConstants.R_PREFIX;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_ARRAY;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_ITEM;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_RESOURCES;
@@ -82,9 +85,6 @@ import lombok.ast.VariableReference;
  * use it.
  */
 public class UnusedResourceDetector extends ResourceXmlDetector implements Detector.JavaScanner {
-    private static final String ATTR_REF_PREFIX = "?attr/";           //$NON-NLS-1$
-    private static final String R_PREFIX = "R.";                      //$NON-NLS-1$
-    private static final String R_ID_PREFIX = "R.id.";                //$NON-NLS-1$
 
     /** Unused resources (other than ids). */
     public static final Issue ISSUE = Issue.create("UnusedResources", //$NON-NLS-1$
@@ -317,7 +317,7 @@ public class UnusedResourceDetector extends ResourceXmlDetector implements Detec
                 int index = text.indexOf(ATTR_REF_PREFIX);
                 if (index != -1) {
                     String name = text.substring(index + ATTR_REF_PREFIX.length()).trim();
-                    mReferences.add(R_PREFIX + RESOURCE_CLZ_ATTR + '.' + name);
+                    mReferences.add(R_ATTR_PREFIX + name);
                 } else {
                     index = text.indexOf('@');
                     if (index != -1 && text.indexOf('/', index) != -1
@@ -351,13 +351,12 @@ public class UnusedResourceDetector extends ResourceXmlDetector implements Detec
             String r = R_PREFIX + value.substring(1).replace('/', '.');
             mReferences.add(r);
         } else if (value.startsWith(ATTR_REF_PREFIX)) {
-            mReferences.add(R_PREFIX + RESOURCE_CLZ_ATTR + '.'
-                    + value.substring(ATTR_REF_PREFIX.length()));
+            mReferences.add(R_ATTR_PREFIX + value.substring(ATTR_REF_PREFIX.length()));
         }
 
         if (attribute.getNamespaceURI() != null
                 && !ANDROID_URI.equals(attribute.getNamespaceURI())) {
-            mReferences.add(R_PREFIX + RESOURCE_CLZ_ATTR + '.' + attribute.getLocalName());
+            mReferences.add(R_ATTR_PREFIX + attribute.getLocalName());
         }
     }
 
@@ -377,8 +376,8 @@ public class UnusedResourceDetector extends ResourceXmlDetector implements Detec
     }
 
     @Override
-    public void visitResourceReference(JavaContext context, AstVisitor visitor, VariableReference node,
-            String type, String name) {
+    public void visitResourceReference(JavaContext context, AstVisitor visitor,
+            VariableReference node, String type, String name) {
         String reference = R_PREFIX + type + '.' + name;
         mReferences.add(reference);
     }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetectorTest.java
index 2d6df3e..beba9d9 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/MergeRootFrameLayoutDetectorTest.java
@@ -25,9 +25,28 @@ public class MergeRootFrameLayoutDetectorTest extends AbstractCheckTest {
         return new MergeRootFrameLayoutDetector();
     }
 
-    public void testMerge() throws Exception {
+
+    public void testMergeRefFromJava() throws Exception {
+        assertEquals(
+                "simple.xml:3: Warning: This <FrameLayout> can be replaced with a <merge> tag",
+                lintProject(
+                        "res/layout/simple.xml",
+                        "src/test/pkg/ImportFrameActivity.java.txt=>src/test/pkg/ImportFrameActivity.java"
+                        ));
+    }
+
+    public void testMergeRefFromInclude() throws Exception {
         assertEquals(
                 "simple.xml:3: Warning: This <FrameLayout> can be replaced with a <merge> tag",
-                lintFiles("res/layout/simple.xml"));
+                lintProject(
+                        "res/layout/simple.xml",
+                        "res/layout/simpleinclude.xml"
+                        ));
+    }
+
+    public void testNotIncluded() throws Exception {
+        assertEquals(
+                "No warnings.",
+                lintProject("res/layout/simple.xml"));
     }
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/simpleinclude.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/simpleinclude.xml
new file mode 100644
index 0000000..4cb994b
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/simpleinclude.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8"?>
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
+    android:layout_width="match_parent"
+    android:layout_height="match_parent"
+    android:orientation="vertical" >
+
+    <include
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        layout="@layout/simple" />
+
+    <Button
+        android:id="@+id/button1"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        android:text="Button" />
+
+    <Button
+        android:id="@+id/button2"
+        android:layout_width="wrap_content"
+        android:layout_height="wrap_content"
+        android:text="Button" />
+
+</LinearLayout>
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/ImportFrameActivity.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/ImportFrameActivity.java.txt
new file mode 100644
index 0000000..510dadc
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/ImportFrameActivity.java.txt
@@ -0,0 +1,13 @@
+package test.pkg;
+
+import foo.bar.R;
+import android.app.Activity;
+import android.os.Bundle;
+
+public class ImportFrameActivity extends Activity {
+    @Override
+    public void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        setContentView(R.layout.simple);
+    }
+}