Skip to content

Commit

Permalink
Refactor common thread safety checking code
Browse files Browse the repository at this point in the history 
MOE_MIGRATED_REVID=166551768
  • Loading branch information
@cushon
mboyington authored and cushon committed Aug 28, 2017
1 parent e906de1 commit 32c340f
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 29 deletions.
18 changes: 12 additions & 6 deletions ...threadsafety/ImmutableAnnotationInfo.java → ...patterns/threadsafety/AnnotationInfo.java
View file
Expand Up @@ -18,24 +18,30 @@

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import java.util.Set;

/**
* A copy of the information in {@link com.google.errorprone.annotations.Immutable}.
* Specifies information about a type which may be a container specified by generic type arguments,
* e.g. {@link com.google.errorprone.annotations.Immutable}.
*
* <p>Useful for providing information for immutable classes we can't easily annotate, e.g. those in
* the JDK.
*/
@AutoValue
public abstract class ImmutableAnnotationInfo {
public abstract class AnnotationInfo {
public abstract String typeName();

public abstract ImmutableSet<String> containerOf();
public Set<String> containerOf() {
return internalContainerOf();
}

abstract ImmutableSet<String> internalContainerOf();

public static ImmutableAnnotationInfo create(String typeName, Iterable<String> containerOf) {
return new AutoValue_ImmutableAnnotationInfo(typeName, ImmutableSet.copyOf(containerOf));
public static AnnotationInfo create(String typeName, Iterable<String> containerOf) {
return new AutoValue_AnnotationInfo(typeName, ImmutableSet.copyOf(containerOf));
}

public static ImmutableAnnotationInfo create(String typeName) {
public static AnnotationInfo create(String typeName) {
return create(typeName, ImmutableSet.<String>of());
}
}
24 changes: 11 additions & 13 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableAnalysis.java
View file
Expand Up @@ -145,8 +145,7 @@ Violation checkForImmutability(
}

for (Type interfaceType : state.getTypes().interfaces(type)) {
ImmutableAnnotationInfo interfaceAnnotation =
getImmutableAnnotation(interfaceType.tsym, state);
AnnotationInfo interfaceAnnotation = getImmutableAnnotation(interfaceType.tsym, state);
if (interfaceAnnotation == null) {
continue;
}
Expand Down Expand Up @@ -200,7 +199,7 @@ private Violation checkSuper(ImmutableSet<String> immutableTyParams, ClassType t
return Violation.absent();
}

ImmutableAnnotationInfo superannotation = getImmutableAnnotation(superType.tsym, state);
AnnotationInfo superannotation = getImmutableAnnotation(superType.tsym, state);
if (superannotation != null) {
// If the superclass does happen to be immutable, we don't need to recursively
// inspect it. We just have to check that it's instantiated correctly:
Expand Down Expand Up @@ -325,7 +324,7 @@ private Violation isFieldImmutable(
* @param type the type to check
*/
Violation immutableInstantiation(
ImmutableSet<String> immutableTyParams, ImmutableAnnotationInfo annotation, Type type) {
ImmutableSet<String> immutableTyParams, AnnotationInfo annotation, Type type) {
if (!annotation.containerOf().isEmpty()
&& type.tsym.getTypeParameters().size() != type.getTypeArguments().size()) {
return Violation.of(
Expand Down Expand Up @@ -413,7 +412,7 @@ public Violation visitType(Type type, Void s) {
// TODO(b/25630189): add enforcement
return Violation.absent();
}
ImmutableAnnotationInfo annotation = getImmutableAnnotation(type.tsym, state);
AnnotationInfo annotation = getImmutableAnnotation(type.tsym, state);
if (annotation != null) {
return immutableInstantiation(immutableTyParams, annotation, type);
}
Expand All @@ -437,9 +436,9 @@ public Violation visitType(Type type, Void s) {
* Gets the {@link Symbol}'s {@code @Immutable} annotation info, either from an annotation on the
* symbol or from the list of well-known immutable types.
*/
ImmutableAnnotationInfo getImmutableAnnotation(Symbol sym, VisitorState state) {
AnnotationInfo getImmutableAnnotation(Symbol sym, VisitorState state) {
String nameStr = sym.flatName().toString();
ImmutableAnnotationInfo known = wellKnownMutability.getKnownImmutableClasses().get(nameStr);
AnnotationInfo known = wellKnownMutability.getKnownImmutableClasses().get(nameStr);
if (known != null) {
return known;
}
Expand All @@ -450,18 +449,17 @@ ImmutableAnnotationInfo getImmutableAnnotation(Symbol sym, VisitorState state) {
* Gets the possibly inherited {@code @Immutable} annotation on the given symbol, and
* reverse-propagates containerOf spec's from super-classes.
*/
static ImmutableAnnotationInfo getInheritedAnnotation(Symbol sym, VisitorState state) {
static AnnotationInfo getInheritedAnnotation(Symbol sym, VisitorState state) {
if (!(sym instanceof ClassSymbol)) {
return null;
}
Compound attr = sym.attribute(state.getSymbolFromString(Immutable.class.getName()));
if (attr != null) {
return ImmutableAnnotationInfo.create(
sym.getQualifiedName().toString(), containerOf(state, attr));
return AnnotationInfo.create(sym.getQualifiedName().toString(), containerOf(state, attr));
}
// @Immutable is inherited from supertypes
Type superClass = ((ClassSymbol) sym).getSuperclass();
ImmutableAnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state);
AnnotationInfo superAnnotation = getInheritedAnnotation(superClass.asElement(), state);
if (superAnnotation == null) {
return null;
}
Expand All @@ -483,7 +481,7 @@ static ImmutableAnnotationInfo getInheritedAnnotation(Symbol sym, VisitorState s
containerOf.add(argSym.getSimpleName().toString());
}
}
return ImmutableAnnotationInfo.create(sym.getQualifiedName().toString(), containerOf.build());
return AnnotationInfo.create(sym.getQualifiedName().toString(), containerOf.build());
}

private static ImmutableList<String> containerOf(VisitorState state, Compound attr) {
Expand Down Expand Up @@ -516,7 +514,7 @@ public Void visitArray(List<? extends AnnotationValue> list, Void unused) {
* Gets the {@link Tree}'s {@code @Immutable} annotation info, either from an annotation on the
* symbol or from the list of well-known immutable types.
*/
ImmutableAnnotationInfo getImmutableAnnotation(Tree tree, VisitorState state) {
AnnotationInfo getImmutableAnnotation(Tree tree, VisitorState state) {
Symbol sym = ASTHelpers.getSymbol(tree);
return sym == null ? null : getImmutableAnnotation(sym, state);
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java
View file
Expand Up @@ -78,7 +78,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
return handleAnonymousClass(tree, state, analysis);
}

ImmutableAnnotationInfo annotation = analysis.getImmutableAnnotation(tree, state);
AnnotationInfo annotation = analysis.getImmutableAnnotation(tree, state);
if (annotation == null) {
// If the type isn't annotated we don't check for immutability, but we do
// report an error if it extends/implements any @Immutable-annotated types.
Expand Down Expand Up @@ -241,7 +241,7 @@ private static ImmutableSet<String> immutableTypeParametersInScope(
default:
break;
}
ImmutableAnnotationInfo annotation = analysis.getImmutableAnnotation(s, state);
AnnotationInfo annotation = analysis.getImmutableAnnotation(s, state);
if (annotation == null) {
continue;
}
Expand Down
17 changes: 9 additions & 8 deletions core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java
View file
Expand Up @@ -32,6 +32,7 @@
import java.lang.reflect.TypeVariable;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

/** A collection of types with with known mutability. */
Expand All @@ -51,19 +52,19 @@ public static WellKnownMutability fromFlags(ErrorProneFlags flags) {
return new WellKnownMutability(immutable, unsafe);
}

public ImmutableMap<String, ImmutableAnnotationInfo> getKnownImmutableClasses() {
public Map<String, AnnotationInfo> getKnownImmutableClasses() {
return knownImmutableClasses;
}

public ImmutableSet<String> getKnownUnsafeClasses() {
public Set<String> getKnownUnsafeClasses() {
return knownUnsafeClasses;
}

/** Types that are known to be immutable. */
private final ImmutableMap<String, ImmutableAnnotationInfo> knownImmutableClasses;
private final ImmutableMap<String, AnnotationInfo> knownImmutableClasses;

static class Builder {
final ImmutableMap.Builder<String, ImmutableAnnotationInfo> mapBuilder = ImmutableMap.builder();
final ImmutableMap.Builder<String, AnnotationInfo> mapBuilder = ImmutableMap.builder();

public Builder addClasses(Set<Class<?>> clazzs) {
for (Class<?> clazz : clazzs) {
Expand Down Expand Up @@ -94,24 +95,24 @@ public Builder add(Class<?> clazz, String... containerOf) {
}
mapBuilder.put(
clazz.getName(),
ImmutableAnnotationInfo.create(clazz.getName(), ImmutableList.copyOf(containerOf)));
AnnotationInfo.create(clazz.getName(), ImmutableList.copyOf(containerOf)));
return this;
}

public Builder add(String className, String... containerOf) {
mapBuilder.put(
className, ImmutableAnnotationInfo.create(className, ImmutableList.copyOf(containerOf)));
className, AnnotationInfo.create(className, ImmutableList.copyOf(containerOf)));
return this;
}

public ImmutableMap<String, ImmutableAnnotationInfo> build() {
public ImmutableMap<String, AnnotationInfo> build() {
return mapBuilder.build();
}
}

// TODO(b/35724557): share this list with other code analyzing types for immutability
// TODO(cushon): generate this at build-time to get type-safety without added compile-time deps
private static ImmutableMap<String, ImmutableAnnotationInfo> buildImmutableClasses(
private static ImmutableMap<String, AnnotationInfo> buildImmutableClasses(
List<String> extraKnownImmutables) {
return new Builder()
.addStrings(extraKnownImmutables)
Expand Down

0 comments on commit 32c340f

Please sign in to comment.