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

clean up enum and comma in annotation #159

Merged
Show file tree
Hide file tree
Changes from 2 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
64 changes: 63 additions & 1 deletion java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.tree.JCTree;
import com.uber.piranha.config.Config;
import com.uber.piranha.config.PiranhaConfigurationException;
import com.uber.piranha.config.PiranhaEnumRecord;
Expand All @@ -75,6 +76,7 @@
import java.util.OptionalInt;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import javax.lang.model.element.ElementKind;
Expand Down Expand Up @@ -1374,7 +1376,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
// Note that the AST elements referenced by deletableAnnotations and deletableIdentifiers
// should not overlap, given the logic above, so deleting each independently is safe.
for (ExpressionTree expr : deletableIdentifiers) {
fixBuilder.delete(expr);
fixBuilder = deleteExprWithComma(state, expr, resolvedTestAnnotations, fixBuilder);
decrementAllSymbolUsages(expr, state, fixBuilder);
}
for (AnnotationTree at : deletableAnnotations) {
Expand Down Expand Up @@ -1564,6 +1566,66 @@ private boolean endsWithReturn(StatementTree stmt) {
return false;
}

/** removes enum in annotation and the comma if there is more than one enum in it */
private SuggestedFix.Builder deleteExprWithComma(
VisitorState state,
ExpressionTree expressionTree,
ImmutableSet<ResolvedTestAnnotation> resolvedTestAnnotations,
SuggestedFix.Builder fixBuilder) {
for (ResolvedTestAnnotation resolvedTestAnnotation : resolvedTestAnnotations) {

// remove the comma before or after the enum depending on the index
int index =
IntStream.range(0, resolvedTestAnnotation.getFlags().size())
.filter(i -> resolvedTestAnnotation.getFlags().get(i).getValue().equals(xpFlagName))
.findFirst()
.orElse(-1);
if (index != -1) {
JCTree node = (JCTree) resolvedTestAnnotation.getSourceTree();
int startAbsolute = node.getStartPosition();
int lower = ((JCTree) expressionTree).getStartPosition() - startAbsolute;
int upper = state.getEndPosition(expressionTree) - startAbsolute;
CharSequence source = state.getSourceForNode(node);

lower = getLower(source, lower, index, resolvedTestAnnotation.getFlags().size());

upper = getUpper(source, upper);

fixBuilder.replace(startAbsolute + lower, startAbsolute + upper, "");
}
}
return fixBuilder;
}

private int getLower(CharSequence source, int lower, int index, int flagSize) {
while (lower >= 0
&& source.charAt(lower) != '{'
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what scenarios would we encounter a { or a = in an Enum List? Can you please add a test scenario for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for = i was thinking in @ToggleTesting(treated = TestExperimentName.STALE_FLAG) but you are right that it is not needed at this stage so i'll remove it
for { the scenario is tested in testRemoveFirstEnumFromAnnotationRemovingCommaAndKeepingOtherEnum where the enum is at the beginning of the list: @ToggleTesting(treated = {TestExperimentName.STALE_FLAG, TestExperimentName.OTHER_FLAG})

&& source.charAt(lower) != '='
&& source.charAt(lower) != ',') {
lower--;
}
// do not remove { or = neither the comma when it is not the last enum
if (source.charAt(lower) == '{'
|| source.charAt(lower) == '='
|| (index != flagSize - 1 && source.charAt(lower) == ',')) {
lower++;
}
return lower;
}

private int getUpper(CharSequence source, int upper) {
while (upper < source.length()
&& source.charAt(upper) != ','
&& source.charAt(upper) != '}'
&& source.charAt(upper) != ')') {
upper++;
}
if (source.charAt(upper) == ',') {
upper++;
}
return upper;
}

/**
* A small tuple of counters for scanning unit tests to be deleted by the setters heuristic. See
* {@link #shouldCleanBySetters(MethodTree, VisitorState)}
Expand Down
Loading