Skip to content

Commit

Permalink
Provide possible assignment solutions
Browse files Browse the repository at this point in the history
  • Loading branch information
Stephan202 committed Nov 9, 2023
1 parent c732711 commit 2668359
Show file tree
Hide file tree
Showing 19 changed files with 137 additions and 77 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.MethodInvocationTree;
Expand Down Expand Up @@ -46,11 +47,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}

SuggestedFix.Builder fix = SuggestedFix.builder();

// XXX: Using `fix.merge(<some code>);` make sure we rename the method invocation to `isNull`.
// See the `SuggestedFixes` class ;).

SuggestedFix.Builder fix =
SuggestedFix.builder().merge(SuggestedFixes.renameMethodInvocation(tree, "isNull", state));
tree.getArguments().forEach(arg -> fix.merge(SuggestedFix.delete(arg)));

return describeMatch(tree, fix.build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodTree;

/** A {@link BugChecker} that flags empty methods that seemingly can simply be deleted. */
Expand All @@ -25,9 +27,12 @@ public DeleteEmptyMethod() {}

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
// XXX: Part 1: Ensure that we only delete methods that contain no statements.
// XXX: Part 2: Don't delete methods that are annotated with `@Override`.
if (tree.getBody() == null
|| !tree.getBody().getStatements().isEmpty()
|| ASTHelpers.hasAnnotation(tree, Override.class.getName(), state)) {
return Description.NO_MATCH;
}

return Description.NO_MATCH;
return describeMatch(tree, SuggestedFix.delete(tree));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.TypesWithUndefinedEquality;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
Expand All @@ -24,6 +25,7 @@
import com.sun.tools.javac.code.Types;
import java.util.Arrays;
import java.util.List;
import tech.picnic.errorprone.workshop.bugpatterns.util.SourceCode;

/** A {@link BugChecker} that flags redundant identity conversions. */
@BugPattern(
Expand Down Expand Up @@ -63,9 +65,7 @@ public DeleteIdentityConversion() {}
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
List<? extends ExpressionTree> arguments = tree.getArguments();
// XXX: Make sure we skip invocations that do not pass exactly one argument, by using the
// `tree`.
if (!IS_CONVERSION_METHOD.matches(tree, state)) {
if (arguments.size() != 1 || !IS_CONVERSION_METHOD.matches(tree, state)) {
return Description.NO_MATCH;
}

Expand All @@ -88,9 +88,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

return buildDescription(tree)
// XXX: Use the `.addFix()` to suggest replacing the original `tree` with the `sourceTree`.
// Tip: You can get the actual String representation of a Tree by using the
// `SourceCode#treeToString`.
.addFix(SuggestedFix.replace(tree, SourceCode.treeToString(sourceTree, state)))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.MultiMatcher;
import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import java.util.ArrayList;
import java.util.List;
import tech.picnic.errorprone.workshop.bugpatterns.util.SourceCode;

Expand All @@ -38,9 +38,10 @@ public DropAutowiredConstructorAnnotation() {}

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
// XXX: Using the `ASTHelpers#getConstructors` method, return `Description.NO_MATCH` if we do
// not have exactly 1 constructor (so drop the `new ArrayList<>()` part).
List<MethodTree> constructors = new ArrayList<>();
List<MethodTree> constructors = ASTHelpers.getConstructors(tree);
if (constructors.size() != 1) {
return Description.NO_MATCH;
}

MultiMatchResult<AnnotationTree> hasAutowiredAnnotation =
AUTOWIRED_ANNOTATION.multiMatchResult(Iterables.getOnlyElement(constructors), state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.List;
import tech.picnic.errorprone.workshop.bugpatterns.util.SourceCode;

/**
Expand All @@ -34,11 +35,13 @@ public DropMockitoEq() {}

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
// XXX: Make sure to return `Description.NO_MATCH` if the `tree` doesn't have arguments, or if
// `isEqInvocation` returns `false` for at least one argument.
List<? extends ExpressionTree> arguments = tree.getArguments();
if (arguments.isEmpty() || !arguments.stream().allMatch(arg -> isEqInvocation(arg, state))) {
return Description.NO_MATCH;
}

SuggestedFix.Builder suggestedFix = SuggestedFix.builder();
for (ExpressionTree arg : tree.getArguments()) {
for (ExpressionTree arg : arguments) {
suggestedFix.replace(
arg,
SourceCode.treeToString(
Expand All @@ -48,7 +51,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return describeMatch(tree, suggestedFix.build());
}

@SuppressWarnings("UnusedMethod" /* Recommended to use when implementing this assignment. */)
private static boolean isEqInvocation(ExpressionTree tree, VisitorState state) {
return tree instanceof MethodInvocationTree && MOCKITO_EQ_METHOD.matches(tree, state);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.MultiMatcher;
import com.sun.source.tree.AnnotationTree;
Expand All @@ -27,10 +28,6 @@
summary = "JUnit method declaration can likely be improved",
severity = WARNING,
tags = SIMPLIFICATION)
@SuppressWarnings({
"UnusedMethod",
"UnusedVariable"
} /* This check is yet to be implemented as part of the demo. */)
public final class JUnitTestMethodDeclaration extends BugChecker implements MethodTreeMatcher {
private static final long serialVersionUID = 1L;
private static final ImmutableSet<Modifier> ILLEGAL_MODIFIERS =
Expand All @@ -43,12 +40,13 @@ public JUnitTestMethodDeclaration() {}

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
// XXX: Part 1: Return `Description.NO_MATCH` if the method is not a `TEST_METHOD`.
if (!TEST_METHOD.matches(tree, state)) {
return Description.NO_MATCH;
}

SuggestedFix.Builder fixBuilder = SuggestedFix.builder();

// XXX: Part 2: Make sure that JUnit test methods don't use `ILLEGAL_MODIFIERS` by using
// `SuggestedFixes#removeModifiers` and `SuggestedFix.Builder#merge`.
SuggestedFixes.removeModifiers(tree.getModifiers(), state, ILLEGAL_MODIFIERS)
.ifPresent(fixBuilder::merge);

if (fixBuilder.isEmpty()) {
return Description.NO_MATCH;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,22 @@
package tech.picnic.errorprone.workshop.refasterrules;

import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;

/** Refaster rule used as example for the assignments of the workshop. */
final class WorkshopAssignment0Rules {
private WorkshopAssignment0Rules() {}

/** Prefer {@link String#isEmpty()} over alternatives that consult the string's length. */
static final class ExampleStringIsEmpty {}
static final class ExampleStringIsEmpty {
@BeforeTemplate
boolean before(String s) {
return s.length() == 0;
}

@AfterTemplate
boolean after(String s) {
return s.isEmpty();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
package tech.picnic.errorprone.workshop.refasterrules;

import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;

/** Refaster rules for the first assignment of the workshop. */
final class WorkshopAssignment1Rules {
private WorkshopAssignment1Rules() {}

/** Prefer {@link String#String(char[])} over {@link String#copyValueOf(char[])}. */
static final class NewStringCharArray {
// XXX: Implement this Refaster rule.
@BeforeTemplate
String before(char[] chars) {
return String.copyValueOf(chars);
}

@AfterTemplate
String after(char[] chars) {
return new String(chars);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package tech.picnic.errorprone.workshop.refasterrules;

import com.google.common.collect.ImmutableList;
import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import java.util.Collections;
import java.util.List;

/** Refaster rules for the second assignment of the workshop. */
@SuppressWarnings("UnusedTypeParameter" /* Ignore this for demo purposes. */)
final class WorkshopAssignment2Rules {
private WorkshopAssignment2Rules() {}

Expand All @@ -12,6 +16,14 @@ private WorkshopAssignment2Rules() {}
* immutability of the resulting list at the type level.
*/
static final class ImmutableListOfOne<T> {
// XXX: Implement this Refaster rule.
@BeforeTemplate
List<T> before(T element) {
return Refaster.anyOf(Collections.singletonList(element), List.of(element));
}

@AfterTemplate
ImmutableList<T> after(T element) {
return ImmutableList.of(element);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,21 +1,46 @@
package tech.picnic.errorprone.workshop.refasterrules;

import static com.google.common.base.Preconditions.checkArgument;
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;

/** Refaster rules for the third assignment of the workshop. */
@SuppressWarnings("UnusedTypeParameter" /* Ignore this for demo purposes. */)
final class WorkshopAssignment3Rules {
private WorkshopAssignment3Rules() {}

// XXX: Tip: check the input and output files to see the *expected* refactoring.

/** Prefer {@link Preconditions#checkArgument(boolean)} over if statements. */
static final class CheckArgumentWithoutMessage {
// XXX: Implement the Refaster rule to get the test green.
@BeforeTemplate
void before(boolean expression) {
if (expression) {
throw new IllegalArgumentException();
}
}

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

/** Prefer {@link Preconditions#checkArgument(boolean, Object)} over if statements. */
static final class CheckArgumentWithMessage {
// XXX: Implement the Refaster rule to get the test green.
@BeforeTemplate
void before(boolean expression, String message) {
if (expression) {
throw new IllegalArgumentException(message);
}
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after(boolean expression, String message) {
checkArgument(!expression, message);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
package tech.picnic.errorprone.workshop.refasterrules;

import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.AlsoNegation;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import java.util.Objects;

/** Refaster rules for the fourth assignment of the workshop. */
@SuppressWarnings("java:S1698" /* Reference comparison is valid for enums. */)
final class WorkshopAssignment4Rules {
private WorkshopAssignment4Rules() {}

// The test fails because non Enum comparisons are also rewritten.
// Fix the test by tweaking the type parameters.

// XXX: Get the test to pass by improving the Refaster rule (uncommented it first).
static final class PrimitiveOrReferenceEqualityEnum<T extends Enum<T>> {
@BeforeTemplate
boolean before(T a, T b) {
return Refaster.anyOf(a.equals(b), Objects.equals(a, b));
}

// static final class PrimitiveOrReferenceEqualityEnum<T> {
// @BeforeTemplate
// boolean before(T a, T b) {
// return Refaster.anyOf(a.equals(b), Objects.equals(a, b));
// }
//
// @AfterTemplate
// @AlsoNegation
// boolean after(T a, T b) {
// return a == b;
// }
// }
@AfterTemplate
@AlsoNegation
boolean after(T a, T b) {
return a == b;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
package tech.picnic.errorprone.workshop.refasterrules;

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 java.util.stream.Stream;

/** Refaster rules for the fifth assignment of the workshop. */
@SuppressWarnings("UnusedTypeParameter" /* Ignore this for demo purposes. */)
final class WorkshopAssignment5Rules {
private WorkshopAssignment5Rules() {}

abstract static class StreamDoAllMatch<T> {
// XXX: Implement the Refaster rule to get the test green.
// Tip: use the `@Placeholder` annotation.
@Placeholder
abstract boolean test(@MayOptionallyUse T element);

@BeforeTemplate
boolean before(Stream<T> stream) {
return stream.noneMatch(v -> !test(v));
}

@AfterTemplate
boolean after(Stream<T> stream) {
return stream.allMatch(v -> test(v));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@
import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

@Disabled("Enable this when implementing the BugChecker.")
final class AssertJIsNullMethodTest {
@Test
void identification() {
Expand Down
Loading

0 comments on commit 2668359

Please sign in to comment.