Skip to content

Commit 3d817b0

Browse files
cushonError Prone Team
cushon
authored and
Error Prone Team
committed
Handle var in UnnecessaryBoxedVariable
Add a helper for replacing local variable types that handles `var`. Often we want to avoid replacing types of `var` local variables, but in this case using an explicit unboxed type will avoid the unnecessary boxing, and using `var` for primitive types seems low value. #5522 PiperOrigin-RevId: 875284484
1 parent ad26f3e commit 3d817b0

3 files changed

Lines changed: 169 additions & 6 deletions

File tree

‎check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,15 @@
2121
import static com.google.common.collect.ImmutableList.toImmutableList;
2222
import static com.google.common.collect.ImmutableSet.toImmutableSet;
2323
import static com.google.common.collect.Iterables.getLast;
24+
import static com.google.common.collect.Iterables.getOnlyElement;
2425
import static com.google.common.collect.Streams.stream;
2526
import static com.google.errorprone.fixes.ErrorProneEndPosTable.getEndPosition;
2627
import static com.google.errorprone.util.ASTHelpers.getAnnotation;
2728
import static com.google.errorprone.util.ASTHelpers.getAnnotationWithSimpleName;
2829
import static com.google.errorprone.util.ASTHelpers.getModifiers;
2930
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
3031
import static com.google.errorprone.util.ASTHelpers.getSymbol;
32+
import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
3133
import static com.google.errorprone.util.ASTHelpers.hasImplicitType;
3234
import static com.google.errorprone.util.ASTHelpers.isRecord;
3335
import static com.sun.tools.javac.code.TypeTag.CLASS;
@@ -56,6 +58,7 @@
5658
import com.google.errorprone.util.ASTHelpers;
5759
import com.google.errorprone.util.ErrorProneComment;
5860
import com.google.errorprone.util.ErrorProneToken;
61+
import com.google.errorprone.util.ErrorProneTokens;
5962
import com.google.errorprone.util.FindIdentifiers;
6063
import com.sun.source.doctree.DocTree;
6164
import com.sun.source.doctree.ParamTree;
@@ -1759,5 +1762,41 @@ public static String castTree(ExpressionTree expressionTree, String toType, Visi
17591762
+ (needsParentheses ? ")" : "");
17601763
}
17611764

1765+
/**
1766+
* Replaces the type of the given variable with {@code replacementType}.
1767+
*
1768+
* <p>If the tree is a {@code var} declaration, the {@code var} keyword will be replaced with the
1769+
* {@code replacementType}.
1770+
*/
1771+
public static Optional<SuggestedFix> replaceVariableType(
1772+
VariableTree tree, String replacementType, VisitorState state) {
1773+
Tree type = tree.getType();
1774+
if (hasExplicitSource(type, state)) {
1775+
return Optional.of(SuggestedFix.replace(type, replacementType));
1776+
}
1777+
int pos = getStartPosition(type);
1778+
if (pos == Position.NOPOS) {
1779+
pos = getStartPosition(tree);
1780+
}
1781+
int end =
1782+
tree.getInitializer() != null
1783+
? getStartPosition(tree.getInitializer())
1784+
: state.getEndPosition(tree);
1785+
ImmutableList<ErrorProneToken> tokens =
1786+
ErrorProneTokens.getTokens(
1787+
state.getSourceCode().subSequence(pos, end).toString(), state.context)
1788+
.stream()
1789+
.filter(
1790+
token ->
1791+
token.kind().equals(TokenKind.IDENTIFIER) && token.name().contentEquals("var"))
1792+
.collect(toImmutableList());
1793+
if (tokens.size() != 1) {
1794+
return Optional.empty();
1795+
}
1796+
ErrorProneToken token = getOnlyElement(tokens);
1797+
return Optional.of(
1798+
SuggestedFix.replace(pos + token.pos(), pos + token.endPos(), replacementType));
1799+
}
1800+
17621801
private SuggestedFixes() {}
17631802
}

‎core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariable.java‎

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,25 @@
1717
package com.google.errorprone.bugpatterns;
1818

1919
import static com.google.common.base.Preconditions.checkArgument;
20+
import static com.google.common.collect.Iterables.getOnlyElement;
21+
import static com.google.errorprone.fixes.SuggestedFixes.replaceVariableType;
2022
import static com.google.errorprone.matchers.Description.NO_MATCH;
2123
import static com.google.errorprone.matchers.Matchers.anyOf;
2224
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
2325
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
2426
import static com.google.errorprone.util.ASTHelpers.getSymbol;
27+
import static com.google.errorprone.util.ASTHelpers.getType;
28+
import static com.google.errorprone.util.ASTHelpers.isSameType;
2529

30+
import com.google.common.base.Ascii;
2631
import com.google.common.collect.ArrayListMultimap;
2732
import com.google.common.collect.ListMultimap;
2833
import com.google.errorprone.BugPattern;
2934
import com.google.errorprone.BugPattern.SeverityLevel;
3035
import com.google.errorprone.VisitorState;
3136
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
3237
import com.google.errorprone.fixes.SuggestedFix;
38+
import com.google.errorprone.fixes.SuggestedFixes;
3339
import com.google.errorprone.matchers.Description;
3440
import com.google.errorprone.matchers.Matcher;
3541
import com.google.errorprone.util.ASTHelpers;
@@ -154,11 +160,16 @@ private Optional<Description> fixVariable(
154160
}
155161

156162
SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
157-
fixBuilder.replace(tree.getType(), unboxed.tsym.getSimpleName().toString());
163+
replaceVariableType(tree, unboxed.tsym.getSimpleName().toString(), state)
164+
.ifPresent(fixBuilder::merge);
158165

159166
fixMethodInvocations(usages.fixableSimpleMethodInvocations.get(varSymbol), fixBuilder, state);
160167
fixNullCheckInvocations(usages.fixableNullCheckInvocations.get(varSymbol), fixBuilder, state);
161168
fixCastingInvocations(usages.fixableCastMethodInvocations.get(varSymbol), fixBuilder, state);
169+
fixAssignments(usages.fixableAssignments.get(varSymbol), fixBuilder, state);
170+
if (tree.getInitializer() != null) {
171+
fixAssignment(tree.getInitializer(), fixBuilder, state);
172+
}
162173

163174
// Remove @Nullable annotation, if present.
164175
AnnotationTree nullableAnnotation =
@@ -266,6 +277,56 @@ private static void fixCastingInvocations(
266277
}
267278
}
268279

280+
private static void fixAssignments(
281+
List<TreePath> paths, SuggestedFix.Builder fixBuilder, VisitorState state) {
282+
for (TreePath path : paths) {
283+
ExpressionTree expressionTree = (ExpressionTree) path.getLeaf();
284+
fixAssignment(expressionTree, fixBuilder, state);
285+
}
286+
}
287+
288+
private static void fixAssignment(
289+
ExpressionTree expressionTree, SuggestedFix.Builder fixBuilder, VisitorState state) {
290+
Type boxedType = getType(expressionTree);
291+
Type unboxedType = state.getTypes().unboxedType(boxedType);
292+
if (unboxedType.hasTag(TypeTag.NONE)) {
293+
return;
294+
}
295+
String name = unboxedType.tsym.getSimpleName().toString();
296+
String parseName = "parse" + Ascii.toUpperCase(name.charAt(0)) + name.substring(1);
297+
switch (expressionTree) {
298+
case MethodInvocationTree methodInvocation -> {
299+
if (VALUE_OF_MATCHER.matches(expressionTree, state)) {
300+
Tree argument = getOnlyElement(methodInvocation.getArguments());
301+
Type argumentType = ASTHelpers.getType(argument);
302+
if (isSameType(argumentType, state.getSymtab().stringType, state)) {
303+
fixBuilder.merge(
304+
SuggestedFixes.renameMethodInvocation(
305+
(MethodInvocationTree) expressionTree, parseName, state));
306+
} else if (isSameType(argumentType, unboxedType, state)) {
307+
fixBuilder.replace(expressionTree, state.getSourceForNode(argument));
308+
}
309+
}
310+
}
311+
case NewClassTree newClassTree -> {
312+
Tree argument = getOnlyElement(newClassTree.getArguments());
313+
Type argumentType = ASTHelpers.getType(argument);
314+
if (isSameType(argumentType, state.getSymtab().stringType, state)) {
315+
fixBuilder.replace(
316+
expressionTree,
317+
String.format(
318+
"%s.%s(%s)",
319+
SuggestedFixes.qualifyType(state, fixBuilder, boxedType),
320+
parseName,
321+
state.getSourceForNode(argument)));
322+
} else if (isSameType(argumentType, unboxedType, state)) {
323+
fixBuilder.replace(expressionTree, state.getSourceForNode(argument));
324+
}
325+
}
326+
default -> {}
327+
}
328+
}
329+
269330
/**
270331
* Check to see if the variable should be considered for replacement, i.e.
271332
*
@@ -346,6 +407,7 @@ private static class FindBoxedUsagesScanner extends TreePathScanner<Void, Void>
346407
ArrayListMultimap.create();
347408
private final ListMultimap<VarSymbol, TreePath> fixableCastMethodInvocations =
348409
ArrayListMultimap.create();
410+
private final ListMultimap<VarSymbol, TreePath> fixableAssignments = ArrayListMultimap.create();
349411

350412
private final Set<VarSymbol> boxedUsageFound = new HashSet<>();
351413
private final Set<VarSymbol> dereferenced = new HashSet<>();
@@ -369,13 +431,15 @@ public Void visitAssignment(AssignmentTree node, Void unused) {
369431
if (!isBoxed(nodeSymbol, state)) {
370432
return super.visitAssignment(node, null);
371433
}
434+
VarSymbol varSymbol = (VarSymbol) nodeSymbol;
435+
fixableAssignments.put(varSymbol, new TreePath(getCurrentPath(), node.getExpression()));
372436
// The variable of interest is being assigned. Check if the expression is non-primitive,
373437
// and go on to scan the expression.
374438
if (!checkAssignmentExpression(node.getExpression())) {
375439
return scan(node.getExpression(), null);
376440
}
377441

378-
boxedUsageFound.add((VarSymbol) nodeSymbol);
442+
boxedUsageFound.add(varSymbol);
379443
return null;
380444
}
381445

‎core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryBoxedVariableTest.java‎

Lines changed: 64 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -289,11 +289,11 @@ void negative_methodInvocation() {
289289
}
290290
291291
void positive_assignedValueOf() {
292-
int i = Integer.valueOf(0);
292+
int i = 0;
293293
}
294294
295295
int positive_assignedValueOf_return() {
296-
int i = Integer.valueOf(0);
296+
int i = 0;
297297
return i;
298298
}
299299
@@ -355,12 +355,12 @@ int negative_assignmentInReturn() {
355355
356356
int positive_assignmentInReturn() {
357357
int myVariable;
358-
return myVariable = Integer.valueOf(42);
358+
return myVariable = 42;
359359
}
360360
361361
int positive_assignmentInReturn2() {
362362
int myVariable;
363-
return myVariable = Integer.valueOf(42);
363+
return myVariable = 42;
364364
}
365365
366366
int positive_hashCode() {
@@ -428,10 +428,12 @@ static int positive_nullChecked_expression_message(int i) {
428428
}
429429
430430
static int positive_nullChecked_statement(int i) {
431+
431432
return i;
432433
}
433434
434435
static int positive_nullChecked_statement_message(int i) {
436+
435437
return i;
436438
}
437439
@@ -623,4 +625,62 @@ public ImmutableList<Integer> foos() {
623625
""")
624626
.doTest();
625627
}
628+
629+
@Test
630+
public void var() {
631+
helper
632+
.addInputLines(
633+
"Test.java",
634+
"""
635+
class Test {
636+
private int a() {
637+
var y = Integer.valueOf(42);
638+
return y;
639+
}
640+
}
641+
""")
642+
.addOutputLines(
643+
"Test.java",
644+
"""
645+
class Test {
646+
private int a() {
647+
int y = 42;
648+
return y;
649+
}
650+
}
651+
""")
652+
.doTest();
653+
}
654+
655+
@Test
656+
public void assignment() {
657+
helper
658+
.addInputLines(
659+
"Test.java",
660+
"""
661+
class Test {
662+
private void a() {
663+
Integer y = Integer.valueOf("42");
664+
y = Integer.valueOf("43");
665+
y = Integer.valueOf(43);
666+
y = new Integer("44");
667+
y = new Integer(44);
668+
}
669+
}
670+
""")
671+
.addOutputLines(
672+
"Test.java",
673+
"""
674+
class Test {
675+
private void a() {
676+
int y = Integer.parseInt("42");
677+
y = Integer.parseInt("43");
678+
y = 43;
679+
y = Integer.parseInt("44");
680+
y = 44;
681+
}
682+
}
683+
""")
684+
.doTest();
685+
}
626686
}

0 commit comments

Comments
 (0)