Incorrect/Inconsistent Sonar Squid Rule regarding Lambdas and Curly Braces - squid:S1602

Version - Version 6.7.6 (build 38781) - LGPL v3

Problem:

This Squid is slightly “Counterproductive” and doesn’t cause issues as it claims

Example:

Consider the following block of java statement:

        Callable<List<String>> loader = () -> {
            return computeForCache(key);
        };

The above is one of the usual cases for using a Callable objects for cache/data loading. It uses the squid:S1604 incorrectly here. There is no way around it. It hinders simplicity by using “Readability” as a metric.

We would request that this squid is revisited and if possible, removed to avoid such issues cropping up.

There is also a similar discussion around Stack Overflow:

Thanks,

Hello M.Manna,

I’m a bit lost here, and do not follow your reasoning.

It’s perfectly possible to get rid of the curly braces in the case you are showing… As rule S1602 is recommending.

Of course since the body of the lambda contains a return statement, you should remove the return keywords, but it’s completely equivalent, as shown in the following code.

import java.util.List;
import java.util.concurrent.Callable;

abstract class A {

  void foo(String key) {
    Callable<List<String>> loader1 = () -> {
      return computeForCache(key);
    };

    // equivalent code without curly braces
    Callable<List<String>> loader2 = () -> computeForCache(key);
  }

  abstract List<String> computeForCache(String key);
}

From my understanding, removing cumbersome/not-required code is definitely improving readability… which is also what’s the StackOverflow answers is saying.

Finally, note that since 2014 (year of SOF question creation), the rule has been improved to correctly handle statements which can not be replaced. I would also recommend you to upgrade to SonarQube LTS 7.9, as 6.7.6 is not supported anymore.

Cheers,
Michael

1 Like

Hi,
Sonarqube definitely still detects curly braces that are essential as useless (as of SQ 8.1)

Consider following two methods that accept functional interfaces:

protected static <T> T asTransaction(Function<Session, T> function);
protected static void asTransaction(Consumer<Session> function);

then the curly braces are necessary because following two calls are valid:

asTransaction(session -> {
      session.save(this);
});

asTransaction(session -> {
      return session.get(this);
});

while following are compile errors:

asTransaction(session -> session.save(this))
asTransaction(session -> session.get(this));

both fail with

reference to asTransaction is ambiguous
both method <T>asTransaction(java.util.function.Function<org.hibernate.Session,T>) in <redacted> and
method asTransaction(java.util.function.Consumer<org.hibernate.Session>) in <redacted> match

Hello,

I’m wondering if it’s not possible in your case to write something like this:

asTransaction((Consumer<Session>) session -> session.get(this));

It disambiguates the call for both the compiler and a human reader, what do you think about it?

That being said, doing so incorrectly triggers S1905: Redundant casts should not be used, I will probably create a ticket for it…

Quentin, how do you apply that advice to the other method that does not return anything?
asTransaction(session -> session.save(this)) is still a compiler error.

Edit: To be clear in this case I could change the asTransaction interface to Void, but that looks like overkill, when the laguage already has means to distinguish between the situations (the “unnecessary” braces), and in general, you don’t always own the called interface to be able to make changes in it.
Edit2: Looking at the details, the Void would have to replace void in many signatures, and in my opinion raises more confusion than the “unnecessary” braces, as their removal promptly provides explanation by the compiler, while reason for widespread Void usage would need to be explained far away from the original place that “required” it.

Ignore the above post, it’s completely incorrect.

I was confused by what you did, as you used the cast exactly in the opposite way from how it should be used. The correct calls are:

    return asTransaction((Function<Session,ResultType>) session -> session.get(this));
    asTransaction((Consumer<Session>) session -> session.save(this));

I’d strongly question whether they are more readable/less confusing than the original:

asTransaction(session -> {
      session.save(this);
});

return asTransaction(session -> {
      return session.get(this);
});

The initial goal of my post was to discuss the use of casting, I did not make sure the code was correct, sorry for the confusion.

Without taking into account the (arguable) question of readability, this is a tricky case to handle, I expect a significant effort to improve the behavior in this kind of situation, without a strong incentive, I’m afraid nobody will take the time for it.
Therefore, on your side, do you feel that this situation is a corner case, that it’s enough to resolve as “Won’t fix” or “False positive” or is it really bad and you strongly believe it should be improved?

In any case, this is an interesting problem, thanks for raising it!

Currently this was the only such interface in our codebase, but it was used on many (~50) places. We are just only starting to use the functional programming style.
We have so far chosen to adopt another workaround, disambiguating the asTransaction functions by name (transactionalFunction for the one that returns value and transactionalProcedure for the one that does not). That workaround does not have the drawback of triggering S1905.

If SQ was able to detect that the variant without braces/casting is in fact invalid Java construct that would be preferable (so that neither S1905 nor S1602 are triggered) as the function/procedure is already expressed in another way and is obvious in the context, but ultimately this isn’t the only code bent to appease the tooling, so we will likely learn to live with it.

Thanks for looking into this.

1 Like

Glad to see that you found a solution that (somehow) works for you.

Feel free to get back to us if you face a similar problem and that the suggestions discussed above are not enough.

Best,
Quentin