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

adangel · 2024-02-29T13:42:02Z

Fixes [java] UnusedPrivateMethod false-positive used in lambda #4817

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Complete build ./mvnw clean verify passes (checked automatically by github actions)
Added (in-code) documentation (if needed)

.../src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateMethodRule.java

pmd-test · 2024-02-29T14:16:40Z

	1 Message
📖	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 0 errors and 0 configuration errors. Download full report as build artifact
✅	Compared to master: This changeset changes 0 violations, introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 7 violations, 0 errors and 7 configuration errors. Download full report as build artifact

Generated by 🚫 Danger

jsotuyod · 2024-03-10T01:19:35Z

Ok, I've spent my whole Saturday juggling between the JLS and the code, but I think I'm starting to understand what the underlaying issue is…

Running this in verbose mode, we get some clues. The code runs in loop through the different stages, but takes care to understand which are invocation runs and which are not… mostly!

As it starts analyzing on the first STRICT phase looking for applicability of methods, it goes:

Phase STRICT           newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>
    ARGUMENTS
    
    Success: newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>
Skipping instantiation of net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder.newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>, it's already complete
Phase STRICT           build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super X, Y>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<X, Y>
    Context 3          build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'a, 'b>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<'a, 'b>
    ARGUMENTS
        Phase STRICT           from(java.util.function.Function<K, V>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<K, V>
            Context 5          from(java.util.function.Function<'c, 'd>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'c, 'd>
            ARGUMENTS
                Argument 0 is not pertinent to applicability
                    At:   file:13:37
                    Expr: clientId -> this.notCachedGetAllDps(clientId)

Effectively, an implicitly typed lambda expression isn't pertitent to applicability as per jls-15.12.2.2. Therefore, applicability is decided by simply checking wether the B2 set is reducible as per jls-18.5.2.1.

And so, the code performs a boolean isDone = infCtx.solve(/*onlyBoundedVars:*/isPreJava8());, and effectively, it is reducible…

            Ivar instantiated   (ctx 5):   'c := java.lang.Object
            Ivar instantiated   (ctx 5):   'd := java.lang.Object
            Success: from(java.util.function.Function<java.lang.Object, java.lang.Object>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<java.lang.Object, java.lang.Object>

So the inference moves, and it's now on invocation phase:

        Checking arg 0 against net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'a, 'b>
            At:   file:13:20
            Expr: CacheLoader.from(clientId -> this.notCachedGetAllDps(clientId))
            Phase INVOC_STRICT     from(java.util.function.Function<K, V>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<K, V>
                Context 7          from(java.util.function.Function<'e, 'f>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'e, 'f>
                RETURN
                    New bound           (ctx 7):   'e >: 'a
                    New bound           (ctx 3):   'b = 'f
                
                ARGUMENTS
                    Checking arg 0 against java.util.function.Function<'e, 'f>
                        At:   file:13:37
                        Expr: clientId -> this.notCachedGetAllDps(clientId)
                    
                
                New bound           (ctx 3):   'a <: 'e
                Ctx 3 adopts ['e, 'f] from ctx 7
                Success: from(java.util.function.Function<'e, 'f>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'e, 'f>

It set's the lambda's inferred type to a still partial type java.util.function.Function<'e, 'f>, and as this is an invocation run, sets a listener on the context waiting for resolution of e and f.

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

Lines 477 to 482 in 8cc2d29

    
           lambda.setInferredType(groundTargetType); 
        
           lambda.setFunctionalMethod(groundFun); 
        
           // set the final type when done 
        
           if (phase.isInvocation()) { 
        
               infCtx.addInstantiationListener(

Here is were things go south… as that inference bubbles up, we eventually get back to context 3 (the one being used in a STRICT evaluation looking for a match on the build method). The context 7 constraints start getting bubbled up and eventually reach context 3, which again, as is on an applicability test, will try to reduce b2 to ensure applicability. Once again, this can be done by reducing e and a to java.lang.Object among other things:

    Ivar merged         (ctx 3):   'b <=> 'f
    Ivar instantiated   (ctx 3):   'a := java.lang.Object
    New bound           (ctx 3):   'e >: java.lang.Object
    Ivar instantiated   (ctx 3):   'e := java.lang.Object

But in context 7 we actually set listeners for the resolution of e and f, so we get a report that e is java.lang.Object, leaving the lambda's inferred type as java.util.function.Function<java.lang.Object, 'f>.

And so, as the algorithm moves forward…

    Phase STRICT           notCachedGetAllDps(java.lang.String) -> java.util.List<? extends java.lang.String>
        ARGUMENTS
            Checking arg 0 against java.lang.String
                At:   file:13:73
                Expr: clientId
                Failed: Incompatible formals: java.lang.Object is not convertible to java.lang.String
        FAILED! SAD!

It sees the lambda actually expects a java.lang.String, which doesn't fit with java.util.function.Function<java.lang.Object, 'f> and fails.

I'm not 100% sure yet, but I believe we need to split how InferenceContext.solve works, so that during applicability it's actually a canSolve vs actually solving it. Maybe just trying to solve on a copy of the context with no listeners can be enough…

This seems to be in compliance with the jls-18.5.2 where it reads:

It is important to note that multiple "rounds" of inference are involved in finding the type of a method invocation. This is necessary, for example, to allow a target type to influence the type of the invocation without allowing it to influence the choice of an applicable method. The first round (§18.5.1) produces a bound set and tests that a resolution exists, but does not commit to that resolution. Subsequent rounds reduce additional constraints until a final resolution step determines the "real" type of the expression.

I'll keep tinkering, but maybe this is enough for @oowekyala to know better…

jsotuyod · 2024-03-10T02:00:41Z

I did a very dirty test, and it seems to work… now I get this beautiful log without failures, and the FP is gone without changing the rule 🎉

Phase STRICT           newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>
    ARGUMENTS
    
    Success: newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>
Skipping instantiation of net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder.newBuilder() -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheBuilder<java.lang.Object, java.lang.Object>, it's already complete
Phase STRICT           build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super X, Y>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<X, Y>
    Context 3          build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'a, 'b>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<'a, 'b>
    ARGUMENTS
        Phase STRICT           from(java.util.function.Function<K, V>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<K, V>
            Context 5          from(java.util.function.Function<'c, 'd>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'c, 'd>
            ARGUMENTS
                Argument 0 is not pertinent to applicability
                    At:   file:13:37
                    Expr: clientId -> this.notCachedGetAllDps(clientId)
                
            
            Success: from(java.util.function.Function<'c, 'd>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'c, 'd>
        Checking arg 0 against net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'a, 'b>
            At:   file:13:20
            Expr: CacheLoader.from(clientId -> this.notCachedGetAllDps(clientId))
            Phase INVOC_STRICT     from(java.util.function.Function<K, V>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<K, V>
                Context 7          from(java.util.function.Function<'e, 'f>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'e, 'f>
                RETURN
                    New bound           (ctx 7):   'e >: 'a
                    New bound           (ctx 3):   'b = 'f
                
                ARGUMENTS
                    Checking arg 0 against java.util.function.Function<'e, 'f>
                        At:   file:13:37
                        Expr: clientId -> this.notCachedGetAllDps(clientId)
                    
                
                New bound           (ctx 3):   'a <: 'e
                Ctx 3 adopts ['e, 'f] from ctx 7
                Success: from(java.util.function.Function<'e, 'f>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'e, 'f>
        
    
    Ivar merged         (ctx 3):   'b <=> 'f
    Success: build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'a, 'f>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<'a, 'f>
Phase INVOC_STRICT     build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super X, Y>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<X, Y>
    Context 8          build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'g, 'h>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<'g, 'h>
    RETURN
        New bound           (ctx 8):   'g = java.lang.String
        New bound           (ctx 8):   'h = java.util.List<? extends java.lang.String>
    
    ARGUMENTS
        Checking arg 0 against net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super 'g, 'h>
            At:   file:13:20
            Expr: CacheLoader.from(clientId -> this.notCachedGetAllDps(clientId))
            Phase INVOC_STRICT     from(java.util.function.Function<K, V>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<K, V>
                Context 11         from(java.util.function.Function<'i, 'j>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'i, 'j>
                RETURN
                    New bound           (ctx 11):   'i >: 'g
                    New bound           (ctx 8):   'h = 'j
                
                ARGUMENTS
                    Checking arg 0 against java.util.function.Function<'i, 'j>
                        At:   file:13:37
                        Expr: clientId -> this.notCachedGetAllDps(clientId)
                    
                
                New bound           (ctx 8):   'g <: 'i
                Ctx 8 adopts ['i, 'j] from ctx 11
                Success: from(java.util.function.Function<'i, 'j>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<'i, 'j>
        
    
    New bound           (ctx 8):   'i >: java.lang.String
    New bound           (ctx 8):   'j = java.util.List<? extends java.lang.String>
    Ivar merged         (ctx 8):   'h <=> 'j
    Ivar instantiated   (ctx 8):   'g := java.lang.String
    Ivar instantiated   (ctx 8):   'i := java.lang.String
    Phase STRICT           notCachedGetAllDps(java.lang.String) -> java.util.List<? extends java.lang.String>
        ARGUMENTS
            Checking arg 0 against java.lang.String
                At:   file:13:73
                Expr: clientId
            
        
        Success: notCachedGetAllDps(java.lang.String) -> java.util.List<? extends java.lang.String>
    New bound           (ctx 8):   'j >: java.util.List<? extends java.lang.String>
    Skipping instantiation of net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.NotUsedPrivateMethodFalsePositive.notCachedGetAllDps(java.lang.String) -> java.util.List<? extends java.lang.String>, it's already complete
    Ivar instantiated   (ctx 8):   'h := java.util.List<? extends java.lang.String>
    Success: build(net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.CacheLoader<? super java.lang.String, java.util.List<? extends java.lang.String>>) -> net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<java.lang.String, java.util.List<? extends java.lang.String>>
Phase STRICT           singletonList(T) -> java.util.List<T>
    Context 15         singletonList('k) -> java.util.List<'k>
    ARGUMENTS
        Checking arg 0 against 'k
            At:   file:16:42
            Expr: clientId
            New bound           (ctx 15):   'k >: java.lang.String
        
    
    Success: singletonList('k) -> java.util.List<'k>
Phase INVOC_STRICT     singletonList(T) -> java.util.List<T>
    Context 16         singletonList('l) -> java.util.List<'l>
    RETURN
        New bound           (ctx 16):   'l <: java.lang.String
    
    ARGUMENTS
        Checking arg 0 against 'l
            At:   file:16:42
            Expr: clientId
            New bound           (ctx 16):   'l >: java.lang.String
        
    
    Ivar instantiated   (ctx 16):   'l := java.lang.String
    Success: singletonList(java.lang.String) -> java.util.List<java.lang.String>
Phase STRICT           getUnchecked(java.lang.String) -> java.util.List<? extends java.lang.String>
    ARGUMENTS
        Checking arg 0 against java.lang.String
            At:   file:20:44
            Expr: \"\"
        
    
    Success: getUnchecked(java.lang.String) -> java.util.List<? extends java.lang.String>
Skipping instantiation of net.sourceforge.pmd.lang.java.rule.bestpractices.unusedprivatemethod.cache.LoadingCache<java.lang.String, java.util.List<? extends java.lang.String>>.getUnchecked(java.lang.String) -> java.util.List<? extends java.lang.String>, it's already complete

I'll clean it up, check a full build, and hopefully update this PR with it.

Interestingly, it is ~42% smaller, which hopefully also means this is actually faster, by avoiding several phases / runs by being more accurate… at least on this scenario.

- This should fix some inference errors

- The inference now works for this case

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

adangel · 2024-03-10T09:41:46Z

@jsotuyod Awesome! Thanks for digging into this!

- remove TODO - ensure listeners are called if we tried to solve

adangel · 2024-03-12T17:53:32Z

I had a look at the regression report now:

👍 we don't introduce new violations, or remove unintended violations. That means, this change didn't affect other rules.
👎 ❓ Not sure, if we should keep digging here: My initial commit removed a couple of false positives (removes 7 violations). At least, I assumed, these were actual false positives:
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Integer.java#L170 'toStringUTF16(int, int)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Integer.java#L338 'toUnsignedString0(int, int)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Integer.java#L389 'formatUnsignedIntUTF16(int, int, byte[], int, int)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Long.java#L154 'toStringUTF16(long, int)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Long.java#L241 'toUnsignedBigInteger(long)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Long.java#L417 'formatUnsignedLong0UTF16(long, int, byte[], int, int)'
- https://github.com/openjdk/jdk/tree/jdk-11+28/src/java.base/share/classes/java/lang/Object.java#L41 'registerNatives()'

We could also declare these false positives as a separate issue. Maybe we should - they all seem to be static methods...

I'll try to retest my test case for #4817 with the changes from here, to make sure, the original issue is indeed fixed (and the rule test case really works).

jsotuyod · 2024-03-13T12:09:09Z

@adangel good catch! I had completely missed that. And yes, there seems to be some issues with static methods, definitely worth having a separate issue to track and fix it regardless of wether we change the rule here or not.

The case for #4817 is still part of this PR, the additional test cases for the rule are in here and they pass. Or did you have an additional scenario?

jsotuyod · 2024-03-14T15:01:15Z

A very preliminar look into the FP on openjdk shows that the node is null not because inference failed, but because, as those classes (ie: java.lang.Integer) are part of the auxclasspath, they resolve to an ASM-backed symbol instead of an AST-backed symbol… and ASM-backed symbols can't reference the AST, so tryGetNode() returns null.

3 possible ways around this would be:

Understand why ASM is prefered over AST-backed and see if it makes sense to switch over (probably there are performance issues at play, and ASM is objectively more reliable with things such as extending a class in a library).
Change the rule to compare matches by JMethodSig instead of by node (this is in practice similar to what this PR tried, but without reimplementing arg-matching manually).

adangel · 2024-03-14T16:51:06Z

The case for #4817 is still part of this PR, the additional test cases for the rule are in here and they pass. Or did you have an additional scenario?

I've tested it now (6.55.0 vs. 7.0.0-rc4 vs 7.0.0-SNAPSHOT) - and #4817 is still fixed. So we are good (just wanted to double check, as the regression tester didn't show any changes anymore).

I'll create a separate issue for these static methods false positives now....

adangel · 2024-03-18T18:05:52Z

I'm going to merge this now. The original bug is fixed and there are no obvious side effects on other rules. In case we find a better/more correct solution, we can ship this improved code with the next version.

oowekyala · 2024-03-19T18:13:11Z

Yeah fwiw I fiddled around with the code and I also think this is a good solution. I think there might still problems with how we type resolve lambdas but well... For now it's the best thing we have. Thanks Juan for implementing this and Andreas for taking care it is merged!

[java] UnusedPrivateMethod - fix false positive with lambdas

dc3a473

Fixes pmd#4817

adangel added this to the 7.0.0 milestone Feb 29, 2024

Merge branch 'master' into issue-4817-UnusedPrivateMethod

9248b55

jsotuyod reviewed Feb 29, 2024

View reviewed changes

.../src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateMethodRule.java Outdated Show resolved Hide resolved

jsotuyod added 4 commits March 10, 2024 01:37

Fix param name

faf3d65

Rework inference to more closely match the JLS

3d8c4fc

- This should fix some inference errors

Remove no longer needed workaround

da9ce6b

- The inference now works for this case

Remove unsued imports

a62ac41

jsotuyod reviewed Mar 10, 2024

View reviewed changes

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/types/internal/infer/Infer.java Outdated Show resolved Hide resolved

Cleanups

cca5c9c

- remove TODO - ensure listeners are called if we tried to solve

oowekyala self-requested a review March 13, 2024 14:09

adangel mentioned this pull request Mar 14, 2024

[java] UnusedPrivateMethod - false positive with static methods #4861

Open

Merge branch 'master' into issue-4817-UnusedPrivateMethod

90ecae9

adangel merged commit 307598e into pmd:master Mar 18, 2024
3 checks passed

adangel deleted the issue-4817-UnusedPrivateMethod branch March 18, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adangel commented Feb 29, 2024

pmd-test commented Feb 29, 2024 •

edited

jsotuyod commented Mar 10, 2024 •

edited

jsotuyod commented Mar 10, 2024 •

edited

adangel commented Mar 10, 2024

adangel commented Mar 12, 2024

jsotuyod commented Mar 13, 2024

jsotuyod commented Mar 14, 2024

adangel commented Mar 14, 2024

adangel commented Mar 18, 2024

oowekyala commented Mar 19, 2024

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

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

Conversation

adangel commented Feb 29, 2024

Ready?

pmd-test commented Feb 29, 2024 • edited

jsotuyod commented Mar 10, 2024 • edited

jsotuyod commented Mar 10, 2024 • edited

adangel commented Mar 10, 2024

adangel commented Mar 12, 2024

jsotuyod commented Mar 13, 2024

jsotuyod commented Mar 14, 2024

adangel commented Mar 14, 2024

adangel commented Mar 18, 2024

oowekyala commented Mar 19, 2024

pmd-test commented Feb 29, 2024 •

edited

jsotuyod commented Mar 10, 2024 •

edited

jsotuyod commented Mar 10, 2024 •

edited