Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] UnusedPrivateMethod FP with raw type missing from the classpath #5097

Closed
wac84s opened this issue Jul 2, 2024 · 11 comments · Fixed by #5340
Closed

[java] UnusedPrivateMethod FP with raw type missing from the classpath #5097

wac84s opened this issue Jul 2, 2024 · 11 comments · Fixed by #5340
Assignees
Labels
a:false-positive PMD flags a piece of code that is not problematic in:type-resolution Affects the type resolution code
Milestone

Comments

@wac84s
Copy link

wac84s commented Jul 2, 2024

Affects PMD Version:
7.3.0

Rule: UnusedPrivateMethod

Description:
private method referenced by method reference operators not getting detected as used

this::mymethod

Code Sample demonstrating the issue:

package com.mytest;

import jakarta.validation.ConstraintViolation; //imported from jakarta.validation:jakarta.validation-api:3.0.2
import java.util.List;
import java.util.Set;

public class UnusedPrivateMethodFalsePositive {
    //this does not trigger UnusedPrivateMethod
    private void doWork(List obj) {
        obj.toString();
    }

    public void execute(Set<List<?>> listOfLists) {
        listOfLists.forEach(this::doWork);
    }

    //BUT this does???
    //UnusedPrivateMethod -  this as a false positive - but what is different?
    private void addValidationError(ConstraintViolation constraintViolation) {
        constraintViolation.toString();
    }

    public void addValidationErrors(Set<ConstraintViolation<?>> constraintViolations) {
        constraintViolations.forEach(this::addValidationError);
    }

}

Expected outcome:

It is not an unused private method

PMD reports a violation at line ..., but that's wrong. That's a false positive.

UnusedPrivateMethodFalsePositive.java:19: UnusedPrivateMethod: Avoid unused private methods such as 'addValidationError(ConstraintViolation)'.

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]

Maven AND cli both report it

@wac84s wac84s added the a:false-positive PMD flags a piece of code that is not problematic label Jul 2, 2024
@wac84s
Copy link
Author

wac84s commented Jul 2, 2024

#5083 might be related. Also has use of method reference operator

@oowekyala
Copy link
Member

I can't reproduce this. Your example does not compile. If I import java.util.Set or replace the Set by a List, everything's fine.

@wac84s
Copy link
Author

wac84s commented Jul 2, 2024

Thanks updated it. I was quickly shrinking my real class into a mock example. Now it does not report a violation :( .... Need to rework my example. A feature fix in itself, compile error should not yield a rule violation should it?

@oowekyala
Copy link
Member

I think you're right it shouldn't... Tbh since the 7.0.0 release these FPs have pointed out several bugs in the type resolution code and allowed us to improve it, which is why I have delayed fixing the rule...

@wac84s
Copy link
Author

wac84s commented Jul 2, 2024

@oowekyala i updated the above example. I was trying to not add a dependency but it only fails with that for me anyway

@wac84s
Copy link
Author

wac84s commented Jul 2, 2024

not familiar with this acronym "FPs". what is that?

@jsotuyod
Copy link
Member

jsotuyod commented Jul 2, 2024

@wac84s FP is short for "false positive"

@adangel
Copy link
Member

adangel commented Jul 26, 2024

I can only reproduce it, when jakarta-validation-api is missing on the auxclasspath.

@oowekyala
Copy link
Member

Surprisingly this has nothing to do with method references. The following also reproduces the bug:

        public class UnusedPrivateMethodFalsePositive {
            private void doWork(List obj){}

            public void execute(List<?> listOfLists) {
                doWork(listOfLists);
            }

        }

Importantly List must be unresolved here. The bug is that for unresolved symbols List<?> is not considered a subtype of the raw List, when it should.

@oowekyala oowekyala changed the title [java] UnusedPrivateMethod with method reference operator [java] UnusedPrivateMethod FP with raw type missing from the classpath Nov 15, 2024
@oowekyala oowekyala self-assigned this Nov 15, 2024
@oowekyala oowekyala added the in:type-resolution Affects the type resolution code label Nov 15, 2024
@oowekyala oowekyala added this to the 7.8.0 milestone Nov 15, 2024
jsotuyod added a commit that referenced this issue Nov 15, 2024
@sd-ebrukanat
Copy link

Following test also gives false positive

 
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.junit.jupiter.MockitoExtension;
import reactor.core.publisher.Mono;
import reactor.function.TupleUtils;
import java.util.Optional;
import static org.assertj.core.api.Assertions.assertThat;
@ExtendWith(MockitoExtension.class)
class PmdTest2 {
    @Test
    void test() {
        var res = Mono.zip(Mono.just(Optional.of("Some test")), Mono.just(TestClass.of("some 2")))
                .flatMap(TupleUtils.function((par1, par2) -> Mono.just(createProperty(par1, par2))))
                .map(objectProperty -> addRequired(objectProperty))
                .block();
        assertThat(res.required).isEqualTo("pop");
        assertThat(res.val).isEqualTo("Some test");
    }
    private TestClass createProperty(Optional<String> par1, TestClass objectProperty) {
        return TestClass.of(par1.orElse("par5"));
    }
    private TestClass addRequired(TestClass objectProperty) {
        return objectProperty.withRequiredProperty("pop");
    }
    private static class TestClass {
        private String required;
        private String val;
        TestClass(String val) {
            this.val = val;
        }
        public static TestClass of(String val) {
            return new TestClass(val);
        }
        public TestClass withRequiredProperty(String s) {
            required = s;
            return this;
        }
        public String getRequired() {
            return required;
        }
        public String getVal() {
            return val;
        }
    }
}
 

@oowekyala
Copy link
Member

@sd-ebrukanat Please open a new ticket. This does not appear to be related to the original issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic in:type-resolution Affects the type resolution code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants