From 8f121f5322e9b9ea897a4f4bf73ccdaaf3096f58 Mon Sep 17 00:00:00 2001 From: Tor Norbye Date: Thu, 16 Feb 2012 09:12:07 -0800 Subject: Add onClick check for lint This lint check makes sure that if a layout references android:onClick="foo" then one of the Java classes provide a public void foo(View) method. It also looks for simple typos, and warns if the access modifiers are not right. (It also moves some ASM-related utility code out of specific detectors and into the ClassContext class) Change-Id: Ifb0820221175a0760b83992f54e99f761b4d7097 --- .../tools/lint/detector/api/ClassContext.java | 108 ++++++++- .../tools/lint/detector/api/LintConstants.java | 1 + .../com/android/tools/lint/checks/ApiDetector.java | 26 +-- .../tools/lint/checks/BuiltinIssueRegistry.java | 3 +- .../android/tools/lint/checks/OnClickDetector.java | 246 +++++++++++++++++++++ .../tools/lint/checks/OnClickDetectorTest.java | 54 +++++ .../data/bytecode/OnClickActivity.class.data | Bin 0 -> 1093 bytes .../checks/data/bytecode/OnClickActivity.java.txt | 44 ++++ .../tools/lint/checks/data/res/layout/onclick.xml | 67 ++++++ 9 files changed, 521 insertions(+), 28 deletions(-) create mode 100644 lint/libs/lint_checks/src/com/android/tools/lint/checks/OnClickDetector.java create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.class.data create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.java.txt create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/onclick.xml diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/ClassContext.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/ClassContext.java index a2899d8..08da20c 100644 --- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/ClassContext.java +++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/ClassContext.java @@ -24,8 +24,11 @@ import com.android.annotations.Nullable; import com.android.tools.lint.client.api.LintDriver; import com.google.common.annotations.Beta; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AbstractInsnNode; import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.FieldNode; +import org.objectweb.asm.tree.LineNumberNode; import org.objectweb.asm.tree.MethodNode; import java.io.File; @@ -223,8 +226,12 @@ public class ClassContext extends Context { File sourceFile = getSourceFile(); if (sourceFile != null) { // ASM line numbers are 1-based, and lint line numbers are 0-based - return Location.create(sourceFile, getSourceContents(), line - 1, - patternStart, patternEnd); + if (line != -1) { + return Location.create(sourceFile, getSourceContents(), line - 1, + patternStart, patternEnd); + } else { + return Location.create(sourceFile); + } } return Location.create(file); @@ -302,4 +309,101 @@ public class ClassContext extends Context { } report(issue, location, message, data); // also checks the class node } + + /** + * Finds the line number closest to the given node + * + * @param node the instruction node to get a line number for + * @return the closest line number, or -1 if not known + */ + public static int findLineNumber(AbstractInsnNode node) { + AbstractInsnNode curr = node; + + // First search backwards + while (curr != null) { + if (curr.getType() == AbstractInsnNode.LINE) { + return ((LineNumberNode) curr).line; + } + curr = curr.getPrevious(); + } + + // Then search forwards + curr = node; + while (curr != null) { + if (curr.getType() == AbstractInsnNode.LINE) { + return ((LineNumberNode) curr).line; + } + curr = curr.getNext(); + } + + return -1; + } + + /** + * Finds the line number closest to the given method declaration + * + * @param node the method node to get a line number for + * @return the closest line number, or -1 if not known + */ + public static int findLineNumber(MethodNode node) { + if (node.instructions != null && node.instructions.size() > 0) { + return findLineNumber(node.instructions.get(0)); + } + + return -1; + } + + /** + * Computes a user-readable type signature from the given class owner, name + * and description + * + * @param owner the class name + * @param name the method name + * @param desc the method description + * @return a user-readable string + */ + public static String createSignature(String owner, String name, String desc) { + StringBuilder sb = new StringBuilder(); + + if (desc != null) { + Type returnType = Type.getReturnType(desc); + sb.append(getTypeString(returnType)); + sb.append(' '); + } + + if (owner != null) { + sb.append(owner.replace('/', '.').replace('$','.')); + } + if (name != null) { + sb.append('#'); + sb.append(name); + if (desc != null) { + Type[] argumentTypes = Type.getArgumentTypes(desc); + if (argumentTypes != null && argumentTypes.length > 0) { + sb.append('('); + boolean first = true; + for (Type type : argumentTypes) { + if (first) { + first = false; + } else { + sb.append(", "); + } + sb.append(getTypeString(type)); + } + sb.append(')'); + } + } + } + + return sb.toString(); + } + + private static String getTypeString(Type type) { + String s = type.getClassName(); + if (s.startsWith("java.lang.")) { //$NON-NLS-1$ + s = s.substring("java.lang.".length()); //$NON-NLS-1$ + } + + return s; + } } 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 9d9cf6d..371046f 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 @@ -133,6 +133,7 @@ public class LintConstants { public static final String ATTR_LABEL = "label"; //$NON-NLS-1$ public static final String ATTR_HINT = "hint"; //$NON-NLS-1$ public static final String ATTR_PROMPT = "prompt"; //$NON-NLS-1$ + public static final String ATTR_ON_CLICK = "onClick"; //$NON-NLS-1$ public static final String ATTR_INPUT_TYPE = "inputType"; //$NON-NLS-1$ public static final String ATTR_INPUT_METHOD = "inputMethod"; //$NON-NLS-1$ public static final String ATTR_LAYOUT_GRAVITY = "layout_gravity"; //$NON-NLS-1$ diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ApiDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ApiDetector.java index 9e744e4..5e469d9 100644 --- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ApiDetector.java +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ApiDetector.java @@ -39,7 +39,6 @@ import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.FieldInsnNode; import org.objectweb.asm.tree.InsnList; import org.objectweb.asm.tree.LdcInsnNode; -import org.objectweb.asm.tree.LineNumberNode; import org.objectweb.asm.tree.LocalVariableNode; import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.tree.MethodNode; @@ -328,32 +327,9 @@ public class ApiDetector extends LayoutDetector implements Detector.ClassScanner return -1; } - private static int findLineNumber(AbstractInsnNode node) { - AbstractInsnNode curr = node; - - // First search backwards - while (curr != null) { - if (curr.getType() == AbstractInsnNode.LINE) { - return ((LineNumberNode) curr).line; - } - curr = curr.getPrevious(); - } - - // Then search forwards - curr = node; - while (curr != null) { - if (curr.getType() == AbstractInsnNode.LINE) { - return ((LineNumberNode) curr).line; - } - curr = curr.getNext(); - } - - return -1; - } - private void report(final ClassContext context, String message, AbstractInsnNode node, MethodNode method, String patternStart, String patternEnd) { - int lineNumber = node != null ? findLineNumber(node) : -1; + int lineNumber = node != null ? ClassContext.findLineNumber(node) : -1; Location location = context.getLocationForLine(lineNumber, patternStart, patternEnd); context.report(UNSUPPORTED, method, location, message, null); } 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 e3deb3f..6df3def 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 = 77; + final int initialCapacity = 78; List issues = new ArrayList(initialCapacity); issues.add(AccessibilityDetector.ISSUE); @@ -83,6 +83,7 @@ public class BuiltinIssueRegistry extends IssueRegistry { issues.add(TooManyViewsDetector.TOO_MANY); issues.add(TooManyViewsDetector.TOO_DEEP); issues.add(GridLayoutDetector.ISSUE); + issues.add(OnClickDetector.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/OnClickDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/OnClickDetector.java new file mode 100644 index 0000000..47e063e --- /dev/null +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/OnClickDetector.java @@ -0,0 +1,246 @@ +/* + * 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.ATTR_ON_CLICK; + +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.ClassScanner; +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.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.google.common.base.Joiner; +import com.sun.xml.internal.ws.org.objectweb.asm.Opcodes; + +import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.MethodNode; +import org.w3c.dom.Attr; +import org.w3c.dom.Node; + +import java.io.File; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.EnumSet; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Checks for missing onClick handlers + */ +public class OnClickDetector extends LayoutDetector implements ClassScanner { + /** Missing onClick handlers */ + public static final Issue ISSUE = Issue.create( + "OnClick", //$NON-NLS-1$ + "Ensures that onClick attribute values refer to real methods", + + "The onClick attribute value should be the name of a method in this View's context " + + "to invoke when the view is clicked. This name must correspond to a public method " + + "that takes exactly one parameter of type View.\n" + + "\n" + + "Must be a string value, using '\\;' to escape characters such as '\\n' or " + + "'\\uxxxx' for a unicode character.", + Category.CORRECTNESS, + 10, + Severity.ERROR, + OnClickDetector.class, + EnumSet.of(Scope.ALL_RESOURCE_FILES, Scope.CLASS_FILE)); + + private Map mNames; + private Map> mSimilar; + private boolean mHaveBytecode; + + /** Constructs a new {@link OnClickDetector} */ + public OnClickDetector() { + } + + @Override + public Speed getSpeed() { + return Speed.FAST; + } + + @Override + public boolean appliesTo(Context context, File file) { + return true; + } + + @Override + public void afterCheckProject(Context context) { + if (mNames != null && mNames.size() > 0 && mHaveBytecode) { + List names = new ArrayList(mNames.keySet()); + Collections.sort(names); + LintDriver driver = context.getDriver(); + for (String name : names) { + Handle handle = mNames.get(name); + + Object clientData = handle.getClientData(); + if (clientData instanceof Node) { + if (driver.isSuppressed(ISSUE, (Node) clientData)) { + continue; + } + } + + Location location = handle.resolve(); + String message = String.format( + "Corresponding method handler 'public void %1$s(android.view.View)' not found", + name); + List similar = mSimilar != null ? mSimilar.get(name) : null; + if (similar != null) { + Collections.sort(similar); + message = message + String.format(" (did you mean %1$s ?)", + Joiner.on(", ").join(similar)); + } + context.report(ISSUE, location, message, null); + } + } + } + + // ---- Implements XmlScanner ---- + + @Override + public Collection getApplicableAttributes() { + return Collections.singletonList(ATTR_ON_CLICK); + } + + @Override + public void visitAttribute(XmlContext context, Attr attribute) { + String value = attribute.getValue(); + if (value.isEmpty() || value.trim().isEmpty()) { + context.report(ISSUE, attribute, context.getLocation(attribute), + "onClick attribute value cannot be empty", null); + } else if (!value.equals(value.trim())) { + context.report(ISSUE, attribute, context.getLocation(attribute), + "There should be no whitespace around attribute values", null); + } else { + if (mNames == null) { + mNames = new HashMap(); + } + Handle handle = context.parser.createLocationHandle(context, attribute); + handle.setClientData(attribute); + + // Replace unicode characters with the actual value since that's how they + // appear in the ASM signatures + if (value.contains("\\u")) { //$NON-NLS-1$ + Pattern pattern = Pattern.compile("\\\\u(\\d\\d\\d\\d)"); //$NON-NLS-1$ + Matcher matcher = pattern.matcher(value); + StringBuilder sb = new StringBuilder(value.length()); + int remainder = 0; + while (matcher.find()) { + sb.append(value.substring(0, matcher.start())); + String unicode = matcher.group(1); + int hex = Integer.parseInt(unicode, 16); + sb.append((char) hex); + remainder = matcher.end(); + } + sb.append(value.substring(remainder)); + value = sb.toString(); + } + + mNames.put(value, handle); + } + } + + // ---- Implements ClassScanner ---- + + @SuppressWarnings("rawtypes") + @Override + public void checkClass(ClassContext context, ClassNode classNode) { + if (mNames == null) { + // No onClick attributes in the XML files + return; + } + + mHaveBytecode = true; + + List methodList = classNode.methods; + for (Object m : methodList) { + MethodNode method = (MethodNode) m; + boolean rightArguments = method.desc.equals("(Landroid/view/View;)V"); //$NON-NLS-1$ + if (!mNames.containsKey(method.name)) { + if (rightArguments) { + // See if there's a possible typo instead + for (String n : mNames.keySet()) { + if (LintUtils.editDistance(n, method.name) <= 2) { + recordSimilar(n, classNode, method); + break; + } + } + } + continue; + } + + // TODO: Validate class hierarchy: should extend a context method + // Longer term, also validate that it's in a layout that corresponds to + // the given activity + + if (rightArguments){ + // Found: remove from list to be checked + mNames.remove(method.name); + + // Make sure the method is public + if ((method.access & Opcodes.ACC_PUBLIC) == 0) { + int lineNumber = ClassContext.findLineNumber(method); + Location location = null; + location = context.getLocationForLine(lineNumber, method.name, null); + String message = String.format( + "On click handler %1$s(View) must be public", + method.name); + context.report(ISSUE, location, message, null); + } else if ((method.access & Opcodes.ACC_STATIC) != 0) { + int lineNumber = ClassContext.findLineNumber(method); + Location location = null; + location = context.getLocationForLine(lineNumber, method.name, null); + String message = String.format( + "On click handler %1$s(View) should not be static", + method.name); + context.report(ISSUE, location, message, null); + } + + if (mNames.isEmpty()) { + mNames = null; + return; + } + } + } + } + + private void recordSimilar(String name, ClassNode classNode, MethodNode method) { + if (mSimilar == null) { + mSimilar = new HashMap>(); + } + List list = mSimilar.get(name); + if (list == null) { + list = new ArrayList(); + mSimilar.put(name, list); + } + + String signature = ClassContext.createSignature(classNode.name, method.name, method.desc); + list.add(signature); + } +} 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 new file mode 100644 index 0000000..240e0ce --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/OnClickDetectorTest.java @@ -0,0 +1,54 @@ +/* + * 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 OnClickDetectorTest extends AbstractCheckTest { + @Override + protected Detector getDetector() { + return new OnClickDetector(); + } + + public void test() throws Exception { + assertEquals( + "onclick.xml:10: Error: Corresponding method handler 'public void nonexistent(android.view.View)' not found\n" + + "onclick.xml:16: Error: Corresponding method handler 'public void wrong1(android.view.View)' not found\n" + + "onclick.xml:22: Error: Corresponding method handler 'public void wrong2(android.view.View)' not found\n" + + "onclick.xml:28: Error: Corresponding method handler 'public void wrong3(android.view.View)' not found\n" + + "onclick.xml:34: Error: Corresponding method handler 'public void wrong4(android.view.View)' not found\n" + + "onclick.xml:46: Error: Corresponding method handler 'public void simple_typo(android.view.View)' not found (did you mean test.pkg.OnClickActivity#simple_tyop(Landroid/view/View;)V ?)", + + lintProject( + "bytecode/.classpath=>.classpath", + "bytecode/AndroidManifest.xml=>AndroidManifest.xml", + "res/layout/onclick.xml=>res/layout/onclick.xml", + "bytecode/OnClickActivity.java.txt=>src/test/pkg/OnClickActivity.java", + "bytecode/OnClickActivity.class.data=>bin/classes/test/pkg/OnClickActivity.class" + )); + } + + public void testOk() throws Exception { + // No onClick attributes + assertEquals( + "No warnings.", + + lintProject("res/layout/accessibility.xml")); + } + +} diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.class.data new file mode 100644 index 0000000..efe894a Binary files /dev/null and b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.class.data differ diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.java.txt new file mode 100644 index 0000000..44402bd --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/OnClickActivity.java.txt @@ -0,0 +1,44 @@ +package test.pkg; + +import android.app.Activity; +import android.view.View; + +/** Test data for the OnClickDetector */ +public class OnClickActivity extends Activity { + // Wrong argument type 1 + public void wrong1() { + } + + // Wrong argument type 2 + public void wrong2(int i) { + } + + // Wrong argument type 3 + public void wrong3(View view, int i) { + } + + // Wrong return type + public int wrong4(View view) { + return 0; + } + + // Wrong modifier (not public) + void wrong5(View view) { + } + + // Wrong modifier (is static) + public static void wrong6(View view) { + } + + public void ok(View view) { + } + + // Ok: Unicode escapes + public void my\u1234method(View view) { + } + + // Typo + public void simple_tyop(View view) { + } + +} diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/onclick.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/onclick.xml new file mode 100644 index 0000000..d271a30 --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/res/layout/onclick.xml @@ -0,0 +1,67 @@ + + + +