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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Guava Preconditions Refaster rules #292

Merged
merged 8 commits into from
Oct 20, 2022
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 @@ -46,11 +46,33 @@ int before(int index, int size) {
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
int after(int index, int size) {
return checkIndex(index, size);
}
}

/**
* Prefer {@link Objects#checkIndex(int, int)} over less descriptive or more verbose alternatives.
*
* <p>If a custom error message is desired, consider using Guava's {@link
* com.google.common.base.Preconditions#checkElementIndex(int, int, String)}.
*/
static final class CheckIndexConditional {
@BeforeTemplate
void before(int index, int size) {
if (index < 0 || index >= size) {
throw new IndexOutOfBoundsException();
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(int index, int size) {
checkIndex(index, size);
}
}

// XXX: We could add a rule for `new EnumMap(Map<K, ? extends V> m)`, but that constructor does
// not allow an empty non-EnumMap to be provided.
static final class CreateEnumMap<K extends Enum<K>, V> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkElementIndex;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkPositionIndex;
import static com.google.common.base.Preconditions.checkState;
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;

import com.google.common.base.Preconditions;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

/** Refaster templates related to statements dealing with {@link Preconditions}. */
@OnlineDocumentation
final class PreconditionsRules {
Copy link
Member

Choose a reason for hiding this comment

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

One more thing:

Suggested change
final class PreconditionsRules {
@OnlineDocumentation
final class PreconditionsRules {

private PreconditionsRules() {}

/** Prefer {@link Preconditions#checkArgument(boolean)} over more verbose alternatives. */
static final class CheckArgument {
@BeforeTemplate
void before(boolean condition) {
if (condition) {
throw new IllegalArgumentException();
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(boolean condition) {
checkArgument(!condition);
}
}

/** Prefer {@link Preconditions#checkArgument(boolean, Object)} over more verbose alternatives. */
static final class CheckArgumentWithMessage {
@BeforeTemplate
void before(boolean condition, String message) {
if (condition) {
throw new IllegalArgumentException(message);
}
}
Comment on lines +39 to +44
Copy link
Member Author

Choose a reason for hiding this comment

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

Opening a thread w/ @Stephan202's message:

The format string variants require special handling and are perhaps better deferred to another ticket (though I have some ideas around that; can write some stuff about that after the meetings I have now).

I also thought of leaving many of those as follow up. 馃憤

One thought on this however. Wouldn't e.g. a String#format() call already partially be handled by the case I had pointed out here? As a result, we'd get

checkArgument(!condition, String.format(message, obj1));

which I'd then expect to be re-written by another Preconditions rule. Curious to hear the thinking.

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 philosophy of the Refaster rules defined in this repository are that they define the smallest/most generic possible improvement, so that in unison they improve the largest amount of code. So what you propose is the way to go.

There's one caveat here: not all String.format calls can be replaced. What I suspect we need is a Matcher that identifies valid Strings.lenientFormat format strings; then we can instruct Refaster to rewrite only eligible expressions using @Matches(IsLenientFormatTemplateString.class). (Best name TBD.)

Copy link
Member

Choose a reason for hiding this comment

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

^ It'll be more "scalable" to handle the String.format logic as a BugChecker, so let's defer that to a separate PR.


@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(boolean condition, String message) {
checkArgument(!condition, message);
}
}

/**
* Prefer {@link Preconditions#checkElementIndex(int, int, String)} over less descriptive or more
* verbose alternatives.
*
* <p>Note that the two-argument {@link Preconditions#checkElementIndex(int, int)} is better
* replaced with {@link java.util.Objects#checkIndex(int, int)}.
*/
static final class CheckElementIndexWithMessage {
@BeforeTemplate
void before(int index, int size, String message) {
if (index < 0 || index >= size) {
throw new IndexOutOfBoundsException(message);
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(int index, int size, String message) {
checkElementIndex(index, size, message);
}
}

/** Prefer {@link Preconditions#checkNotNull(Object)} over more verbose alternatives. */
static final class CheckNotNull<T> {
@BeforeTemplate
void before(T object) {
if (object == null) {
throw new NullPointerException();
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(T object) {
checkNotNull(object);
}
}

/** Prefer {@link Preconditions#checkNotNull(Object, Object)} over more verbose alternatives. */
static final class CheckNotNullWithMessage<T> {
@BeforeTemplate
void before(T object, String message) {
if (object == null) {
throw new NullPointerException(message);
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(T object, String message) {
checkNotNull(object, message);
}
}

/**
* Prefer {@link Preconditions#checkPositionIndex(int, int)} over less descriptive or more verbose
* alternatives.
*/
static final class CheckPositionIndex {
@BeforeTemplate
void before(int index, int size) {
if (index < 0 || index > size) {
throw new IndexOutOfBoundsException();
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(int index, int size) {
checkPositionIndex(index, size);
}
}

/**
* Prefer {@link Preconditions#checkPositionIndex(int, int, String)} over less descriptive or more
* verbose alternatives.
*/
static final class CheckPositionIndexWithMessage {
@BeforeTemplate
void before(int index, int size, String message) {
if (index < 0 || index > size) {
throw new IndexOutOfBoundsException(message);
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(int index, int size, String message) {
checkPositionIndex(index, size, message);
}
}

/** Prefer {@link Preconditions#checkState(boolean)} over more verbose alternatives. */
static final class CheckState {
@BeforeTemplate
void before(boolean condition) {
if (condition) {
throw new IllegalStateException();
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(boolean condition) {
checkState(!condition);
}
}

/** Prefer {@link Preconditions#checkState(boolean, Object)} over more verbose alternatives. */
static final class CheckStateWithMessage {
@BeforeTemplate
void before(boolean condition, String message) {
if (condition) {
throw new IllegalStateException(message);
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(boolean condition, String message) {
checkState(!condition, message);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ final class RefasterRulesTest {
MultimapRules.class,
NullRules.class,
OptionalRules.class,
PreconditionsRules.class,
PrimitiveRules.class,
ReactorRules.class,
RxJava2AdapterRules.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ int testCheckIndex() {
return Preconditions.checkElementIndex(0, 1);
}

void testCheckIndexConditional() {
if (1 < 0 || 1 >= 2) {
throw new IndexOutOfBoundsException();
}
}

Map<RoundingMode, String> testCreateEnumMap() {
return new HashMap<>();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Sets.toImmutableEnumSet;
import static java.util.Objects.checkIndex;

import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
Expand All @@ -19,7 +20,6 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

Expand All @@ -38,7 +38,11 @@ public ImmutableSet<?> elidedTypesAndStaticImports() {
}

int testCheckIndex() {
return Objects.checkIndex(0, 1);
return checkIndex(0, 1);
}

void testCheckIndexConditional() {
checkIndex(1, 2);
}

Map<RoundingMode, String> testCreateEnumMap() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package tech.picnic.errorprone.refasterrules;

import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class PreconditionsRulesTest implements RefasterRuleCollectionTestCase {
void testCheckArgument() {
if ("foo".isEmpty()) {
throw new IllegalArgumentException();
}
}

void testCheckArgumentWithMessage() {
if ("foo".isEmpty()) {
throw new IllegalArgumentException("The string is empty");
}
}

void testCheckElementIndexWithMessage() {
if (1 < 0 || 1 >= 2) {
throw new IndexOutOfBoundsException("My index");
}
}

void testCheckNotNull() {
if ("foo" == null) {
throw new NullPointerException();
}
}

void testCheckNotNullWithMessage() {
if ("foo" == null) {
throw new NullPointerException("The string is null");
}
}

void testCheckPositionIndex() {
if (1 < 0 || 1 > 2) {
throw new IndexOutOfBoundsException();
}
}

void testCheckPositionIndexWithMessage() {
if (1 < 0 || 1 > 2) {
throw new IndexOutOfBoundsException("My position");
}
}

void testCheckState() {
if ("foo".isEmpty()) {
throw new IllegalStateException();
}
}

void testCheckStateWithMessage() {
if ("foo".isEmpty()) {
throw new IllegalStateException("The string is empty");
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package tech.picnic.errorprone.refasterrules;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkElementIndex;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkPositionIndex;
import static com.google.common.base.Preconditions.checkState;

import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class PreconditionsRulesTest implements RefasterRuleCollectionTestCase {
void testCheckArgument() {
checkArgument(!"foo".isEmpty());
}

void testCheckArgumentWithMessage() {
checkArgument(!"foo".isEmpty(), "The string is empty");
}

void testCheckElementIndexWithMessage() {
checkElementIndex(1, 2, "My index");
}

void testCheckNotNull() {
checkNotNull("foo");
}

void testCheckNotNullWithMessage() {
checkNotNull("foo", "The string is null");
}

void testCheckPositionIndex() {
checkPositionIndex(1, 2);
}

void testCheckPositionIndexWithMessage() {
checkPositionIndex(1, 2, "My position");
}

void testCheckState() {
checkState(!"foo".isEmpty());
}

void testCheckStateWithMessage() {
checkState(!"foo".isEmpty(), "The string is empty");
}
}