From 625fb36b14ecb450519506743471ed2f51278f48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Tue, 14 May 2024 13:25:44 +0200 Subject: [PATCH] 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 --- .../lang/java/ast/internal/JavaAstUtils.java | 21 ++++++++++ .../rule/codestyle/UnnecessaryBoxingRule.java | 20 ++++----- .../pmd/lang/java/types/TypeOps.java | 20 +++++++++ .../pmd/lang/java/types/ast/ExprContext.java | 2 +- .../types/ast/internal/PolyResolution.java | 40 ++++++++++-------- .../java/types/ast/ConversionContextTests.kt | 39 +++++++++++++++++- .../rule/codestyle/xml/UnnecessaryBoxing.xml | 41 ++++++++++++++----- 7 files changed, 140 insertions(+), 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 index f71a5c1a7e7..31149fc70cb 100644 --- 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,6 +51,7 @@ 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.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/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 index 27b45b62793..381c4ff3d7f 100644 --- 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 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 @@ 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; } @@ -140,7 +140,7 @@ private void checkBox( 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 @@ 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); @@ -160,6 +163,7 @@ private void checkBox( } } + 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 index 0398a5c2d89..5104bbedfaa 100644 --- 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; + } // } 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 index 15912b40830..ac11aa11075 100644 --- 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: * 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 index 1f6142aa1e7..e6ac7601930 100644 --- 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 index 03ae05f13e0..1a67878e70e 100644 --- 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 { R apply(T t); } + interface ToLongFun { long apply(T t); } + R map(T t, Fun fun) {} + long mapToLong(T t, ToLongFun 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/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 index a7b46cdccb5..1fa269aeab6 100644 --- 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 {{ Unnecessary explicit unboxing Unnecessary explicit conversion from Integer to double - Unnecessary explicit conversion from Integer to double + Unnecessary explicit conversion from Integer to double through long Patch 2075906: Add toString() to the rule UnnecessaryWrapperObjectCreation - 1 - - Unnecessary explicit boxing - + 0 #1057 False positive for UnnecessaryWrapperObjectCreation - 1 - 3 - - Unnecessary explicit conversion from int to Float - + 0 + + [java] UnnecessaryBoxing FP in lambda + 1 + 11 + + Unnecessary explicit conversion from int to long through Long + + Long.valueOf(it.cents())); + } + Object map2(Item item) { + // here it is not necessary + return mapToLong(item, it -> Long.valueOf(it.cents())); + } + interface Fun { R apply(T t); } + interface ToLongFun { long apply(T t); } + R map(T t, Fun fun) {} + long mapToLong(T t, ToLongFun fun) {} + } + ]]> +