Skip to content

Commit

Permalink
@VisibleForTesting is now detected on non static fields, too.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Sebastian Eckey committed Feb 3, 2015
1 parent f920b70 commit 943caae
Show file tree
Hide file tree
Showing 3 changed files with 167 additions and 138 deletions.
129 changes: 70 additions & 59 deletions src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

/**
* <p>A detector to ensure that implementation (class in src/main/java) doesn't call
Expand All @@ -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;
Expand All @@ -44,86 +48,93 @@ 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);
}
}
}

/**
* <p>Report if specified method is package-private and annotated by {@code @VisibleForTesting}.</p>
* @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<AnnotationValue> 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
boolean isInvoking(int opcode) {
return opcode == INVOKESPECIAL || opcode == INVOKEINTERFACE || opcode == INVOKESTATIC || opcode == INVOKEVIRTUAL;
}

@VisibleForTesting
boolean isWorkingOnAField(final int opcode) {
return opcode == GETFIELD || opcode == PUTFIELD;
}
}
6 changes: 3 additions & 3 deletions src/main/meta/messages.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,16 @@

<BugPattern type="GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING">
<ShortDescription>
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.
</ShortDescription>
<LongDescription>
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.
</LongDescription>
<Details>
<![CDATA[
<p>You cannot access to package-private method which is annotated by @VisibleForTesting.</p>
<p>You must not access to package-private method or field 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>
]]>
Expand Down
Loading

0 comments on commit 943caae

Please sign in to comment.