false-posttive, conceptual problem, and erroneous report text for `FII_USE_METHOD_REFERENCE`

[Vampire]

FII_USE_METHOD_REFERENCE seems to have some problems, I hope it is ok that I write them up here in one issue. Feel free to split it into multiple or to ask me to split it if you must. :-)

I updated from 7.6.9 to 7.7.1 and got a new finding for FII_USE_METHOD_REFERENCE.

---

For reference, the complained about method is:

public T get(P parameter) {
    return get(() -> valueFunction.apply(parameter));
}

where T and P are type parameters of the class and valueFunction is this field of the class: private final Function<P, T> valueFunction;

---

First issue is the explanatory text and with it the conceptual problem:

• This method defines an anonymous lambda function to be called to fetch a single value from the passed in value. While this will work, it is needlessly complex as this function merely calls a single getter method on the object, and thus the code can be simplied by just passing in a method reference instead.

  This only describes the first of the two cases that are considered by the rule and shown in the examples, and in this case it does not match the method as the rule thinks it found the second case.

• or if the lambda expression is a where the parameter

  subject missing? a ... what?

• or if the lambda expression is a [missingsubject] where the parameter is passed a method reference like

  Even with the subject, I don't get the sentence, the parameter is not passed a method reference, you cannot pass a parameter anything. The parameter is passed to a method.

• baubles.stream().map(b -> magic.getSpell(bauble)).collect(Collectors.toSet())

  bauble should probably be b?

• do baubles.stream().map(magic::getSpell).collect(Collectors.toSet())

  This does not necessarily result in the same semantics and behavior and thus is a very dangerous recommendation without conditions. As far as I have seen you just parse op codes there, so probably do not a deeper analysis, pardon-me if I got that wrong from a cursory look. The original code checks at the time the lambda is executed what instance magic currently holds and calls getSpell on that. The recommended code always uses the instance magic is pointing to at the time the map call is made. So if magic is some final field for example, the method reference might be appropriate and give the same result, but if magic is in any way mutable between getting the method reference and the last invocation, then the semantics and behaviour changed.

  I like the rule to use method reference where safely possible, but I really think the second half is too fragile and always needs a concious human decision that this is a spot where it is safe to use a method reference and I would recommend to just remove that case from the rule.

  When not removing that part from the rule, I'd at least like to ask for splitting it off into its own bug pattern that can be completely disabled independently.

---

The false-positive should be obvious I hope. As valueFunction is a final field, it would here indeed be fine to use a method reference on it. But the argument is not the parameter of the lambda expression, but the parameter of the enclosing method. If I got the rule right, it asks me to instead write

public T get(P parameter) {
    return get(valueFunction::apply);
}

which would then not compile, as there there is no no-arg apply method.

Or even worse, if there were also a no-arg apply method, it would compile, but with changed semantics, as it would no longer be equivalent to return get(() -> valueFunction.apply(parameter)); but to return get(() -> valueFunction.apply());.

[mebigfatguy]

trying to expand your first example?

public class Issue503<P, T> {

    private Function<P, T> valueFunction;

    public T get(P parameter) {
        return get(() -> valueFunction.apply(parameter));
    }
}

i'm missing something i guess.

[Vampire]

Hm, we might have found another bug? Namely that it is not found when compiling with new enough Java version. The code you have is fine except for the missing get method to make it compile. (protected T get(Supplier<T> valueSupplier) { return null; }) If you compile the class with Java 8 or 11, the pattern is found, if you compile with Java 21, the pattern is not found, so maybe also other patterns are not detected anymore with newer compiler versions.

[alianman]

This block reports FII_USE_METHOD_REFERENCE on Java 25:

    var resultSet =
        list.stream()
            .map(MyClass::getMyValue)
            .collect(Collectors.toSet());

[mebigfatguy]

@alianman i used this class compiled in 25, and did not receive any FII reports

Did i do something wrong?

import java.util.List;
import java.util.stream.Collectors;

public class FII {

    public void test(List<FII> list) {
           var resultSet =
                list.stream()
                    .map(FII::getMyValue)
                    .collect(Collectors.toSet());
    }

    public String getMyValue() {
        return "foobar";
    }
}

[alianman]

@mebigfatguy, thanks for the reply! You are right, this snippet works as expected.

Reproducer appeared to be more complex than I expected.

Two DTOs in own package:

package project.dto;

class BaseDto {

  public String getMyString() {
    return "myString";
  }
}

package project.dto;

public class MyDto extends BaseDto {

}

Service in own package:

package project.service;

import java.util.Collection;
import java.util.stream.Collectors;
import project.dto.MyDto;

public class Service {

  public void processDto(Collection<MyDto> myDtos) {
    myDtos.stream().map(MyDto::getMyString).collect(Collectors.toSet());
  }
}

The key point is separate packages and package-private modifier in BaseDto.

Decompiled byte-code for Service class looks as follows:

package project.service;

import java.util.Collection;
import java.util.stream.Collectors;
import project.dto.MyDto;

public class Service {
    public void processDto(Collection<MyDto> myDtos) {
        myDtos.stream().map((rec$) -> rec$.getMyString()).collect(Collectors.toSet());
    }
}

If I move DTOs to Service package or change BaseDto access modifier to public, then it's ok:

package project.service;

import java.util.Collection;
import java.util.stream.Collectors;

public class Service {
    public void processDto(Collection<MyDto> myDtos) {
        myDtos.stream().map(BaseDto::getMyString).collect(Collectors.toSet());
    }
}