Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ImmutableCollection templates #40

Merged
merged 15 commits into from
Apr 12, 2022
Merged

Introduce ImmutableCollection templates #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
29a031d
Introduce new templates to use Immutables
Feb 18, 2022
2904a75
Adding listOf template
Feb 18, 2022
be9117d
Adding listOf templates for up to 5 items
Feb 21, 2022
55a2437
CR remarks, adding javadoc, updating tests
Feb 23, 2022
eb4d03d
Making method names consistent
Feb 24, 2022
6630b6e
Removing typed tests
Feb 25, 2022
1876587
To object
Feb 25, 2022
a92c05e
CR remarks
Feb 25, 2022
a58862f
List.of changes
Feb 28, 2022
7aa801d
CR remarks
Mar 1, 2022
f4b0285
`Immutable{Set,List}Test{Input,Output}` use Integer instead of String
rickie Mar 2, 2022
f8c2048
Re-add emptyList, emptySet and emptyMap templates
Apr 1, 2022
59ef9e9
Suggestions
rickie Apr 4, 2022
dbae0c1
Post-rebase fix
Stephan202 Apr 10, 2022
920a2e1
Suggestions
Stephan202 Apr 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 113 additions & 31 deletions ...ontrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListTemplates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,37 +41,6 @@ ImmutableList.Builder<T> after() {
}
}

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

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

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

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

/**
* Prefer {@link ImmutableList#copyOf(Iterable)} and variants over more contrived alternatives.
*/
Expand Down Expand Up @@ -186,4 +155,117 @@ ImmutableList<T> after(Stream<T> stream) {
return stream.collect(toImmutableSet()).asList();
}
}

/**
* Prefer {@link ImmutableList#of()} over more contrived alternatives or alternatives that don't
* communicate the immutability of the resulting list at the type level.
*/
// XXX: The `Stream` variant may be too contrived to warrant inclusion. Review its usage if/when
// this and similar Refaster templates are replaced with an Error Prone check.
static final class ImmutableListOf<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other templates in this class have a Javadoc, let's add it for the other templates we introduce in this PR as well.

Perhaps we could do something like this example:

  /**
   * Prefer {@link ImmutableList#of(Object)} over alternatives that don't communicate the
   * immutability of the resulting list at the type level.
   */

The implementation of List.of(element ...) is:

static <E> List<E> of(E e1, E e2, E e3) {
    return new ImmutableCollections.ListN<>(e1, e2, e3);
}

So the example Javadoc has some similarities with the Refaster templates we are adding in this PR.

This also applies for the other introduced Refaster templates in Immutable{Map,Set}.java.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most templates the Javadoc is now good. Only for this case, the emptyList() I'm not 100% sure. So the List.of(..) calls uses the ImmutableCollections under the hood. However, for the emptyList() this is not the case:

    public static final <T> List<T> emptyList() {
        return (List<T>) EMPTY_LIST;
    }

I'm not sure what to write in the Javadoc there, so also leaving this open for other reviewers to give their input :)

Copy link
Member

@Badbond Badbond Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be very careful here (also when considering List.of() as before template, see other comment). We have three different List implementations with different behavior:

  • Existing @Before Collections.emptyList(): uses Java's Collections.EmptyList which silently ignores calls to sort(), replaceAll() and removeIf() as there are no elements anyway.
  • Suggested @Before List.of(): uses Java's ImmutableCollections.ListN which throws UnsupportedOperationException on those methods.
  • Existing @After ImmutableList.of(): uses Guava's ImmutableList which throws UnsupportedOperationException on those methods.

My suggestion (after further implementation investigation):
List.of() and ImmutableList.of() have the most similar implementation and I would be happy to make that conversion. I would create an ErrorProne check for Collections.emptyList() to suggest using ImmutableList.of() warning them for possible side-effects due to differences in List implementations. WDYT?

Regarding JavaDoc, I'm happy with current state on this method, except that it mentions {@link ImmutableList#of(Object)} instead of {@link ImmutableList#of()}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good remark, we should do this 😄.

For the Collections.emptyList() we can use the same setup as FluxFlatMapUsageCheck.java. We should do two suggested fixes there, such that it will say: "Do you want to use ImmutableList.of() or add a SuppressWarnings("...")"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets create a new PR to create an error prone rule to validate Collections.emptyList() and suggest ImmutableList.of()

And on this PR I will change current refaster template to List.of() -> ImmutableList.of()

@BeforeTemplate
List<T> before() {
return Refaster.anyOf(
ImmutableList.<T>builder().build(),
Stream.<T>empty().collect(toImmutableList()),
Collections.emptyList(),
List.of());
}

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

/**
* Prefer {@link ImmutableList#of(Object)} over more contrived alternatives or alternatives that
* don't communicate the immutability of the resulting list at the type level.
*/
// XXX: Note that the replacement of `Collections#singletonList` is incorrect for nullable
// elements.
static final class ImmutableListOf1<T> {
@BeforeTemplate
List<T> before(T e1) {
return Refaster.anyOf(
ImmutableList.<T>builder().add(e1).build(), Collections.singletonList(e1), List.of(e1));
}

@AfterTemplate
ImmutableList<T> after(T e1) {
return ImmutableList.of(e1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking the signature uses element as parameter name. But I actually prefer this for consistency 😄 (Set also uses e1 for single argument method). Just wanted to point it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer this for consistency :)

}
}

/**
* Prefer {@link ImmutableList#of(Object, Object)} over alternatives that don't communicate the
* immutability of the resulting list at the type level.
*/
// XXX: Consider writing an Error Prone check which also flags straightforward
// `ImmutableList.builder()` usages.
static final class ImmutableListOf2<T> {
@BeforeTemplate
List<T> before(T e1, T e2) {
return List.of(e1, e2);
}

@AfterTemplate
ImmutableList<T> after(T e1, T e2) {
return ImmutableList.of(e1, e2);
}
}

/**
* Prefer {@link ImmutableList#of(Object, Object, Object)} over alternatives that don't
* communicate the immutability of the resulting list at the type level.
*/
// XXX: Consider writing an Error Prone check which also flags straightforward
// `ImmutableList.builder()` usages.
static final class ImmutableListOf3<T> {
@BeforeTemplate
List<T> before(T e1, T e2, T e3) {
return List.of(e1, e2, e3);
}

@AfterTemplate
ImmutableList<T> after(T e1, T e2, T e3) {
return ImmutableList.of(e1, e2, e3);
}
}

/**
* Prefer {@link ImmutableList#of(Object, Object, Object, Object)} over alternatives that don't
* communicate the immutability of the resulting list at the type level.
*/
// XXX: Consider writing an Error Prone check which also flags straightforward
// `ImmutableList.builder()` usages.
static final class ImmutableListOf4<T> {
@BeforeTemplate
List<T> before(T e1, T e2, T e3, T e4) {
return List.of(e1, e2, e3, e4);
}

@AfterTemplate
ImmutableList<T> after(T e1, T e2, T e3, T e4) {
return ImmutableList.of(e1, e2, e3, e4);
}
}

/**
* Prefer {@link ImmutableList#of(Object, Object, Object, Object, Object)} over alternatives that
* don't communicate the immutability of the resulting list at the type level.
*/
// XXX: Consider writing an Error Prone check which also flags straightforward
// `ImmutableList.builder()` usages.
static final class ImmutableListOf5<T> {
@BeforeTemplate
List<T> before(T e1, T e2, T e3, T e4, T e5) {
return List.of(e1, e2, e3, e4, e5);
}

@AfterTemplate
ImmutableList<T> after(T e1, T e2, T e3, T e4, T e5) {
return ImmutableList.of(e1, e2, e3, e4, e5);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Location-wise I would probably have located these templates further up in the file, as we generally start with "construction" checks. But since we're planning to auto-sort the templates lexicographically: let's roll with this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the other files, I got the idea that we list templates with different number of params overloads at the end of a file.
(I know it is also because you auto-generated some of the templates 😄).

Nonetheless, good point about "construction" checks :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the auto-generated checks in AssertJTemplates are "special", and indeed at the bottom. (Additional candidates for either an Error Prone check or a Refaster extension.)

}
142 changes: 107 additions & 35 deletions ...contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableMapTemplates.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,41 +40,6 @@ ImmutableMap.Builder<K, V> after() {
}
}

/** Prefer {@link ImmutableMap#of()} over more contrived alternatives. */
static final class EmptyImmutableMap<K, V> {
@BeforeTemplate
ImmutableMap<K, V> before() {
return ImmutableMap.<K, V>builder().build();
}

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

/**
* Prefer {@link ImmutableMap#of(Object, Object)} over more contrived alternatives and
* alternatives that don't communicate the immutability of the resulting map at the type level.
*/
// XXX: One can define variants for more than one key-value pair, but at some point the builder
// actually produces nicer code. So it's not clear we should add Refaster templates for those
// variants.
// XXX: Note that the `singletonMap` rewrite rule is incorrect for nullable elements.
static final class PairToImmutableMap<K, V> {
@BeforeTemplate
Map<K, V> before(K key, V value) {
return Refaster.anyOf(
ImmutableMap.<K, V>builder().put(key, value).build(),
Collections.singletonMap(key, value));
}

@AfterTemplate
ImmutableMap<K, V> after(K key, V value) {
return ImmutableMap.of(key, value);
}
}

/** Prefer {@link ImmutableMap#of(Object, Object)} over more contrived alternatives. */
static final class EntryToImmutableMap<K, V> {
@BeforeTemplate
Expand Down Expand Up @@ -242,6 +207,113 @@ ImmutableMap<K, V2> after(Map<K, V1> map) {
}
}

/**
* Prefer {@link ImmutableMap#of()} over more contrived alternatives or alternatives that don't
* communicate the immutability of the resulting map at the type level.
*/
static final class ImmutableMapOf<K, V> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be merged with EmptyImmutableMap. Likewise for a number of other templates.

@BeforeTemplate
Map<K, V> before() {
return Refaster.anyOf(ImmutableMap.<K, V>builder().build(), Collections.emptyMap(), Map.of());
}

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

/**
* Prefer {@link ImmutableMap#of(Object, Object)} over more contrived alternatives or alternatives
* that don't communicate the immutability of the resulting map at the type level.
*/
// XXX: Note that the replacement of `Collections#singletonMap` is incorrect for nullable
// elements.
static final class ImmutableMapOf1<K, V> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be merged with PairToImmutableMap.

@BeforeTemplate
Map<K, V> before(K k1, V v1) {
return Refaster.anyOf(
ImmutableMap.<K, V>builder().put(k1, v1).build(),
Collections.singletonMap(k1, v1),
Map.of(k1, v1));
}

@AfterTemplate
ImmutableMap<K, V> after(K k1, V v1) {
return ImmutableMap.of(k1, v1);
}
}

/**
* Prefer {@link ImmutableMap#of(Object, Object, Object, Object)} over alternatives that don't
* communicate the immutability of the resulting map at the type level.
*/
// XXX: Also rewrite the `ImmutableMap.builder()` variant?
static final class ImmutableMapOf2<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2) {
return Map.of(k1, v1, k2, v2);
}

@AfterTemplate
ImmutableMap<K, V> after(K k1, V v1, K k2, V v2) {
return ImmutableMap.of(k1, v1, k2, v2);
}
}

/**
* Prefer {@link ImmutableMap#of(Object, Object, Object, Object, Object, Object)} over
* alternatives that don't communicate the immutability of the resulting map at the type level.
*/
// XXX: Also rewrite the `ImmutableMap.builder()` variant?
static final class ImmutableMapOf3<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3) {
return Map.of(k1, v1, k2, v2, k3, v3);
}

@AfterTemplate
ImmutableMap<K, V> after(K k1, V v1, K k2, V v2, K k3, V v3) {
return ImmutableMap.of(k1, v1, k2, v2, k3, v3);
}
}

/**
* Prefer {@link ImmutableMap#of(Object, Object, Object, Object, Object, Object, Object, Object)}
* over alternatives that don't communicate the immutability of the resulting map at the type
* level.
*/
// XXX: Also rewrite the `ImmutableMap.builder()` variant?
static final class ImmutableMapOf4<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {
return Map.of(k1, v1, k2, v2, k3, v3, k4, v4);
}

@AfterTemplate
ImmutableMap<K, V> after(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {
return ImmutableMap.of(k1, v1, k2, v2, k3, v3, k4, v4);
}
}

/**
* Prefer {@link ImmutableMap#of(Object, Object, Object, Object, Object, Object, Object, Object,
* Object, Object)} over alternatives that don't communicate the immutability of the resulting map
* at the type level.
*/
// XXX: Also rewrite the `ImmutableMap.builder()` variant?
static final class ImmutableMapOf5<K, V> {
@BeforeTemplate
Map<K, V> before(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V v5) {
return Map.of(k1, v1, k2, v2, k3, v3, k4, v4, k5, v5);
}

@AfterTemplate
ImmutableMap<K, V> after(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V v5) {
return ImmutableMap.of(k1, v1, k2, v2, k3, v3, k4, v4, k5, v5);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a newline between the class and the comments.


// XXX: Add a template for this:
// Maps.transformValues(streamOfEntries.collect(groupBy(fun)), ImmutableMap::copyOf)
// ->
Expand Down
Loading