Merge pull request #3364 from oowekyala:new-rule-UnnecessaryConversion

[java] New rule UnnecessaryBoxing #3364

.ci/files/all-java.xml

@@ -106,6 +106,7 @@
     <rule ref="category/java/codestyle.xml/UnnecessaryAnnotationValueElement"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryCast"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryConstructor"/>
+    <rule ref="category/java/codestyle.xml/UnnecessaryBoxing"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>
     <!-- <rule ref="category/java/codestyle.xml/UnnecessaryImport"/> -->
     <rule ref="category/java/codestyle.xml/UnnecessaryLocalBeforeReturn"/>

docs/pages/7_0_0_release_notes.md

@@ -74,8 +74,15 @@ The default version is always ES6.
 
 #### New Rules
 
+##### Apex
+
 *   The Apex rule {% rule "apex/design/UnusedMethod" %} finds unused methods in your code.
 
+##### Java
+
+*   {% rule "java/codestyle/UnnecessaryBoxing" %} reports boxing and unboxing
+conversions that may be made implicit.
+
 #### Changed Rules
 
 ##### Java

pmd-core/src/main/resources/rulesets/releases/700.xml

@@ -10,5 +10,7 @@ This ruleset contains links to rules that are new in PMD v7.0.0
 
     <rule ref="category/apex/design.xml/UnusedMethod"/>
 
+    <rule ref="category/java/codestyle.xml/UnnecessaryBoxing"/>
+
 </ruleset>
 

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

@@ -0,0 +1,168 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.codestyle;
+
+import static net.sourceforge.pmd.util.CollectionUtil.setOf;
+
+import java.util.Set;
+
+import net.sourceforge.pmd.RuleContext;
+import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
+import net.sourceforge.pmd.lang.java.ast.ASTExpression;
+import net.sourceforge.pmd.lang.java.ast.ASTList;
+import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
+import net.sourceforge.pmd.lang.java.ast.InvocationNode;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.types.JMethodSig;
+import net.sourceforge.pmd.lang.java.types.JTypeMirror;
+import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
+import net.sourceforge.pmd.lang.java.types.TypePrettyPrint;
+import net.sourceforge.pmd.lang.java.types.ast.ExprContext;
+
+/**
+ *
+ */
+public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
+
+    private static final Set<String> INTERESTING_NAMES = setOf(
+        "valueOf",
+        "booleanValue",
+        "charValue",
+        "byteValue",
+        "shortValue",
+        "intValue",
+        "longValue",
+        "floatValue",
+        "doubleValue"
+    );
+
+    public UnnecessaryBoxingRule() {
+        super(ASTMethodCall.class, ASTConstructorCall.class);
+    }
+
+    @Override
+    public Object visit(ASTConstructorCall node, Object data) {
+        if (node.getTypeMirror().isBoxedPrimitive()) {
+            ASTExpression arg = ASTList.singleOrNull(node.getArguments());
+            if (arg == null) {
+                return null;
+            }
+            JTypeMirror argT = arg.getTypeMirror();
+            if (argT.isPrimitive()) {
+                checkBox((RuleContext) data, "boxing", node, arg, node.getMethodType().getFormalParameters().get(0));
+            }
+        }
+        return null;
+    }
+
+
+    @Override
+    public Object visit(ASTMethodCall node, Object data) {
+        if (INTERESTING_NAMES.contains(node.getMethodName())) {
+            OverloadSelectionResult overload = node.getOverloadSelectionInfo();
+            if (overload.isFailed()) {
+                return null;
+            }
+            JMethodSig m = overload.getMethodType();
+            boolean isValueOf = "valueOf".equals(node.getMethodName());
+            ASTExpression qualifier = node.getQualifier();
+
+            if (isValueOf && isWrapperValueOf(m)) {
+                checkBox((RuleContext) data, "boxing", node, node.getArguments().get(0), m.getFormalParameters().get(0));
+            } else if (!isValueOf && isUnboxingCall(m) && qualifier != null) {
+                checkBox((RuleContext) data, "unboxing", node, qualifier, qualifier.getTypeMirror());
+            }
+        }
+        return null;
+    }
+
+    private boolean isUnboxingCall(JMethodSig m) {
+        return !m.isStatic() && m.getDeclaringType().isBoxedPrimitive() && m.getArity() == 0;
+    }
+
+    private boolean isWrapperValueOf(JMethodSig m) {
+        return m.isStatic()
+            && m.getArity() == 1
+            && m.getDeclaringType().isBoxedPrimitive()
+            && m.getFormalParameters().get(0).isPrimitive();
+    }
+
+    private void checkBox(
+        RuleContext rctx,
+        String opKind,
+        ASTExpression conversionExpr,
+        ASTExpression convertedExpr,
+        JTypeMirror conversionInput
+    ) {
+        // the conversion looks like
+        //  CTX _ = conversion(sourceExpr)
+
+        // we have the following data flow:
+        //      sourceExpr -> convInput -> convOutput -> ctx
+        //                 1            2             3
+        // where 1 and 3 are implicit conversions which we assume are
+        // valid because the code should compile.
+
+        // we want to report a violation if this is equivalent to
+        //      sourceExpr -> ctx
+
+        // which basically means testing that convInput -> 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.
+
+        JTypeMirror sourceType = convertedExpr.getTypeMirror();
+        JTypeMirror conversionOutput = conversionExpr.getTypeMirror();
+        ExprContext ctx = conversionExpr.getConversionContext();
+        JTypeMirror ctxType = ctx.getTargetType();
+        if (ctxType == null && conversionExpr instanceof InvocationNode) {
+            ctxType = conversionOutput;
+        }
+
+        if (ctxType != null) {
+
+            if (isImplicitlyConvertible(conversionInput, conversionOutput)) {
+
+                boolean simpleConv = isReferenceSubtype(sourceType, conversionInput);
+
+                final String reason;
+                if (simpleConv && conversionInput.unbox().equals(conversionOutput)) {
+                    reason = "explicit unboxing";
+                } else if (simpleConv && conversionInput.box().equals(conversionOutput)) {
+                    reason = "explicit boxing";
+                } else if (sourceType.equals(conversionOutput)) {
+                    reason = "boxing of boxed value";
+                } else {
+                    if (sourceType.equals(ctxType)) {
+                        reason = opKind;
+                    } else {
+                        reason = "explicit conversion from " + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType) + " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
+                    }
+                }
+
+                addViolation(rctx, conversionExpr, reason);
+            }
+        }
+    }
+
+    private boolean isImplicitlyConvertible(JTypeMirror i, JTypeMirror o) {
+        return i.box().isSubtypeOf(o.box())
+            || i.unbox().isSubtypeOf(o.unbox());
+    }
+
+    /**
+     * Whether {@code S <: T}, but ignoring primitive widening.
+     * {@code isReferenceSubtype(int, double) == false} even though
+     * {@code int.isSubtypeOf(double)}.
+     */
+    private static boolean isReferenceSubtype(JTypeMirror s, JTypeMirror t) {
+        return s.isPrimitive() ? t.equals(s)
+                               : s.isSubtypeOf(t);
+    }
+
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypePrettyPrint.java

@@ -31,6 +31,10 @@ public final class TypePrettyPrint {
         return prettyPrint(t, new TypePrettyPrinter());
     }
 
+    public static @NonNull String prettyPrintWithSimpleNames(@NonNull JTypeVisitable t) {
+        return prettyPrint(t, new TypePrettyPrinter().useSimpleNames(true));
+    }
+
     public static String prettyPrint(@NonNull JTypeVisitable t, TypePrettyPrinter prettyPrinter) {
         t.acceptVisitor(PrettyPrintVisitor.INSTANCE, prettyPrinter);
         return prettyPrinter.consumeResult();

pmd-java/src/main/resources/category/java/codestyle.xml

@@ -1477,6 +1477,36 @@ public class Foo {
         </example>
     </rule>
 
+    <rule name="UnnecessaryBoxing"
+          language="java"
+          since="7.0.0"
+          minimumLanguageVersion="1.5"
+          message="Unnecessary {0}"
+          class="net.sourceforge.pmd.lang.java.rule.codestyle.UnnecessaryBoxingRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_java_bestpractices.html#unnecessaryboxing">
+        <description>
+            Reports explicit boxing and unboxing conversions that may safely be removed,
+            either because they would be inserted by the compiler automatically,
+            or because they're semantically a noop (eg unboxing a value to rebox it immediately).
+
+            Note that this only handles boxing and unboxing conversions occurring through
+            calls to `valueOf` or one of the `intValue`, `byteValue`, etc. methods. Casts
+            that command a conversion are reported by {% rule UnnecessaryCast %} instead.
+        </description>
+        <priority>3</priority>
+        <example><![CDATA[
+            {
+                    // Instead of
+                    Integer integer = Integer.valueOf(2);
+                    // you may just write
+                    Integer integer = 2;
+
+                    int i = integer.intValue(); // similarly for unboxing
+            }
+            ]]>
+        </example>
+    </rule>
+
     <!-- This is only restricted to java 5+ because the rule doesn't support
          the type system pre-java5, where there were no autoboxing conversions. -->
     <rule name="UnnecessaryCast"

pmd-java/src/main/resources/rulesets/java/quickstart.xml

@@ -104,6 +104,7 @@
     <!-- <rule ref="category/java/codestyle.xml/PrematureDeclaration" /> -->
     <!-- <rule ref="category/java/codestyle.xml/TooManyStaticImports" /> -->
     <rule ref="category/java/codestyle.xml/UnnecessaryAnnotationValueElement"/>
+    <!-- <rule ref="category/java/codestyle.xml/UnnecessaryBoxing" /> -->
     <!-- <rule ref="category/java/codestyle.xml/UnnecessaryCast" /> -->
     <rule ref="category/java/codestyle.xml/UnnecessaryConstructor"/>
     <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingTest.java

@@ -0,0 +1,11 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.codestyle;
+
+import net.sourceforge.pmd.testframework.PmdRuleTst;
+
+public class UnnecessaryBoxingTest extends PmdRuleTst {
+    // no additional unit tests
+}

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

@@ -0,0 +1,173 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+    xmlns="http://pmd.sourceforge.net/rule-tests"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+
+    <test-code>
+        <description>pos - new Integer(int)</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>2</expected-linenumbers>
+        <code><![CDATA[
+public class Foo {
+    Integer i = new Integer(42);
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>neg - new Integer(String)</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    Integer i = new Integer("42");
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>neg - Integer.valueOf(String)</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    Integer i = Integer.valueOf("42");
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>pos - Integer.valueOf(int)</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>2</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit boxing</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Foo {
+    Integer i = Integer.valueOf(42);
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>pos - new Integer(int)</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>2</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit boxing</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Foo {
+    Integer i = new Integer(42);
+}
+        ]]></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>
+        <code><![CDATA[
+public class Foo {{
+    char c = 0;
+    Integer i = new Integer(c);
+}}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>pos - new Integer(int) assigned to object</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>3</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit boxing</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Foo {{
+    char c = 0;
+    Object obj = new Integer(2);
+}}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>pos - unboxing</description>
+        <expected-problems>3</expected-problems>
+        <expected-linenumbers>3,4,5</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit unboxing</message>
+            <message>Unnecessary explicit conversion from Integer to double</message>
+            <message>Unnecessary explicit conversion from Integer to double</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Foo {
+    void fun(Integer c) {
+        int i0 = c.intValue();
+        double d = c.doubleValue();
+        d = c.longValue();
+
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>neg - unboxing to smaller type</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Foo {
+    void fun(Integer c) {
+        byte i2 = c.byteValue();  // necessary
+        short i = c.shortValue(); // necessary
+        int i = c.byteValue();    // necessary, because it truncates the value before widening it.
+
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>pos - unboxing to smaller type</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>5,6,7,8,9</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit boxing</message>
+            <message>Unnecessary explicit unboxing</message>
+            <message>Unnecessary explicit unboxing</message>
+            <message>Unnecessary explicit boxing</message>
+            <message>Unnecessary boxing of boxed value</message>
+        </expected-messages>
+        <code><![CDATA[
+            class Scratch {
+                public static void main(String[] args) {
+                    Integer integer = 2; // ok
+
+                    Object a = Integer.valueOf(2);  // explicit boxing where the value would be autoboxed
+                    int b = integer.intValue();     // explicit unboxing where the value would be auto-unboxed
+                    Object c = integer.intValue();  // unboxing where the value is immediately reboxed
+                    int i = Integer.valueOf(0);     // boxing where the value is immediately unboxed
+                    Integer.valueOf(integer);       // boxing of already boxed value
+                }
+            }
+        ]]></code>
+    </test-code>
+    <test-code>
+        <!-- Casts are left to UnnecessaryCast -->
+        <description>Unnecessary (primitive -> primitive) casts</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit boxing</message>
+        </expected-messages>
+        <code><![CDATA[
+            class Scratch {
+                public static void main(String[] args) {
+                    Integer integer = 2; // ok
+
+                    long a = (int) Integer.valueOf(2);  // explicit boxing where the value would be autoboxed
+                    int b = (int) 2;                    // ignored, left to UnnecessaryCast
+                    long c = (int) 4;                   // widening can be implicit
+                    double c = (int) 4;                 // widening can be implicit
+                    double c = (int) 4.0;               // cast is narrowing so necessary
+                }
+            }
+        ]]></code>
+    </test-code>
+</test-data>