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

Remove leading whitespace for UnusedVariable suggested fixes #2524

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove leading whitespace for UnusedVariable suggested fixes #2524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67e0682
Add EXACT_TEXT_MATCH constant to TestMode enum
tnterdan Aug 16, 2021
74f11bd
Add replace and delete methods to remove leftover whitespace
tnterdan Aug 23, 2021
e19e707
Update UnusedVariable to remove leading whitespace
tnterdan Aug 25, 2021
37ba08f
Use EXACT_TEXT_MATCH on unused variable tests
tnterdan Aug 25, 2021
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
129 changes: 129 additions & 0 deletions check_api/src/main/java/com/google/errorprone/fixes/SuggestedFix.java
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
Expand Down Expand Up @@ -100,6 +101,10 @@ public static SuggestedFix replace(Tree tree, String replaceWith) {
return builder().replace(tree, replaceWith).build();
}

public static SuggestedFix replace(Tree tree, VisitorState state, String replaceWith) {
return builder().replace(tree, state, replaceWith).build();
}

/**
* Replace the characters from startPos, inclusive, until endPos, exclusive, with the given
* string.
Expand Down Expand Up @@ -136,16 +141,100 @@ public static SuggestedFix prefixWith(Tree node, String prefix) {
return builder().prefixWith(node, prefix).build();
}

/** {@link Builder#prefixWith(Tree, VisitorState, String)} */
public static SuggestedFix prefixWith(Tree node, VisitorState state, String prefix) {
return builder().prefixWith(node, state, prefix).build();
}

/** {@link Builder#postfixWith(Tree, String)} */
public static SuggestedFix postfixWith(Tree node, String postfix) {
return builder().postfixWith(node, postfix).build();
}

/**
* Returns a number to adjust the start position of a piece of source code that encompasses any
* whitespace between that source's starting position and any prior non-whitespace character
*
* <p>This is useful when removing source code to not leave extra whitespace gaps where removed source used to be
*/
public static int getAdjustedStartPosition(Tree node, VisitorState state) {
String source = state.getSourceForNode(node);
if (source == null) {
return 0;
}

int endPosition = state.getEndPosition(node);
int startPosition = endPosition - source.length();

Tree parentTree = state.getPath().getParentPath() != null
? state.getPath().getParentPath().getLeaf()
: state.getPath().getCompilationUnit();

String parentSource = state.getSourceForNode(parentTree);
if (parentSource == null) {
return 0;
}

int parentEndPosition = state.getEndPosition(parentTree);
int parentStartPosition = parentEndPosition - parentSource.length();

// Finding position in parent source
// This is because child source may not contain the surrounding whitespace
//
// -1 to get whitespace character before starting position
int position = startPosition - parentStartPosition - 1;
if (position < 0 || position >= parentSource.length()) {
// This should only happen if our source is invalid
return 0;
}

int adjustment = 0;
char currentChar = parentSource.charAt(position);

while (Character.isWhitespace(currentChar)) {
if (position + adjustment < 0 || position + adjustment > parentSource.length()) {
// This should only happen if our source is invalid
return 0;
}

// We are at the beginning of the file
if (position + adjustment == 0) {
return adjustment;
}

// Only remove up to a single line of whitespace
if (isNewline(currentChar)) {
if (position + adjustment > 1) {
char previousChar = parentSource.charAt(position + adjustment - 1);
// Clean up Windows-style line ending
if (previousChar == '\r') {
return adjustment - 2;
}
}
return adjustment - 1;
}

adjustment--;
currentChar = parentSource.charAt(position + adjustment);
}

return adjustment;
}

private static boolean isNewline(char c) {
return c == '\n' || c == '\r';
}

/** {@link Builder#delete(Tree)} */
public static SuggestedFix delete(Tree node) {
return builder().delete(node).build();
}

public static SuggestedFix delete(Tree node, VisitorState state) {
int startPos = getAdjustedStartPosition(node, state);
return builder().replace(node, "", startPos, 0).build();
}

/** {@link Builder#swap(Tree, Tree)} */
public static SuggestedFix swap(Tree node1, Tree node2) {
return builder().swap(node1, node2).build();
Expand Down Expand Up @@ -201,6 +290,23 @@ public Builder replace(Tree node, String replaceWith) {
return with(new ReplacementFix((DiagnosticPosition) node, replaceWith));
}

/**
* Similar to {@link Builder#replace(Tree, String)}, but uses the extra VisitorState parameter
* to get source and remove extra leading whitespace. This avoids leaving whitespace gaps
* behind when removing a line of code.
*/
public Builder replace(
Tree node,
VisitorState state,
String replaceWith) {
checkNotSyntheticConstructor(node);
int startPos = getAdjustedStartPosition(node, state);
return with(
new ReplacementFix(
new AdjustedPosition((JCTree) node, startPos, 0),
replaceWith));
}

/**
* Replace the characters from startPos, inclusive, until endPos, exclusive, with the given
* string.
Expand Down Expand Up @@ -242,6 +348,18 @@ public Builder prefixWith(Tree node, String prefix) {
return with(new PrefixInsertion((DiagnosticPosition) node, prefix));
}

/**
* Similar to {@link Builder#prefixWith(Tree, String)}, but uses the extra VisitorState
* parameter to get source and remove extra leading whitespace. This avoids leaving whitespace
* gaps behind when removing a line of code.
*/
public Builder prefixWith(Tree node, VisitorState state, String prefix) {
checkNotSyntheticConstructor(node);
int startPosAdj = getAdjustedStartPosition(node, state);
DiagnosticPosition pos = new AdjustedPosition((JCTree) node, startPosAdj, 0);
return with(new PrefixInsertion(pos, prefix));
}

public Builder postfixWith(Tree node, String postfix) {
checkNotSyntheticConstructor(node);
return with(new PostfixInsertion((DiagnosticPosition) node, postfix));
Expand All @@ -252,6 +370,17 @@ public Builder delete(Tree node) {
return replace(node, "");
}

/**
* Similar to {@link Builder#delete(Tree)}, but uses the extra VisitorState parameter to get
* source and remove extra leading whitespace. This avoids leaving whitespace gaps behind when
* removing a line of code.
*/
public Builder delete(Tree node, VisitorState state) {
checkNotSyntheticConstructor(node);
int startPos = getAdjustedStartPosition(node, state);
return replace(node, "", startPos, 0);
}

public Builder swap(Tree node1, Tree node2) {
checkNotSyntheticConstructor(node1);
checkNotSyntheticConstructor(node2);
Expand Down
22 changes: 19 additions & 3 deletions check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.Iterables.getLast;
import static com.google.common.collect.Streams.stream;
import static com.google.errorprone.fixes.SuggestedFix.getAdjustedStartPosition;
import static com.google.errorprone.util.ASTHelpers.getAnnotation;
import static com.google.errorprone.util.ASTHelpers.getAnnotationWithSimpleName;
import static com.google.errorprone.util.ASTHelpers.getModifiers;
Expand Down Expand Up @@ -1515,10 +1516,19 @@ private static ImmutableSet<Tree.Kind> supportedTreeTypes(Element exemptingAnnot
*/
public static SuggestedFix replaceIncludingComments(
TreePath path, String replacement, VisitorState state) {
return replaceIncludingComments(path, replacement, state, false);
}

/**
* Includes a parameter to also remove leading whitespace. If set to true, this avoids leaving
* behind extra whitespace when removing a tree.
*/
public static SuggestedFix replaceIncludingComments(
TreePath path, String replacement, VisitorState state, boolean removeWhitespace) {
Tree tree = path.getLeaf();
Tree parent = path.getParentPath().getLeaf();
if (!(parent instanceof ClassTree)) {
return SuggestedFix.replace(tree, replacement);
return SuggestedFix.replace(tree, state, replacement);
}
Tree previousMember = null;
ClassTree classTree = (ClassTree) parent;
Expand All @@ -1545,10 +1555,15 @@ public static SuggestedFix replaceIncludingComments(
tokens = getTokensAfterOpeningBrace(tokens);
}
if (tokens.isEmpty()) {
if (removeWhitespace) {
return SuggestedFix.replace(tree, state, replacement);
}
return SuggestedFix.replace(tree, replacement);
}
if (tokens.get(0).comments().isEmpty()) {
return SuggestedFix.replace(tokens.get(0).pos(), state.getEndPosition(tree), replacement);
int tokenPos = tokens.get(0).pos();
int startPosAdjustment = removeWhitespace ? getAdjustedStartPosition(tree, state) : 0;
return SuggestedFix.replace(tokenPos + startPosAdjustment, state.getEndPosition(tree), replacement);
}
ImmutableList<Comment> comments =
ImmutableList.sortedCopyOf(
Expand All @@ -1569,7 +1584,8 @@ public static SuggestedFix replaceIncludingComments(
}
startPos = comment.getSourcePos(0);
}
return SuggestedFix.replace(startPos, state.getEndPosition(tree), replacement);
int startPosAdjustment = removeWhitespace ? getAdjustedStartPosition(tree, state) : 0;
return SuggestedFix.replace(startPos + startPosAdjustment, state.getEndPosition(tree), replacement);
}

private static List<ErrorProneToken> getTokensAfterOpeningBrace(List<ErrorProneToken> tokens) {
Expand Down
20 changes: 11 additions & 9 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
View file
Open in desktop
Expand Up @@ -266,14 +266,16 @@ private static SuggestedFix makeAssignmentDeclaration(
Optional<VariableTree> removedVariableTree =
allUsageSites.stream()
.filter(tp -> tp.getLeaf() instanceof VariableTree)
.findFirst()
// Use last usage site to avoid issues with whitespace cleanup
.reduce((first, second) -> second)
.map(tp -> (VariableTree) tp.getLeaf());
Optional<AssignmentTree> reassignment =
specs.stream()
.map(UnusedSpec::terminatingAssignment)
.flatMap(Streams::stream)
.filter(a -> allUsageSites.stream().noneMatch(tp -> tp.getLeaf().equals(a)))
.findFirst();
// Use last assignment to avoid issues with whitespace cleanup
.reduce((first, second) -> second);
if (removedVariableTree.isPresent()
&& reassignment.isPresent()
&& !TOP_LEVEL_EXPRESSIONS.contains(reassignment.get().getExpression().getKind())) {
Expand Down Expand Up @@ -383,11 +385,11 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
varSymbol.isStatic() ? "static " : "", state.getSourceForNode(initializer));
keepSideEffectsFix.merge(
SuggestedFixes.replaceIncludingComments(usagePath, newContent, state));
removeSideEffectsFix.replace(statement, "");
removeSideEffectsFix.replace(statement, state, "");
} else {
keepSideEffectsFix.replace(
statement, String.format("%s;", state.getSourceForNode(initializer)));
removeSideEffectsFix.replace(statement, "");
removeSideEffectsFix.replace(statement, state, "");
}
} else if (isEnhancedForLoopVar(usagePath)) {
String modifiers =
Expand All @@ -407,9 +409,9 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
} else {
String replacement = needsBlock(usagePath) ? "{}" : "";
keepSideEffectsFix.merge(
SuggestedFixes.replaceIncludingComments(usagePath, replacement, state));
SuggestedFixes.replaceIncludingComments(usagePath, replacement, state, true));
removeSideEffectsFix.merge(
SuggestedFixes.replaceIncludingComments(usagePath, replacement, state));
SuggestedFixes.replaceIncludingComments(usagePath, replacement, state, true));
}
continue;
} else if (statement.getKind() == Kind.EXPRESSION_STATEMENT) {
Expand All @@ -433,13 +435,13 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
encounteredSideEffects = true;
keepSideEffectsFix.replace(
tree.getStartPosition(), ((JCAssign) tree).getExpression().getStartPosition(), "");
removeSideEffectsFix.replace(statement, "");
removeSideEffectsFix.replace(statement, state,"");
continue;
}
}
}
String replacement = needsBlock(usagePath) ? "{}" : "";
keepSideEffectsFix.replace(statement, replacement);
keepSideEffectsFix.replace(statement, state, replacement);
removeSideEffectsFix.replace(statement, replacement);
}
return encounteredSideEffects
Expand All @@ -453,7 +455,7 @@ private static ImmutableList<SuggestedFix> buildUnusedParameterFixes(
int index = methodSymbol.params.indexOf(varSymbol);
SuggestedFix.Builder fix = SuggestedFix.builder();
for (TreePath path : usagePaths) {
fix.delete(path.getLeaf());
fix.delete(path.getLeaf(), state);
}
new TreePathScanner<Void, Void>() {
@Override
Expand Down