New Check: Unnecessary Block Braces

[pjljvandelaar]

I could only find the following documentation:
https://checkstyle.org/checks/coding/unnecessaryparentheses.html
https://checkstyle.sourceforge.io/checks/blocks/needbraces.html

I have downloaded the latest cli from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

Running
java -jar .\checkstyle-10.xx.X-all.jar -c .\config.xml .\Example.java

With the following config file with non existing Check:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN" "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
  <module name="TreeWalker"> 
    <module name="UnnecessaryBlockBraces or any other name">
      <property name="target" value="LAMBDA, FOR_EACH_CLAUSE, LITERAL_FOR, LITERAL_WHILE">
    </module>
  </module>
</module>

and the following java file (that correctly builds with javac .\Example.java)

import java.util.function.Consumer;
import java.util.function.Function;

public class Example {
  public static void main(String[] args) {
    Consumer<Integer> printFiveCorrect = x -> System.out.println(x+5);
    Consumer<Integer> printFiveWrong = x -> { System.out.println(x+5); };

    Function<Integer, Integer> addFiveCorrect = x -> x+5;
    Function<Integer, Integer> addFiveWrong = x -> { return x+5; };
  }
}

Describe the solution you'd like
I would like to get warnings on line 7
Consumer<Integer> printFiveWrong = x -> { System.out.println(x+5); };
and line 10
Function<Integer, Integer> addFiveWrong = x -> { return x+5; };
Something like

[ERROR] C:\temp\checkstyle\.\Example.java:7:48: Unnecessary curly braces block tokens around lambda body. [UnnecessaryBlockBraces]
[ERROR] C:\temp\checkstyle\.\Example.java:10:56: Unnecessary curly braces block tokens around lambda body. [UnnecessaryBlockBraces]

Indeed lambda function consisting out of a single statement / expression don't need block braces.
Note that typically block braces are called either "(curly) braces" or "curly brackets".

An usage outside of lambda, only if-, while-, for-, and switch-statements with a single statement in a branch come to mind.
Checkstyle already checks their presence (instead of absence): https://checkstyle.sourceforge.io/checks/blocks/needbraces.html

[pjljvandelaar]

With

    <module name="NeedBraces">
      <property name="tokens" value="LAMBDA"/>
      <property name="allowSingleLineStatement" value="false"/>
    </module>

in the config file. One gets

Starting audit...
[ERROR] C:\temp\checkstyle\.\Example.java:6:44: '->' construct must use '{}'s. [NeedBraces]
[ERROR] C:\temp\checkstyle\.\Example.java:9:51: '->' construct must use '{}'s. [NeedBraces]
Audit done.

Exactly, the opposite of what this feature request proposes ;-)

[romani]

I am ok with UnnecessaryBlockBraces
It is ok to not reference lambda in name , as eventually we can extend this Check to more broad scope so we will introduce tokens selection later on.

@nrmancuso , @rnveach , please review.

[rnveach]

Name doesn't match the violations. ; isn't a brace, and can it really be unnecessary? If all this will violate is braces, then I don't see the need for Unnecessary tokens in the violation message. The message should be more friendly in this case.

An usage outside of lambda, only if-, while-, for-, and switch-statements with a single statement in a branch come to mind.

I am understanding this. Will this violate switches too? Examples doesn't make this clear.

Indeed lambda function consisting out of a single statement / expression don't need block braces.

Will this violate only single lines or single statements? What if we have a single statement that spans multiple lines? This will conflict with NeedBraces .

[romani]

yes, it is left over from initial proposal.
message is changed to Unnecessary curly braces block tokens around lambda body.

An usage outside of lambda, only if-, while-, for-, and switch-statements with a single statement in a branch come to mind.

@pjljvandelaar , please share examples to make it very clear what is a target or lets remove for now from design, to not confuse on scope.

Will this violate only single lines or single statements? What if we have a single statement that spans multiple lines? This will conflict with NeedBraces .

this check will be in clear conflict with NeedBraces, use need to decide what to choose as preferable.
I am voting for "single lines" to not have braces, but if there are few opinions around we can make property options in this Check.

[rnveach]

this check will be in clear conflict with NeedBraces
I am voting for "single lines" to not have braces

This doesn't have to be in conflict if we give it the appropriate properties.

https://checkstyle.org/checks/blocks/needbraces.html#NeedBraces
NeedBraces has an option for allowSingleLineStatement. If this check has a repspective property to only apply to single line statements, then there is no conflict. Just the opposite, it enforces the appropriate validation that we are looking at all cases. If something is not validated, programmers will do whatever they want.

message is changed to Unnecessary curly braces block tokens around lambda body.

If this is the message going with, then this check needs to be renamed to something like UnecessaryLambdaBlockBraces. Ifs and fors have braces, but it seems we are only focusing on lambdas.

[pjljvandelaar]

When comparing

    Consumer<Integer> printFiveCorrect = x ->   System.out.println(x+5)   ;
    Consumer<Integer> printFiveWrong   = x -> { System.out.println(x+5); };

We see that {, }, but also one ; is not essential to implement this functionality.

What is preferred, the top or bottom line, is up to the user of checkstyle.
Some want NeedBraces and others UnnecessaryBlockBraces.
Some might even want to distinguish between single line and multi line lambda functions.

[romani]

This doesn't have to be in conflict if we give it the appropriate properties.

can you suggest some properties ? can we add them later on? as we see Check in in action on real code and we get real feedback from users.
until extra properties are implemented new check will be not in use on user side, in worse case.

needs to be renamed to something like UnecessaryLambdaBlockBraces

I thought on this and think we can use general name if we find some other cases where block braces are not required.
May be:

 while(someXondition) {i++}; // violation
for(int i : values) {system.out.print(i)} // violation

But I am not strong on this, and if more people vote on more specific Check, I am ok with it. More specific Checks has more ways to be customized to do what users wants

[pjljvandelaar]

Indeed, having warnings for for and while loops with a single statement

while(someXondition) {i++;} // violation
for(int i : values) {system.out.print(i);} // violation

helps to improve readability.
So a more general check seems valuable.

[romani]

ok, so in this case we will need property target to let users decide what to validate and what to skip.
I updated issue description.

@rnveach , please review

[rnveach]

This is still not clear.

Will this violate only single lines or single statements? What if we have a single statement that spans multiple lines?

There are no examples or concrete explanation. don't need is like a maybe and not saying one way or the other. Technically if the examples span multiple lines they still don't require a curly.

[romani]

@pjljvandelaar, please update issue description with more examples and configs to make it clear for all how it should work and in what cases it will raise violations.

[pjljvandelaar]

First configuration:

<module name="Checker">
  <module name="TreeWalker"> 
    <module name="UnnecessaryBlockBraces">
    </module>
  </module>
</module>

Remove unnecessary block braces to minimize the code everywhere.

Second configuration

<module name="Checker">
  <module name="TreeWalker"> 
    <module name="UnnecessaryBlockBraces">
      <property name="keepAroundMultipleLines" value="true"/>
    </module>
  </module>
</module>

Improve readability by removing block braces when possible.
In this case, block braces surrounding lambda expression that cover multiple lines are NOT removed.

Third configuration

 <module name="Checker">
  <module name="TreeWalker"> 
    <module name="UnnecessaryBlockBraces">
      <property name="target" value="LAMBDA">
    </module>
  </module>
</module>

Remove unnecessary block braces to minimize the code only for LAMBDA.
Other targets, such as FOR_EACH_CLAUSE, LITERAL_FOR, LITERAL_WHILE, are not affected.

[pjljvandelaar]

@romani I hope my previous comment answers your question!

[romani]

Please update your comment above to have java code snippets under each config example, to show how config will highlight code with violations. If there will be violation, please put a comment on same line.