From baefeabefc64bb30c293a3dfa9e45a0017b7696b Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 30 Apr 2019 17:12:55 -0700 Subject: [PATCH] Type-check tests for @GroupedList.Compressed. Each test enforces that all objects referenced by @GroupedList.Compressed in the package originate from one of the annotated static methods in GroupedList. To be submitted after https://github.com/bazelbuild/bazel/pull/8212. PiperOrigin-RevId: 246052739 --- .../java/com/google/devtools/build/lib/BUILD | 1 + .../devtools/build/lib/util/GroupedList.java | 42 ++++++++++++------- .../build/skyframe/InMemoryNodeEntry.java | 18 ++++---- .../build/lib/util/GroupedListTest.java | 4 +- 4 files changed, 39 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 1c1bc6d9d16e45..72635b62b1aacd 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -261,6 +261,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/common/options", "//third_party:apache_commons_lang", + "//third_party:checker_framework_annotations", "//third_party:error_prone_annotations", "//third_party:guava", "//third_party:jsr305", diff --git a/src/main/java/com/google/devtools/build/lib/util/GroupedList.java b/src/main/java/com/google/devtools/build/lib/util/GroupedList.java index ab2ece43cb694d..5e54a94a93e680 100644 --- a/src/main/java/com/google/devtools/build/lib/util/GroupedList.java +++ b/src/main/java/com/google/devtools/build/lib/util/GroupedList.java @@ -28,6 +28,10 @@ import java.util.Iterator; import java.util.List; import java.util.Set; +import org.checkerframework.framework.qual.DefaultQualifierInHierarchy; +import org.checkerframework.framework.qual.ImplicitFor; +import org.checkerframework.framework.qual.LiteralKind; +import org.checkerframework.framework.qual.SubtypeOf; /** * Encapsulates a list of groups. Is intended to be used in "batch" mode -- to set the value of a @@ -47,15 +51,17 @@ public class GroupedList implements Iterable> { * Indicates that the annotated element is a compressed {@link GroupedList}, so that it can be * safely passed to {@link #create} and friends. */ - // TODO(jhorvitz): enforce this annotation via compile-time checks. - @Target({ - ElementType.PARAMETER, - ElementType.FIELD, - ElementType.LOCAL_VARIABLE, - ElementType.TYPE_USE - }) + @SubtypeOf(DefaultObject.class) + @Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER}) + @ImplicitFor(literals = LiteralKind.NULL) public @interface Compressed {} + /** Default annotation for type-safety checks of {@link Compressed}. */ + @DefaultQualifierInHierarchy + @SubtypeOf({}) + @Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER}) + private @interface DefaultObject {} + // Total number of items in the list. At least elements.size(), but might be larger if there are // any nested lists. private int size = 0; @@ -222,7 +228,7 @@ public int numElements() { return size; } - public static int numElements(Object compressed) { + public static int numElements(@Compressed Object compressed) { if (compressed == EMPTY_LIST) { return 0; } @@ -264,6 +270,16 @@ public static Iterable compressedToIterable(@Compressed Object compressed return ImmutableList.of((T) compressed); } + /** + * Casts an {@code Object} which is known to be {@link Compressed}. + * + *

This method should only be used when it is not possible to enforce the type via annotations. + */ + public static @Compressed Object castAsCompressed(Object obj) { + Preconditions.checkArgument(!(obj instanceof GroupedList), obj); + return (@Compressed Object) obj; + } + /** Returns true if this list contains no elements. */ public boolean isEmpty() { return elements.isEmpty(); @@ -334,7 +350,7 @@ public static GroupedList create(@Compressed Object compressed) { } /** Creates an already compressed {@code GroupedList} for storage. */ - public static @Compressed Object createCompressedWithTwoGroupes( + public static @Compressed Object createCompressedWithTwoGroups( E singletonElementOfFirstGroup, List elementsOfSecondGroup) { switch (elementsOfSecondGroup.size()) { case 0: @@ -441,10 +457,7 @@ public boolean equals(Object other) { @Override public String toString() { - return MoreObjects.toStringHelper(this) - .add("elements", elements) - .add("size", size).toString(); - + return MoreObjects.toStringHelper(this).add("elements", elements).add("size", size).toString(); } /** @@ -591,7 +604,8 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("groupedList", groupedList) .add("elements", elements) - .add("currentGroup", currentGroup).toString(); + .add("currentGroup", currentGroup) + .toString(); } } } diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java index 94d6de287adef8..4b348b9454d939 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryNodeEntry.java @@ -138,11 +138,8 @@ public class InMemoryNodeEntry implements NodeEntry { */ @Nullable protected volatile DirtyBuildingState dirtyBuildingState = null; - /** - * Construct a InMemoryNodeEntry. Use ONLY in Skyframe evaluation and graph implementations. - */ - public InMemoryNodeEntry() { - } + /** Construct a InMemoryNodeEntry. Use ONLY in Skyframe evaluation and graph implementations. */ + public InMemoryNodeEntry() {} // Public only for use in alternate graph implementations. public KeepEdgesPolicy keepEdges() { @@ -218,12 +215,12 @@ public synchronized Iterable getDirectDeps() { public synchronized @GroupedList.Compressed Object getCompressedDirectDepsForDoneEntry() { assertKeepDeps(); Preconditions.checkState(isDone(), "no deps until done. NodeEntry: %s", this); - return Preconditions.checkNotNull(directDeps, "deps can't be null: %s", this); + Preconditions.checkNotNull(directDeps, "deps can't be null: %s", this); + return GroupedList.castAsCompressed(directDeps); } public int getNumDirectDeps() { - Preconditions.checkState(isDone(), "no deps until done. NodeEntry: %s", this); - return GroupedList.numElements(directDeps); + return GroupedList.numElements(getCompressedDirectDepsForDoneEntry()); } @Override @@ -520,7 +517,8 @@ public synchronized MarkedDirtyResult markDirty(DirtyType dirtyType) { assertKeepDeps(); if (isDone()) { dirtyBuildingState = - DirtyBuildingState.create(dirtyType, GroupedList.create(directDeps), value); + DirtyBuildingState.create( + dirtyType, GroupedList.create(getCompressedDirectDepsForDoneEntry()), value); value = null; directDeps = null; return new MarkedDirtyResult(ReverseDepsUtility.getReverseDeps(this)); @@ -733,7 +731,7 @@ protected synchronized MoreObjects.ToStringHelper toStringHelper() { .add( "directDeps", isDone() && keepEdges() != KeepEdgesPolicy.NONE - ? GroupedList.create(directDeps) + ? GroupedList.create(getCompressedDirectDepsForDoneEntry()) : directDeps) .add("reverseDeps", ReverseDepsUtility.toString(this)) .add("dirtyBuildingState", dirtyBuildingState); diff --git a/src/test/java/com/google/devtools/build/lib/util/GroupedListTest.java b/src/test/java/com/google/devtools/build/lib/util/GroupedListTest.java index 0a29e8bc3efcae..f1f0e626b3255b 100644 --- a/src/test/java/com/google/devtools/build/lib/util/GroupedListTest.java +++ b/src/test/java/com/google/devtools/build/lib/util/GroupedListTest.java @@ -215,10 +215,10 @@ public void createCompressed() { GroupedList groupedList = new GroupedList<>(); groupedList.appendGroup(ImmutableList.of("a")); groupedList.appendGroup(ImmutableList.of("b", "c")); - assertThat(GroupedList.createCompressedWithTwoGroupes("a", ImmutableList.of("b", "c"))) + assertThat(GroupedList.createCompressedWithTwoGroups("a", ImmutableList.of("b", "c"))) .isEqualTo(groupedList.compress()); groupedList.remove(ImmutableSet.of("b")); - assertThat(GroupedList.createCompressedWithTwoGroupes("a", ImmutableList.of("c"))) + assertThat(GroupedList.createCompressedWithTwoGroups("a", ImmutableList.of("c"))) .isEqualTo(groupedList.compress()); }