Fix #4924 - UnnecessaryBoxing FP in lambda

Change a bit the behavior for some test cases. Previously the rule reported necessary boxing that could be simplified another way, but that is an edge case and not worth complexifying the rule
2024-05-14 13:25:44 +02:00
parent fc24eced58
commit 625fb36b14
7 changed files with 139 additions and 42 deletions
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/internal/JavaAstUtils.java
@@ -37,7 +37,9 @@ import net.sourceforge.pmd.lang.java.ast.ASTBreakStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTCastExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTCatchClause;
 import net.sourceforge.pmd.lang.java.ast.ASTClassType;
+import net.sourceforge.pmd.lang.java.ast.ASTCompactConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
+import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTExecutableDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTExplicitConstructorInvocation;
 import net.sourceforge.pmd.lang.java.ast.ASTExpression;
@@ -49,6 +51,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
 import net.sourceforge.pmd.lang.java.ast.ASTInfixExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTInitializer;
 import net.sourceforge.pmd.lang.java.ast.ASTLabeledStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTLambdaExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTList;
 import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTLoopStatement;
@@ -56,6 +59,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral;
 import net.sourceforge.pmd.lang.java.ast.ASTNumericLiteral;
+import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchBranch;
@@ -802,4 +806,21 @@ public final class JavaAstUtils {
                       || it instanceof ASTExplicitConstructorInvocation
                   );
    }
+
+
+    /**
+     * Return the target of the return. May be an {@link ASTMethodDeclaration},
+     * {@link ASTLambdaExpression}, {@link ASTInitializer},
+     * {@link ASTConstructorDeclaration} or {@link ASTCompactConstructorDeclaration}.
+     *
+     */
+    public static @Nullable JavaNode getReturnTarget(ASTReturnStatement stmt) {
+        return stmt.ancestors().first(
+            it -> it instanceof ASTMethodDeclaration
+                || it instanceof ASTLambdaExpression
+                || it instanceof ASTConstructorDeclaration
+                || it instanceof ASTInitializer
+                || it instanceof ASTCompactConstructorDeclaration
+        );
+    }
 }
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingRule.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/UnnecessaryBoxingRule.java
@@ -12,9 +12,9 @@ import org.apache.commons.lang3.StringUtils;

 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.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;
@@ -132,7 +132,7 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
        JTypeMirror conversionOutput = conversionExpr.getTypeMirror();
        ExprContext ctx = conversionExpr.getConversionContext();
        JTypeMirror ctxType = ctx.getTargetType();
-        if (ctxType == null && conversionExpr instanceof InvocationNode) {
+        if (ctxType == null && conversionExpr.getParent() instanceof ASTExpressionStatement) {
            ctxType = conversionOutput;
        }

@@ -140,7 +140,7 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {

            if (isImplicitlyConvertible(sourceType, conversionOutput)) {

-                final String reason;
+                String reason;
                if (sourceType.equals(conversionOutput)) {
                    reason = "boxing of boxed value";
                } else if (sourceType.unbox().equals(conversionOutput)) {
@@ -153,6 +153,9 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
                    reason = "explicit conversion from "
                        + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType)
                        + " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
+                    if (!conversionOutput.equals(ctxType)) {
+                        reason += " through " + TypePrettyPrint.prettyPrintWithSimpleNames(conversionOutput);
+                    }
                }

                rctx.addViolation(conversionExpr, reason);
@@ -160,6 +163,7 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
        }
    }

+
    private void checkUnboxing(
            RuleContext rctx,
            ASTMethodCall methodCall,
@@ -192,14 +196,4 @@ public class UnnecessaryBoxingRule extends AbstractJavaRulechainRule {
            || 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);
-    }
-
 }
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeOps.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/TypeOps.java
@@ -2075,5 +2075,25 @@ public final class TypeOps {
        return t instanceof JArrayType ? ((JArrayType) t).getComponentType() : null;
    }

+
+    /**
+     * Return true if the method is context dependent. That
+     * means its return type is influenced by the surrounding
+     * context during type inference. Generic constructors
+     * are always context dependent.
+     */
+    public static boolean isContextDependent(JMethodSig sig) {
+        JExecutableSymbol symbol = sig.getSymbol();
+        if (symbol.isGeneric() || symbol.getEnclosingClass().isGeneric()) {
+            if (symbol instanceof JMethodSymbol) {
+                JTypeMirror returnType = ((JMethodSymbol) symbol).getReturnType(EMPTY);
+                return mentionsAny(returnType, symbol.getTypeParameters())
+                    || mentionsAny(returnType, symbol.getEnclosingClass().getTypeParameters());
+            }
+            // generic ctors are context dependent
+            return true;
+        }
+        return false;
+    }
    // </editor-fold>
 }
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/ast/ExprContext.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/ast/ExprContext.java
@@ -130,7 +130,7 @@ public abstract class ExprContext {
         * Assignment context. This includes:
         * <ul>
         * <li>RHS of an assignment
-         * <li>Return statement
+         * <li>Return statement or return expression of a lambda
         * <li>Array initializer
         * <li>Superclass constructor invocation
         * </ul>
--- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/ast/internal/PolyResolution.java
+++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/ast/internal/PolyResolution.java
@@ -20,7 +20,6 @@ import java.util.List;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;

-import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayAccess;
 import net.sourceforge.pmd.lang.java.ast.ASTArrayInitializer;
@@ -44,7 +43,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTSwitchExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchLabel;
 import net.sourceforge.pmd.lang.java.ast.ASTSwitchLike;
 import net.sourceforge.pmd.lang.java.ast.ASTType;
-import net.sourceforge.pmd.lang.java.ast.ASTTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
 import net.sourceforge.pmd.lang.java.ast.ASTVoidType;
 import net.sourceforge.pmd.lang.java.ast.ASTYieldStatement;
@@ -361,29 +359,24 @@ public final class PolyResolution {
    }

    private static @Nullable JTypeMirror returnTargetType(ASTReturnStatement context) {
-        Node methodDecl =
-            context.ancestors().first(
-                it -> it instanceof ASTMethodDeclaration
-                    || it instanceof ASTLambdaExpression
-                    || it instanceof ASTTypeDeclaration
-            );
+        JavaNode methodDecl = JavaAstUtils.getReturnTarget(context);

-        if (methodDecl == null || methodDecl instanceof ASTTypeDeclaration) {
-            // in initializer, or constructor decl, return with expression is forbidden
-            // (this is an error)
-            return null;
-        } else if (methodDecl instanceof ASTLambdaExpression) {
+        if (methodDecl instanceof ASTLambdaExpression) {
            // return within a lambda
            // "assignment context", deferred to lambda inference
            JMethodSig fun = ((ASTLambdaExpression) methodDecl).getFunctionalMethod();
            return fun == null ? null : fun.getReturnType();
-        } else {
+        } else if (methodDecl instanceof ASTMethodDeclaration) {
            @NonNull ASTType resultType = ((ASTMethodDeclaration) methodDecl).getResultTypeNode();
            return resultType instanceof ASTVoidType ? null // (this is an error)
                                                     : resultType.getTypeMirror();
        }
+        // Return within ctor or initializer or the like,
+        // return with value is disallowed. This is an error.
+        return null;
    }

+
    /**
     * Returns the node on which the type of the given node depends.
     * This addresses the fact that poly expressions depend on their
@@ -477,6 +470,7 @@ public final class PolyResolution {

            return newAssignmentCtx(returnTargetType((ASTReturnStatement) papa));

+
        } else if (papa instanceof ASTVariableDeclarator
            && !((ASTVariableDeclarator) papa).getVarId().isTypeInferred()) {

@@ -520,6 +514,19 @@ public final class PolyResolution {
            return node.getIndexInParent() == 0 ? booleanCtx // condition
                                                : stringCtx; // message

+        } else if (papa instanceof ASTLambdaExpression && node.getIndexInParent() == 1) {
+            // lambda expression body
+
+
+            JMethodSig fun = ((ASTLambdaExpression) papa).getFunctionalMethod();
+            if (fun == null || TypeOps.isContextDependent(fun)) {
+                // Missing context, because the expression type itself
+                // is used to infer the context type.
+                return ExprContext.getMissingInstance();
+            }
+            return newAssignmentCtx(fun.getReturnType());
+
+
        } else if (papa instanceof ASTIfStatement
            || papa instanceof ASTLoopStatement && !(papa instanceof ASTForeachStatement)) {

@@ -611,8 +618,7 @@ public final class PolyResolution {
        return node instanceof ASTSwitchExpression && child.getIndexInParent() != 0 // not the condition
            || node instanceof ASTSwitchArrowBranch
            || node instanceof ASTConditionalExpression && child.getIndexInParent() != 0 // not the condition
-            // lambdas "forward the context" when you have nested lambdas, eg: `x -> y -> f(x, y)`
-            || node instanceof ASTLambdaExpression && child.getIndexInParent() == 1; // the body expression
+            || internalUse && node instanceof ASTLambdaExpression && child.getIndexInParent() == 1;
    }


--- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/types/ast/ConversionContextTests.kt
+++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/types/ast/ConversionContextTests.kt
@@ -7,13 +7,16 @@ package net.sourceforge.pmd.lang.java.types.ast

 import io.kotest.assertions.withClue
 import io.kotest.matchers.shouldBe
-import net.sourceforge.pmd.lang.test.ast.component6
-import net.sourceforge.pmd.lang.test.ast.shouldBe
 import net.sourceforge.pmd.lang.java.ast.*
 import net.sourceforge.pmd.lang.java.ast.internal.JavaAstUtils
 import net.sourceforge.pmd.lang.java.types.STRING
+import net.sourceforge.pmd.lang.java.types.TypeTestUtil
+import net.sourceforge.pmd.lang.java.types.ast.ExprContext.ExprContextKind.ASSIGNMENT
+import net.sourceforge.pmd.lang.java.types.ast.ExprContext.ExprContextKind.INVOCATION
 import net.sourceforge.pmd.lang.java.types.parseWithTypeInferenceSpy
 import net.sourceforge.pmd.lang.java.types.shouldHaveType
+import net.sourceforge.pmd.lang.test.ast.component6
+import net.sourceforge.pmd.lang.test.ast.shouldBe

 class ConversionContextTests : ProcessorTestSpec({

@@ -268,4 +271,36 @@ class ConversionContextTests : ProcessorTestSpec({
            l1.rightOperand.conversionContext::getTargetType shouldBe long
        }
    }
+
+    parserTest("Lambda ctx") {
+
+        val (acu, spy) = parser.parseWithTypeInferenceSpy("""
+            class Foo {
+                record Item(int cents) {}
+                Object map(Item item) {
+                    // here the conversion is necessary to determine the context type
+                    return map(item, it -> Long.valueOf(it.cents()));
+                }
+                Object map2(Item item) {
+                    // here it is not necessary
+                    return mapToLong(item, it -> Long.valueOf(it.cents()));
+                }
+                interface Fun<T,R> { R apply(T t); }
+                interface ToLongFun<T> { long apply(T t); }
+                <T,R> R map(T t, Fun<T,R> fun) {}
+                <T> long mapToLong(T t, ToLongFun<T> fun) {}
+            }
+        """)
+
+        val (lambda, lambdaToLong) = acu.descendants(ASTLambdaExpression::class.java).toList()
+
+        spy.shouldBeOk {
+            lambda.expressionBody!!.conversionContext shouldBe ExprContext.getMissingInstance()
+
+            lambdaToLong.expressionBody!!.conversionContext::getTargetType shouldBe long
+            lambdaToLong.expressionBody!!.conversionContext::getKind shouldBe ASSIGNMENT
+
+            lambda.conversionContext::getKind shouldBe INVOCATION
+        }
+    }
 })
--- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryBoxing.xml
+++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/codestyle/xml/UnnecessaryBoxing.xml
@@ -96,7 +96,7 @@ public class Foo {{
        <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>
+            <message>Unnecessary explicit conversion from Integer to double through long</message>
        </expected-messages>
        <code><![CDATA[
 public class Foo {
@@ -172,10 +172,7 @@ public class Foo {
    </test-code>
    <test-code>
        <description>Patch 2075906: Add toString() to the rule UnnecessaryWrapperObjectCreation</description>
-        <expected-problems>1</expected-problems>
-        <expected-messages>
-            <message>Unnecessary explicit boxing</message>
-        </expected-messages>
+        <expected-problems>0</expected-problems>
        <code><![CDATA[
            public class Bar {
                void foo(boolean value) {
@@ -187,11 +184,7 @@ public class Foo {

    <test-code>
        <description>#1057 False positive for UnnecessaryWrapperObjectCreation</description>
-        <expected-problems>1</expected-problems>
-        <expected-linenumbers>3</expected-linenumbers>
-        <expected-messages>
-            <message>Unnecessary explicit conversion from int to Float</message>
-        </expected-messages>
+        <expected-problems>0</expected-problems>
        <code><![CDATA[
            public class Test {
                public void test() {
@@ -357,4 +350,32 @@ public class Foo {
            }
        ]]></code>
    </test-code>
+    <test-code>
+        <description> [java] UnnecessaryBoxing FP in lambda</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>11</expected-linenumbers>
+        <expected-messages>
+            <message>Unnecessary explicit conversion from int to long through Long</message>
+        </expected-messages>
+        <code><![CDATA[
+
+            public class Example {
+
+                record Item(int cents) {}
+
+                Object map(Item item) {
+                    // here the conversion is necessary to determine the context type
+                    return map(item, it -> Long.valueOf(it.cents()));
+                }
+                Object map2(Item item) {
+                    // here it is not necessary
+                    return mapToLong(item, it -> Long.valueOf(it.cents()));
+                }
+                interface Fun<T,R> { R apply(T t); }
+                interface ToLongFun<T> { long apply(T t); }
+                <T,R> R map(T t, Fun<T,R> fun) {}
+                <T> long mapToLong(T t, ToLongFun<T> fun) {}
+            }
+        ]]></code>
+    </test-code>
 </test-data>