From 943caae4c0c83d87f53e73566b0e0f2349eb52b4 Mon Sep 17 00:00:00 2001 From: Sebastian Eckey Date: Tue, 3 Feb 2015 15:37:06 +0100 Subject: [PATCH] @VisibleForTesting is now detected on non static fields, too. Therefore the plugin is no longer working on bcel methods but on findbugs xMethods and xFields. Using xMethods and xFields the performance of the plugin increased. --- .../guava/UnexpectedAccessDetector.java | 129 +++++++------ src/main/meta/messages.xml | 6 +- .../guava/UnexpectedAccessDetectorTest.java | 170 ++++++++++-------- 3 files changed, 167 insertions(+), 138 deletions(-) diff --git a/src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java b/src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java index f3668e0..bfbf3f0 100644 --- a/src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java +++ b/src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java @@ -2,13 +2,10 @@ import static com.google.common.base.Preconditions.checkNotNull; +import java.util.Collection; + import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import org.apache.bcel.Repository; -import org.apache.bcel.classfile.AnnotationEntry; -import org.apache.bcel.classfile.JavaClass; -import org.apache.bcel.classfile.Method; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -17,9 +14,13 @@ 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.ba.AccessibleEntity; +import edu.umd.cs.findbugs.ba.ClassMember; +import edu.umd.cs.findbugs.ba.XFactory; +import edu.umd.cs.findbugs.ba.XField; +import edu.umd.cs.findbugs.ba.XMethod; import edu.umd.cs.findbugs.classfile.ClassDescriptor; -import edu.umd.cs.findbugs.classfile.MethodDescriptor; +import edu.umd.cs.findbugs.classfile.analysis.AnnotationValue; /** *

A detector to ensure that implementation (class in src/main/java) doesn't call @@ -29,7 +30,10 @@ * @see com.google.common.annotations.VisibleForTesting */ public class UnexpectedAccessDetector extends BytecodeScanningDetector { - private static final String VISIBLE_FOR_TESTING_ANNOTATION_TYPE = "Lcom/google/common/annotations/VisibleForTesting;"; + + private static final String GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING = "GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING"; + @VisibleForTesting + static final String VISIBLE_FOR_TESTING_ANNOTATION = "com/google/common/annotations/VisibleForTesting:{}"; @Nonnull private final BugReporter bugReporter; @@ -44,81 +48,84 @@ public UnexpectedAccessDetector(final @Nonnull BugReporter bugReporter) { } @Override - public void sawOpcode(int opcode) { - if (!isInvoking(opcode)) { - return; + public void sawOpcode(final int opcode) { + if (isWorkingOnAField(opcode)) { + final XField invokedField = XFactory.createReferencedXField(this); + final ClassDescriptor currentClass = getClassDescriptor(); + if (hasToCheck(invokedField, currentClass)) { + checkField(invokedField); + } } - - final ClassDescriptor currentClass = getClassDescriptor(); - final 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 { - final MethodDescriptor invokedMethod = getMethodDescriptorOperand(); - try { - verifyVisibility(invokedClass, invokedMethod, true); - } catch (ClassNotFoundException e) { - final String message = String.format("Detector could not find %s, you should add this class into CLASSPATH", invokedClass.getDottedClassName()); - bugReporter.logError(message, e); + if (isInvoking(opcode)) { + final XMethod invokedMethod = XFactory.createReferencedXMethod(this); + final ClassDescriptor currentClass = getClassDescriptor(); + if (hasToCheck(invokedMethod, currentClass)) { + checkMethod(invokedMethod); } } } /** - *

Report if specified method is package-private and annotated by {@code @VisibleForTesting}.

+ * @param member + * @param currentClass + * @return true, if the member is called within the same package */ - @VisibleForTesting - void verifyVisibility(final @Nonnull ClassDescriptor invokedClass, final @Nonnull MethodDescriptor invokedMethod, boolean reportCaller) throws ClassNotFoundException { - final JavaClass bcelClass = Repository.getRepository().loadClass(invokedClass.getDottedClassName()); - final Method bcelMethod = findMethod(bcelClass, invokedMethod); - - if (isIllegalAccesDetected(bcelMethod)) { - final BugInstance bug = new BugInstance(this, "GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING", HIGH_PRIORITY); - if (reportCaller) { - bug.addCalledMethod(this).addClassAndMethod(this).addSourceLine(this); - } - bugReporter.reportBug(bug); + private boolean hasToCheck(final @Nonnull ClassMember member, final @Nonnull ClassDescriptor currentClass) { + if (currentClass.equals(member.getClassDescriptor())) { + log.debug("No check: " + member.getName() + " is called within the same class: " + currentClass.getClassName()); + // no need to check, because method is called by owner + return false; + } else if (!member.getPackageName().equals(currentClass.getPackageName())) { + log.debug("No check: " + member.getName() + " is called from outside the its package. Member package: " // + + member.getPackageName() + ". Caller package: " + currentClass.getPackageName()); + // no need to check, because method is called by class in other package + return false; } + return true; } - private boolean isIllegalAccesDetected(final @Nullable Method bcelMethod) { - return bcelMethod != null && checkVisibility(bcelMethod) && checkAnnotated(bcelMethod); + @VisibleForTesting + void checkMethod(final @Nonnull XMethod invokedMethod) { + if (checkVisibility(invokedMethod)) { + checkAnnotations(invokedMethod.getAnnotations(), true); + } else { + log.debug("No check: Method " + invokedMethod.getName() + " it not package visible."); + } } @VisibleForTesting - Method findMethod(final @Nonnull JavaClass bcelClass, final @Nonnull MethodDescriptor invokedMethod) { - for (Method bcelMethod : bcelClass.getMethods()) { - MethodDescriptor methodDescriptor = BCELUtil.getMethodDescriptor(bcelClass, bcelMethod); - if (methodDescriptor.equals(invokedMethod)) { - return bcelMethod; - } + void checkField(final @Nonnull XField invokedField) { + if (checkVisibility(invokedField)) { + checkAnnotations(invokedField.getAnnotations(), true); + } else { + log.debug("No check: Field " + invokedField.getName() + " it not package visible."); } - log.warn("Method not found: " + invokedMethod); - return null; } /** - * @return true if visibility of specified method is package-private. + * checks a collection of annotations for VisibleForTesting + * @param annotations from method or field + * @param reportCaller should report be added to {@link BugInstance}, for testing */ @VisibleForTesting - boolean checkVisibility(final @Nonnull Method bcelMethod) { - return !(bcelMethod.isPrivate() || bcelMethod.isProtected() || bcelMethod.isPublic()); + void checkAnnotations(final Collection annotations, final boolean reportCaller) { + for (AnnotationValue annotationValue : annotations) { + if (VISIBLE_FOR_TESTING_ANNOTATION.equals(annotationValue.toString())) { + final BugInstance bug = new BugInstance(this, GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING, HIGH_PRIORITY); + if (reportCaller) { + bug.addClassAndMethod(this).addSourceLine(this); + } + bugReporter.reportBug(bug); + } + } } /** - * @return true if specified method is annotated by {@code VisibleForTesting}. + * @return true if visibility of specified AccessibleEntity, implemented by method or field, is package-private. */ @VisibleForTesting - boolean checkAnnotated(final @Nonnull Method bcelMethod) { - for (AnnotationEntry annotation : bcelMethod.getAnnotationEntries()) { - String type = annotation.getAnnotationType(); - if (VISIBLE_FOR_TESTING_ANNOTATION_TYPE.equals(type)) { - return true; - } - } - return false; + boolean checkVisibility(final @Nonnull AccessibleEntity entry) { + return !(entry.isPrivate() || entry.isProtected() || entry.isPublic()); } @VisibleForTesting @@ -126,4 +133,8 @@ boolean isInvoking(int opcode) { return opcode == INVOKESPECIAL || opcode == INVOKEINTERFACE || opcode == INVOKESTATIC || opcode == INVOKEVIRTUAL; } + @VisibleForTesting + boolean isWorkingOnAField(final int opcode) { + return opcode == GETFIELD || opcode == PUTFIELD; + } } diff --git a/src/main/meta/messages.xml b/src/main/meta/messages.xml index e70fba2..9eed064 100755 --- a/src/main/meta/messages.xml +++ b/src/main/meta/messages.xml @@ -271,16 +271,16 @@ - You cannot access to package-private method which is annotated by @VisibleForTesting. + You must not access to package-private method or field which is annotated by @VisibleForTesting. - You cannot access to package-private method which is annotated by @VisibleForTesting. + You must not access to package-private method or field 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.
You cannot access to package-private method which is annotated by @VisibleForTesting.

+

You must not access to package-private method or field 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.

]]> diff --git a/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java b/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java index 10b246f..f012bbe 100644 --- a/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java +++ b/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java @@ -1,45 +1,34 @@ package jp.co.worksap.oss.findbugs.guava; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyByte; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; +import java.util.Arrays; + import org.apache.bcel.Constants; -import org.apache.bcel.classfile.AnnotationEntry; -import org.apache.bcel.classfile.Attribute; -import org.apache.bcel.classfile.Constant; -import org.apache.bcel.classfile.ConstantPool; -import org.apache.bcel.classfile.ConstantUtf8; -import org.apache.bcel.classfile.JavaClass; -import org.apache.bcel.classfile.Method; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import org.objectweb.asm.Opcodes; - -import com.google.common.annotations.VisibleForTesting; import edu.umd.cs.findbugs.BugInstance; import edu.umd.cs.findbugs.BugReporter; -import edu.umd.cs.findbugs.classfile.ClassDescriptor; -import edu.umd.cs.findbugs.classfile.DescriptorFactory; -import edu.umd.cs.findbugs.classfile.MethodDescriptor; - +import edu.umd.cs.findbugs.ba.AccessibleEntity; +import edu.umd.cs.findbugs.ba.XField; +import edu.umd.cs.findbugs.ba.XMethod; +import edu.umd.cs.findbugs.classfile.analysis.AnnotationValue; /** * @author tolina GmbH - * */ public class UnexpectedAccessDetectorTest { - @Mock private BugReporter bugReporter; @@ -52,59 +41,75 @@ public void initMocks() { } /** - * Tests reporting a Bug when: - * - called Method has 'default'-visibility - * - called Method is annotated with {@link VisibleForTesting} - * @throws Exception + * Checks if the visibility is checked and the annotations are loaded. */ @Test - public void testVerifyVisibility() throws Exception { - UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - ClassDescriptor invokedClassDescriptor; - MethodDescriptor invokedMethodDescriptor; + public void checkMethod() throws Exception { + final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + final XMethod invokedMethod = mock(XMethod.class); - invokedClassDescriptor = DescriptorFactory.createClassDescriptor(Object.class); - invokedMethodDescriptor = DescriptorFactory.instance().getMethodDescriptor("java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false); - detector.verifyVisibility(invokedClassDescriptor, invokedMethodDescriptor, false); + when(invokedMethod.isPrivate()).thenReturn(true); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(false); - invokedClassDescriptor = DescriptorFactory.createClassDescriptor(TestClass.class); - invokedMethodDescriptor = DescriptorFactory.instance().getMethodDescriptor("jp/co/worksap/oss/findbugs/guava/TestClass", "test", "()V", false); + detector.checkMethod(invokedMethod); - detector.verifyVisibility(invokedClassDescriptor, invokedMethodDescriptor, false); - verify(bugReporter).reportBug(any(BugInstance.class)); + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(true); + when(invokedMethod.isPublic()).thenReturn(false); + + detector.checkMethod(invokedMethod); + + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(true); + + detector.checkMethod(invokedMethod); + + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(false); + + detector.checkMethod(invokedMethod); + + // This method should only be invoked when it is package protected + verify(invokedMethod, times(1)).getAnnotations(); } /** - * Tests searching for {@link Method}s of a {@link JavaClass} + * Checks if the visibility is checked and the annotations are loaded. */ @Test - public void testFindMethod() { - UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - ConstantPool constant_pool = mock(ConstantPool.class); - JavaClass bcelClass = mock(JavaClass.class); + public void checkField() throws Exception { + final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + final XField invokedField = mock(XField.class); - Method testMethod = new Method(); - int name_index = 0; - int signature_index = 1; - testMethod.setNameIndex(name_index); - testMethod.setSignatureIndex(signature_index); - testMethod.setConstantPool(constant_pool); - Constant nameConstant = new ConstantUtf8("equals"); - Constant signatureConstant = new ConstantUtf8("(Ljava/lang/Object;)Z"); + when(invokedField.isPrivate()).thenReturn(true); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(false); - when(constant_pool.getConstant(eq(name_index), anyByte())).thenReturn(nameConstant); - when(constant_pool.getConstant(eq(signature_index), anyByte())).thenReturn(signatureConstant); + detector.checkField(invokedField); - Method[] methods = { testMethod }; - when(bcelClass.getMethods()).thenReturn(methods); - when(bcelClass.getClassName()).thenReturn("java.lang.Object"); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(true); + when(invokedField.isPublic()).thenReturn(false); - MethodDescriptor invokedMethod = new MethodDescriptor("java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false); // mock(MethodDescriptor.class); + detector.checkField(invokedField); - Method method = detector.findMethod(bcelClass, invokedMethod); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(true); - assertNotNull(method); + detector.checkField(invokedField); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(false); + + detector.checkField(invokedField); + + // This method should only be invoked when it is package protected + verify(invokedField, times(1)).getAnnotations(); } /** @@ -113,44 +118,49 @@ public void testFindMethod() { @Test public void testCheckVisibility() { UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - Method privateMethod = new Method(); - privateMethod.setModifiers(Opcodes.ACC_PRIVATE); + final AccessibleEntity privateMethod = mock(AccessibleEntity.class); + when(privateMethod.isPrivate()).thenReturn(true); assertFalse(detector.checkVisibility(privateMethod)); - Method protectedMethod = new Method(); - protectedMethod.setModifiers(Opcodes.ACC_PROTECTED); + final AccessibleEntity protectedMethod = mock(AccessibleEntity.class); + when(protectedMethod.isProtected()).thenReturn(true); assertFalse(detector.checkVisibility(protectedMethod)); - Method publicMethod = new Method(); - publicMethod.setModifiers(Opcodes.ACC_PUBLIC); + final AccessibleEntity publicMethod = mock(AccessibleEntity.class); + when(publicMethod.isPublic()).thenReturn(true); assertFalse(detector.checkVisibility(publicMethod)); - Method defaultVisibilityMethod = new Method(); - assertTrue(detector.checkVisibility(defaultVisibilityMethod)); + final AccessibleEntity defaultMethod = mock(AccessibleEntity.class); + assertTrue(detector.checkVisibility(defaultMethod)); } /** - * Checks, if detecting of annotated Methods works + * Checks, if detecting of the annotation works. */ @Test public void testCheckAnnotated() { UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - Method method = new Method(); - method.setAttributes(new Attribute[] {}); + final XMethod method = mock(XMethod.class); + + detector.checkAnnotations(method.getAnnotations(), false); - assertFalse(detector.checkAnnotated(method)); + verifyZeroInteractions(bugReporter); - AnnotationEntry someAnnotationEntry = mock(AnnotationEntry.class); - when(someAnnotationEntry.getAnnotationType()).thenReturn("Lorg/junit/Test;"); - method.addAnnotationEntry(someAnnotationEntry); + final AnnotationValue someAnnotationEntry = mock(AnnotationValue.class); + when(someAnnotationEntry.toString()).thenReturn("some other annotaion"); + when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry)); - assertFalse(detector.checkAnnotated(method)); + detector.checkAnnotations(method.getAnnotations(), false); - AnnotationEntry visibleForTestingAnnotationEntry = mock(AnnotationEntry.class); - when(visibleForTestingAnnotationEntry.getAnnotationType()).thenReturn("Lcom/google/common/annotations/VisibleForTesting;"); - method.addAnnotationEntry(visibleForTestingAnnotationEntry); + verifyZeroInteractions(bugReporter); - assertTrue(detector.checkAnnotated(method)); + final AnnotationValue visibleForTestingAnnotationEntry = mock(AnnotationValue.class); + when(visibleForTestingAnnotationEntry.toString()).thenReturn(UnexpectedAccessDetector.VISIBLE_FOR_TESTING_ANNOTATION); + when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry, visibleForTestingAnnotationEntry)); + + detector.checkAnnotations(method.getAnnotations(), false); + + verify(bugReporter).reportBug(any(BugInstance.class)); } /** @@ -163,7 +173,15 @@ public void testIsInvoking() { assertTrue(detector.isInvoking(Constants.INVOKEINTERFACE)); assertTrue(detector.isInvoking(Constants.INVOKESTATIC)); assertTrue(detector.isInvoking(Constants.INVOKEVIRTUAL)); - } + /** + * Checks, if 'field' OP-Codes are detected + */ + @Test + public void testIsWorkingOnAField() { + UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + assertTrue(detector.isWorkingOnAField(Constants.PUTFIELD)); + assertTrue(detector.isWorkingOnAField(Constants.GETFIELD)); + } }