Skip to content

Commit

Permalink
Implement a suggested fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@carterkozak
carterkozak committed Oct 11, 2019
1 parent a3494b5 commit 0dfcca6
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 12 deletions.
68 changes: 56 additions & 12 deletions baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ThrowError.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,18 @@
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ThrowTree;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.Optional;

@AutoService(BugChecker.class)
@BugPattern(
Expand All @@ -44,23 +51,60 @@
+ "that throws AssertionError.")
public final class ThrowError extends BugChecker implements BugChecker.ThrowTreeMatcher {

private static final Matcher<ExpressionTree> compileTimeConstExpressionMatcher =
new CompileTimeConstantExpressionMatcher();
private static final String ERROR_NAME = Error.class.getName();

@Override
public Description matchThrow(ThrowTree tree, VisitorState state) {
ExpressionTree expression = tree.getExpression();
if (expression instanceof NewClassTree) {
NewClassTree newClassTree = (NewClassTree) expression;
if (ASTHelpers.isCastable(
ASTHelpers.getType(newClassTree.getIdentifier()),
state.getTypeFromString(ERROR_NAME),
state)
// Don't discourage developers from testing edge cases involving Errors.
// It's also fine for tests throw AssertionError internally in test objects.
&& !TestCheckUtils.isTestCode(state)) {
return describeMatch(tree);
}
if (!(expression instanceof NewClassTree)) {
return Description.NO_MATCH;
}
return Description.NO_MATCH;
NewClassTree newClassTree = (NewClassTree) expression;
Type throwableType = ASTHelpers.getType(newClassTree.getIdentifier());
if (!ASTHelpers.isCastable(
throwableType,
state.getTypeFromString(ERROR_NAME),
state)
// Don't discourage developers from testing edge cases involving Errors.
// It's also fine for tests throw AssertionError internally in test objects.
|| TestCheckUtils.isTestCode(state)) {
return Description.NO_MATCH;
}
return buildDescription(tree)
.addFix(generateFix(newClassTree, state))
.build();
}

private static Optional<SuggestedFix> generateFix(NewClassTree newClassTree, VisitorState state) {
Type throwableType = ASTHelpers.getType(newClassTree.getIdentifier());
// AssertionError is the most common failure case we've encountered, likely because it sounds
// similar to IllegalStateException. In this case we suggest replacing it with IllegalStateException.
if (!ASTHelpers.isSameType(
throwableType,
state.getTypeFromString(AssertionError.class.getName()),
state)) {
return Optional.empty();
}
List<? extends ExpressionTree> arguments = newClassTree.getArguments();
if (arguments.isEmpty()) {
SuggestedFix.Builder fix = SuggestedFix.builder();
String qualifiedName = SuggestedFixes.qualifyType(state, fix, IllegalStateException.class.getName());
return Optional.of(fix.replace(newClassTree.getIdentifier(), qualifiedName).build());
}
ExpressionTree firstArgument = arguments.get(0);
if (ASTHelpers.isSameType(
ASTHelpers.getResultType(firstArgument),
state.getTypeFromString(String.class.getName()),
state)) {
SuggestedFix.Builder fix = SuggestedFix.builder();
String qualifiedName = SuggestedFixes.qualifyType(state, fix,
compileTimeConstExpressionMatcher.matches(firstArgument, state)
? "com.palantir.logsafe.exceptions.SafeIllegalStateException"
: IllegalStateException.class.getName());
return Optional.of(fix.replace(newClassTree.getIdentifier(), qualifiedName).build());
}
return Optional.empty();
}
}
50 changes: 50 additions & 0 deletions baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ThrowErrorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.palantir.baseline.errorprone;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.Execution;
Expand Down Expand Up @@ -78,7 +79,56 @@ void testRethrowIsAllowed() {
).doTest();
}

@Test
void testFix() {
fix()
.addInputLines(
"Test.java",
"class Test {",
" void f1() {",
" throw new AssertionError();",
" }",
" void f2(String nonConstant) {",
" throw new AssertionError(nonConstant);",
" }",
" void f3() {",
" throw new AssertionError(\"constant\");",
" }",
" void f4(String nonConstant, Throwable t) {",
" throw new AssertionError(nonConstant, t);",
" }",
" void f5(Throwable t) {",
" throw new AssertionError(\"constant\", t);",
" }",
"}")
.addOutputLines(
"Test.java",
"import com.palantir.logsafe.exceptions.SafeIllegalStateException;",
"class Test {",
" void f1() {",
" throw new IllegalStateException();",
" }",
" void f2(String nonConstant) {",
" throw new IllegalStateException(nonConstant);",
" }",
" void f3() {",
" throw new SafeIllegalStateException(\"constant\");",
" }",
" void f4(String nonConstant, Throwable t) {",
" throw new IllegalStateException(nonConstant, t);",
" }",
" void f5(Throwable t) {",
" throw new SafeIllegalStateException(\"constant\", t);",
" }",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

private CompilationTestHelper helper() {
return CompilationTestHelper.newInstance(ThrowError.class, getClass());
}

private BugCheckerRefactoringTestHelper fix() {
return BugCheckerRefactoringTestHelper.newInstance(new ThrowError(), getClass());
}
}
1 change: 1 addition & 0 deletions ...ne-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public class BaselineErrorProneExtension {
"PreferSafeLoggingPreconditions",
"StrictUnusedVariable",
"StringBuilderConstantParameters",
"ThrowError",

// Built-in checks
"ArrayEquals",
Expand Down

0 comments on commit 0dfcca6

Please sign in to comment.