Use simpler functional interfaces wherever possible

Rule description & motivation
S4276 checks for uses of functional interfaces such as Function<Integer, R> that could be replaced with IntFunction<R> which avoids autoboxing.

However, apart from such cases, developers who are not yet familiar with all the functional interfaces in the JDK will use general functional interfaces instead of specialised ones. In a large code base where the team recently began using lambdas, I found several such cases and was surprised that SonarQube did not detect them.

I came up with this list of possible replacements that should be detected by a new rule or by S4276:

Current Interface Preferred Interface Remarks
Function<X, X> UnaryOperator<X> the original pattern I found
BiFunction<X, X, X> BinaryOperator<X>
Predicate<File> FileFilter
Predicate<Path> PathMatcher
Callable<X> Supplier<X> if it does not throw a checked exception
Function<File, Boolean> FileFilter if possible
Function<Path, Boolean> PathMatcher if possible
Function<X, Boolean> Predicate<X> if possible
BiFunction<X Y, Boolean> BiPredicate<X,Y> if possible

Impact to keep this code as it is

  • The general interface arguably does not capture the author’s intention as well as the specialised one.
  • Using the general *Function interfaces makes it impossible to use the and(), or(), negate() methods offered by *Predicate.

Notes
S4276 is titled “Functional Interfaces should be as specialised as possible”, which makes no mention of autoboxing. If a new rule is added for my suggestion, S4276 title should be rephrased to differentiate it from the new one.

References

Type
Code Smell

Tags
clumsy

Hello,

thank you for your suggestion. I think these additional interfaces can be handled by the same rule and we don’t need to create a new rule for them. Also, cases Function<X,X> and BiFunction<X,X,X> should already be handled by the rule, do you have an example code where this doesn’t work?

However, I am not convinced about your proposal to replace Callable with Supplier. Callable is used extensively within java.util.concurrent framework and it is not possible to use Supplier with this API. Such change would break a lot of existing code.

Also changing Predicate<File> with FileFilter means that API which is working with generic Predicate<?> will stop to work. Same for PathMatcher.

I created the ticket to improve the rule with rest of your suggestions
https://jira.sonarsource.com/browse/SONARJAVA-2831

Feel free to object to my ideas here and I will update the ticket :slight_smile:

1 Like

Come to think of it, I agree with your reasoning regarding Predicate vs FileFilter/PathMatcher. Also, although you didn’t mention it, a similar case can be made for Function<File, Boolean> to FileFilter and Function<Path, Boolean> to PathMatcher as that would mean losing the compose()/andThen() functionality. So I’m fine with leaving all that out.

Yes, but not in a public code base. I verified that the quality gate includes S4276 and that no issue was reported on the class (we’re using SonarCloud), so this seems like a false negative that should be fixed. The class looks like the following; I only changed names and removed irrelevant members.

package com.example.s4276;

import java.util.function.Function;
import javax.inject.Inject;
import lombok.AccessLevel;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;

@Slf4j
@RequiredArgsConstructor(onConstructor = @__(@Inject), access = AccessLevel.PROTECTED)
class FooModifier
{
    public void modify(
        @NonNull SomeObject someObject,
        @NonNull SomeEnum someEnum,
        @NonNull Function<Foo, Foo> fooFunction)
    {
        // ...
    }
}

Note: When I read the introductory paragraphs in the rule description, I concluded it only covers boxing-related replacements and never bothered to check the table for this case. Independently of fixing the false negative, you should definitively add a paragraph about those other replacements, probably similar to the rationale I gave above.

Are you providing bytecode for your analysis? Bytecode for Foo needs to be provided (unless Foo is defined in the same source file), otherwise analyzer cannot deduce that type arguments are the same symbol.

How can I verify this? I would assume bytecode is available to the scanner due to these points:

  • We’re using the Maven plugin in the same way as the docs recommend.
  • Foo is part of another package that is compiled in another Maven module which is part of the same aggregator build, and the current module has a regular dependency to that module.

Note, however, that Foo also uses Lombok (specifically @Value, @Builder(toBuilder = true) and @NonNull) - then again, it still ends up as bytecode in a jar, so that fact should not really be relevant when scanning FooModifier, right?

You can run your maven command with -X , this will print more debug information (it’s quite verbose for big projects). When .class file is not found for some class it will print something like

[DEBUG] 09:47:18.688 .class not found for test.System

(note that currently there is minor issue that it prints such statements for packages, you can ignore that).

Also it is important how you launch sonar:sonar goal. It should be one of these two ways

  • mvn clean verify sonar:sonar (single command)
  • mvn clean install && mvn sonar:sonar (different commands, install is required)

I now manually launched the analysis like this:

mvn \
    -X \
    -B \
    -e \
    -DperformRelease=true \
    org.jacoco:jacoco-maven-plugin:0.7.9:prepare-agent \
    verify \
    org.codehaus.mojo:sonar-maven-plugin:3.3.0.603:sonar \
    -Dsonar.host.url=https://sonarcloud.io \
    -Dsonar.organization=$SONARCLOUD_ORGANIZATION \
    -Dsonar.login=$SONARCLOUD_LOGIN_TOKEN

[ edit: I forgot to post the -X, but I did include it in the command ]

No issue was raised with FooModifier, and no debug message indicated bytecode for Foo was missing (though I did see the harmless .class not found for com.example.s4276 messages you mentioned).

(Our automated build uses the same, just with deploy instead of verify, and without -X.)

Sorry, I did a mistake and overlooked the fact that we are talking about method parameter. The rule as it is currently implemented won’t raise issue on method parameter, because it only consider implements clause and variables with initializer.

I created ticket to add this improvement https://jira.sonarsource.com/browse/SONARJAVA-2853

1 Like