Fix pmd#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
oowekyala · May 14, 2024 · 625fb36 · 625fb36
1 parent fc24ece
commit 625fb36
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 43 deletions.
diff --git 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.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,13 +51,15 @@
 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;
 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 static boolean isInStaticCtx(JavaNode node) {
                        || 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
+        );
+    }
 }
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
@@ -12,9 +12,9 @@
 
 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,15 +132,15 @@ private void checkBox(
         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;
         }
 
         if (ctxType != null) {
 
             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,13 +153,17 @@ private void checkBox(
                     reason = "explicit conversion from "
                         + TypePrettyPrint.prettyPrintWithSimpleNames(sourceType)
                         + " to " + TypePrettyPrint.prettyPrintWithSimpleNames(ctxType);
+                    if (!conversionOutput.equals(ctxType)) {
+                        reason += " through " + TypePrettyPrint.prettyPrintWithSimpleNames(conversionOutput);
+                    }
                 }
 
                 rctx.addViolation(conversionExpr, reason);
             }
         }
     }
 
+
     private void checkUnboxing(
             RuleContext rctx,
             ASTMethodCall methodCall,
@@ -192,14 +196,4 @@ private boolean isImplicitlyConvertible(JTypeMirror i, JTypeMirror o) {
             || 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);
-    }
-
 }
diff --git 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 static boolean isUnresolvedOrNull(@Nullable JTypeMirror t) {
         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>
 }
diff --git 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 enum ExprContextKind {
          * 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>

diff --git 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 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.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 @@ ExprContext getTopLevelConversionContext(TypeNode e) {
     }
 
     private static @Nullable JTypeMirror returnTargetType(ASTReturnStatement context) {
-        Node methodDecl =
-            context.ancestors().first(
-                it -> it instanceof ASTMethodDeclaration
-                    || it instanceof ASTLambdaExpression
-                    || it instanceof ASTTypeDeclaration
-            );
-
-        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) {
+        JavaNode methodDecl = JavaAstUtils.getReturnTarget(context);
+
+        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 @@ ExprContext getTopLevelConversionContext(TypeNode e) {
 
             return newAssignmentCtx(returnTargetType((ASTReturnStatement) papa));
 
+
         } else if (papa instanceof ASTVariableDeclarator
             && !((ASTVariableDeclarator) papa).getVarId().isTypeInferred()) {
 
@@ -520,6 +514,19 @@ private ExprContext conversionContextOf(JavaNode node, JavaNode papa) {
             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 @@ private boolean doesCascadesContext(JavaNode node, JavaNode child, boolean inter
         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;
     }
 
 

diff --git 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
+        }
+    }
 })
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
@@ -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>