RightCurlyCheck: split option into 2 properties

[rnveach]

After discussing with @romani on right curly options for RightCurlyCheck when reviewing #4085 (comment) and how documentation for them should be expanded, it has become apparent that the rules are not so simple to fit into 3 values for a single property.

$ cat TestClass.java
public class Test {
    public void demo() {
        if (true) {doSmth();}
        if (true) {doSmth();} else {doSmthElse();}
        if (true) {
            doSmth();
        }
        if (true) {
            doSmth();     }
        if (true) {
            doSmth();
        } else {
            doSmthElse();
        }
    }

    void doSmth() {}
    void doSmthElse() {}
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
         <module name="RightCurly">
             <property name="option" value="same"/>
         </module>
    </module>
</module>

$ java -jar checkstyle-7.6-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:9:27: '}' at column 27 should have line break before. [RightCurly]
Audit done.
Checkstyle ends with 1 errors.

As example shows, there is no consistency in how brace should be validated. Should it require \n}, }\n, or allowed to be all single line? As it stands now, our own checkstyle config will allow in bad examples if we don't see them first. IMO, RightCurlyCheck should control every aspect of the curlies placement.

I propose we split option into 2 properties to control the placement before and after the }.
I propose the names beforeOption and afterOption. beforeOption will have the values alone and singleline. afterOption will have the values alone and same.
I don't think beforeOption needs same as I can't imagine anyone writing like that. All my examples use if and if-else to show single block, multiple-blocks, and demonstrate behavior of conditional-multiblocks. Examples can apply to any other token like try/catch, etc.

alone means \n should come before or after curly. 'after' is only considered if it has a multi-block statement after it as documented further down.
if (condition) { } violation for alone before
if (condition) { \n} ok for alone before
if (condition) { \n test(); } violation for alone before
if (condition) { } else { violation for alone after
if (condition) { } \n else { ok for alone after

same means \n should not come after curly. 'after' is only considered if it has a multi-block statement after it as documented further down.
if (condition) { } ok for same after
if (condition) { } else { ok for same after
if (condition) { } \n else { violation for same after

singleline means \n should not come before the right curly unless it is not on the same line as the left curly. If it is not on the same line, it will behave as alone.
if (condition) { } ok for singleline
if (condition) { \n} ok for singleline
if (condition) { \n test(); } violation for singleline

For afterOption, it is connected to the next multi-block statement (Ex: else of if, catch of try). If there is no next multi-block statement (if with no else) and no continuation of the current statement, it will specifically default to alone and allow nothing after it. beforeOption has no connection to what comes before it except for singleline option as described above.
Examples:
if (condition) { } int i = 5; 1 violation using any value of afterOption.
if (condition) { }\n ok with any value of afterOption.
if (condition) { } else { 1 violations with alone on afterOption. ok with same.
if (condition) { }\n else { 1 violations with same on afterOption. ok with alone.

    Thread t = new Thread(new Runnable() {
    }); // violation on 'alone' on 'afterOption', ok on 'same'. This is considered "continuation of the current statement".

I finally propose that any changes we make to RightCurlyCheck should be mirrored in LeftCurlyCheck. This part could be done in a separate issue. I am just mentioning it so we don't forget.

[MEZk]

@rnveach
I do not agree with the proposal for alone option. It will become very difficult to understand.
ALONE should mean that no other tokens except } should be placed on the same line. That's all.

It took me a long time understanding these examples.

if (condition) { } violation for alone before
if (condition) { \n} ok for alone before
if (condition) { \n test(); } violation for alone before
if (condition) { } else { violation for alone after
if (condition) { } \n else { ok for alone after

To my mind, they all should be a violation for alone option.

These are ok as for me. I agree.

same means \n should not come after curly. 'after' is only considered if it has a multi-block statement after it as documented further down.
if (condition) { } ok for same after
if (condition) { } else { ok for same after
if (condition) { } \n else { violation for same after

singleline means \n should not come before the right curly unless it is not on the same line as the left curly. If it is not on the same line, it will behave as alone.

Why? Why it should behave as alone?
Judging by the name it should allow only singleline blocks.

If it is not on the same line, it will behave as alone.

And once again this will lead us to misunderstanding in future.

[MEZk]

@rnveach
On the other hand, if we name the options NOTHING_AFTER, NOTHING_BEFORE, SAME_AS_NEXT your proposal will make scene. However, I cannot get along with the names proposed by you now.

[rnveach]

@MEZk

ALONE should mean that no other tokens except } should be placed on the same line.

That is what it means generally, but remember it applies to the before and after properties. Alone in before means it should be \n}, and alone in after means it should be }\n.
This is the more accurate definition I am going for: ALONE means that no other tokens should be placed before/after the '}' on the same line.

On the other hand, if we name the options NOTHING_AFTER, NOTHING_BEFORE, SAME_AS_NEXT your proposal will make scene.

Since the properties are named before and after, I was trying to avoid using the same text in the option names, but I am open to any suggestions. We cannot maintain 1 property as it prevents users from fully customizing the curly location which is why I split into 2.
We could go with LeftCurlyCheck's naming convention of eol and nl if that makes more sense, but then singleline is left out which is my main reason for sticking to these names.

To my mind, they all should be a violation for alone option.

Please remember there is separate before and after property. Each example specifies if before or after is set to alone for the violation. No examples show violations if both are set to alone.
You are probably thinking of the old way where it was assumed alone meant before and after. This is not clear in the documentation and not the case with current validation, which is the purpose of this issue. Feel free to run example in first post with alone to see what I mean.

singleline
Why? Why it should behave as alone?
And once again this will lead us to misunderstanding in future.

It is to reproduce RightCurlyCheck's current option of alone_or_singleline which acts the same. I am fine to rename singleline to singleline_or_alone to be clearer.
It has to allow more than singleline because we can't request this for all blocks regardless of context. A good example is if statements. No if can be on one long line especially if it has many statements inside it. User should be allowed to singleline it if it is small and simple like if (condition) { return; } and then multi-line it if it is very complex.

[MEZk]

@rnveach

That is what it means generally, but remember it applies to the before and after properties. Alone in before means it should be \n}, and alone in after means it should be }\n.

That is a problem, I guess. You try to suggest adding new options which main purpose is to help to resolve other options problems (SAME, ALONE_OR_SINGLELINE), however, ALONE option does not have these problems at all now. ALONE purpose is clear now and does not need additional workarounds. Could you please give an example what is wrong with ALONE, why do we need to have before and after treatment for it?

Please, understand what I mean. All your examples are good to resolve problems and incorrect behavior of ALONE_OR_SINGLELINE, SAME brace policies, yet, ALONE is not affected by the problems mentioned now.

[rnveach]

ALONE is not affected by the problems mentioned now.

Current documentation on alone does not go into details on if it should be \n} or }\n or \n}\n. Right now documentation is very subjective so saying it "works" can't be demonstrated in a very meaningful way.

Could you please give an example what is wrong with ALONE

If you run the first post's example against alone these are the current false negatives:
if (true) {doSmth();} - no violation. imo, brace isn't alone.
doSmth(); } - no violation. imo, brace isn't alone.

All your examples are good to resolve problems and incorrect behavior of ALONE_OR_SINGLELINE, SAME brace policies

If it fixes problems, then what is the matter with combing alone into the mold? We shouldn't abandon the idea just because alone "works" (note the quotes) in the old way. It is also not clear if alone should apply to before or after the curly. With this method, it is quite clear and can be no mistake even with little examples.

Also, if we don't split the properties into 2 then that means we need to multiply the options for the single property to get the same result.
We would currently need 4 options: ALONE_ALONE, ALONE_SAME, SINGLELINE_ALONE, SINGELINE_SAME. Names woudn't convey their meaning very well and would have to rely on heavy documentation.
This will also be multipled further if we decide we need a new option later on. I avoided adding same to the before property because it made no sense to me right now. If we decided it was needed, we would need 8 options in total if we stuck to 1 property.

[MEZk]

Current documentation on alone does not go into details on if it should be \n} or }\n or \n}\n. Right now documentation is very subjective so saying it "works" can't be demonstrated in a very meaningful way.

Right now the documentations says that

The brace must be alone on the line.

It does not say that right curly must be alone on the line with some statement before or after. It clearly states that no other tokens should be on the line.

If you run the first post's example against alone these are the current false negatives:

I meant that the options behavior is not controversial as the behavior of SAME and ALONE_OR_SINGLELINE.

It is also not clear if alone should apply to before or after the curly.

Why should it be applied to before or after the curly? It should be applied to the the whole line (before and after), isn't it?

OK, lets assume we will have an ability to set before or after for alone. What should the user set for this example?

public class A {
    void foo1() { } // line 2
} // line 3

beforeOption = ?
afterOption = ?

[rnveach]

OK, lets assume we will have an ability to set before or after for alone. What should the user set for this example?

after would apply to neither one as they don't have a next multi-block statement or continuation of the current statement. It would always require nothing (excluding comments as this check isn't comment aware) after the brace. The ideal value to make sense when reading the configuration is alone.
before for CLASS_DEF would be alone. before for METHOD_DEF would be singleline.

[MEZk]

@rnveach
I read our discussion once again and finally agreed on the fact that your proposal makes sense, however, I think that we need to concentrate our discussion on the properties names.

My proposal:
Property name afterBrace , values: nl for new line oreol for end of line.
Property name beforeBrace , values: nl or ?. ? means that I cannot find a good name for the case when there is a single line statement.

From my point of view, these names for the options and their values will be more verbose, but ```?`` still remains and that is the problem with my proposal (

[MEZk]

After private discussion with @rnveach we both agreed that afterBrace and beforeBrace may be considered as a good names for the new options. However, we did not agree on the possible option values.

I suggested the following names:

beforeBrace: NL, NONL;
afterBrace: NL, NONL,

where NL - new line, NONL - no new line.

In my opinion, the existing option values (ALONE, SAME, SINGLELINE) can be replaced with:

ALONE -> beforeBrace = NL, afterBrace = NL;
SAME -> beforeBrace = NL, afterBrace = NONL;
SINGLELINE -> beforeBrace = NONL, afterBrace = NONL.

@rnveach does not support my idea about SINGLELINE replacement with the suggested beforeBrace and afterBrace values:

singleline still needs its own thing as it depends on where the starting { is. if (condition) { \n method(); } isn't single line.
This is why my before/after proposal didn't use the same enumerations for each
I can't imagine anyone using NONL with before without requiring singleline, but we can still leave it in for full customization if we want

@rnveach
Please, correct me, If I missed something.

[romani]

@rnveach , I tried to understand new design several times , spent a lot of time on it and failed.

Alone in before means it should be \n}, and alone in after means it should be }\n.

this blow my mind......

================

Example that you have in description is expected behavior, especially with consideration of #5970 ("Bad samples for rcurly of RighCurly") and #5945 ("New Check: RightCurlyOfExpression").

=============

I propose to change strategy and try one more time from scratch. Even more lets think about it as new Check, not as update for existing and use names that are clear and have nothing in common with any other implementation. Lets imagine that there is no validation for right curly at all and we do all from scratch.
Proposed update is so huge so it is better to create completely new Check and let users migrate who have problems with implementation of RightCurly.

[rnveach]

It is whatever you wish to decide but I don't see a reason to keep old RightCurly when it is so vague and misleading. If conflicts ever occurred, we would either be required to make overrides or suppress one of the checks.

[romani]

RightCurly might be bad and problematic, but it is in use in Google config. Messing it with such big update will be completed for users and us. In new Check we have freedom to avoid all know problems, make it mature by several releases and eventually substitute in Google config. All who like old Check will continue to use it till find it inconvenient and migrate to new Check.

New Check design, as for now, is also far from ideal, so I propose restart discussion from scratch, looks like in last comments there is some light to make it more clear.