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 ClassRules Refaster rule collection #811

Merged
merged 1 commit into from
Oct 9, 2023
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
@@ -0,0 +1,65 @@
package tech.picnic.errorprone.refasterrules;

import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import java.util.function.Predicate;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;

/** Refaster rules related to expressions dealing with classes. */
@OnlineDocumentation
final class ClassRules {
private ClassRules() {}

/** Prefer {@link Class#isInstance(Object)} over more contrived alternatives. */
static final class ClassIsInstance<T, S> {
@BeforeTemplate
boolean before(Class<T> clazz, S object) {
return clazz.isAssignableFrom(object.getClass());
}

@AfterTemplate
boolean after(Class<T> clazz, S object) {
return clazz.isInstance(object);
}
}

/** Prefer using the {@code instanceof} keyword over less idiomatic alternatives. */
static final class Instanceof<T, S> {
@BeforeTemplate
boolean before(S object) {
return Refaster.<T>clazz().isInstance(object);
}

@AfterTemplate
boolean after(S object) {
return Refaster.<T>isInstance(object);
}
}
Comment on lines +14 to +38
Copy link
Member Author

Choose a reason for hiding this comment

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

These rules are split out, because the second rewrite is possible only for literal class references.


/** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
static final class ClassLiteralIsInstancePredicate<T, S> {
@BeforeTemplate
Predicate<S> before() {
return o -> Refaster.<T>isInstance(o);
}

@AfterTemplate
Predicate<S> after() {
return Refaster.<T>clazz()::isInstance;
}
}

/** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
static final class ClassReferenceIsInstancePredicate<T, S> {
@BeforeTemplate
Predicate<S> before(Class<T> clazz) {
return o -> clazz.isInstance(o);
}

@AfterTemplate
Predicate<S> after(Class<T> clazz) {
return clazz::isInstance;
}
}
Comment on lines +40 to +64
Copy link
Member Author

@Stephan202 Stephan202 Oct 4, 2023

Choose a reason for hiding this comment

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

Hmm, I just realized I don't need to split these out; will add another commit. Similarly here: the first case only applies to literal class references.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package tech.picnic.errorprone.refasterrules;

import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class ClassRulesTest implements RefasterRuleCollectionTestCase {
boolean testClassIsInstance() throws IOException {
return CharSequence.class.isAssignableFrom("foo".getClass());
}

ImmutableSet<Boolean> testInstanceof() throws IOException {
Class<?> clazz = CharSequence.class;
return ImmutableSet.of(CharSequence.class.isInstance("foo"), clazz.isInstance("bar"));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that means we can actually drop this second test case right? What's the reason for keeping it?

Copy link
Member Author

@Stephan202 Stephan202 Oct 6, 2023

Choose a reason for hiding this comment

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

We could, but I wanted to be explicit about covering both cases. We could keep just the second, if you prefer, cause that one is more generic (but that's only clear in the context of the other rules).

Sorry, mixing things up (yes, this PR was harder to write than it looks): that second test case is to prove that non-static references are not replaced, as the result wouldn't compile. (Without this extra check one can replace the Refaster.<T>clazz() with a Class<?> parameter without the test failing; that's what I want to avoid.)

Copy link
Member

@rickie rickie Oct 8, 2023

Choose a reason for hiding this comment

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

Ahh okay clear, let's keep it 😄.

}

Predicate<String> testClassLiteralIsInstancePredicate() throws IOException {
return s -> s instanceof CharSequence;
}

Predicate<String> testClassReferenceIsInstancePredicate() throws IOException {
Class<?> clazz = CharSequence.class;
return s -> clazz.isInstance(s);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package tech.picnic.errorprone.refasterrules;

import com.google.common.collect.ImmutableSet;
import java.io.IOException;
import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;

final class ClassRulesTest implements RefasterRuleCollectionTestCase {
boolean testClassIsInstance() throws IOException {
return CharSequence.class.isInstance("foo");
}

ImmutableSet<Boolean> testInstanceof() throws IOException {
Class<?> clazz = CharSequence.class;
return ImmutableSet.of("foo" instanceof CharSequence, clazz.isInstance("bar"));
}

Predicate<String> testClassLiteralIsInstancePredicate() throws IOException {
return CharSequence.class::isInstance;
}

Predicate<String> testClassReferenceIsInstancePredicate() throws IOException {
Class<?> clazz = CharSequence.class;
return clazz::isInstance;
}
}