Take "break" into consideration in FinalLocalVariable #4082

Closed
Luolc opened this Issue Mar 22, 2017 · 2 comments

Comments

Projects
None yet
3 participants
@Luolc
Contributor

Luolc commented Mar 22, 2017

Taken from #4060 (comment)

There is a regression after the fix in #4060 :
http://www.luolc.com/checkstyle-diff-report/issue3172/openjdk8/xref/Users/LuoLiangchen/personal/develop/java/checkstyle/contribution/checkstyle-tester/repositories/openjdk8/src/share/classes/com/sun/jndi/ldap/LdapCtx.java.html#L2901

The reason is that we didn't take break or return into consideration at all. There are even no LITERAL_RETURN or LITERAL_BREAK in the acceptable tokens set of the check.

Example:

$ javac Sample.java
$ cat Sample.java
public class Sample {
    public static void main(String args[]) throws Exception {
        Exception e; // Doesn't warn - incorrect
        final int a = (int) Math.random();
        final int b = (int) Math.random();

        switch (a) {
        case 0:
            e = new Exception();
            break;
        case 1:
            if (b == 0) {
                e = new Exception(); // (0)
                break; // (1)
            }

            if (b == 1) {
                e = new Exception(); // (2)
            }
            else {
                e = new Exception();
            }
            break;
        case 2:
            if (b == 0) {
                return;
            }

            e = new Exception();
            break;
        default:
            e = new Exception();
            break;
        }

        throw e;
    }
}
$ cat config.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">
    <module name="TreeWalker">
        <module name="FinalLocalVariable">
        </module>
    </module>
</module>
$ java -jar checkstyle-7.7-SNAPSHOT-all.jar -c config.xml Sample.java
Starting audit...
Audit done.

Expected: violation on 3th line.

The check just ignores the break at (1) and since there are two assignments of e at (0) and (2), the violation won't be raised. That's not correct.

@Luolc Luolc changed the title from Take `return` and `break` into consideration in FinalLocalVariable to Take "return" and "break" into consideration in FinalLocalVariable Mar 28, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 28, 2017

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 28, 2017

Contributor

I am on it. :)

Contributor

Luolc commented Mar 28, 2017

I am on it. :)

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 28, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 28, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 28, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 30, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 30, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 9, 2017

@Luolc Luolc changed the title from Take "return" and "break" into consideration in FinalLocalVariable to Take "break" into consideration in FinalLocalVariable Apr 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 13, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 16, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 16, 2017

Luolc added a commit to Luolc/checkstyle that referenced this issue Apr 16, 2017

rnveach added a commit that referenced this issue Apr 18, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 18, 2017

Member

Fix is merged

Member

rnveach commented Apr 18, 2017

Fix is merged

@rnveach rnveach closed this Apr 18, 2017

@rnveach rnveach added the bug label Apr 18, 2017

@rnveach rnveach added this to the 7.7 milestone Apr 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment