-
Notifications
You must be signed in to change notification settings - Fork 201
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
[PiranhaJava] Delete statements using EasyMock and JUnit API #156
Changes from 5 commits
4ac47a7
2efd810
a79301a
fc3d50f
4844f6b
1bd3994
96b7b09
a70ca2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
/** | ||
/* | ||
* Copyright (c) 2019 Uber Technologies, Inc. | ||
* | ||
* <p>Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file | ||
|
@@ -14,13 +14,8 @@ | |
package com.uber.piranha; | ||
|
||
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; | ||
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL; | ||
import static com.google.errorprone.matchers.Matchers.allOf; | ||
import static com.google.errorprone.matchers.Matchers.instanceMethod; | ||
import static com.google.errorprone.matchers.Matchers.methodInvocation; | ||
import static com.google.errorprone.matchers.Matchers.receiverOfInvocation; | ||
import static com.google.errorprone.matchers.Matchers.staticMethod; | ||
import static com.google.errorprone.matchers.Matchers.toType; | ||
|
||
import com.facebook.infer.annotation.Initializer; | ||
import com.google.auto.service.AutoService; | ||
|
@@ -34,7 +29,6 @@ | |
import com.google.errorprone.bugpatterns.BugChecker; | ||
import com.google.errorprone.fixes.SuggestedFix; | ||
import com.google.errorprone.matchers.Description; | ||
import com.google.errorprone.matchers.Matcher; | ||
import com.google.errorprone.util.ASTHelpers; | ||
import com.google.errorprone.util.FindIdentifiers; | ||
import com.sun.source.tree.AnnotationTree; | ||
|
@@ -78,8 +72,8 @@ | |
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.OptionalInt; | ||
import java.util.Set; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
import javax.annotation.Nullable; | ||
|
@@ -369,9 +363,9 @@ private API getXPAPI( | |
for (PiranhaMethodRecord methodRecord : methodRecordsForName) { | ||
// when argumentIndex is specified, if mit's argument at argIndex doesn't match xpFlagName, | ||
// skip to next method property map | ||
Optional<Integer> optionalArgumentIdx = methodRecord.getArgumentIdx(); | ||
OptionalInt optionalArgumentIdx = methodRecord.getArgumentIdx(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My type change tool suggested this :) |
||
if (optionalArgumentIdx.isPresent()) { | ||
int argumentIndex = optionalArgumentIdx.get().intValue(); | ||
int argumentIndex = optionalArgumentIdx.getAsInt(); | ||
if (argumentIndex < mit.getArguments().size()) { | ||
ExpressionTree argTree = mit.getArguments().get(argumentIndex); | ||
Symbol argSym = ASTHelpers.getSymbol(argTree); | ||
|
@@ -705,30 +699,38 @@ private Description updateCode( | |
return Description.NO_MATCH; | ||
} | ||
|
||
private static final String MOCKITO_QN = "org.mockito.Mockito"; | ||
private static final String MOCKITO_WHEN = "when"; | ||
private static final Pattern MOCKITO_THEN = Pattern.compile("then\\W*\\w*"); | ||
|
||
private final Matcher<Tree> MOCKITO_UNNECESSARY_MOCKING_PATTERN = | ||
toType( | ||
MethodInvocationTree.class, | ||
allOf( | ||
instanceMethod().anyClass().withNameMatching(MOCKITO_THEN), | ||
receiverOfInvocation( | ||
methodInvocation( | ||
staticMethod().onClass(MOCKITO_QN).named(MOCKITO_WHEN), | ||
ALL, | ||
(argument, vs) -> { | ||
Value value = evalExpr(argument, vs); | ||
return value == Value.TRUE || value == Value.FALSE; | ||
})))); | ||
|
||
/** | ||
* This method picks up the unnecessary test method as configured in properties.json and converts | ||
* them into a Error-prone AST Matcher and then deletes the containing AST statement. | ||
* | ||
* @param state | ||
* @return Suggestion Fix for deleting the statement containing a unnecessary test method | ||
* invocation | ||
*/ | ||
private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) { | ||
ExpressionStatementTree stmt = | ||
MethodInvocationTree enclosingMit = | ||
ASTHelpers.findEnclosingNode(state.getPath(), MethodInvocationTree.class); | ||
|
||
ExpressionStatementTree enclosingEst = | ||
ASTHelpers.findEnclosingNode(state.getPath(), ExpressionStatementTree.class); | ||
if (stmt != null && MOCKITO_UNNECESSARY_MOCKING_PATTERN.matches(stmt.getExpression(), state)) { | ||
endPos = state.getEndPosition(stmt); | ||
return SuggestedFix.builder().delete(stmt); | ||
|
||
if (enclosingMit != null | ||
&& enclosingEst != null | ||
&& config | ||
.getUnnecessaryTestMethodRecords() | ||
.stream() | ||
.map( | ||
mthd -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expand |
||
mthd.isStatic() | ||
? mthd.getReceiverType() | ||
.map(r -> staticMethod().onClass(r)) | ||
.orElseGet(() -> staticMethod().anyClass()) | ||
: mthd.getReceiverType() | ||
.map(r -> instanceMethod().onExactClass(r)) | ||
.orElseGet(() -> instanceMethod().anyClass())) | ||
.anyMatch(matcher -> matcher.matches(enclosingMit, state))) { | ||
endPos = state.getEndPosition(enclosingMit); | ||
return SuggestedFix.builder().delete(enclosingEst); | ||
} | ||
return SuggestedFix.builder(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
/** | ||
* Copyright (c) 2021 Uber Technologies, Inc. | ||
* | ||
* <p>Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file | ||
* except in compliance with the License. You may obtain a copy of the License at | ||
* | ||
* <p>http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* <p>Unless required by applicable law or agreed to in writing, software distributed under the | ||
* License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.uber.piranha.config; | ||
|
||
import static com.uber.piranha.config.PiranhaRecord.getArgumentIndexFromMap; | ||
import static com.uber.piranha.config.PiranhaRecord.getValueBooleanFromMap; | ||
import static com.uber.piranha.config.PiranhaRecord.getValueStringFromMap; | ||
|
||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.OptionalInt; | ||
import javax.annotation.Nullable; | ||
|
||
/** A class representing a method configuration record from properties.json */ | ||
public class MethodRecord { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extracted the super class MethodRecord from PiranhaMethodRecord |
||
|
||
// Allowed fields for a method property in the config file. | ||
// Entered under the top-level "methodProperties" in properties.json. | ||
// By default, the flagType, methodName and argumentIndex fields are mandatory. | ||
// The returnType and receiverType fields are optional. | ||
protected static final String FLAG_TYPE_KEY = "flagType"; | ||
protected static final String METHOD_NAME_KEY = "methodName"; | ||
protected static final String ARGUMENT_INDEX_KEY = "argumentIndex"; | ||
protected static final String RETURN_TYPE_STRING = "returnType"; | ||
protected static final String RECEIVER_TYPE_STRING = "receiverType"; | ||
protected static final String METHOD_IS_STATIC = "isStatic"; | ||
|
||
/** | ||
* Holds the mapping of flagType string to API. Eg: "treated" -> API.IS_TREATED. Is initialized | ||
* once and then accessed without updating. | ||
*/ | ||
private final String methodName; | ||
|
||
@Nullable private final Integer argumentIdx; | ||
@Nullable private final String receiverType; | ||
@Nullable private final String returnType; | ||
private final Boolean isStatic; | ||
|
||
MethodRecord( | ||
String methodName, | ||
@Nullable Integer argumentIdx, | ||
@Nullable String receiverType, | ||
@Nullable String returnType, | ||
@Nullable Boolean isStatic) { | ||
this.methodName = methodName; | ||
this.argumentIdx = argumentIdx; | ||
this.receiverType = receiverType; | ||
this.returnType = returnType; | ||
this.isStatic = isStatic != null && isStatic; | ||
} | ||
|
||
public String getMethodName() { | ||
return methodName; | ||
} | ||
|
||
public OptionalInt getArgumentIdx() { | ||
return Optional.ofNullable(argumentIdx).map(OptionalInt::of).orElseGet(OptionalInt::empty); | ||
} | ||
|
||
public Optional<String> getReceiverType() { | ||
return Optional.ofNullable(receiverType); | ||
} | ||
|
||
public Optional<String> getReturnType() { | ||
return Optional.ofNullable(returnType); | ||
} | ||
|
||
/** | ||
* Parse the entry for a single method from piranha.json that has been previously decoded into a | ||
* map | ||
* | ||
* @param methodPropertyEntry The decoded json entry (as a Map of property names to values) | ||
* @param isArgumentIndexOptional Whether argumentIdx should be treated as optional | ||
* @return A PiranhaMethodRecord corresponding to the given map/json record. | ||
* @throws PiranhaConfigurationException if there was any issue reading or parsing the | ||
* configuration file. | ||
*/ | ||
static MethodRecord parseFromJSONPropertyEntryMap( | ||
Map<String, Object> methodPropertyEntry, boolean isArgumentIndexOptional) | ||
throws PiranhaConfigurationException { | ||
String methodName = getValueStringFromMap(methodPropertyEntry, METHOD_NAME_KEY); | ||
Integer argumentIndexInteger = getArgumentIndexFromMap(methodPropertyEntry, ARGUMENT_INDEX_KEY); | ||
if (methodName == null) { | ||
throw new PiranhaConfigurationException( | ||
"methodProperty is missing mandatory methodName field. Check:\n" + methodPropertyEntry); | ||
} else if (!isArgumentIndexOptional && argumentIndexInteger == null) { | ||
throw new PiranhaConfigurationException( | ||
"methodProperty did not have argumentIndex. By default, Piranha requires an argument index for flag " | ||
+ "APIs, to which the flag name/symbol will be passed. This is to avoid over-deletion of all " | ||
+ "occurrences of a flag API method. If you are sure you want to delete all instances of the " | ||
+ "method below, consider using Piranha:ArgumentIndexOptional=true to override this behavior. " | ||
+ "Check:\n" | ||
+ methodPropertyEntry); | ||
} else if (argumentIndexInteger != null && argumentIndexInteger < 0) { | ||
throw new PiranhaConfigurationException( | ||
"Invalid argumentIndex field. Arguments are zero indexed. Check:\n" | ||
+ methodPropertyEntry); | ||
} | ||
|
||
return new MethodRecord( | ||
methodName, | ||
argumentIndexInteger, | ||
getValueStringFromMap(methodPropertyEntry, RECEIVER_TYPE_STRING), | ||
getValueStringFromMap(methodPropertyEntry, RETURN_TYPE_STRING), | ||
getValueBooleanFromMap(methodPropertyEntry, METHOD_IS_STATIC)); | ||
} | ||
|
||
public boolean isStatic() { | ||
return isStatic; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo:
receiverType