Merge pull request #4841 from adangel:issue-4817-UnusedPrivateMethod

[java] UnusedPrivateMethod - fix false positive with lambdas #4841

docs/pages/release_notes.md

@@ -379,6 +379,7 @@ The rules have been moved into categories with PMD 6.
 * java-bestpractices
   * [#4603](https://github.com/pmd/pmd/issues/4603): \[java] UnusedAssignment false positive in record compact constructor
   * [#4625](https://github.com/pmd/pmd/issues/4625): \[java] UnusedPrivateMethod false positive: Autoboxing into Number
+  * [#4817](https://github.com/pmd/pmd/issues/4817): \[java] UnusedPrivateMethod false-positive used in lambda
 * java-codestyle
   * [#2847](https://github.com/pmd/pmd/issues/2847): \[java] New Rule: Use Explicit Types
   * [#4239](https://github.com/pmd/pmd/issues/4239): \[java] UnnecessaryLocalBeforeReturn - false positive with catch clause

docs/pages/release_notes_pmd7.md

@@ -3356,6 +3356,7 @@ Language specific fixes:
   * [#4603](https://github.com/pmd/pmd/issues/4603): \[java] UnusedAssignment false positive in record compact constructor
   * [#4625](https://github.com/pmd/pmd/issues/4625): \[java] UnusedPrivateMethod false positive: Autoboxing into Number
   * [#4634](https://github.com/pmd/pmd/issues/4634): \[java] JUnit4TestShouldUseTestAnnotation false positive with TestNG
+  * [#4817](https://github.com/pmd/pmd/issues/4817): \[java] UnusedPrivateMethod false-positive used in lambda
 * java-codestyle
   * [#1208](https://github.com/pmd/pmd/issues/1208): \[java] PrematureDeclaration rule false-positive on variable declared to measure time
   * [#1429](https://github.com/pmd/pmd/issues/1429): \[java] PrematureDeclaration as result of method call (false positive)

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/InternalApiBridge.java

@@ -140,8 +140,8 @@ public static void setCompileTimeDecl(ASTMethodReference methodReference, JMetho
         methodReference.setCompileTimeDecl(methodType);
     }
 
-    public static void initTypeResolver(ASTCompilationUnit acu, JavaAstProcessor processor, TypeInferenceLogger typeResolver) {
-        acu.setTypeResolver(new LazyTypeResolver(processor, typeResolver));
+    public static void initTypeResolver(ASTCompilationUnit acu, JavaAstProcessor processor, TypeInferenceLogger logger) {
+        acu.setTypeResolver(new LazyTypeResolver(processor, logger));
     }
 
     public static void setOverload(InvocationNode expression, OverloadSelectionResult result) {

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/Infer.java

@@ -546,6 +546,8 @@ private JMethodSig instantiateMaybeNoInfer(JMethodSig m, MethodCallSite site, Me
      * then we delegate the solving to the call site's inference context,
      * which knows more, however we add inference vars and their constraints
      * to it.
+     * During non-invocation phases, this inference
+     * checks for validity but doesn't commit any inferred types.
      */
     private JMethodSig instantiateImpl(JMethodSig m, MethodCallSite site, MethodResolutionPhase phase) {
 
@@ -561,52 +563,75 @@ private JMethodSig instantiateImpl(JMethodSig m, MethodCallSite site, MethodReso
             addArgsConstraints(infCtx, m, site, phase); // c
             infCtx.incorporate(); // b2
 
-            if (phase.isInvocation()) {
-
-                boolean shouldPropagate = shouldPropagateOutwards(m.getReturnType(), site, infCtx);
-
-                //propagate outwards if needed
-                if (shouldPropagate) {
-                    // propagate inference context outwards and exit
-                    // the outer context will solve the variables and call listeners
-                    // of this context
-                    LOG.propagateAndAbort(infCtx, site.getOuterCtx());
-                    infCtx.duplicateInto(site.getOuterCtx());
-                    return infCtx.mapToIVars(m);
+            if (phase.isInvocation() || site.canSkipInvocation()) {
+                // this may throw for incompatible bounds
+                return tryToSolve(m, site, infCtx, phase);
+            } else {
+                // we solve on a **copy**. We are only testing applicability
+                // see: https://docs.oracle.com/javase/specs/jls/se9/html/jls-18.html#jls-18.5.1
+                // as per https://docs.oracle.com/javase/specs/jls/se9/html/jls-18.html#jls-18.5.2
+                // we only test it can reduce, we don't commit inferred types at this stage
+                InferenceContext ctxCopy = infCtx.copy();
+                LOG.applicabilityTest(ctxCopy, m);
+                ctxCopy.solve(/*onlyBoundedVars:*/isPreJava8());
+
+                // if unchecked conversion was needed, update the site for invocation pass
+                if (ctxCopy.needsUncheckedConversion()) {
+                    site.setNeedsUncheckedConversion();
                 }
-            }
 
-            // this may throw for incompatible bounds
-            boolean isDone = infCtx.solve(/*onlyBoundedVars:*/isPreJava8());
-
-            if (isPreJava8() && !isDone) {
-                // this means we're not in an invocation context,
-                // if we are, we must ignore it in java 7
-                if (site.getOuterCtx().isEmpty()) {
-                    // Then add the return contraints late
-                    // Java 7 only uses the context type if the arguments are not enough
-                    // https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.12.2.8
-                    m = doReturnChecksAndChangeReturnType(m, site, infCtx);
-                }
-                // otherwise force solving remaining vars
-                infCtx.solve();
+                // don't commit any types
+                return m;
             }
-
-            if (infCtx.needsUncheckedConversion()) {
-                site.setNeedsUncheckedConversion();
-            }
-
-            // instantiate vars and return
-            return InferenceContext.finalGround(infCtx.mapToIVars(m));
         } finally {
             // Note that even if solve succeeded, listeners checking deferred
             // bounds may still throw ResolutionFailedException, in which case
             // by the laws of finally, this exception will be thrown and the
             // return value will be ignored.
-            infCtx.callListeners();
+            if (phase.isInvocation() || site.canSkipInvocation()) {
+                infCtx.callListeners();
+            }
         }
     }
 
+    /**
+     * Actually tries to solve and commit inference types as per
+     * https://docs.oracle.com/javase/specs/jls/se9/html/jls-18.html#jls-18.5.2
+     * {@code infCtx} must already at the B2 state for this method to be called.
+     */
+    private JMethodSig tryToSolve(JMethodSig m, MethodCallSite site, InferenceContext infCtx, MethodResolutionPhase phase) {
+        boolean shouldPropagate = phase.isInvocation() && shouldPropagateOutwards(m.getReturnType(), site, infCtx);
+
+        //propagate outwards if needed
+        if (shouldPropagate) {
+            // propagate inference context outwards and exit
+            // the outer context will solve the variables and call listeners
+            // of this context
+            LOG.propagateAndAbort(infCtx, site.getOuterCtx());
+            infCtx.duplicateInto(site.getOuterCtx());
+            return infCtx.mapToIVars(m);
+        }
+
+        // this may throw for incompatible bounds
+        boolean isDone = infCtx.solve(/*onlyBoundedVars:*/isPreJava8());
+
+        if (isPreJava8() && !isDone) {
+            // this means we're not in an invocation context,
+            // if we are, we must ignore it in java 7
+            if (site.getOuterCtx().isEmpty()) {
+                // Then add the return contraints late
+                // Java 7 only uses the context type if the arguments are not enough
+                // https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.12.2.8
+                m = doReturnChecksAndChangeReturnType(m, site, infCtx);
+            }
+            // otherwise force solving remaining vars
+            infCtx.solve();
+        }
+
+        // instantiate vars and return
+        return InferenceContext.finalGround(infCtx.mapToIVars(m));
+    }
+
     private JMethodSig doReturnChecksAndChangeReturnType(JMethodSig m, MethodCallSite site, InferenceContext infCtx) {
         LOG.startReturnChecks();
         JTypeMirror actualResType = addReturnConstraints(infCtx, m, site); // b3

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/InferenceContext.java

@@ -96,6 +96,21 @@ final class InferenceContext {
         }
     }
 
+    public InferenceContext copy() {
+        final InferenceContext copy = new InferenceContext(ts, supertypeCheckCache, Collections.emptyList(), logger);
+        copy.freeVars.addAll(this.freeVars);
+        copy.inferenceVars.addAll(this.inferenceVars);
+        copy.incorporationActions.addAll(this.incorporationActions);
+        copy.mapping = mapping; // mapping is immutable, so we can share it safely
+
+        // recursively copy parents…
+        if (this.parent != null) {
+            copy.parent = this.parent.copy();
+        }
+
+        return copy;
+    }
+
     public int getId() {
         return id;
     }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/TypeInferenceLogger.java

@@ -61,6 +61,8 @@ default void ambiguityError(MethodCallSite site, @Nullable MethodCtDecl selected
 
     default void ctxInitialization(InferenceContext ctx, JMethodSig sig) { }
 
+    default void applicabilityTest(InferenceContext ctx, JMethodSig sig) { }
+
     default void startArgsChecks() { }
 
     default void startArg(int i, ExprMirror expr, JTypeMirror formal) { }
@@ -366,6 +368,11 @@ public void ctxInitialization(InferenceContext ctx, JMethodSig sig) {
             println(String.format("Context %-11d%s", ctx.getId(), ppHighlight(ctx.mapToIVars(sig))));
         }
 
+        @Override
+        public void applicabilityTest(InferenceContext ctx, JMethodSig sig) {
+            println(String.format("Applicability testing with Context %-11d%s", ctx.getId(), ppHighlight(ctx.mapToIVars(sig))));
+        }
+
         @Override
         public void endInference(@Nullable JMethodSig result) {
             rollback(result != null ? "Success: " + ppHighlight(result)

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/unusedprivatemethod/cache/CacheBuilder.java

@@ -0,0 +1,15 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache;
+
+public class CacheBuilder<K, V> {
+    public static CacheBuilder<Object, Object> newBuilder() {
+        return null;
+    }
+
+    public <X extends K, Y extends V> LoadingCache<X, Y> build(CacheLoader<? super X, Y> loader) {
+        return null;
+    }
+}

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/unusedprivatemethod/cache/CacheLoader.java

@@ -0,0 +1,15 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache;
+
+import java.util.function.Function;
+
+public class CacheLoader<K, V> {
+    private CacheLoader() {}
+
+    public static <K, V> CacheLoader<K, V> from(Function<K, V> r) {
+        return null;
+    }
+}

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/bestpractices/unusedprivatemethod/cache/LoadingCache.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.bestpractices.unusedprivatemethod.cache;
+
+public class LoadingCache<K, V> {
+    public V getUnchecked(K k) {
+        return null;
+    }
+}

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UnusedPrivateMethod.xml

@@ -1835,6 +1835,35 @@ public class OuterClass {
 ]]></code>
     </test-code>
 
+    <test-code>
+        <description>[java] UnusedPrivateMethod false-positive used in lambda #4817</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+package net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod;
+
+import java.util.Collections;
+import java.util.List;
+
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder;
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader;
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache;
+
+public class NotUsedPrivateMethodFalsePositive {
+
+    private final LoadingCache<String, List<? extends String>> clientIdsToDps = CacheBuilder.newBuilder()
+            .build(CacheLoader.from(clientId -> notCachedGetAllDps(clientId))); // unqualified call
+
+    private List<? extends String> notCachedGetAllDps(String clientId) { // UnusedPrivateMethod:	Avoid unused private methods such as 'notCachedGetAllDps(String)'
+        return Collections.singletonList(clientId);
+    }
+
+    public List<?> getClientIdsToDps() {
+        return clientIdsToDps.getUnchecked("");
+    }
+}
+]]></code>
+    </test-code>
+
     <test-code>
         <description>#3627 FN with anonymous class</description>
         <expected-problems>1</expected-problems>
@@ -1844,6 +1873,34 @@ public class Tester {
       private void foo() {}  // unused, should report a warning
   };
 }
+]]></code>
+    </test-code>
+    <test-code>
+        <description>[java] UnusedPrivateMethod false-positive used in lambda - qualified #4817</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+package net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod;
+
+import java.util.Collections;
+import java.util.List;
+
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder;
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader;
+import net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache;
+
+public class NotUsedPrivateMethodFalsePositive {
+
+    private final LoadingCache<String, List<? extends String>> clientIdsToDps = CacheBuilder.newBuilder()
+            .build(CacheLoader.from(clientId -> this.notCachedGetAllDps(clientId))); // qualified call
+
+    private List<? extends String> notCachedGetAllDps(String clientId) { // UnusedPrivateMethod:	Avoid unused private methods such as 'notCachedGetAllDps(String)'
+        return Collections.singletonList(clientId);
+    }
+
+    public List<?> getClientIdsToDps() {
+        return clientIdsToDps.getUnchecked("");
+    }
+}
 ]]></code>
     </test-code>
 </test-data>