Skip to content

Commit

Permalink
Add type arguments in @AfterTemplates for builders
Browse files Browse the repository at this point in the history
  • Loading branch information
rickie committed Jan 3, 2023
1 parent d456821 commit fcafc5c
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ private ImmutableListMultimapRules() {}
* Prefer {@link ImmutableListMultimap#builder()} over the associated constructor on constructions
* that produce a less-specific type.
*/
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableListMultimapBuilder<K, V> {
@BeforeTemplate
ImmutableMultimap.Builder<K, V> before() {
Expand All @@ -48,7 +46,7 @@ ImmutableMultimap.Builder<K, V> before() {

@AfterTemplate
ImmutableListMultimap.Builder<K, V> after() {
return ImmutableListMultimap.builder();
return ImmutableListMultimap.<K, V>builder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ final class ImmutableListRules {
private ImmutableListRules() {}

/** Prefer {@link ImmutableList#builder()} over the associated constructor. */
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableListBuilder<T> {
@BeforeTemplate
ImmutableList.Builder<T> before() {
Expand All @@ -38,7 +36,7 @@ ImmutableList.Builder<T> before() {

@AfterTemplate
ImmutableList.Builder<T> after() {
return ImmutableList.builder();
return ImmutableList.<T>builder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ final class ImmutableMapRules {
private ImmutableMapRules() {}

/** Prefer {@link ImmutableMap#builder()} over the associated constructor. */
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableMapBuilder<K, V> {
@BeforeTemplate
ImmutableMap.Builder<K, V> before() {
Expand All @@ -39,7 +37,7 @@ ImmutableMap.Builder<K, V> before() {

@AfterTemplate
ImmutableMap.Builder<K, V> after() {
return ImmutableMap.builder();
return ImmutableMap.<K, V>builder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ final class ImmutableMultisetRules {
private ImmutableMultisetRules() {}

/** Prefer {@link ImmutableMultiset#builder()} over the associated constructor. */
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableMultisetBuilder<T> {
@BeforeTemplate
ImmutableMultiset.Builder<T> before() {
Expand All @@ -31,7 +29,7 @@ ImmutableMultiset.Builder<T> before() {

@AfterTemplate
ImmutableMultiset.Builder<T> after() {
return ImmutableMultiset.builder();
return ImmutableMultiset.<T>builder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ final class ImmutableSetMultimapRules {
private ImmutableSetMultimapRules() {}

/** Prefer {@link ImmutableSetMultimap#builder()} over the associated constructor. */
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableSetMultimapBuilder<K, V> {
@BeforeTemplate
ImmutableSetMultimap.Builder<K, V> before() {
Expand All @@ -39,7 +37,7 @@ ImmutableSetMultimap.Builder<K, V> before() {

@AfterTemplate
ImmutableSetMultimap.Builder<K, V> after() {
return ImmutableSetMultimap.builder();
return ImmutableSetMultimap.<K, V>builder();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ ImmutableSortedMap.Builder<K, V> after(Comparator<K> cmp) {
* Prefer {@link ImmutableSortedMap#naturalOrder()} over the alternative that requires explicitly
* providing the {@link Comparator}.
*/
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableSortedMapNaturalOrderBuilder<K extends Comparable<? super K>, V> {
@BeforeTemplate
ImmutableSortedMap.Builder<K, V> before() {
Expand All @@ -47,16 +45,14 @@ ImmutableSortedMap.Builder<K, V> before() {

@AfterTemplate
ImmutableSortedMap.Builder<K, V> after() {
return ImmutableSortedMap.naturalOrder();
return ImmutableSortedMap.<K, V>naturalOrder();
}
}

/**
* Prefer {@link ImmutableSortedMap#reverseOrder()} over the alternative that requires explicitly
* providing the {@link Comparator}.
*/
// XXX: This drops generic type information, sometimes leading to non-compilable code. See
// https://github.com/google/error-prone/pull/2706.
static final class ImmutableSortedMapReverseOrderBuilder<K extends Comparable<? super K>, V> {
@BeforeTemplate
ImmutableSortedMap.Builder<K, V> before() {
Expand All @@ -65,7 +61,7 @@ ImmutableSortedMap.Builder<K, V> before() {

@AfterTemplate
ImmutableSortedMap.Builder<K, V> after() {
return ImmutableSortedMap.reverseOrder();
return ImmutableSortedMap.<K, V>reverseOrder();
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package tech.picnic.errorprone.refastertemplates;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.stream.Collectors.collectingAndThen;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets.SetView;
import com.google.common.collect.Streams;
import com.google.errorprone.refaster.ImportPolicy;
import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.Set;
import java.util.stream.Stream;

/** Refaster templates related to expressions dealing with {@link ImmutableSet}s. */
final class ImmutableSetTemplates {
private ImmutableSetTemplates() {}

/** Prefer {@link ImmutableSet#builder()} over the associated constructor. */
// XXX: Picnic's Error Prone fork supports method invocation type argument inlining in the
// `@AfterTemplate`. Without using the fork, the expression in the `@AfterTemplate` can result in
// non-compilable code. See: https://github.com/google/error-prone/pull/2706.
static final class ImmutableSetBuilder<T> {
@BeforeTemplate
ImmutableSet.Builder<T> before() {
return new ImmutableSet.Builder<>();
}

@AfterTemplate
ImmutableSet.Builder<T> after() {
return ImmutableSet.<T>builder();
}
}

/** Prefer {@link ImmutableSet#of()} over more contrived alternatives. */
static final class EmptyImmutableSet<T> {
@BeforeTemplate
ImmutableSet<T> before() {
return Refaster.anyOf(
ImmutableSet.<T>builder().build(), Stream.<T>empty().collect(toImmutableSet()));
}

@AfterTemplate
ImmutableSet<T> after() {
return ImmutableSet.of();
}
}

/**
* Prefer {@link ImmutableSet#of(Object)} over alternatives that don't communicate the
* immutability of the resulting set at the type level.
*/
// XXX: Note that this rewrite rule is incorrect for nullable elements.
static final class SingletonImmutableSet<T> {
@BeforeTemplate
Set<T> before(T element) {
return Collections.singleton(element);
}

@AfterTemplate
ImmutableSet<T> after(T element) {
return ImmutableSet.of(element);
}
}

/** Prefer {@link ImmutableSet#copyOf(Iterable)} and variants over more contrived alternatives. */
static final class IterableToImmutableSet<T> {
@BeforeTemplate
ImmutableSet<T> before(T[] iterable) {
return Refaster.anyOf(
ImmutableSet.<T>builder().add(iterable).build(),
Arrays.stream(iterable).collect(toImmutableSet()));
}

@BeforeTemplate
ImmutableSet<T> before(Iterator<T> iterable) {
return Refaster.anyOf(
ImmutableSet.<T>builder().addAll(iterable).build(),
Streams.stream(iterable).collect(toImmutableSet()));
}

@BeforeTemplate
ImmutableSet<T> before(Iterable<T> iterable) {
return Refaster.anyOf(
ImmutableSet.<T>builder().addAll(iterable).build(),
Streams.stream(iterable).collect(toImmutableSet()));
}

@BeforeTemplate
ImmutableSet<T> before(Collection<T> iterable) {
return iterable.stream().collect(toImmutableSet());
}

@AfterTemplate
ImmutableSet<T> after(Iterable<T> iterable) {
return ImmutableSet.copyOf(iterable);
}
}

/** Prefer {@link ImmutableSet#toImmutableSet()} over less idiomatic alternatives. */
// XXX: Once the code base has been sufficiently cleaned up, we might want to also rewrite
// `Collectors.toSet(`), with the caveat that it allows mutation (though this cannot be relied
// upon) as well as nulls. Another option is to explicitly rewrite those variants to
// `Collectors.toSet(HashSet::new)`.
static final class StreamToImmutableSet<T> {
@BeforeTemplate
ImmutableSet<T> before(Stream<T> stream) {
return Refaster.anyOf(
ImmutableSet.copyOf(stream.iterator()),
stream.distinct().collect(toImmutableSet()),
stream.collect(collectingAndThen(toList(), ImmutableSet::copyOf)),
stream.collect(collectingAndThen(toSet(), ImmutableSet::copyOf)));
}

@AfterTemplate
@UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)
ImmutableSet<T> after(Stream<T> stream) {
return stream.collect(toImmutableSet());
}
}

/** Don't unnecessarily copy an {@link ImmutableSet}. */
static final class ImmutableSetCopyOfImmutableSet<T> {
@BeforeTemplate
ImmutableSet<T> before(ImmutableSet<T> set) {
return ImmutableSet.copyOf(set);
}

@AfterTemplate
ImmutableSet<T> after(ImmutableSet<T> set) {
return set;
}
}

/** Prefer {@link SetView#immutableCopy()} over the more verbose alternative. */
static final class ImmutableSetCopyOfSetView<T> {
@BeforeTemplate
ImmutableSet<T> before(SetView<T> set) {
return ImmutableSet.copyOf(set);
}

@AfterTemplate
ImmutableSet<T> after(SetView<T> set) {
return set.immutableCopy();
}
}
}

0 comments on commit fcafc5c

Please sign in to comment.