Merge pull request WorksApplications#23 from KengoTODA/visible-for-te…

…sting

Introducing 2 new detectors which checks annotation

README.md

@@ -29,6 +29,8 @@ To use this product, please configure your findbugs-maven-plugin like below.
 
 ## 0.0.3
 
+- added UnexpectedAccessDetector
+- added UndocumentedSuppressFBWarningsDetector
 - upgraded JDK from 1.6 to 1.7
 
 ## 0.0.2

src/main/java/jp/co/worksap/oss/findbugs/findbugs/UndocumentedSuppressFBWarningsDetector.java

@@ -0,0 +1,57 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+
+import javax.annotation.Nonnull;
+
+import org.apache.bcel.classfile.ElementValue;
+
+import com.google.common.collect.Sets;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.BytecodeScanningDetector;
+import edu.umd.cs.findbugs.internalAnnotations.DottedClassName;
+
+/**
+ * <p>A detector to ensure that FindBugs&apos; SuppressWarnings annotation has justification.</p>
+ * @see edu.umd.cs.findbugs.annotations.SuppressWarnings
+ * @see edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+ * @author Kengo TODA ([email protected])
+ */
+public class UndocumentedSuppressFBWarningsDetector extends BytecodeScanningDetector {
+    private static final Set<String> TARGET_ANNOTATIONS = Collections.unmodifiableSet(Sets.newHashSet(
+            "edu.umd.cs.findbugs.annotations.SuppressWarnings",
+            "edu.umd.cs.findbugs.annotations.SuppressFBWarnings"
+    ));
+
+    @Nonnull
+    private final BugReporter bugReporter;
+
+    public UndocumentedSuppressFBWarningsDetector(BugReporter bugReporter) {
+        this.bugReporter = checkNotNull(bugReporter);
+    }
+
+    @Override
+    public void visitAnnotation(@DottedClassName String annotationClass,
+            Map<String, ElementValue> map, boolean runtimeVisible) {
+        if (! TARGET_ANNOTATIONS.contains(annotationClass)) {
+            return;
+        }
+
+        final ElementValue reason = map.get("justification");
+        if (reason == null || reason.stringifyValue().trim().isEmpty()) {
+            BugInstance bugInstance = new BugInstance("FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS",
+                    HIGH_PRIORITY).addClass(this);
+            if (visitingMethod()) {
+                bugInstance.addMethod(this).addSourceLine(this);
+            }
+            bugReporter.reportBug(bugInstance);
+        }
+    }
+
+}

src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java

@@ -0,0 +1,110 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import javax.annotation.Nonnull;
+
+import org.apache.bcel.Repository;
+import org.apache.bcel.classfile.AnnotationEntry;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.BytecodeScanningDetector;
+import edu.umd.cs.findbugs.bcel.BCELUtil;
+import edu.umd.cs.findbugs.classfile.ClassDescriptor;
+import edu.umd.cs.findbugs.classfile.MethodDescriptor;
+
+/**
+ * <p>A detector to ensure that implementation (class in src/main/java) doesn't call
+ * package-private method in other class which is annotated by {@code @VisibleForTesting}.</p>
+ *
+ * @author Kengo TODA ([email protected])
+ * @see com.google.common.annotations.VisibleForTesting
+ */
+public class UnexpectedAccessDetector extends BytecodeScanningDetector {
+    @Nonnull
+    private final BugReporter bugReporter;
+
+    public UnexpectedAccessDetector(BugReporter bugReporter) {
+        this.bugReporter = checkNotNull(bugReporter);
+    }
+
+    @Override
+    public void sawOpcode(int opcode) {
+        if (! isInvoking(opcode)) {
+            return;
+        }
+
+        ClassDescriptor currentClass = getClassDescriptor();
+        ClassDescriptor invokedClass = getClassDescriptorOperand();
+        if (currentClass.equals(invokedClass)) {
+            // no need to check, because method is called by owner
+        } else if (! currentClass.getPackageName().equals(invokedClass.getPackageName())) {
+            // no need to check, because method is called by class in other package
+        } else {
+            MethodDescriptor invokedMethod = getMethodDescriptorOperand();
+
+            try {
+                verifyVisibility(invokedClass, invokedMethod);
+            } catch (ClassNotFoundException e) {
+                String message = String.format("Detector could not find %s, you should add this class into CLASSPATH", invokedClass.getDottedClassName());
+                bugReporter.logError(message, e);
+            }
+        }
+    }
+
+    /**
+     * <p>Report if specified method is package-private and annotated by {@code @VisibleForTesting}.</p>
+     */
+    private void verifyVisibility(ClassDescriptor invokedClass, MethodDescriptor invokedMethod) throws ClassNotFoundException {
+        JavaClass bcelClass = Repository.getRepository().loadClass(invokedClass.getDottedClassName());
+        Method bcelMethod = findMethod(bcelClass, invokedMethod);
+
+        if (checkVisibility(bcelMethod) && checkAnnotated(bcelMethod)) {
+            BugInstance bug = new BugInstance(this, "GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING", HIGH_PRIORITY)
+                    .addCalledMethod(this).addClassAndMethod(this).addSourceLine(this);
+            bugReporter.reportBug(bug);
+        }
+    }
+
+    private Method findMethod(JavaClass bcelClass, MethodDescriptor invokedMethod) {
+        for (Method bcelMethod : bcelClass.getMethods()) {
+            MethodDescriptor methodDescriptor = BCELUtil.getMethodDescriptor(bcelClass, bcelMethod);
+            if (methodDescriptor.equals(invokedMethod)) {
+                return bcelMethod;
+            }
+        }
+
+        throw new IllegalArgumentException("Method not found: " + invokedMethod);
+    }
+
+    /**
+     * @return true if visibility of specified method is package-private.
+     */
+    private boolean checkVisibility(Method bcelMethod) {
+        return ! (bcelMethod.isPrivate() || bcelMethod.isProtected() || bcelMethod.isPublic());
+    }
+
+    /**
+     * @return true if specified method is annotated by {@code VisibleForTesting}.
+     */
+    private boolean checkAnnotated(Method bcelMethod) {
+        for (AnnotationEntry annotation : bcelMethod.getAnnotationEntries()) {
+            String type = annotation.getAnnotationType();
+            if ("Lcom/google/common/annotations/VisibleForTesting;".equals(type)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private boolean isInvoking(int opcode) {
+        return opcode == INVOKESPECIAL ||
+                opcode == INVOKEINTERFACE ||
+                opcode == INVOKESTATIC ||
+                opcode == INVOKEVIRTUAL;
+    }
+
+}

src/main/meta/bugrank.txt

@@ -12,3 +12,5 @@
 0 BugPattern ILLEGAL_LENGTH
 0 BugPattern USE_COLUMN_DEFINITION
 0 BugPattern NULLABLE_PRIMITIVE
+0 BugPattern GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING
+0 BugPattern FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS

src/main/meta/findbugs.xml

@@ -64,4 +64,14 @@
   <BugPattern type="UNDOCUMENTED_IGNORE" abbrev="JUNIT"
     category="BAD_PRACTICE" />
 
+  <Detector class="jp.co.worksap.oss.findbugs.guava.UnexpectedAccessDetector"
+    speed="fast" hidden="false" reports="GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING" />
+  <BugPattern type="GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING" abbrev="GUAVA"
+    category="CORRECTNESS" />
+
+  <Detector class="jp.co.worksap.oss.findbugs.findbugs.UndocumentedSuppressFBWarningsDetector"
+    speed="fast" hidden="false" reports="FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS" />
+  <BugPattern type="FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS" abbrev="FINDBUGS"
+    category="BAD_PRACTICE" />
+
 </FindbugsPlugin>

src/main/meta/messages.xml

@@ -264,8 +264,52 @@
     </Details>
   </BugPattern>
 
+  <Detector class="jp.co.worksap.oss.findbugs.guava.UnexpectedAccessDetector">
+    <Details>
+    </Details>
+  </Detector>
+
+  <BugPattern type="GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING">
+    <ShortDescription>
+      You cannot access to package-private method which is annotated by @VisibleForTesting.
+    </ShortDescription>
+    <LongDescription>
+      You cannot access to package-private method which is annotated by @VisibleForTesting.
+      This annotation means that visibility was widened only for test code, so your implementation code
+      shouldn't access to method which is annotated by this annotation.
+    </LongDescription>
+    <Details>
+  <![CDATA[
+      <p>You cannot access to package-private method which is annotated by @VisibleForTesting.</p>
+      <p>This annotation means that visibility was widened only for test code, so your implementation code
+      shouldn't access to method which is annotated by this annotation.</p>
+  ]]>
+    </Details>
+  </BugPattern>
+
+  <Detector class="jp.co.worksap.oss.findbugs.findbugs.UndocumentedSuppressFBWarningsDetector">
+    <Details>
+      This detector will find suppressed FindBugs warning which has no justification about why it is suppressed.
+    </Details>
+  </Detector>
+
+  <BugPattern type="FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS">
+    <ShortDescription>To tell why this FindBugs warning is suppressed, put justification as a parameter.
+    </ShortDescription>
+    <LongDescription>
+      To tell why this FindBugs warning is suppressed, put justification as a parameter.
+    </LongDescription>
+    <Details>
+  <![CDATA[
+    <p>To tell why this FindBugs warning is suppressed, put justification as a parameter.</p>
+  ]]>
+    </Details>
+  </BugPattern>
+
   <BugCode abbrev="SYS">SYS</BugCode>
   <BugCode abbrev="JSR305">JSR305</BugCode>
   <BugCode abbrev="JPA">JPA</BugCode>
   <BugCode addrev="JUNIT">JUnit</BugCode>
+  <BugCode addrev="GUAVA">Google Guava</BugCode>
+  <BugCode addrev="FINDBUGS">FindBugs</BugCode>
 </MessageCollection>

src/test/java/jp/co/worksap/oss/findbugs/findbugs/DocumentedSuppressFBWarnings.java

@@ -0,0 +1,9 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class DocumentedSuppressFBWarnings {
+    @SuppressFBWarnings(justification = "only for unit test.")
+    public void method() {
+    }
+}

src/test/java/jp/co/worksap/oss/findbugs/findbugs/DocumentedSuppressWarnings.java

@@ -0,0 +1,8 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import edu.umd.cs.findbugs.annotations.SuppressWarnings;
+
+@SuppressWarnings(justification = "only for unit test.")
+public class DocumentedSuppressWarnings {
+
+}

src/test/java/jp/co/worksap/oss/findbugs/findbugs/UndocumentedSuppressFBWarnings.java

@@ -0,0 +1,9 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class UndocumentedSuppressFBWarnings {
+    @SuppressFBWarnings
+    public void method() {
+    }
+}

src/test/java/jp/co/worksap/oss/findbugs/findbugs/UndocumentedSuppressFBWarningsDetectorTest.java

@@ -0,0 +1,33 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import static com.youdevise.fbplugins.tdd4fb.DetectorAssert.*;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.BugReporter;
+
+public class UndocumentedSuppressFBWarningsDetectorTest {
+
+    private UndocumentedSuppressFBWarningsDetector detector;
+    private BugReporter bugReporter;
+
+    @Before
+    public void setup() {
+        bugReporter = bugReporterForTesting();
+        detector = new UndocumentedSuppressFBWarningsDetector(bugReporter);
+    }
+
+    @Test
+    public void testDocumentedClasses() throws Exception {
+        assertNoBugsReported(DocumentedSuppressWarnings.class, detector, bugReporter);
+        assertNoBugsReported(DocumentedSuppressFBWarnings.class, detector, bugReporter);
+    }
+
+    @Test
+    public void testUndocumentedClasses() throws Exception {
+        assertBugReported(UndocumentedSuppressWarnings.class, detector, bugReporter, ofType("FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS"));
+        assertBugReported(UndocumentedSuppressFBWarnings.class, detector, bugReporter, ofType("FINDBUGS_UNDOCUMENTED_SUPPRESS_WARNINGS"));
+    }
+
+}

src/test/java/jp/co/worksap/oss/findbugs/findbugs/UndocumentedSuppressWarnings.java

@@ -0,0 +1,8 @@
+package jp.co.worksap.oss.findbugs.findbugs;
+
+import edu.umd.cs.findbugs.annotations.SuppressWarnings;
+
+@SuppressWarnings
+public class UndocumentedSuppressWarnings {
+
+}

src/test/java/jp/co/worksap/oss/findbugs/guava/ClassWhichCallsNormalMethod.java

@@ -0,0 +1,12 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+public class ClassWhichCallsNormalMethod {
+    public void method() {
+        new MethodWithoutVisibleForTesting().method();
+    }
+
+    public void anotherMethod() {
+        System.out.println("this method invoking has no problem, because package isn't same.");
+        method();
+    }
+}

src/test/java/jp/co/worksap/oss/findbugs/guava/ClassWhichCallsVisibleMethodForTesting.java

@@ -0,0 +1,7 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+public class ClassWhichCallsVisibleMethodForTesting {
+    public void method() {
+        new MethodWithVisibleForTesting().method();
+    }
+}

src/test/java/jp/co/worksap/oss/findbugs/guava/MethodWithVisibleForTesting.java

@@ -0,0 +1,8 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+import com.google.common.annotations.VisibleForTesting;
+
+public class MethodWithVisibleForTesting {
+    @VisibleForTesting
+    void method() {}
+}

src/test/java/jp/co/worksap/oss/findbugs/guava/MethodWithoutVisibleForTesting.java

@@ -0,0 +1,5 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+public class MethodWithoutVisibleForTesting {
+    void method() {}
+}

src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java

@@ -0,0 +1,34 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+import static com.youdevise.fbplugins.tdd4fb.DetectorAssert.assertBugReported;
+import static com.youdevise.fbplugins.tdd4fb.DetectorAssert.assertNoBugsReported;
+import static com.youdevise.fbplugins.tdd4fb.DetectorAssert.bugReporterForTesting;
+import static com.youdevise.fbplugins.tdd4fb.DetectorAssert.ofType;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.BugReporter;
+
+public class UnexpectedAccessDetectorTest {
+
+    private UnexpectedAccessDetector detector;
+    private BugReporter bugReporter;
+
+    @Before
+    public void setup() {
+        bugReporter = bugReporterForTesting();
+        detector = new UnexpectedAccessDetector(bugReporter);
+    }
+
+    @Test
+    public void testNormalMethod() throws Exception {
+        assertNoBugsReported(ClassWhichCallsNormalMethod.class, detector, bugReporter);
+    }
+
+    @Test
+    public void testCallingAnnotatedMethod() throws Exception {
+        assertBugReported(ClassWhichCallsVisibleMethodForTesting.class, detector, bugReporter, ofType("GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING"));
+    }
+
+}