hiifong/pmd
1
0
Fork 0
forked from phoedos/pmd
Code Pull Requests Activity

Fix many problems with UnnecessaryBoxing

Browse Source
This commit is contained in:
Clément Fournier
2024-05-15 20:15:29 +02:00
parent 625fb36b14
commit eb17c9aab7
2 changed files with 150 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

111
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingRule.java
View File

@ -9,12 +9,14 @@ import static net.sourceforge.pmd.util.CollectionUtil.setOf;
import java.util.Set;
import org.apache.commons.lang3.StringUtils;
import org.checkerframework.checker.nullness.qual.Nullable;
import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
import net.sourceforge.pmd.lang.java.ast.ASTExpression;
import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement;
import net.sourceforge.pmd.lang.java.ast.ASTList;
import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.ast.QualifiableExpression;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.types.JMethodSig;
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
@ -54,7 +56,7 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
}
JTypeMirror argT = arg.getTypeMirror();
if (argT.isPrimitive()) {
checkBox((RuleContext) data, "boxing", node, arg);
checkBox((RuleContext) data, node, arg);
}
}
return null;
@ -73,18 +75,19 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
ASTExpression qualifier = node.getQualifier();
if (isValueOf && isWrapperValueOf(m)) {
checkBox((RuleContext) data, "boxing", node, node.getArguments().get(0));
} else if (isValueOf && isStringValueOf(m) && qualifier != null) {
checkUnboxing((RuleContext) data, node, qualifier.getTypeMirror());
} else if (!isValueOf && isUnboxingCall(m) && qualifier != null) {
checkBox((RuleContext) data, "unboxing", node, qualifier);
checkBox((RuleContext) data, node, node.getArguments().get(0));
} else if (isValueOf && isBoxValueOfString(m)) {
checkUnboxing((RuleContext) data, node, m.getDeclaringType());
} else if (!isValueOf && qualifier != null && isUnboxingCall(m)) {
checkBox((RuleContext) data, node, qualifier);
}
}
return null;
}
private boolean isUnboxingCall(JMethodSig m) {
return !m.isStatic() && m.getDeclaringType().isBoxedPrimitive() && m.getArity() == 0;
return !m.isStatic() && m.getDeclaringType().isBoxedPrimitive() && m.getArity() == 0
&& m.getReturnType().isPrimitive();
}
private boolean isWrapperValueOf(JMethodSig m) {
@ -94,7 +97,8 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
&& m.getFormalParameters().get(0).isPrimitive();
}
private boolean isStringValueOf(JMethodSig m) {
private boolean isBoxValueOfString(JMethodSig m) {
// eg Integer.valueOf("2")
return m.isStatic()
&& (m.getArity() == 1 || m.getArity() == 2)
&& m.getDeclaringType().isBoxedPrimitive()
@ -103,7 +107,6 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
private void checkBox(
RuleContext rctx,
String opKind,
ASTExpression conversionExpr,
ASTExpression convertedExpr
) {
@ -119,8 +122,10 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
// we want to report a violation if this is equivalent to
// sourceExpr -> ctx
// which basically means testing that sourceExpr -> convOutput
// may be performed implicitly.
// which means testing that
// 1. the result of the implicit conversion of sourceExpr
// with context type ctx is the same type as the result of conversion 3
// 2. conversion 2 does not truncate the value
// We cannot just test compatibility of the source to the ctx,
// because of situations like
@ -132,35 +137,49 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
JTypeMirror conversionOutput = conversionExpr.getTypeMirror();
ExprContext ctx = conversionExpr.getConversionContext();
JTypeMirror ctxType = ctx.getTargetType();
if (ctxType == null && conversionExpr.getParent() instanceof ASTExpressionStatement) {
ctxType = conversionOutput;
if (sourceType.isPrimitive()
&& !conversionOutput.isPrimitive()
&& ctxType == null
&& isObjectConversionNecessary(conversionExpr)) {
// eg Integer.valueOf(2).equals(otherInteger)
return;
}
if (ctxType != null) {
if (isImplicitlyConvertible(sourceType, conversionOutput)) {
String reason;
if (sourceType.equals(conversionOutput)) {
reason = "boxing of boxed value";
} else if (sourceType.unbox().equals(conversionOutput)) {
String reason = null;
if (sourceType.equals(conversionOutput)) {
reason = "boxing of boxed value";
} else if (ctxType != null) {
JTypeMirror conv = implicitConversionResult(sourceType, ctxType);
if (conv != null
&& conv.equals(implicitConversionResult(conversionOutput, ctxType))
&& conversionDoesNotChangesValue(sourceType, conversionOutput)) {
if (sourceType.unbox().equals(conversionOutput)) {
reason = "explicit unboxing";
} else if (sourceType.box().equals(conversionOutput)) {
reason = "explicit boxing";
} else if (sourceType.equals(ctxType)) {
reason = opKind;
} else {
reason = "explicit conversion from "
+ TypePrettyPrint.prettyPrintWithSimpleNames(sourceType)
reason = "explicit conversion from " + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType)
+ " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
if (!conversionOutput.equals(ctxType)) {
reason += " through " + TypePrettyPrint.prettyPrintWithSimpleNames(conversionOutput);
}
}
rctx.addViolation(conversionExpr, reason);
}
}
if (reason != null) {
rctx.addViolation(conversionExpr, reason);
}
}
private boolean isObjectConversionNecessary(ASTExpression e) {
JavaNode parent = e.getParent();
if (e.getIndexInParent() == 0) {
return parent instanceof QualifiableExpression;
}
return false;
}
@ -191,9 +210,41 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
// There is no implicit conversions between box types,
// only between primitives
return i.equals(o);
} else if (i.isPrimitive() && o.isPrimitive()) {
return i.isSubtypeOf(o);
} else {
return i.unbox().equals(o.unbox());
}
return i.box().isSubtypeOf(o.box())
|| i.unbox().isSubtypeOf(o.unbox());
}
/**
* Type of the converted i in context ctx. If no implicit
* conversion is possible then return null.
*/
private static @Nullable JTypeMirror implicitConversionResult(JTypeMirror i, JTypeMirror ctx) {
if (!ctx.isPrimitive()) {
// boxing
return i.box().isSubtypeOf(ctx) ? i.box() : null;
} else if (i.isBoxedPrimitive() && ctx.isPrimitive()) {
// Integer x = 1;
// long l = x;
// unboxing then optional widening
return i.unbox().isSubtypeOf(ctx) ? ctx : null;
} else if (i.isPrimitive() && ctx.isPrimitive()) {
// widening
return i.isSubtypeOf(ctx) ? ctx : null;
}
return null;
}
/**
* Whether the explicit conversion from i to o changes the value.
* This is e.g. truncating an integer.
*/
private static boolean conversionDoesNotChangesValue(JTypeMirror i, JTypeMirror o) {
return i.box().isSubtypeOf(o.box()) || i.unbox().isSubtypeOf(o.unbox());
}
}

76
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryBoxing.xml
View File

@ -62,12 +62,8 @@ public class Foo {
]]></code>
</test-code>
<test-code>
<description>pos - new Integer(int) with widening</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>3</expected-linenumbers>
<expected-messages>
<message>Unnecessary explicit conversion from char to Integer</message>
</expected-messages>
<description>neg - char -> Integer is not possible implicitly</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {{
char c = 0;
@ -351,7 +347,7 @@ public class Foo {
]]></code>
</test-code>
<test-code>
<description> [java] UnnecessaryBoxing FP in lambda</description>
<description>[java] UnnecessaryBoxing FP in lambda #4924</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>11</expected-linenumbers>
<expected-messages>
@ -378,4 +374,70 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>If method is called on result then boxing is necessary</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<expected-messages>
<message>Unnecessary boxing of boxed value</message>
</expected-messages>
<code><![CDATA[
public class Example {
int val;
boolean eq(Integer val) {
return Integer.valueOf(this.val).equals(val) // ok
|| Integer.valueOf(val).equals(this.val); // warn
}
}
]]></code>
</test-code>
<test-code>
<description>valueOf something else</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Example {
int val;
boolean eq(Integer val) {
Example x = valueOf("abc");
x = Example.valueOf("abc");
}
static Example valueOf(String s) {
return null;
}
}
]]></code>
</test-code>
<test-code>
<description>imported valueOf</description>
<expected-problems>1</expected-problems>
<expected-messages>
<message>Unnecessary explicit boxing</message>
</expected-messages>
<code><![CDATA[
import static java.lang.Integer.valueOf;
public class Example {
int val;
Integer eq(int val) {
return valueOf(val);
}
}
]]></code>
</test-code>
<test-code>
<description>Boxing to another type</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import static java.lang.Long.valueOf;
public class Example {
int val;
Object eq(int val) {
// here removing this would box the
// int to an Integer not a Long
return valueOf(val);
}
}
]]></code>
</test-code>
</test-data>