Merge branch 'pr-5176'

Monits · Aug 23, 2024 · dcee6e6 · dcee6e6
2 parents 07f96e7 + 2182225
commit dcee6e6
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 39 deletions.
diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md
@@ -17,6 +17,8 @@ This is a {{ site.pmd.release_type }} release.
 ### 🐛 Fixed Issues
 * apex-performance
   * [#5139](https://github.com/pmd/pmd/issues/5139): \[apex] OperationWithHighCostInLoop not firing in triggers
+* pmd-bestpractices
+  * [#5145](https://github.com/pmd/pmd/issues/5145): \[java] False positive UnusedPrivateMethod
 * plsql-bestpractices
   * [#5132](https://github.com/pmd/pmd/issues/5132): \[plsql] TomKytesDespair - exception for more complex exception handler
 

diff --git a/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/BaseInvocMirror.java b/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/BaseInvocMirror.java
@@ -31,9 +31,17 @@ abstract class BaseInvocMirror<T extends InvocationNode> extends BasePolyMirror<
 
     private MethodCtDecl ctDecl;
     private List<ExprMirror> args;
-
-    BaseInvocMirror(JavaExprMirrors mirrors, T call, @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
+    /**
+     * Some method invocations may appear to be poly expressions,
+     * but they have no context type (for instance because they
+     * are in the initializer of a local with inferred type).
+     * These must be treated as standalone expressions.
+     */
+    protected final boolean mayBePoly;
+
+    BaseInvocMirror(JavaExprMirrors mirrors, T call, boolean mustBeStandalone, @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
         super(mirrors, call, parent, subexprMaker);
+        mayBePoly = !mustBeStandalone;
     }
 
     @Override
@@ -61,8 +69,13 @@ public boolean isEquivalentToUnderlyingAst() {
 
     protected MethodCtDecl getStandaloneCtdecl() {
         MethodCallSite site = factory.infer.newCallSite(this, null);
-        // this is cached for later anyway
-        return factory.infer.getCompileTimeDecl(site);
+        if (mayBePoly) {
+            // this is cached for later anyway
+            return factory.infer.getCompileTimeDecl(site);
+        } else {
+            factory.infer.inferInvocationRecursively(site);
+            return site.getExpr().getCtDecl();
+        }
     }
 
     @Override

diff --git a/...in/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/ConditionalMirrorImpl.java b/...in/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/ConditionalMirrorImpl.java
@@ -19,7 +19,6 @@
 import net.sourceforge.pmd.lang.java.types.TypeConversion;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ExprMirror;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ExprMirror.BranchingMirror;
-import net.sourceforge.pmd.lang.java.types.internal.infer.MethodCallSite;
 import net.sourceforge.pmd.lang.java.types.internal.infer.ast.JavaExprMirrors.MirrorMaker;
 
 class ConditionalMirrorImpl extends BasePolyMirror<ASTConditionalExpression> implements BranchingMirror {
@@ -135,23 +134,7 @@ private JTypeMirror standaloneExprTypeInConditional(ExprMirror mirror, ASTExpres
         }
 
         if (e instanceof ASTMethodCall) {
-            /*
-                A method invocation expression (§15.12) for which the chosen most specific method (§15.12.2.5) has return type boolean or Boolean.
-                Note that, for a generic method, this is the type before instantiating the method's type arguments.
-
-            */
-            JTypeMirror current = InternalApiBridge.getTypeMirrorInternal(e);
-            if (current != null) {
-                // don't redo the compile-time decl resolution
-                // The CTDecl is cached on the mirror, not the node
-                return current;
-            }
-
-            MethodCallSite site = factory.infer.newCallSite((InvocationMirror) mirror, null);
-
-            return factory.infer.getCompileTimeDecl(site)
-                                .getMethodType()
-                                .getReturnType();
+            return mirror.getStandaloneType();
         }
 
         return null;

diff --git a/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/CtorInvocMirror.java b/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/CtorInvocMirror.java
@@ -29,8 +29,9 @@
 
 class CtorInvocMirror extends BaseInvocMirror<ASTConstructorCall> implements CtorInvocationMirror {
 
-    CtorInvocMirror(JavaExprMirrors mirrors, ASTConstructorCall call, ExprMirror parent, MirrorMaker subexprMaker) {
-        super(mirrors, call, parent, subexprMaker);
+    CtorInvocMirror(JavaExprMirrors mirrors, ASTConstructorCall call,
+                    boolean mustBeStandalone, ExprMirror parent, MirrorMaker subexprMaker) {
+        super(mirrors, call, mustBeStandalone, parent, subexprMaker);
     }
 
     @Override
@@ -47,6 +48,8 @@ class CtorInvocMirror extends BaseInvocMirror<ASTConstructorCall> implements Cto
     @Override
     public JTypeMirror getStandaloneType() {
         if (isDiamond()) {
+            // todo if the expr must be standalone then we
+            // should infer this from the provided arguments.
             return null;
         }
         return getNewType();
@@ -130,7 +133,7 @@ static class EnumCtorInvocMirror extends BaseInvocMirror<ASTEnumConstant> implem
 
 
         EnumCtorInvocMirror(JavaExprMirrors mirrors, ASTEnumConstant call, ExprMirror parent, MirrorMaker subexprMaker) {
-            super(mirrors, call, parent, subexprMaker);
+            super(mirrors, call, false, parent, subexprMaker);
         }
 
         @Override
@@ -163,7 +166,7 @@ static class ExplicitCtorInvocMirror extends BaseInvocMirror<ASTExplicitConstruc
 
 
         ExplicitCtorInvocMirror(JavaExprMirrors mirrors, ASTExplicitConstructorInvocation call, ExprMirror parent, MirrorMaker subexprMaker) {
-            super(mirrors, call, parent, subexprMaker);
+            super(mirrors, call, false, parent, subexprMaker);
         }
 
         @Override

diff --git a/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/JavaExprMirrors.java b/...src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/JavaExprMirrors.java
@@ -68,7 +68,7 @@ boolean mayMutateAst() {
 
     ExprMirror makeSubexprDefault(ASTExpression e, @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
         if (e instanceof InvocationNode) {
-            return getInvocationMirror((InvocationNode) e, parent, subexprMaker);
+            return getInvocationMirror((InvocationNode) e, parent, false, subexprMaker);
         } else if (e instanceof ASTLambdaExpression || e instanceof ASTMethodReference) {
             return getFunctionalMirror(e, parent, subexprMaker);
         } else if (e instanceof ASTConditionalExpression) {
@@ -81,11 +81,13 @@ ExprMirror makeSubexprDefault(ASTExpression e, @Nullable ExprMirror parent, Mirr
         }
     }
 
-    ExprMirror getBranchMirrorSubexpression(ASTExpression e, boolean isStandalone, @NonNull BranchingMirror parent, MirrorMaker subexprMaker) {
+    ExprMirror getBranchMirrorSubexpression(ASTExpression e, boolean mustBeStandalone, @NonNull BranchingMirror parent, MirrorMaker subexprMaker) {
         if (e instanceof ASTConditionalExpression) {
-            return new ConditionalMirrorImpl(this, (ASTConditionalExpression) e, isStandalone, parent, subexprMaker);
+            return new ConditionalMirrorImpl(this, (ASTConditionalExpression) e, mustBeStandalone, parent, subexprMaker);
         } else if (e instanceof ASTSwitchExpression) {
-            return new SwitchMirror(this, (ASTSwitchExpression) e, isStandalone, parent, subexprMaker);
+            return new SwitchMirror(this, (ASTSwitchExpression) e, mustBeStandalone, parent, subexprMaker);
+        } else if (e instanceof InvocationNode) {
+            return getInvocationMirror((InvocationNode) e, parent, mustBeStandalone, subexprMaker);
         } else {
             return subexprMaker.createMirrorForSubexpression(e, parent, subexprMaker);
         }
@@ -96,14 +98,15 @@ public InvocationMirror getTopLevelInvocationMirror(InvocationNode e) {
     }
 
     public InvocationMirror getInvocationMirror(InvocationNode e, MirrorMaker subexprMaker) {
-        return getInvocationMirror(e, null, subexprMaker);
+        return getInvocationMirror(e, null, false, subexprMaker);
     }
 
-    private InvocationMirror getInvocationMirror(InvocationNode e, @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
+    private InvocationMirror getInvocationMirror(InvocationNode e, @Nullable ExprMirror parent,
+                                                 boolean mustBeStandalone, MirrorMaker subexprMaker) {
         if (e instanceof ASTMethodCall) {
-            return new MethodInvocMirror(this, (ASTMethodCall) e, parent, subexprMaker);
+            return new MethodInvocMirror(this, (ASTMethodCall) e, mustBeStandalone, parent, subexprMaker);
         } else if (e instanceof ASTConstructorCall) {
-            return new CtorInvocMirror(this, (ASTConstructorCall) e, parent, subexprMaker);
+            return new CtorInvocMirror(this, (ASTConstructorCall) e, mustBeStandalone, parent, subexprMaker);
         } else if (e instanceof ASTExplicitConstructorInvocation) {
             return new CtorInvocMirror.ExplicitCtorInvocMirror(this, (ASTExplicitConstructorInvocation) e, parent, subexprMaker);
         } else if (e instanceof ASTEnumConstant) {

diff --git a/...c/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/MethodInvocMirror.java b/...c/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/ast/MethodInvocMirror.java
@@ -26,14 +26,16 @@
 class MethodInvocMirror extends BaseInvocMirror<ASTMethodCall> {
 
 
-    MethodInvocMirror(JavaExprMirrors mirrors, ASTMethodCall call, @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
-        super(mirrors, call, parent, subexprMaker);
+    MethodInvocMirror(JavaExprMirrors mirrors, ASTMethodCall call,
+                      boolean isStandalone,
+                      @Nullable ExprMirror parent, MirrorMaker subexprMaker) {
+        super(mirrors, call, isStandalone, parent, subexprMaker);
     }
 
     @Override
     public @Nullable JTypeMirror getStandaloneType() {
         JMethodSig ctdecl = getStandaloneCtdecl().getMethodType();
-        return isContextDependent(ctdecl) ? null : ctdecl.getReturnType();
+        return mayBePoly && isContextDependent(ctdecl) ? null : ctdecl.getReturnType();
     }
 
     private static boolean isContextDependent(JMethodSig m) {

diff --git a/...src/test/kotlin/net/sourceforge/pmd/lang/java/types/internal/infer/BranchingExprsTests.kt b/...src/test/kotlin/net/sourceforge/pmd/lang/java/types/internal/infer/BranchingExprsTests.kt
@@ -279,7 +279,7 @@ class Scratch {
         val (ternary1, ternary2) = acu.descendants(ASTConditionalExpression::class.java).toList()
 
         spy.shouldBeOk {
-            ternary1 shouldHaveType gen.t_Collection[captureMatcher(`?`)] // java.util.Collection<capture#534 of ?>
+            ternary1 shouldHaveType gen.t_Collection[ts.OBJECT] // java.util.Collection<Object>
             ternary2 shouldHaveType gen.`t_Collection{String}` // java.util.Collection<java.lang.String>
         }
     }

diff --git a/...c/test/kotlin/net/sourceforge/pmd/lang/java/types/internal/infer/LocalVarInferenceTest.kt b/...c/test/kotlin/net/sourceforge/pmd/lang/java/types/internal/infer/LocalVarInferenceTest.kt
@@ -4,10 +4,11 @@
 
 package net.sourceforge.pmd.lang.java.types.internal.infer
 
-import net.sourceforge.pmd.lang.test.ast.shouldMatchN
 import net.sourceforge.pmd.lang.java.ast.ProcessorTestSpec
 import net.sourceforge.pmd.lang.java.ast.variableAccess
 import net.sourceforge.pmd.lang.java.types.*
+import net.sourceforge.pmd.lang.test.ast.component6
+import net.sourceforge.pmd.lang.test.ast.shouldMatchN
 
 /**
  *
@@ -101,4 +102,39 @@ class Scratch {
             acu.varId("k") shouldHaveType Runnable::class.decl
         }
     }
+    parserTest("Local var with conditional and 2 poly expressions") {
+        val (acu, spy) = parser.parseWithTypeInferenceSpy(
+            """
+interface Collector<T, A, R> {}
+interface Stream<T> {
+    <R, A> R collect(Collector<? super T, A, R> collector);
+}
+interface List<T> {}
+interface Function<T,R> { R apply(T t); }
+public class Scratch {
+
+    static <T, R> Collector<T, ?, List<List<T>>> groupingByNullable(Function<? super T, ? extends R> key){}
+    static {
+        var lookup = System.currentTimeMillis() > 100L // permission based
+            ? Dao.source1().collect(groupingByNullable(Data::parent)) // DAO call #1
+            : Dao.source2().collect(groupingByNullable(Data::parent)); // DAO call #2
+    }
+
+    interface Data {
+        Integer parent();
+    }
+    static class Dao {
+       static Stream<Data> source1(){}
+       static Stream<Data> source2(){}
+    }
+}
+            """.trimIndent()
+        )
+
+        spy.shouldBeOk {
+            val (_, _, list, _, _, data) = acu.declaredTypeSignatures()
+            // not the anon type
+            acu.varId("lookup") shouldHaveType list[list[data]]
+        }
+    }
 })