Issue #6345: RightCurly with option alone false negative for class, method and constructor

[fzdy1914]

Fixes #6345.

When I working with this pr, I found that this may be an intended behavior of checkstyle.
See here, it allows empty body.

If it is not the intended behavior, I will just delete the line that allows empty body.

[rnveach]

If @romani agrees issue is still valid, we will need new tests to show changes.

[rnveach]

@fzdy1914 Please make all CI green and keep number of commits at 1.
Once CI is green I will review.

[fzdy1914]

@rnveach Can you help look into the failing test?

[rnveach]

@fzdy1914
pitest failure is:

RightCurlyCheck.java.html:<td class='covered'><pre><span class='survived'> if (lcurly.getNextSibling().getType() == TokenTypes.RCURLY) {</span></pre></td></tr>

You will have to run ./.ci/pitest.sh pitest-blocks from base checkstyle directory to be able to see report and find where this line is.
If your not familar with pitest, which is mutation test, it is basically modifying your code to look for tests failure. It found that this line was modified and no tests were failing. Something was failing before, so your change some how led to this.

Let me know if you need more help.

[fzdy1914]

Ready for review.

[fzdy1914]

Ready to review.

[rnveach]

@romani please review and review #6505 (comment) .

[fzdy1914]

@romani Can kindly give your review for this PR?

[rnveach]

@fzdy1914 as PR is entering final stages, please provide us a regression report. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#diffgroovy-diff-report-generation .
Please review the report and ensure you see no issues being reported and we will review it too.
I recommend creating a configuration with as many possible property mutations to ensure we don't miss anything. You can assign each instance of a config with an id. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression with an example config.

If you have any questions feel free to ask us.

[romani]

I am ok with code changed.
@fzdy1914 , thanks a lot, and sorry for long delay in reply.
Regression report is required to make sure we did not do extra damage. Please do regression report for other modes of Check to make sure we did not damage another mode (as it was in original issue).

[fzdy1914]

I recommend creating a configuration with as many possible property mutations to ensure we don't miss anything. You can assign each instance of a config with an id. See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression with an example config.

What does property mutation mean? Do you mean that I should write a config to test all possible combinations of tokens and mode to test if there is a regression?

[fzdy1914]

I wrote the following checks and get an error, can you help to examine my file.

<?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"/>

    <!-- do not change severity to 'error', as that will hide errors caused by exceptions -->
    <property name="severity" value="warning"/>

    <!-- haltOnException is required for exception fixes and reporting of all exceptions -->
    <property name="haltOnException" value="false"/>

    <!-- BeforeExecutionFileFilters is required for sources of java9 -->
    <module name="BeforeExecutionExclusionFileFilter">
        <property name="fileNamePattern" value="module\-info\.java$" />
    </module>

    <module name="TreeWalker">
         <!-- Example of sevntu.checkstyle Check usage -->
         <!-- <module name="NestedSwitchCheck"/> -->

         <!-- Example of checkstyle Check usage -->
         <module name="ThrowsCount">
             <property name="max" value="20000000"/>
         </module>

        <module name="RightCurly">
            <property name="option" value="alone"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY,
            LITERAL_IF, LITERAL_ELSE, CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Alone All Tokens" />
        </module>

        <module name="RightCurly">
            <property name="option" value="alone"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE"/>
            <property name="id" value="RCurly Alone Part Tokens 1" />
        </module>

         <module name="RightCurly">
            <property name="option" value="alone"/>
            <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Alone Part Tokens 2" />
        </module>

        <module name="RightCurly">
            <property name="option" value="same"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY,
            LITERAL_IF, LITERAL_ELSE, CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Same All Tokens" />
        </module>

        <module name="RightCurly">
            <property name="option" value="same"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE"/>
            <property name="id" value="RCurly Same Part Tokens 1" />
        </module>

         <module name="RightCurly">
            <property name="option" value="same"/>
            <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Same Part Tokens 2" />
        </module>

        <module name="RightCurly">
            <property name="option" value="alone_or_singleline"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY,
            LITERAL_IF, LITERAL_ELSE, CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Alone or SingleLine All Tokens" />
        </module>

        <module name="RightCurly">
            <property name="option" value="alone_or_singleline"/>
            <property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE"/>
            <property name="id" value="RCurly Alone or SingleLine Part Tokens 1" />
        </module>

         <module name="RightCurly">
            <property name="option" value="alone_or_singleline"/>
            <property name="tokens" value="CLASS_DEF, METHOD_DEF, CTOR_DEF, LITERAL_FOR, LITERAL_WHILE,
            LITERAL_DO, STATIC_INIT, INSTANCE_INIT"/>
            <property name="id" value="RCurly Alone or SingleLine Part Tokens 2" />
        </module>
    </module>
<!--
    <module name="SeverityMatchFilter">
        <property name="severity" value="warning"/>
        <property name="acceptOnMatch" value="false"/>
    </module>
-->
</module>

The comment I use is groovy diff.groovy --localGitRepo D:/checkstyle --baseBranch master --patchBranch 6345 --config rcurly_check.xml --listOfProjects projects-to-test-on.properties

[rnveach]

Do you mean that I should write a config to test all possible combinations of tokens and mode to test if there is a regression?

Combinations of properties. It is ok if after the default check you specify all tokens.

can you help to examine my file.

<module name="ThrowsCount">

You can remove this as it isn't needed. It was just an example.

Property cacheFile does not exist

There is no problem with your config since you don't specify this. It looks to be an issue with maven-site (or something on the maven side).
I will have to look into this more.
Please copy and paste the full stack trace.

[rnveach]

@fzdy1914 Please check that you are using the latest master in contribution. Your output is saying maven-checkstyle-plugin 2.17 and we are using 3.0 . This is the cause of the failure.

Also, as another recommendation, the more modules you put into a config the more memory you will have to give regression. To ensure you don't run into memory issues and have to restarted regression I usually give it 3 gigs of ram or split up the regression into 3 for each option.

[romani]

It is better to have only one Check in config for one report generation, it will be easier to verify it. You can run diff tool few times, have few reports.

[romani]

I reproduced the same problem with cacheFile, fix is merged to contribution repo, please pull from us code to avoid problem.

[fzdy1914]

This is the report generated: link. It seems ok from my observation.

[romani]

@fzdy1914 , there are CI failures in tests, please fix and relaunch regression

[fzdy1914]

Okay, seems my other PR causes some problem with this one.

[fzdy1914]

@romani There is no need to regenerate the report as I only need to add one line of test code to fix this.
The main code is not touched.

[romani]

ok to merge, report is good.
please rebase to latest code, for some reason not all CIs are executed on you PR.

@rnveach , please do review one more time, as bunch of comments pass since your approval.

minor item to fix:

[fzdy1914]

Changed as request

@romani said ok to merge