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

False positive using PreferMethodReference #919

Open
jdussouillez opened this issue Oct 24, 2022 · 2 comments
Open

False positive using PreferMethodReference #919

jdussouillez opened this issue Oct 24, 2022 · 2 comments

Comments

@jdussouillez
Copy link

jdussouillez commented Oct 24, 2022

http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/PreferMethodReferenceCheck.html

We get some false positive using rule PreferMethodReference. This is a specific case when you can't use method reference because of ambiguous context (because of overloaded methods for example).

I guess it'll be complicated to fix as the rule should detect when a method reference is usable or not. No big deal though, the CHECKSTYLE:OFF: <rule> syntax works fine for the few errors we have with this.

Test.java :

import java.util.function.Consumer;

public class Test {

    public static void main(final String[] args) {
        var user = new User();
        var wrapper = new Wrapper();

        // What Checkstyle is not happy about
        wrapper.consume(t -> user.type(t));

        // What Checkstyle wants us to do, but it doesn't compile: "both method consume(Consumer<String>) in Wrapper and method consume(Runnable) in Wrapper match"
        // wrapper.consume(user::type);

        // Our current workaround (do not forget SuppressWithPlainTextCommentFilter module in the config)
        // CHECKSTYLE:OFF: PreferMethodReference
        wrapper.consume(t -> user.type(t));
        // CHECKSTYLE:ON: PreferMethodReference
    }

    private static class Wrapper {

        public void consume(final Consumer<String> consumer) {
            consumer.accept("foo");
        }

        public void consume(final Runnable runnable) {
            runnable.run();
        }
    }

    private static class User {

        private String type;

        public String type() {
            return type;
        }

        public User type(final String type) {
            this.type = type;
            return this;
        }
    }
}

Checkstyle.xml :

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="error"/>
    <property name="fileExtensions" value="java"/>
    <module name="SuppressWithPlainTextCommentFilter"/>
    <module name="TreeWalker">
        <module name="PreferMethodReference"/>
    </module>
</module>
$ java -cp sevntu-checks-1.42.0.jar:checkstyle-10.3.4-all.jar com.puppycrawl.tools.checkstyle.Main -c checkstyle.xml Test.java

Starting audit...
[ERROR] /.../Test.java:10:27: Lambda can be replaced with method reference. [PreferMethodReference]
Audit done.
Checkstyle ends with 1 errors.

openjdk version "17.0.4" 2022-07-19
OpenJDK Runtime Environment (build 17.0.4+8-Ubuntu-120.04)
OpenJDK 64-Bit Server VM (build 17.0.4+8-Ubuntu-120.04, mixed mode, sharing)
@romani
Copy link
Member

romani commented Oct 26, 2022

This is a specific case when you can't use method reference because of ambiguous context (because of overloaded methods for example).

Yes, we can not anything in this case. Checkstyle is not type aware tool, and no access to other files (whole validation is done based on code of single file)
https://checkstyle.org/writingchecks.html#Limitations

@romani
Copy link
Member

romani commented Oct 26, 2022

I can suggest to use more advanced filters/suppressions to make it less aggressive in code to work around such problems: https://checkstyle.org/config_filters.html attention to xpath, and comments based with "influence", single suppression modules ( to keep suppressions in config , not in code - no pollution in code from tool)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants