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.

Sign up for GitHub

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

Jump to bottom

PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json #39

Merged
merged 11 commits into from Jun 15, 2020
Merged

PiranhaJava // config: add returnType, receiverType and argumentIndex; move to properties.json #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f9847f5
add no flag case in getXPAPI
pranavsb May 19, 2020
7ba5256
Add receiver and return type support; move config to properties.json
pranavsb May 24, 2020
62b200c
Add properties.json to sample; delete old config
pranavsb May 24, 2020
e13b58a
Add properties.json to sample; delete old config
pranavsb May 26, 2020
230a938
Support argumentIndex in properties.json
pranavsb Jun 2, 2020
ef06c2b
Update argumentIndex in README
pranavsb Jun 2, 2020
c89c433
Rebase master, review comments and refactoring
pranavsb Jun 3, 2020
36dbefd
Increase test coverage, refactoring
pranavsb Jun 5, 2020
76c0c8c
Increase test coverage, refactoring
pranavsb Jun 5, 2020
0af7af9
Add ArgumentIndexOptional; add tests
pranavsb Jun 14, 2020
ff59d06
Case-sensitive match return and receiver; refactor
pranavsb Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 15 additions & 12 deletions java/README.md
View file
Open in desktop
Expand Up @@ -67,17 +67,16 @@ The properties file has the following template:
```

The required top-level field is `methodProperties`.
Within that, there is an array of JSON objects, having the required fields `methodName` and `flagType`.
The optional fields are `returnType`, `receiverType` and `argumentIndex`.
Within that, there is an array of JSON objects, having the required fields `methodName`, `flagType` and `argumentIndex`.
The optional fields are `returnType`, `receiverType`.

The `flagType` with `treated` are the APIs which correspond to the treatment behavior of the flag, `control` correspond to the control behavior of the flag. In the above example, the API `flagEnabled` corresponds to treatment behavior. Hence, when `IsTreated` Piranha argument is set to `true`, `flagEnabled(SAMPLE_STALE_FLAG)` will be evaluated to `true`. Similarly, `flagDisabled(SAMPLE_STALE_FLAG)` which corresponds to the control behavior will evaluate to `false`.
The `flagType` with `treated` are the APIs which correspond to the treatment behavior of the flag, `control` correspond to the control behavior of the flag. In the above example, the API `flagEnabled` corresponds to treatment behavior. Hence, when the `IsTreated` Piranha argument is set to `true`, `flagEnabled(SAMPLE_STALE_FLAG)` will be evaluated to `true`. Similarly, `flagDisabled(SAMPLE_STALE_FLAG)` which corresponds to the control behavior will evaluate to `false`.
The `flagType` with `empty` specifies the APIs which need to be discarded from the code. For example, if `enableFlag` is listed as a method in properties.json, with `flagType=empty`, a statement `enableFlag(SAMPLE_STALE_FLAG);` will be deleted from the code.

The `flagType` with `empty` specifies the APIs which need to be discarded from the code. For example, a statement `enableFlag(SAMPLE_STALE_FLAG);` will be deleted from the code.
The `argumentIndex` field specifies where to look for the flag name (given by `-XepOpt:Piranha:FlagName`) in the method's arguments. We follow 0 based indexing.
If your toggle methods take no arguments, or if you want to delete all occurrences of a given `methodName` irrespective of their arguments, you can set the `Piranha:ArgumentIndexOptional` to `true` to make specifying `argumentIndex` optional.

The optional field `argumentIndex` specifies where to look for the flag name (given by `-XepOpt:Piranha:FlagName`) in the method's arguments. We follow 0 based indexing.
If your toggle method takes no arguments, or if you want to delete all occurrences of a given `methodName` irrespective of their arguments, `argumentIndex` need not be specified.

For `returnType` and `receiverType`, types should be written as `boolean` or `void` for inbuilt types, and fully qualified for custom defined types. eg: `com.uber.piranha.XPFlagCleanerPositiveCases.XPTest`
For `returnType` and `receiverType`, types should be written as `boolean` or `void` for primitive types, and fully qualified for custom defined types. eg: `com.uber.piranha.XPTest` or `java.lang.String` (not case-sensitive)

The `annotations` specify the annotations used (e.g., in unit testing) to determine treatment or control behavior. For example:

Expand Down Expand Up @@ -134,19 +133,23 @@ where `properties.json` contains the following,
[
{
"methodName": "flagEnabled",
"flagType": "treated"
"flagType": "treated",
"argumentIndex": 0
},
{
"methodName": "flagDisabled",
"flagType": "control"
"flagType": "control",
"argumentIndex": 0
},
{
"methodName": "enableFlag",
"flagType": "empty"
"flagType": "empty",
"argumentIndex": 0
},
{
"methodName": "disableFlag",
"flagType": "empty"
"flagType": "empty",
"argumentIndex": 0
}
],
"linkURL": "<provide_your_url>",
Expand Down
7 changes: 4 additions & 3 deletions java/piranha/config/properties.json
View file
Open in desktop
Expand Up @@ -29,16 +29,17 @@
},
{
"methodName": "includeEvent",
"flagType": "empty"
"flagType": "empty",
"argumentIndex": 0
},
{
"methodName": "isToggleInGroup",
"flagType": "treatmentGroup"
"flagType": "treatmentGroup",
"argumentIndex": 0
}
],
"linkURL": "<provide_your_url>",
"annotations": [
"ToggleTesting"
]
}

66 changes: 35 additions & 31 deletions java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
View file
Open in desktop
Expand Up @@ -108,12 +108,13 @@ public class XPFlagCleaner extends BugChecker

// Allowed fields for a method property in the config file.
// Entered under the top-level "methodProperties" in properties.json.
// flagType and methodName are mandatory. The rest are optional.
// By default, the flagType, methodName and argumentIndex fields are mandatory.
// The returnType and receiverType fields are optional.
private static final String FLAG_TYPE_KEY = "flagType";
private static final String METHOD_NAME_KEY = "methodName";
private static final String ARGUMENT_INDEX_KEY = "argumentIndex";
private static final String RETURN_TYPE_STRING = "returnType";
private static final String RECEIVER_TYPE_STRING = "receiverType";
private static final String ARGUMENT_INDEX_STRING = "argumentIndex";

/**
* Holds the mapping of flagType string to API. Eg: "treated" -> API.IS_TREATED. Is initialized
Expand Down Expand Up @@ -224,13 +225,19 @@ void init(ErrorProneFlags flags) throws ParseException {
methodProperties.addAll(
(List<Map<String, Object>>) propertiesJson.get("methodProperties"));
} else {
throw new ParseException("", 0);
throw new ParseException("methodProperties not found.", 0);
}
addMethodPropertiesToConfigMethodProperties(methodProperties);
Optional<String> argumentIndexOptional = flags.get("Piranha:ArgumentIndexOptional");
boolean isArgumentIndexOptional = false;
if (argumentIndexOptional.isPresent()
&& TRUE.equalsIgnoreCase(argumentIndexOptional.get())) {
isArgumentIndexOptional = true;
}
addMethodPropertiesToConfigMethodProperties(methodProperties, isArgumentIndexOptional);
} catch (IOException fnfe) {
throw new ParseException("Provided config file is not found", 0);
} catch (ParseException pe) {
throw new ParseException("Invalid or incorrectly formatted config file.", 0);
throw new ParseException("Invalid or incorrectly formatted config file. " + pe, 0);
} catch (Exception e) {
throw new ParseException("Some other exception thrown while parsing config", 0);
}
Expand Down Expand Up @@ -311,20 +318,27 @@ public String linkUrl() {
}

private void addMethodPropertiesToConfigMethodProperties(
Set<Map<String, Object>> methodProperties) {
Set<Map<String, Object>> methodProperties, boolean isArgumentIndexOptional)
throws ParseException {
for (Map<String, Object> methodProperty : methodProperties) {
String flagType = getValueStringFromMap(methodProperty, FLAG_TYPE_KEY);
String methodName = getValueStringFromMap(methodProperty, METHOD_NAME_KEY);
if (flagType != null && methodName != null) {
if (methodName != null && getValueStringFromMap(methodProperty, FLAG_TYPE_KEY) != null) {
if (!isArgumentIndexOptional && getArgumentIndexFromMap(methodProperty) == null) {
throw new ParseException(
"methodProperty did not have argumentIndex. Use Piranha:ArgumentIndexOptional if required. Check:"
pranavsb marked this conversation as resolved.
Show resolved Hide resolved
+ methodProperty,
0);
}
List<Map<String, Object>> methodPropertiesForMethodName =
configMethodProperties.get(methodName);
if (methodPropertiesForMethodName != null) {
methodPropertiesForMethodName.add(methodProperty);
} else {
List<Map<String, Object>> methodPropertiesForNewMethodName = new ArrayList<>();
methodPropertiesForNewMethodName.add(methodProperty);
configMethodProperties.put(methodName, methodPropertiesForNewMethodName);
if (methodPropertiesForMethodName == null) {
methodPropertiesForMethodName = new ArrayList<>();
configMethodProperties.put(methodName, methodPropertiesForMethodName);
}
methodPropertiesForMethodName.add(methodProperty);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

For readability, consider reversing this conditional, so you have:

if (methodName == null || getValueStringFromMap(methodProperty, FLAG_TYPE_KEY) == null) {
   throw ...;
}
if (!isArgumentIndexOptional && getArgumentIndexFromMap(methodProperty) == null) {
   throw ...;
}
List<Map<String, Object>> methodPropertiesForMethodName = ...;
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Also replaced the add methodProperty/new ArrayList logic with a one-liner using computeIfAbsent

throw new ParseException(
"methodProperty did not have methodName or flagType. Check:" + methodProperty, 0);
}
}
}
Expand Down Expand Up @@ -372,15 +386,17 @@ private API getXPAPI(
// if it's not a match, skip to next method property map
String returnType = getValueStringFromMap(currentMethodProperties, RETURN_TYPE_STRING);
if (returnType != null) {
if (!isMethodTreeMatchesMethodProperty(mst, RETURN_TYPE_STRING, returnType)) {
String mReturn = ASTHelpers.getReturnType(mst).toString();
if (!returnType.equalsIgnoreCase(mReturn)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why equalsIgnoreCase instead of case sensitive equals?

Copy link
Contributor Author

@pranavsb pranavsb Jun 15, 2020 •
edited

Choose a reason for hiding this comment

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

Just to allow users the flexibility of wrongly capitalizing stuff. Like java.lang.string or the fully qualified name of their class.

@lazaroclapp - waiting for your followup - if any - before pushing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I say let's compare case-sensitive, since Java itself is case sensitive, I think, e.g. you can have both a type com.foo.Bar and one called com.foo.bar at the same time in the same JVM (you probably shouldn't, but still). That said, I have no super strong opinion here. cc: @mkr-plse Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...you can have both a type com.foo.Bar and one called com.foo.bar ...

Oh, didn't know this. Then a case sensitive match is a better default for now - just in case someone does this.

Updated and fixed testcase.

continue;
}
}
// when receiverType is specified, check if mst's receiver type matches it
// if it's not a match, skip to next method property map
String receiverType = getValueStringFromMap(currentMethodProperties, RECEIVER_TYPE_STRING);
if (receiverType != null) {
if (!isMethodTreeMatchesMethodProperty(mst, RECEIVER_TYPE_STRING, receiverType)) {
String mReceive = ASTHelpers.getReceiverType(mst).toString();
if (!receiverType.equalsIgnoreCase(mReceive)) {
continue;
}
}
Expand All @@ -399,18 +415,6 @@ private static Map<String, API> initializeFlagTypeToAPIMap() {
return Collections.unmodifiableMap(map);
}

private boolean isMethodTreeMatchesMethodProperty(
MemberSelectTree mst, String propKey, String propValue) {
if (RETURN_TYPE_STRING.equals(propKey)) {
String mReturn = ASTHelpers.getReturnType(mst).toString();
return propValue.equalsIgnoreCase(mReturn);
} else if (RECEIVER_TYPE_STRING.equals(propKey)) {
String mReceive = ASTHelpers.getReceiverType(mst).toString();
return propValue.equalsIgnoreCase(mReceive);
}
return false;
}

private boolean isArgumentMatchesFlagName(ExpressionTree argTree, Symbol argSym) {
return (isLiteralTreeAndMatchesFlagName(argTree)
|| isVarSymbolAndMatchesFlagName(argSym)
Expand Down Expand Up @@ -1034,7 +1038,7 @@ private boolean endsWithReturn(StatementTree stmt) {
* @param key - key to check the corresponding value
* @return String if value is a non-empty string, null otherwise
*/
private String getValueStringFromMap(Map<String, Object> map, String key) {
private static String getValueStringFromMap(Map<String, Object> map, String key) {
Object value = map.get(key);
if (value instanceof String && !value.equals(EMPTY)) {
return String.valueOf(value);
Expand All @@ -1049,8 +1053,8 @@ private String getValueStringFromMap(Map<String, Object> map, String key) {
* @param map - map corresponding to a method property
* @return argumentIndex if argument index is a non-negative integer, null otherwise
*/
private Integer getArgumentIndexFromMap(Map<String, Object> map) {
Object value = map.get(ARGUMENT_INDEX_STRING);
private static Integer getArgumentIndexFromMap(Map<String, Object> map) {
Object value = map.get(ARGUMENT_INDEX_KEY);
if (value instanceof Long) {
int argumentIndex = ((Long) value).intValue();
if (argumentIndex >= ZERO) {
Expand Down