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.

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 ImmutableTableRules Refaster rule collection #1489

Merged
merged 2 commits into from
Jan 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public final class ExplicitArgumentEnumeration extends BugChecker
.put(OBJECT_ENUMERABLE_ASSERT, "doesNotContainAnyElementsOf", "doesNotContain")
.put(OBJECT_ENUMERABLE_ASSERT, "hasSameElementsAs", "containsOnly")
.put(STEP_VERIFIER_STEP, "expectNextSequence", "expectNext")
.build();
.buildOrThrow();

/** Instantiates a new {@link ExplicitArgumentEnumeration} instance. */
public ExplicitArgumentEnumeration() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ private static ImmutableTable<String, String, UndesiredStaticImport> getUndesire
}
}

return imports.build();
return imports.buildOrThrow();
}

private static boolean shouldNotBeStaticallyImported(String type, String member) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.collect.ImmutableTable.toImmutableTable;
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;

import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Table;
import com.google.common.collect.Tables;
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.MayOptionallyUse;
import com.google.errorprone.refaster.annotation.Placeholder;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

/** Refaster rules related to expressions dealing with {@link ImmutableTable}s. */
@OnlineDocumentation
final class ImmutableTableRules {
private ImmutableTableRules() {}

/** Prefer {@link ImmutableTable#builder()} over the associated constructor. */
// XXX: This rule may drop generic type information, leading to non-compilable code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it does not drop all generic type information, correct?

Suggested change
// XXX: This rule may drop generic type information, leading to non-compilable code.
// XXX: This rule may drop explicit generic type information, leading to non-compilable code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, but it also says "may", so it's clear that anyway this is an issue only in a subset of cases. :)

static final class ImmutableTableBuilder<R, C, V> {
@BeforeTemplate
ImmutableTable.Builder<R, C, V> before() {
return new ImmutableTable.Builder<>();
}

@AfterTemplate
ImmutableTable.Builder<R, C, V> after() {
return ImmutableTable.builder();
}
}

/**
* Prefer {@link ImmutableTable.Builder#buildOrThrow()} over the less explicit {@link
* ImmutableTable.Builder#build()}.
*/
static final class ImmutableTableBuilderBuildOrThrow<R, C, V> {
@BeforeTemplate
ImmutableTable<R, C, V> before(ImmutableTable.Builder<R, C, V> builder) {
return builder.build();
}

@AfterTemplate
ImmutableTable<R, C, V> after(ImmutableTable.Builder<R, C, V> builder) {
return builder.buildOrThrow();
}
}

/** Prefer {@link ImmutableTable#of(Object, Object, Object)} over more contrived alternatives. */
static final class CellToImmutableTable<R, C, V> {
@BeforeTemplate
ImmutableTable<R, C, V> before(Table.Cell<? extends R, ? extends C, ? extends V> cell) {
return Refaster.anyOf(
ImmutableTable.<R, C, V>builder().put(cell).buildOrThrow(),
Stream.of(cell)
.collect(
toImmutableTable(
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue)));
}

@AfterTemplate
ImmutableTable<R, C, V> after(Table.Cell<? extends R, ? extends C, ? extends V> cell) {
return ImmutableTable.of(cell.getRowKey(), cell.getColumnKey(), cell.getValue());
}
}

/**
* Don't map a stream's elements to table cells, only to subsequently collect them into an {@link
* ImmutableTable}. The collection can be performed directly.
*/
abstract static class StreamOfCellsToImmutableTable<E, R, C, V> {
@Placeholder(allowsIdentity = true)
abstract R rowFunction(@MayOptionallyUse E element);

@Placeholder(allowsIdentity = true)
abstract C columnFunction(@MayOptionallyUse E element);

@Placeholder(allowsIdentity = true)
abstract V valueFunction(@MayOptionallyUse E element);

@BeforeTemplate
ImmutableTable<R, C, V> before(Stream<E> stream) {
return stream
.map(e -> Tables.immutableCell(rowFunction(e), columnFunction(e), valueFunction(e)))
.collect(
toImmutableTable(
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue));
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
ImmutableTable<R, C, V> after(Stream<E> stream) {
return stream.collect(
toImmutableTable(e -> rowFunction(e), e -> columnFunction(e), e -> valueFunction(e)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't these can be lambda expressions?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean method references? 🙃

Unfortunately, Refaster doesn't have a "make this a method reference if possible" feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean method references? 🙃

Yes, sorry 🙈

}
}

/** Prefer {@link ImmutableTable#of()} over more contrived alternatives . */
static final class ImmutableTableOf<R, C, V> {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move this Refaster rule a bit more up? Like right after the builder or the ImmutableTableBuilderBuildOrThrow one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered the same, but modelled this after ImmutableMapRules 🤷. (If/when we decide to auto-sort rules, this kind of more semantic approach to ordering will likely go out the window. Something to consider.)

@BeforeTemplate
ImmutableTable<R, C, V> before() {
return ImmutableTable.<R, C, V>builder().buildOrThrow();
}

@AfterTemplate
ImmutableTable<R, C, V> after() {
return ImmutableTable.of();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ final class RefasterRulesTest {
ImmutableSortedMapRules.class,
ImmutableSortedMultisetRules.class,
ImmutableSortedSetRules.class,
ImmutableTableRules.class,
IntStreamRules.class,
JUnitRules.class,
JUnitToAssertJRules.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.collect.ImmutableTable.toImmutableTable;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Table;
import com.google.common.collect.Tables;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class ImmutableTableRulesTest implements RefasterRuleCollectionTestCase {
@Override
public ImmutableSet<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Table.class);
}

ImmutableTable.Builder<String, Integer, String> testImmutableTableBuilder() {
return new ImmutableTable.Builder<>();
}

ImmutableTable<Object, Object, Object> testImmutableTableBuilderBuildOrThrow() {
return ImmutableTable.builder().build();
}

ImmutableSet<ImmutableTable<String, Integer, String>> testCellToImmutableTable() {
return ImmutableSet.of(
ImmutableTable.<String, Integer, String>builder()
.put(Tables.immutableCell("foo", 1, "bar"))
.buildOrThrow(),
Stream.of(Tables.immutableCell("baz", 2, "qux"))
.collect(
toImmutableTable(
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue)));
}

ImmutableTable<Integer, String, Integer> testStreamOfCellsToImmutableTable() {
return Stream.of(1, 2, 3)
.map(n -> Tables.immutableCell(n, n.toString(), n * 2))
.collect(
toImmutableTable(
Table.Cell::getRowKey, Table.Cell::getColumnKey, Table.Cell::getValue));
}

ImmutableTable<String, String, String> testImmutableTableOf() {
return ImmutableTable.<String, String, String>builder().buildOrThrow();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.collect.ImmutableTable.toImmutableTable;

import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Table;
import com.google.common.collect.Tables;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class ImmutableTableRulesTest implements RefasterRuleCollectionTestCase {
@Override
public ImmutableSet<Object> elidedTypesAndStaticImports() {
return ImmutableSet.of(Table.class);
}

ImmutableTable.Builder<String, Integer, String> testImmutableTableBuilder() {
return ImmutableTable.builder();
}

ImmutableTable<Object, Object, Object> testImmutableTableBuilderBuildOrThrow() {
return ImmutableTable.builder().buildOrThrow();
}

ImmutableSet<ImmutableTable<String, Integer, String>> testCellToImmutableTable() {
return ImmutableSet.of(
ImmutableTable.of(
Tables.immutableCell("foo", 1, "bar").getRowKey(),
Tables.immutableCell("foo", 1, "bar").getColumnKey(),
Tables.immutableCell("foo", 1, "bar").getValue()),
ImmutableTable.of(
Tables.immutableCell("baz", 2, "qux").getRowKey(),
Tables.immutableCell("baz", 2, "qux").getColumnKey(),
Tables.immutableCell("baz", 2, "qux").getValue()));
}

ImmutableTable<Integer, String, Integer> testStreamOfCellsToImmutableTable() {
return Stream.of(1, 2, 3).collect(toImmutableTable(n -> n, n -> n.toString(), n -> n * 2));
}

ImmutableTable<String, String, String> testImmutableTableOf() {
return ImmutableTable.of();
}
}