Fix handling of @SuppressLint on fields

When you have a field initialization like this:

    private int foo = new ForbiddenClass();

the actual code to perform the initialization lives in a method named
<init> (or for a static field, <clinit>).

If you tried to suppress lint errors here by adding an annotation on
the field, it would not be found by lint, since lint looks at method
level annotations and it's sitting on the field node instead.

To fix this, the suppress check needs to identify the field scenario,
and in that case (when checking whether an error is suppressed) look
up the field in the class and check annotations there.

There was a second bug: The lint check which looks for invalid
suppress annotation sites (since they're not allowed on local
variables for class based lint checks) incorrectly concluded that
these types of initializations were local variables rather than field
initializations.

This fixes issue
38626: ADT: Misleading lint rule: SimpleDateFormat

Change-Id: I254f3fb5a6132d6cbe39bd425ffe6d67ed7b84ed

2 files changed, 58 insertions, 10 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 7f6c0ee..fc9487d 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
@@ -60,8 +60,10 @@ import com.google.common.io.Closeables;
 
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.tree.AbstractInsnNode;
 import org.objectweb.asm.tree.AnnotationNode;
 import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.FieldInsnNode;
 import org.objectweb.asm.tree.FieldNode;
 import org.objectweb.asm.tree.MethodNode;
 import org.w3c.dom.Attr;
@@ -1800,27 +1802,62 @@ public class LintDriver {
     // pointers, so we have to have multiple methods which pass in each type
     // of node (class, method, field) to be checked.
 
-    // TODO: The Quickfix should look for lint warnings placed *inside* warnings
-    // and warn that they won't apply to checks that are bytecode oriented!
-
     /**
      * Returns whether the given issue is suppressed in the given method.
      *
      * @param issue the issue to be checked, or null to just check for "all"
+     * @param classNode the class containing the issue
      * @param method the method containing the issue
+     * @param instruction the instruction within the method, if any
      * @return true if there is a suppress annotation covering the specific
      *         issue on this method
      */
-    public boolean isSuppressed(@Nullable Issue issue, @NonNull MethodNode method) {
+    public boolean isSuppressed(
+            @Nullable Issue issue,
+            @NonNull ClassNode classNode,
+            @NonNull MethodNode method,
+            @Nullable AbstractInsnNode instruction) {
         if (method.invisibleAnnotations != null) {
             @SuppressWarnings("unchecked")
             List<AnnotationNode> annotations = method.invisibleAnnotations;
             return isSuppressed(issue, annotations);
         }
 
+        // Initializations of fields end up placed in generated methods (<init>
+        // for members and <clinit> for static fields).
+        if (instruction != null && method.name.charAt(0) == '<') {
+            AbstractInsnNode next = LintUtils.getNextInstruction(instruction);
+            if (next != null && next.getType() == AbstractInsnNode.FIELD_INSN) {
+                FieldInsnNode fieldRef = (FieldInsnNode) next;
+                FieldNode field = findField(classNode, fieldRef.owner, fieldRef.name);
+                if (field != null && isSuppressed(issue, field)) {
+                    return true;
+                }
+            }
+        }
+
         return false;
     }
 
+    @Nullable
+    private FieldNode findField(ClassNode classNode, String owner, String name) {
+        while (classNode != null) {
+            if (owner.equals(classNode.name)) {
+                @SuppressWarnings("rawtypes") // ASM API
+                List fieldList = classNode.fields;
+                for (Object f : fieldList) {
+                    FieldNode field = (FieldNode) f;
+                    if (field.name.equals(name)) {
+                        return field;
+                    }
+                }
+                return null;
+            }
+            classNode = getOuterClassNode(classNode);
+        }
+        return null;
+    }
+
     /**
      * Returns whether the given issue is suppressed for the given field.
      *
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 68c25d9..159180d 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
@@ -280,7 +280,7 @@ public class ClassContext extends Context {
      * Detectors should only call this method if an error applies to the whole class
      * scope and there is no specific method or field that applies to the error.
      * If so, use
-     * {@link #report(Issue, MethodNode, Location, String, Object)} or
+     * {@link #report(Issue, MethodNode, AbstractInsnNode, Location, String, Object)} or
      * {@link #report(Issue, FieldNode, Location, String, Object)}, such that
      * suppress annotations are checked.
      *
@@ -313,7 +313,8 @@ public class ClassContext extends Context {
                             // Found the outer method for this anonymous class; continue
                             // reporting on it (which will also work its way up the parent
                             // class hierarchy)
-                            if (method != null && mDriver.isSuppressed(issue, method)) {
+                            if (method != null && mDriver.isSuppressed(issue, mClassNode, method,
+                                    null)) {
                                 return;
                             }
                             break;
@@ -337,9 +338,18 @@ public class ClassContext extends Context {
      * Reports an issue applicable to a given method node.
      *
      * @param issue the issue to report
-     * @param method the method scope the error applies to. The lint infrastructure
-     *    will check whether there are suppress annotations on this method (or its enclosing
-     *    class) and if so suppress the warning without involving the client.
+     * @param method the method scope the error applies to. The lint
+     *            infrastructure will check whether there are suppress
+     *            annotations on this method (or its enclosing class) and if so
+     *            suppress the warning without involving the client.
+     * @param instruction the instruction within the method the error applies
+     *            to. You cannot place annotations on individual method
+     *            instructions (for example, annotations on local variables are
+     *            allowed, but are not kept in the .class file). However, this
+     *            instruction is needed to handle suppressing errors on field
+     *            initializations; in that case, the errors may be reported in
+     *            the {@code <clinit>} method, but the annotation is found not
+     *            on that method but for the {@link FieldNode}'s.
      * @param location the location of the issue, or null if not known
      * @param message the message for this warning
      * @param data any associated data, or null
@@ -347,10 +357,11 @@ public class ClassContext extends Context {
     public void report(
             @NonNull Issue issue,
             @Nullable MethodNode method,
+            @Nullable AbstractInsnNode instruction,
             @Nullable Location location,
             @NonNull String message,
             @Nullable Object data) {
-        if (method != null && mDriver.isSuppressed(issue, method)) {
+        if (method != null && mDriver.isSuppressed(issue, mClassNode, method, instruction)) {
             return;
         }
         report(issue, location, message, data); // also checks the class node