Wrong error reporting if class name ends with “Error” #60

Closed
romani opened this Issue Nov 14, 2013 · 8 comments

3 participants

@romani
Member
romani commented Nov 14, 2013

Created: 2008-06-05
Creator: Subhash Chandran
SF issue: 515

http://checkstyle.sourceforge.net/config_design.html#MutableException

I am getting an unexpected checkstyle error in case of class names ending with the word “Error”. The error is “The field must be declared as final”. This error is reported for variables which could not be declared as final. Please see the below class.

package src;

public class SyncError
{
   //Mutable Exception: The field 'message' must be declared final.
   private String message;

}

If I am changing the class name such that it ends with any other string other than “Erorr” then this error is not reported by checkstyle.

Reason: checkstyle does not know anything about Types and their hierarchy. It deduce types by RegExp string to match Class name. Default regexp is "^._Exception$|^._Error$"

this Check will never be correct for that validation. All we could is improve a bit by checking that Class do extends of same name notation class ("an exception type must be a subclass of Throwable") and extend default value with "^.*Throwable$".

@mkordas mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 15, 2014
@mkordas mkordas MutableException check requires class to explicitly extend some other…
… class (#60)
8b32b2a
@mkordas mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 15, 2014
@mkordas mkordas MutableException check requires class to explicitly extend some other…
… class (for issue #60)
335a332
@romani
Member
romani commented Dec 18, 2014
  1. to make this Check useful and minimize amount of false-positives we need user defined properties for "format" and for "extendsClassNameFormat" default value is "^.Exception$|^.Error$|^.Throwable$" for both options.

  2. Please update our xdoc files as we generate base on them our site - http://checkstyle.sourceforge.net/config_design.html#MutableException

@mkordas mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 18, 2014
@mkordas mkordas MutableException check requires class to explicitly extend some other…
… class (for issue #60)
d9d8c9f
@mkordas mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 18, 2014
@mkordas mkordas MutableException check requires class to explicitly extend some other…
… class (for issue #60)
941e921
@mkordas mkordas added a commit to mkordas/checkstyle that referenced this issue Dec 20, 2014
@mkordas mkordas MutableException check requires class to explicitly extend some other…
… class (for issue #60)
f69138c
@romani
Member
romani commented Dec 20, 2014

fixed, will be released in 6.2

@romani romani closed this Dec 20, 2014
@mbykovskyy

Doesn't look like this is fixed. I have the following and version 6.5 still complains about The field 'error' must be declared final.

public class Error
{
    private String error;

    public Error withError(final String error)
    {
        this.error = error;
        return this;
    }
}

Renaming it to something else fixes the problem.

@mkordas
Member
mkordas commented Apr 24, 2015

Please share your configuration for this check.

@mkordas
Member
mkordas commented Apr 24, 2015

So according to http://checkstyle.sourceforge.net/config_design.html#MutableException you use default exception class pattern which is ^.*Exception$|^.*Error$|^.*Throwable$. You can for example change it to ^.*Exception$|^.+Error$|^.*Throwable$, so that just Error won't be enforced to be mutable, but e.g. AssertionError will be.

@mbykovskyy

I think you mean so that just Error won't be enforced to be immutable, but I see what you're saying. My understanding of the format property was wrong.

@mbykovskyy

Thanks @mkordas

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