FinalLocalVariable should not to check multi-catch variables #3617

Closed
Kiena opened this Issue Dec 6, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@Kiena

Kiena commented Dec 6, 2016

/var/tmp $ javac FinalLocalVariableMultiCatch.java
No output.

/var/tmp $ cat FinalLocalVariableMultiCatch.java

import static java.util.Arrays.asList;

public class FinalLocalVariableMultiCatch {

    private class A extends Throwable {}
    private class B extends Throwable {}
    private class C extends Throwable {}

    public void demo() throws Throwable {
        for (Throwable throwable : asList(new A(), new B(), new C())) {
            try {
                throw throwable;
            } catch (A a) {
                System.out.println("Caught A.");
            } catch (B | C bOrC) {
                System.out.println("Caught B or maybe C.");
            }
        }
    }

}

/var/tmp $ cat config.xml

<?xml version="1.0" encoding="UTF-8"?>
<!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"/>
    <property name="fileExtensions" value="java"/>
    <module name="TreeWalker">
        <module name="FinalLocalVariable">
            <property name="tokens" value="PARAMETER_DEF,VARIABLE_DEF"/>
            <property name="validateEnhancedForLoopVariable" value="true"/>
        </module>
    </module>
</module>

/var/tmp $ java -jar checkstyle-7.3-all.jar -c config.xml FinalLocalVariableMultiCatch.java

Starting audit...
[ERROR] P:\java\workspaces\test\flvmc\FinalLocalVariableMultiCatch.java:10:24: Variable 'throwable' should be declared final. [FinalLocalVariable]
[ERROR] P:\java\workspaces\test\flvmc\FinalLocalVariableMultiCatch.java:13:24: Variable 'a' should be declared final. [FinalLocalVariable]
[ERROR] P:\java\workspaces\test\flvmc\FinalLocalVariableMultiCatch.java:15:28: Variable 'bOrC' should be declared final. [FinalLocalVariable]
Audit done.
Checkstyle ends with 3 errors.

If the validateEnhancedForLoopVariable property of the FinalLocalVariable module is set to false (which is the default), then the first error (line 10) is not reported. It is reasonable because the enhanced for-loop variable is implicitly final.

I'd like a similar Boolean property, possibly called validateMultiCatchVariable, with also a default value of false, which would avoid the third error (line 15) being reported. The reasoning for this is that multi-catch variables are implicitly final, and having to explicitly declare that in order to use the module is redundant, as well as space-consuming.
From specification: An exception parameter of a multi-catch clause is implicitly declared final if it is not explicitly declared final.

The second error (line 13) must still be reported regardless of the value of the new property, because that variable is not implicitly final.


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 6, 2016

Member

I'd like a similar Boolean property, possibly called validateMultiCatchVariable

if multi-catch are explicitly final - we should just skip validation of them, without any new option.
Are you ok ?

The second error (line 13) must still be reported regardless of the value of the new property, because that variable is not implicitly final.

Just for details: This is due to PARAMETER_DEF , it is not very recommended mode (non default mode), but we should support it.

Member

romani commented Dec 6, 2016

I'd like a similar Boolean property, possibly called validateMultiCatchVariable

if multi-catch are explicitly final - we should just skip validation of them, without any new option.
Are you ok ?

The second error (line 13) must still be reported regardless of the value of the new property, because that variable is not implicitly final.

Just for details: This is due to PARAMETER_DEF , it is not very recommended mode (non default mode), but we should support it.

@romani romani added the approved label Dec 6, 2016

@Kiena

This comment has been minimized.

Show comment
Hide comment
@Kiena

Kiena Dec 7, 2016

if multi-catch are explicitly final - we should just skip validation of them, without any new option.
Are you ok ?

Implicitly, which they are, according to the JLS (linked in the original request [1]) and experience as well. I'd be using the new option, so skipping them by default would certainly work for me, too.

Just for details: This is due to PARAMETER_DEF , it is not very recommended mode (non default mode), but we should support it.

That's right, I should've properly referred to it as parameter, and the suggested property name should've been validateMultiCatchParameter. Since it makes no difference to the users of an API, I find adding final to – or removing it from – parameters no more effort than in variables' case, while it makes sure they are re-assigned only intentionally (and quite rarely). For this reason I'm very glad PARAMETER_DEF can already be used.

[1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.20

Kiena commented Dec 7, 2016

if multi-catch are explicitly final - we should just skip validation of them, without any new option.
Are you ok ?

Implicitly, which they are, according to the JLS (linked in the original request [1]) and experience as well. I'd be using the new option, so skipping them by default would certainly work for me, too.

Just for details: This is due to PARAMETER_DEF , it is not very recommended mode (non default mode), but we should support it.

That's right, I should've properly referred to it as parameter, and the suggested property name should've been validateMultiCatchParameter. Since it makes no difference to the users of an API, I find adding final to – or removing it from – parameters no more effort than in variables' case, while it makes sure they are re-assigned only intentionally (and quite rarely). For this reason I'm very glad PARAMETER_DEF can already be used.

[1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.20

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 7, 2016

Member

ok.No new option is required.
If you need that functionality quicker , be welcome to implement it. I could guide you.

For this reason I'm very glad PARAMETER_DEF can already be used

Just fyi - We use http://checkstyle.sourceforge.net/config_coding.html#ParameterAssignment to keep code under control and keep code less verbose.

Member

romani commented Dec 7, 2016

ok.No new option is required.
If you need that functionality quicker , be welcome to implement it. I could guide you.

For this reason I'm very glad PARAMETER_DEF can already be used

Just fyi - We use http://checkstyle.sourceforge.net/config_coding.html#ParameterAssignment to keep code under control and keep code less verbose.

liscju added a commit to liscju/checkstyle that referenced this issue Dec 7, 2016

romani added a commit that referenced this issue Dec 7, 2016

@romani romani added this to the 7.4 milestone Dec 7, 2016

@romani romani added the bug label Dec 7, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 7, 2016

Member

fix is merged

Member

romani commented Dec 7, 2016

fix is merged

@romani romani closed this Dec 7, 2016

@romani romani changed the title from Option for FinalLocalVariable not to check multi-catch variables to FinalLocalVariable should not to check multi-catch variables Dec 31, 2016

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