sevntu-checkstyle / sevntu.checkstyle

Additional Checkstyle checks, that could be added as extension to EclipseCS plugin and maven-checkstyle-plugin, Sonar checkstyle plugin, extension for CheckStyle IDEA plugin.
http://sevntu-checkstyle.github.io/sevntu.checkstyle/
190 stars 147 forks source link

False positive using PreferMethodReference #919

Open jdussouillez opened 1 year ago

jdussouillez commented 1 year ago

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.

romani commented 1 year ago

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 commented 1 year ago

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)