Implemented a detector which allows the annotation @VisibleForTesting …

…only on package-private methods and fields to avoid misuse.

pom.xml

@@ -176,6 +176,11 @@
 			<groupId>junit</groupId>
 			<artifactId>junit</artifactId>
 		</dependency>
+		<dependency>
+			<groupId>junit-addons</groupId>
+			<artifactId>junit-addons</artifactId>
+			<version>1.4</version>
+		</dependency>
 		<dependency>
 			<groupId>org.mockito</groupId>
 			<artifactId>mockito-all</artifactId>

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

@@ -0,0 +1,88 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+import java.util.Arrays;
+
+import javax.annotation.Nonnull;
+
+import org.apache.bcel.classfile.AnnotationEntry;
+import org.apache.bcel.classfile.Field;
+import org.apache.bcel.classfile.FieldOrMethod;
+import org.apache.bcel.classfile.Method;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Predicate;
+import com.google.common.base.Supplier;
+import com.google.common.collect.Iterables;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.BytecodeScanningDetector;
+
+/**
+ * <p>
+ * Detector to check the corect usage of <code>@VisibleForTesting</code>.
+ * </p>
+ * 
+ * @author tolina GmbH
+ */
+public class MisuseOfVisibleForTestingDetector extends BytecodeScanningDetector {
+
+	/** Create the BugInstance by Supplier for testability */
+	@VisibleForTesting
+	Supplier<BugInstance> bugInstanceSupplier = new Supplier<BugInstance>() {
+		@Override
+		public BugInstance get() {
+			return  new BugInstance(MisuseOfVisibleForTestingDetector.this, GUAVA_MISUSE_OF_VISIBLE_FOR_TESTING, HIGH_PRIORITY);
+		}
+	};
+
+	private static final String VISIBLE_FOR_TESTING_ANNOTATION_PATTERN = "Lcom/google/common/annotations/VisibleForTesting;";
+	private static final String GUAVA_MISUSE_OF_VISIBLE_FOR_TESTING = "GUAVA_MISUSE_OF_VISIBLE_FOR_TESTING";
+
+	private final BugReporter reporter;
+
+	private final static Predicate<AnnotationEntry> hasVisibleForTesting = new Predicate<AnnotationEntry>() {
+		@Override
+		public boolean apply(final AnnotationEntry input) {
+			return VISIBLE_FOR_TESTING_ANNOTATION_PATTERN.equals(input.getAnnotationType());
+		}
+	};
+
+	public MisuseOfVisibleForTestingDetector(final BugReporter reporter) {
+		this.reporter = reporter;
+	}
+
+	@Override
+	public void visit(final Field obj) {
+		if (hasVisibleForTestingAnnotation(obj) && !isPackageVisible(obj)) {
+			final BugInstance bug = bugInstanceSupplier.get();
+			bug.addClass(this);
+			bug.addField(this);
+			reporter.reportBug(bug);
+		}
+	}
+
+	/**
+	 * @param obj
+	 * @return <code>true</code> if {@link FieldOrMethod} is annotated with {@link VisibleForTesting}
+	 */
+	private boolean hasVisibleForTestingAnnotation(final FieldOrMethod obj) {
+		return Iterables.tryFind(Arrays.asList(obj.getAnnotationEntries()), hasVisibleForTesting).isPresent();
+	}
+
+	@Override
+	public void visit(final Method obj) {
+		if (hasVisibleForTestingAnnotation(obj) && !isPackageVisible(obj)) {
+			final BugInstance bug = bugInstanceSupplier.get();
+			bug.addClassAndMethod(this); //.addSourceLine(this);
+			reporter.reportBug(bug);
+		}
+	}
+
+	/**
+	 * @return <code>true</code> if visibility of specified {@link FieldOrMethod} is package-private.
+	 */
+	private boolean isPackageVisible(final @Nonnull FieldOrMethod entry) {
+		return !(entry.isPrivate() || entry.isProtected() || entry.isPublic());
+	}
+}

src/main/meta/findbugs.xml

@@ -69,6 +69,11 @@
   <BugPattern type="GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING" abbrev="GUAVA"
     category="CORRECTNESS" />
 
+  <Detector class="jp.co.worksap.oss.findbugs.guava.MisuseOfVisibleForTestingDetector"
+    speed="fast" hidden="false" reports="GUAVA_MISUSE_OF_VISIBLE_FOR_TESTING" />
+  <BugPattern type="GUAVA_MISUSE_OF_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"

src/main/meta/messages.xml

@@ -287,6 +287,27 @@
     </Details>
   </BugPattern>
 
+  <Detector class="jp.co.worksap.oss.findbugs.guava.MisuseOfVisibleForTestingDetector">
+    <Details>
+    </Details>
+  </Detector>
+
+  <BugPattern type="GUAVA_MISUSE_OF_VISIBLE_FOR_TESTING">
+    <ShortDescription>
+      You must not use @VisibleForTesting for non-package-private methods or fields.
+    </ShortDescription>
+    <LongDescription>
+      You must not annotate fields wit public, protected or private visibility with @VisibleForTesting,
+      because this is not necessary.
+    </LongDescription>
+    <Details>
+  <![CDATA[
+      <p>You must not annotate fields wit public, protected or private visibility with @VisibleForTesting,
+         because this is not necessary.</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.

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

@@ -0,0 +1,158 @@
+package jp.co.worksap.oss.findbugs.guava;
+
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import junitx.util.PrivateAccessor;
+
+import org.apache.bcel.classfile.ClassParser;
+import org.apache.bcel.classfile.Field;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+import org.hamcrest.BaseMatcher;
+import org.hamcrest.Description;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Matchers;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.mockito.stubbing.Answer;
+
+import com.google.common.base.Supplier;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.visitclass.PreorderVisitor;
+
+
+public class MisuseOfVisibleForTestingDetectorTest {
+
+	@Mock
+	private BugReporter reporter;
+	@Mock
+	private Supplier<BugInstance> bugInstanceSupplier;
+
+	private MisuseOfVisibleForTestingDetector detector;
+
+	@SuppressWarnings("javadoc")
+	@Before
+	public void before() {
+		MockitoAnnotations.initMocks(this);
+		detector = new MisuseOfVisibleForTestingDetector(reporter);
+		detector.bugInstanceSupplier = bugInstanceSupplier;
+	}
+
+	/**
+	 * Tests the detector with most combinations of visibility, static etc.
+	 * @see MisuseOfVisibleForTestingDetectorTestObject
+	 * @throws Exception
+	 */
+	@Test
+	public void test() throws Exception {
+
+		// get a suitable BCEL-class
+		final ClassParser parser = new ClassParser(getClass().getResourceAsStream("/jp/co/worksap/oss/findbugs/guava/MisuseOfVisibleForTestingDetectorTestObject.class"), "MisuseOfVisibleForTestingDetectorTestObject");
+		final JavaClass javaClass = parser.parse();
+		// we want our own BugInstance-Stubs
+		when(bugInstanceSupplier.get()).thenAnswer(new Answer<BugInstanceStub>() {
+			public BugInstanceStub answer(org.mockito.invocation.InvocationOnMock invocation) throws Throwable {
+				return new BugInstanceStub();
+			};
+		});
+
+		// visit all fields
+		for(final Field field : javaClass.getFields()) {
+			// tell findbugs we're visiting a field
+			PrivateAccessor.setField(detector, "visitingField", Boolean.TRUE);
+			PrivateAccessor.setField(detector, "fieldName", field.getName());
+			detector.visit(field);
+			PrivateAccessor.setField(detector, "visitingField", Boolean.FALSE);
+			PrivateAccessor.setField(detector, "fieldName", null);
+		}
+		// visit all methods
+		for(final Method method : javaClass.getMethods()) {
+			// tell findbugs we're visiting a method
+			PrivateAccessor.setField(detector, "visitingMethod", Boolean.TRUE);
+			PrivateAccessor.setField(detector, "methodName", method.getName());
+			PrivateAccessor.setField(detector, "method", method);
+			detector.visit(method);
+			PrivateAccessor.setField(detector, "visitingMethod", Boolean.FALSE);
+			PrivateAccessor.setField(detector, "methodName", null);
+			PrivateAccessor.setField(detector, "method", null);
+		}
+
+		// 12 calls for fields
+		verify(reporter, times(12)).reportBug(Matchers.argThat(new BugInstanceMatcher(1,1,0)));
+		// 6 calls for Methods
+		verify(reporter, times(6)).reportBug(Matchers.argThat(new BugInstanceMatcher(1,0,1)));
+
+	}
+
+	// BugInstances are considered equal if # of method calls are equal 
+	private class BugInstanceMatcher extends BaseMatcher<BugInstance> {
+
+		private int addMethod;
+		private int addField;
+		private int addClass;
+
+		public BugInstanceMatcher(final int addClass, final int addField, final int addMethod) {
+			this.addClass = addClass;
+			this.addField = addField;
+			this.addMethod = addMethod;
+		}
+
+		@Override
+		public void describeTo(Description value) {
+			value.appendText(String.format("Calls %s, %s, %s", Integer.valueOf(addClass), Integer.valueOf(addField), Integer.valueOf(addMethod)));
+		}
+
+		@Override
+		public boolean matches(Object value) {
+			BugInstanceStub instance = (BugInstanceStub) value;
+			return instance.addClass==addClass && //
+					instance.addField==addField && //
+					instance.addMethod==addMethod;
+		}
+
+
+	}
+
+	// count used method calls instead of using real BugInstance with all static shit
+	private class BugInstanceStub extends BugInstance {
+		private static final long serialVersionUID = 1L;
+
+		int addClass;
+		int addField;
+		int addMethod;
+
+		public BugInstanceStub() {
+			super("MS_SHOULD_BE_FINAL", 0);
+		}
+
+		@Override
+		public BugInstance addField(PreorderVisitor visitor) {
+			addField++;
+			return this;
+		}
+
+		@Override
+		public BugInstance addClass(PreorderVisitor jclass) {
+			addClass++;
+			return this;
+		}
+
+		@Override
+		public BugInstance addClassAndMethod(PreorderVisitor visitor) {
+			addClass++;
+			addMethod++;
+			return this;
+		}
+
+		@Override
+		public String toString() {
+			return String.format("Calls %s, %s, %s", Integer.valueOf(addClass), Integer.valueOf(addField), Integer.valueOf(addMethod));
+		}
+
+	}
+
+}