diff options
author | Tor Norbye <tnorbye@google.com> | 2012-11-14 11:36:01 +0100 |
---|---|---|
committer | Tor Norbye <tnorbye@google.com> | 2012-11-27 17:45:14 -0800 |
commit | 914c9d45d013ed7648fc8ab558de231842e15b5c (patch) | |
tree | 36652f1a2175de5479514cfad36859117e83f148 /lint/libs | |
parent | 9a5288a413869219120a27ee4fb11c100b9a7a8b (diff) | |
download | sdk-914c9d45d013ed7648fc8ab558de231842e15b5c.zip sdk-914c9d45d013ed7648fc8ab558de231842e15b5c.tar.gz sdk-914c9d45d013ed7648fc8ab558de231842e15b5c.tar.bz2 |
Add lint check for cut & paste errors in findViewById calls
Change-Id: Ie8ff6629a3e787587516f6fa88c70dfdb7a92d0d
Diffstat (limited to 'lint/libs')
8 files changed, 428 insertions, 37 deletions
diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/JavaContext.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/JavaContext.java index ae86568..d91f423 100644 --- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/JavaContext.java +++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/JavaContext.java @@ -23,6 +23,8 @@ import com.android.tools.lint.client.api.LintDriver; import java.io.File; +import lombok.ast.ConstructorDeclaration; +import lombok.ast.MethodDeclaration; import lombok.ast.Node; /** @@ -105,4 +107,21 @@ public class JavaContext extends Context { } super.report(issue, location, message, data); } + + + @Nullable + public static Node findSurroundingMethod(Node scope) { + while (scope != null) { + Class<? extends Node> type = scope.getClass(); + // The Lombok AST uses a flat hierarchy of node type implementation classes + // so no need to do instanceof stuff here. + if (type == MethodDeclaration.class || type == ConstructorDeclaration.class) { + return scope; + } + + scope = scope.getParent(); + } + + return 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 a211029..a475ac6 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 = 129; + final int initialCapacity = 130; List<Issue> issues = new ArrayList<Issue>(initialCapacity); issues.add(AccessibilityDetector.ISSUE); @@ -182,6 +182,7 @@ public class BuiltinIssueRegistry extends IssueRegistry { issues.add(SetJavaScriptEnabledDetector.ISSUE); issues.add(ToastDetector.ISSUE); issues.add(SharedPrefsDetector.ISSUE); + issues.add(CutPasteDetector.ISSUE); issues.add(NonInternationalizedSmsDetector.ISSUE); issues.add(PrivateKeyDetector.ISSUE); issues.add(AnnotationDetector.ISSUE); diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/CutPasteDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/CutPasteDetector.java new file mode 100644 index 0000000..193ab8f --- /dev/null +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/CutPasteDetector.java @@ -0,0 +1,240 @@ +/* + * 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.RESOURCE_CLZ_ID; + +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.Location; +import com.android.tools.lint.detector.api.Scope; +import com.android.tools.lint.detector.api.Severity; +import com.google.common.collect.Maps; + +import java.io.File; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import lombok.ast.ArrayAccess; +import lombok.ast.AstVisitor; +import lombok.ast.BinaryExpression; +import lombok.ast.Cast; +import lombok.ast.Expression; +import lombok.ast.ForwardingAstVisitor; +import lombok.ast.If; +import lombok.ast.MethodInvocation; +import lombok.ast.Node; +import lombok.ast.Select; +import lombok.ast.Statement; +import lombok.ast.VariableDefinitionEntry; +import lombok.ast.VariableReference; + +/** + * Detector looking for cut & paste issues + */ +public class CutPasteDetector extends Detector implements Detector.JavaScanner { + /** The main issue discovered by this detector */ + public static final Issue ISSUE = Issue.create( + "CutPasteId", //$NON-NLS-1$ + "Looks for code cut & paste mistakes in findViewbyId() calls", + + "This lint check looks for cases where you have cut & pasted calls to " + + "`findViewById` but have forgotten to update the R.id field. It's possible " + + "that your code is simply (redundantly) looking up the field repeatedly, " + + "but lint cannot distinguish that from a case where you for example want to " + + "initialize fields `prev` and `next` and you cut & pasted `findViewById(R.id.prev)` " + + "and forgot to update the second initialization to `R.id.next`.", + + Category.CORRECTNESS, + 6, + Severity.WARNING, + CutPasteDetector.class, + Scope.JAVA_FILE_SCOPE); + + private Node mLastMethod; + private Map<String, MethodInvocation> mIds; + private Map<String, String> mLhs; + private Map<String, String> mCallOperands; + + /** Constructs a new {@link CutPasteDetector} check */ + public CutPasteDetector() { + } + + @Override + public boolean appliesTo(@NonNull Context context, @NonNull File file) { + return true; + } + + // ---- Implements JavaScanner ---- + + @Override + public List<String> getApplicableMethodNames() { + return Collections.singletonList("findViewById"); //$NON-NLS-1$ + } + + @Override + public void visitMethod(@NonNull JavaContext context, @Nullable AstVisitor visitor, + @NonNull MethodInvocation call) { + String lhs = getLhs(call); + if (lhs == null) { + return; + } + + Node method = JavaContext.findSurroundingMethod(call); + if (method == null) { + return; + } else if (method != mLastMethod) { + mIds = Maps.newHashMap(); + mLhs = Maps.newHashMap(); + mCallOperands = Maps.newHashMap(); + mLastMethod = method; + } + + String callOperand = call.astOperand() != null ? call.astOperand().toString() : ""; + + Expression first = call.astArguments().first(); + if (first instanceof Select) { + Select select = (Select) first; + String id = select.astIdentifier().astValue(); + Expression operand = select.astOperand(); + if (operand instanceof Select) { + Select type = (Select) operand; + if (type.astIdentifier().astValue().equals(RESOURCE_CLZ_ID)) { + if (mIds.containsKey(id)) { + if (lhs.equals(mLhs.get(id))) { + return; + } + if (!callOperand.equals(mCallOperands.get(id))) { + return; + } + MethodInvocation earlierCall = mIds.get(id); + if (!isReachableFrom(method, earlierCall, call)) { + return; + } + Location location = context.getLocation(call); + Location secondary = context.getLocation(earlierCall); + secondary.setMessage("First usage here"); + location.setSecondary(secondary); + context.report(ISSUE, call, location, String.format( + "The id %1$s has already been looked up in this method; possible " + + "cut & paste error?", first.toString()), null); + } else { + mIds.put(id, call); + mLhs.put(id, lhs); + mCallOperands.put(id, callOperand); + } + } + } + } + } + + @Nullable + private static String getLhs(@NonNull MethodInvocation call) { + Node parent = call.getParent(); + if (parent instanceof Cast) { + parent = parent.getParent(); + } + + if (parent instanceof VariableDefinitionEntry) { + VariableDefinitionEntry vde = (VariableDefinitionEntry) parent; + return vde.astName().astValue(); + } else if (parent instanceof BinaryExpression) { + BinaryExpression be = (BinaryExpression) parent; + Expression left = be.astLeft(); + if (left instanceof VariableReference || left instanceof Select) { + return be.astLeft().toString(); + } else if (left instanceof ArrayAccess) { + ArrayAccess aa = (ArrayAccess) left; + return aa.astOperand().toString(); + } + } + + return null; + } + + private static boolean isReachableFrom( + @NonNull Node method, + @NonNull MethodInvocation from, + @NonNull MethodInvocation to) { + ReachableVisitor visitor = new ReachableVisitor(from, to); + method.accept(visitor); + + return visitor.isReachable(); + } + + private static class ReachableVisitor extends ForwardingAstVisitor { + private final @NonNull MethodInvocation mFrom; + private final @NonNull MethodInvocation mTo; + private boolean mReachable; + private boolean mSeenEnd; + + public ReachableVisitor(@NonNull MethodInvocation from, @NonNull MethodInvocation to) { + mFrom = from; + mTo = to; + } + + boolean isReachable() { + return mReachable; + } + + @Override + public boolean visitMethodInvocation(MethodInvocation node) { + if (node == mFrom) { + mReachable = true; + } else if (node == mTo) { + mSeenEnd = true; + + } + return super.visitMethodInvocation(node); + } + + @Override + public boolean visitIf(If node) { + Expression condition = node.astCondition(); + Statement body = node.astStatement(); + Statement elseBody = node.astElseStatement(); + if (condition != null) { + condition.accept(this); + } + if (body != null) { + boolean wasReachable = mReachable; + body.accept(this); + mReachable = wasReachable; + } + if (elseBody != null) { + boolean wasReachable = mReachable; + elseBody.accept(this); + mReachable = wasReachable; + } + + endVisit(node); + + return false; + } + + @Override + public boolean visitNode(Node node) { + return mSeenEnd; + } + } +} diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/SharedPrefsDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SharedPrefsDetector.java index d38b05e..1f432f7 100644 --- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/SharedPrefsDetector.java +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SharedPrefsDetector.java @@ -81,22 +81,6 @@ public class SharedPrefsDetector extends Detector implements Detector.JavaScanne } @Nullable - private static Node findSurroundingMethod(Node scope) { - while (scope != null) { - Class<? extends Node> type = scope.getClass(); - // The Lombok AST uses a flat hierarchy of node type implementation classes - // so no need to do instanceof stuff here. - if (type == MethodDeclaration.class || type == ConstructorDeclaration.class) { - return scope; - } - - scope = scope.getParent(); - } - - return null; - } - - @Nullable private static NormalTypeBody findSurroundingTypeBody(Node scope) { while (scope != null) { Class<? extends Node> type = scope.getClass(); @@ -156,7 +140,7 @@ public class SharedPrefsDetector extends Detector implements Detector.JavaScanne allowCommitBeforeTarget = false; } - Node method = findSurroundingMethod(parent); + Node method = JavaContext.findSurroundingMethod(parent); if (method == null) { return; } diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ToastDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ToastDetector.java index 52ae8ed..e8f8d6e 100644 --- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ToastDetector.java +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ToastDetector.java @@ -31,11 +31,9 @@ import java.util.Collections; import java.util.List; import lombok.ast.AstVisitor; -import lombok.ast.ConstructorDeclaration; import lombok.ast.Expression; import lombok.ast.ForwardingAstVisitor; import lombok.ast.IntegralLiteral; -import lombok.ast.MethodDeclaration; import lombok.ast.MethodInvocation; import lombok.ast.Node; import lombok.ast.Return; @@ -75,21 +73,6 @@ public class ToastDetector extends Detector implements Detector.JavaScanner { return Collections.singletonList("makeText"); //$NON-NLS-1$ } - private Node findSurroundingMethod(Node scope) { - while (scope != null) { - Class<? extends Node> type = scope.getClass(); - // The Lombok AST uses a flat hierarchy of node type implementation classes - // so no need to do instanceof stuff here. - if (type == MethodDeclaration.class || type == ConstructorDeclaration.class) { - return scope; - } - - scope = scope.getParent(); - } - - return null; - } - @Override public void visitMethod(@NonNull JavaContext context, @Nullable AstVisitor visitor, @NonNull MethodInvocation node) { @@ -118,7 +101,7 @@ public class ToastDetector extends Detector implements Detector.JavaScanner { } } - Node method = findSurroundingMethod(node.getParent()); + Node method = JavaContext.findSurroundingMethod(node.getParent()); if (method == null) { return; } diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/CutPasteDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/CutPasteDetectorTest.java new file mode 100644 index 0000000..df02a41 --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/CutPasteDetectorTest.java @@ -0,0 +1,55 @@ +/* + * 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 CutPasteDetectorTest extends AbstractCheckTest { + @Override + protected Detector getDetector() { + return new CutPasteDetector(); + } + + public void test() throws Exception { + assertEquals( + "src/test/pkg/PasteError.java:15: Warning: The id R.id.textView1 has already been looked up in this method; possible cut & paste error? [CutPasteId]\n" + + " View view2 = findViewById(R.id.textView1);\n" + + " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " src/test/pkg/PasteError.java:14: First usage here\n" + + "src/test/pkg/PasteError.java:71: Warning: The id R.id.textView1 has already been looked up in this method; possible cut & paste error? [CutPasteId]\n" + + " view2 = findViewById(R.id.textView1);\n" + + " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " src/test/pkg/PasteError.java:68: First usage here\n" + + "src/test/pkg/PasteError.java:78: Warning: The id R.id.textView1 has already been looked up in this method; possible cut & paste error? [CutPasteId]\n" + + " view2 = findViewById(R.id.textView1);\n" + + " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " src/test/pkg/PasteError.java:76: First usage here\n" + + "src/test/pkg/PasteError.java:86: Warning: The id R.id.textView1 has already been looked up in this method; possible cut & paste error? [CutPasteId]\n" + + " view2 = findViewById(R.id.textView1);\n" + + " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " src/test/pkg/PasteError.java:83: First usage here\n" + + "src/test/pkg/PasteError.java:95: Warning: The id R.id.textView1 has already been looked up in this method; possible cut & paste error? [CutPasteId]\n" + + " view2 = findViewById(R.id.textView1);\n" + + " ~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" + + " src/test/pkg/PasteError.java:91: First usage here\n" + + "0 errors, 5 warnings\n", + + lintProject("src/test/pkg/PasteError.java.txt=>" + + "src/test/pkg/PasteError.java")); + } +} diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SharedPrefsDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SharedPrefsDetectorTest.java index dbfc838..7035c21 100644 --- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SharedPrefsDetectorTest.java +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SharedPrefsDetectorTest.java @@ -19,7 +19,7 @@ package com.android.tools.lint.checks; import com.android.tools.lint.detector.api.Detector; @SuppressWarnings("javadoc") -public class SharedPrefsDetectorTest extends AbstractCheckTest { +public class SharedPrefsDetectorTest extends AbstractCheckTest { @Override protected Detector getDetector() { return new SharedPrefsDetector(); diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/PasteError.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/PasteError.java.txt new file mode 100644 index 0000000..b12b56b --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/src/test/pkg/PasteError.java.txt @@ -0,0 +1,109 @@ +package test.pkg; + +import android.app.Activity; +import android.view.View; + +public class PasteError extends Activity { + protected void ok() { + Button button1 = (Button) findViewById(R.id.textView1); + mView2 = findViewById(R.id.textView2); + View view3 = findViewById(R.id.activity_main); + } + + protected void error() { + View view1 = findViewById(R.id.textView1); + View view2 = findViewById(R.id.textView1); + View view3 = findViewById(R.id.textView2); + } + + protected void ok2() { + View view1; + if (true) { + view1 = findViewById(R.id.textView1); + } else { + view1 = findViewById(R.id.textView1); + } + } + + @SuppressLint("CutPasteId") + protected void suppressed() { + View view1 = findViewById(R.id.textView1); + View view2 = findViewById(R.id.textView1); + } + + private void ok3() { + if (view == null || view.findViewById(R.id.city_name) == null) { + view = mInflater.inflate(R.layout.city_list_item, parent, false); + } + TextView name = (TextView) view.findViewById(R.id.city_name); + } + + private void ok4() { + mPrevAlbumWrapper = mPrevTrackLayout.findViewById(R.id.album_wrapper); + mNextAlbumWrapper = mNextTrackLayout.findViewById(R.id.album_wrapper); + } + + @Override + public View getView(int position, View convertView, ViewGroup parent) { + View listItem = convertView; + if (getItemViewType(position) == VIEW_TYPE_HEADER) { + TextView header = (TextView) listItem.findViewById(R.id.name); + } else if (getItemViewType(position) == VIEW_TYPE_BOOLEAN) { + TextView filterName = (TextView) listItem.findViewById(R.id.name); + } else { + TextView filterName = (TextView) listItem.findViewById(R.id.name); + } + } + + protected void ok_branch_1() { + if (true) { + view1 = findViewById(R.id.textView1); + } else { + view2 = findViewById(R.id.textView1); + } + } + + protected void error_branch_1() { + if (true) { + view1 = findViewById(R.id.textView1); + } + if (true) { + view2 = findViewById(R.id.textView1); + } + } + + protected void error_branch_2() { + view1 = findViewById(R.id.textView1); + if (true) { + view2 = findViewById(R.id.textView1); + } + } + + protected void error_branch_3() { + view1 = findViewById(R.id.textView1); + if (true) { + } else { + view2 = findViewById(R.id.textView1); + } + } + + protected void error_branch_4() { + view1 = findViewById(R.id.textView1); + if (true) { + } else { + if (true) { + view2 = findViewById(R.id.textView1); + } + } + } + + protected void ok_branch_2() { + if (true) { + view1 = findViewById(R.id.textView1); + } else { + if (true) { + view2 = findViewById(R.id.textView1); + } + } + } +} |