Fix pmd#4903 - UnnecessaryBoxing where explicit conversion is necessary

oowekyala · May 14, 2024 · fc24ece · fc24ece
1 parent f1701df
commit fc24ece
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 19 deletions.
diff --git a/...ava/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingRule.java b/...ava/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingRule.java
@@ -54,7 +54,7 @@ public Object visit(ASTConstructorCall node, Object data) {
  }
  JTypeMirror argT = arg.getTypeMirror();
  if (argT.isPrimitive()) {
- checkBox((RuleContext) data, "boxing", node, arg, node.getMethodType().getFormalParameters().get(0));
+ checkBox((RuleContext) data, "boxing", node, arg);
  }
  }
  return null;
@@ -73,11 +73,11 @@ public Object visit(ASTMethodCall node, Object data) {
  ASTExpression qualifier = node.getQualifier();
 
  if (isValueOf && isWrapperValueOf(m)) {
- checkBox((RuleContext) data, "boxing", node, node.getArguments().get(0), m.getFormalParameters().get(0));
+ 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, qualifier.getTypeMirror());
+ checkBox((RuleContext) data, "unboxing", node, qualifier);
  }
  }
  return null;
@@ -105,8 +105,7 @@ private void checkBox(
  RuleContext rctx,
  String opKind,
  ASTExpression conversionExpr,
- ASTExpression convertedExpr,
- JTypeMirror conversionInput
+ ASTExpression convertedExpr
  ) {
  // the conversion looks like
  // CTX _ = conversion(sourceExpr)
@@ -120,13 +119,14 @@ private void checkBox(
  // we want to report a violation if this is equivalent to
  // sourceExpr -> ctx
 
- // which basically means testing that convInput -> convOutput
+ // which basically means testing that sourceExpr -> convOutput
  // may be performed implicitly.
 
  // We cannot just test compatibility of the source to the ctx,
  // because of situations like
  // int i = integer.byteValue()
- // where the conversion actually truncates the input value.
+ // where the conversion actually truncates the input value
+ // (here we do sourceExpr=Integer (-> convInput=Integer) -> convOutput=byte -> ctx=int).
 
  JTypeMirror sourceType = convertedExpr.getTypeMirror();
  JTypeMirror conversionOutput = conversionExpr.getTypeMirror();
@@ -138,23 +138,21 @@ private void checkBox(
 
  if (ctxType != null) {
 
- if (isImplicitlyConvertible(conversionInput, conversionOutput)) {
-
- boolean simpleConv = isReferenceSubtype(sourceType, conversionInput);
+ if (isImplicitlyConvertible(sourceType, conversionOutput)) {
 
  final String reason;
- if (simpleConv && conversionInput.unbox().equals(conversionOutput)) {
+ if (sourceType.equals(conversionOutput)) {
+ reason = "boxing of boxed value";
+ } else if (sourceType.unbox().equals(conversionOutput)) {
  reason = "explicit unboxing";
- } else if (simpleConv && conversionInput.box().equals(conversionOutput)) {
+ } else if (sourceType.box().equals(conversionOutput)) {
  reason = "explicit boxing";
- } else if (sourceType.equals(conversionOutput)) {
- reason = "boxing of boxed value";
+ } else if (sourceType.equals(ctxType)) {
+ reason = opKind;
  } else {
- if (sourceType.equals(ctxType)) {
- reason = opKind;
- } else {
- reason = "explicit conversion from " + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType) + " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
- }
+ reason = "explicit conversion from "
+ + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType)
+ + " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
  }
 
  rctx.addViolation(conversionExpr, reason);
@@ -185,6 +183,11 @@ private void checkUnboxing(
  }
 
  private boolean isImplicitlyConvertible(JTypeMirror i, JTypeMirror o) {
+ if (i.isBoxedPrimitive() && o.isBoxedPrimitive()) {
+ // There is no implicit conversions between box types,
+ // only between primitives
+ return i.equals(o);
+ }
  return i.box().isSubtypeOf(o.box())
  || i.unbox().isSubtypeOf(o.unbox());
  }

diff --git a/...src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryBoxing.xml b/...src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryBoxing.xml
@@ -333,4 +333,28 @@ public class Foo {
 }
  ]]></code>
  </test-code>
+ <test-code>
+ <description> [java] UnnecessaryBoxing, but explicit conversion is necessary #4903 </description>
+ <expected-problems>0</expected-problems>
+ <code><![CDATA[
+ public final class UnnecessaryBoxing {
+
+ private UnnecessaryBoxing() {
+ }
+
+ public static void addLong(Long parameter) {
+ System.out.println("parameter = " + parameter);
+ }
+
+ public static Integer getValue() {
+ return 42;
+ }
+
+ public static void main(String[] args) {
+ addLong(Long.valueOf(getValue())); // PMD complains: "Unnecessary explicit conversion from Integer to Long"
+ //addLong(getValue()); // Does not compile: "incompatible types: Integer cannot be converted to Long"
+ }
+ }
+ ]]></code>
+ </test-code>
 </test-data>