From 5245c14883aa9ddf9930804c09c13df1f34e70bc Mon Sep 17 00:00:00 2001 From: Oleg Sukhodolsky Date: Tue, 26 Aug 2003 20:06:58 +0000 Subject: [PATCH] Added MutableException check (request 750750). This is a very simple algorithm, but, I believe, it catches most of real-life problems. --- docs/config_design.html | 71 ++++++++- docs/releasenotes.html | 2 + .../checks/design/MutableExceptionCheck.java | 135 ++++++++++++++++++ .../checks/design/messages.properties | 1 + .../design/InputMutableException.java | 25 ++++ .../puppycrawl/tools/checkstyle/AllTests.java | 2 + .../design/MutableExceptionCheckTest.java | 19 +++ 7 files changed, 250 insertions(+), 5 deletions(-) create mode 100644 src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheck.java create mode 100644 src/testinputs/com/puppycrawl/tools/checkstyle/design/InputMutableException.java create mode 100644 src/tests/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheckTest.java diff --git a/docs/config_design.html b/docs/config_design.html index 6cc077bc0..ab48ac427 100644 --- a/docs/config_design.html +++ b/docs/config_design.html @@ -23,19 +23,22 @@ @@ -284,6 +287,64 @@ public class StringUtils // not final to allow subclassing TreeWalker

+

MutableException

Description

+

+ Ensures that exceptions (defined as any class name conforming + to some regular expression) are immutable. That is, have only + final fields. +

+ +

+ The current algorithm is very simple it checks that all + members of exception are final. User can still mutates an + exception's instance (e.g. Throwable has + setStackTrace(StackTraceElement[] stackTrace) method which + changes stack trace). But, at least, all information provided + by this exception type is unchangable. +

+ +

+ Rationale: + Exception instances should represent an error + condition. Having non final fields not only allows the + state to be modified by accident and therefore mask the + original condition but also allows developers to + accidentally forget to initialise state thereby leading + to code catching the exception to draw incorrect + conclusions based on the state. +

+ +

Properties

+ + + + + + + + + + + + + +
namedescriptiontypedefault value
formatpattern for name of exception class.regular expression^.*Exception$|^.*Error$
+ +

Examples

+

+ To configure the check: +

+ 
+<module name="MutableException"/>
+
+

Package

+

+ com.puppycrawl.tools.checkstyle.checks.design +

+

Parent Module

+

+ TreeWalker +

diff --git a/docs/releasenotes.html b/docs/releasenotes.html index a172752ec..34a797708 100644 --- a/docs/releasenotes.html +++ b/docs/releasenotes.html @@ -147,6 +147,8 @@
  • Added JUnitTestCase check. (request 750761).
    • +
  • Added MutableException check (request 750750).
    • +

    diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheck.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheck.java new file mode 100644 index 000000000..d8b7b7014 --- /dev/null +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheck.java @@ -0,0 +1,135 @@ +//////////////////////////////////////////////////////////////////////////////// +// checkstyle: Checks Java source code for adherence to a set of rules. +// Copyright (C) 2001-2003 Oliver Burn +// +// This library is free software; you can redistribute it and/or +// modify it under the terms of the GNU Lesser General Public +// License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public +// License along with this library; if not, write to the Free Software +// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +//////////////////////////////////////////////////////////////////////////////// +package com.puppycrawl.tools.checkstyle.checks.design; + +import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import com.puppycrawl.tools.checkstyle.checks.AbstractFormatCheck; + +import java.util.Stack; + +/** + *

    Ensures that exceptions (defined as any class name conforming + * to some regular expression) are immutable. That is, have only final + * fields.

    + *

    Rationale: Exception instances should represent an error + * condition. Having non final fields not only allows the state to be + * modified by accident and therefore mask the original condition but + * also allows developers to accidentally forget to initialise state + * thereby leading to code catching the exception to draw incorrect + * conclusions based on the state.

    + * + * @author Simon Harris + */ +public final class MutableExceptionCheck extends AbstractFormatCheck +{ + /** Default value for format property. */ + private static final String DEFAULT_FORMAT = "^.*Exception$|^.*Error$"; + /** Stack of checking information for classes. */ + private final Stack mCheckingStack = new Stack(); + /** Should we check current class or not. */ + private boolean mChecking; + + /** Creates new instance of the check. */ + public MutableExceptionCheck() + { + super(DEFAULT_FORMAT); + } + + /** @see com.puppycrawl.tools.checkstyle.api.Check */ + public int[] getDefaultTokens() + { + return new int[] {TokenTypes.CLASS_DEF, TokenTypes.VARIABLE_DEF}; + } + + /** @see com.puppycrawl.tools.checkstyle.api.Check */ + public int[] getRequiredTokens() + { + return getDefaultTokens(); + } + + /** @see com.puppycrawl.tools.checkstyle.api.Check */ + public void visitToken(DetailAST aAST) + { + switch (aAST.getType()) { + case TokenTypes.CLASS_DEF: + visitClassDef(aAST); + break; + case TokenTypes.VARIABLE_DEF: + visitVariableDef(aAST); + break; + default: + throw new IllegalStateException(aAST.toString()); + } + } + + /** @see com.puppycrawl.tools.checkstyle.api.Check */ + public void leaveToken(DetailAST aAST) + { + switch (aAST.getType()) { + case TokenTypes.CLASS_DEF: + leaveClassDef(); + break; + default: + // Do nothing + } + } + + /** + * Called when we start processing class definition. + * @param aAST class definition node + */ + private void visitClassDef(DetailAST aAST) + { + mCheckingStack.push(Boolean.valueOf(mChecking)); + mChecking = + isExceptionClass(aAST.findFirstToken(TokenTypes.IDENT).getText()); + } + + /** Called when we leave class definition. */ + private void leaveClassDef() + { + mChecking = ((Boolean) mCheckingStack.pop()).booleanValue(); + } + + /** + * Checks variable definition. + * @param aAST variable def node for check. + */ + private void visitVariableDef(DetailAST aAST) + { + if (mChecking && aAST.getParent().getType() == TokenTypes.OBJBLOCK) { + DetailAST modifiersAST = aAST.findFirstToken(TokenTypes.MODIFIERS); + + if (!(modifiersAST.findFirstToken(TokenTypes.FINAL) != null)) { + log(aAST.getLineNo(), aAST.getColumnNo(), "mutable.exception", + aAST.findFirstToken(TokenTypes.IDENT).getText()); + } + } + } + + /** + * @param aClassName class name to check + * @return true if a given class name confirms specified format + */ + private boolean isExceptionClass(String aClassName) + { + return getRegexp().match(aClassName); + } +} diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/messages.properties b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/messages.properties index e5b7a211d..57c5090b5 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/messages.properties +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/design/messages.properties @@ -2,3 +2,4 @@ design.forExtension=Method ''{0}'' is not designed for extension - needs to be a final.class=Class {0} should be declared as final. interface.type=interfaces should describe a type and hence have methods. variable.notPrivate=Variable ''{0}'' must be private and have accessor methods. +mutable.exception=The field ''{0}'' must be declared final. diff --git a/src/testinputs/com/puppycrawl/tools/checkstyle/design/InputMutableException.java b/src/testinputs/com/puppycrawl/tools/checkstyle/design/InputMutableException.java new file mode 100644 index 000000000..d2a85ebcb --- /dev/null +++ b/src/testinputs/com/puppycrawl/tools/checkstyle/design/InputMutableException.java @@ -0,0 +1,25 @@ +package com.puppycrawl.tools.checkstyle.checks.design; + +public class InputMutableException { + public class FooException { + private final int _finalErrorCode; + private int _errorCode = 1; + + public FooException() { + _finalErrorCode = 1; + } + + public class FooExceptionThisIsNot { + private final int _finalErrorCode; + private int _errorCode = 1; + + public FooExceptionThisIsNot() { + _finalErrorCode = 1; + } + } + } + + public class FooError { + private int _errorCode; + } +} diff --git a/src/tests/com/puppycrawl/tools/checkstyle/AllTests.java b/src/tests/com/puppycrawl/tools/checkstyle/AllTests.java index b1afd2f62..32d25329a 100644 --- a/src/tests/com/puppycrawl/tools/checkstyle/AllTests.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/AllTests.java @@ -48,6 +48,7 @@ import com.puppycrawl.tools.checkstyle.checks.design.DesignForExtensionCheckTest import com.puppycrawl.tools.checkstyle.checks.design.FinalClassCheckTest; import com.puppycrawl.tools.checkstyle.checks.design.HideUtilityClassConstructorCheckTest; import com.puppycrawl.tools.checkstyle.checks.design.InterfaceIsTypeCheckTest; +import com.puppycrawl.tools.checkstyle.checks.design.MutableExceptionCheckTest; import com.puppycrawl.tools.checkstyle.checks.design.VisibilityModifierCheckTest; import com.puppycrawl.tools.checkstyle.checks.imports.AvoidStarImportTest; import com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheckTest; @@ -164,6 +165,7 @@ public class AllTests { suite.addTest(new TestSuite(MethodLengthCheckTest.class)); suite.addTest(new TestSuite(MethodNameCheckTest.class)); suite.addTest(new TestSuite(ModifierOrderCheckTest.class)); + suite.addTest(new TestSuite(MutableExceptionCheckTest.class)); suite.addTest(new TestSuite(NeedBracesCheckTest.class)); suite.addTest(new TestSuite(NestedIfDepthCheckTest.class)); suite.addTest(new TestSuite(NestedTryDepthCheckTest.class)); diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheckTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheckTest.java new file mode 100644 index 000000000..98b24c7ab --- /dev/null +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheckTest.java @@ -0,0 +1,19 @@ +package com.puppycrawl.tools.checkstyle.checks.design; + +import com.puppycrawl.tools.checkstyle.BaseCheckTestCase; +import com.puppycrawl.tools.checkstyle.DefaultConfiguration; + +import java.io.File; + +public class MutableExceptionCheckTest extends BaseCheckTestCase { + public void test() throws Exception { + DefaultConfiguration checkConfig = createCheckConfig(MutableExceptionCheck.class); + + String[] expected = { + "6:9: The field '_errorCode' must be declared final.", + "23:9: The field '_errorCode' must be declared final.", + }; + + verify(checkConfig, getPath("design" + File.separator + "InputMutableException.java"), expected); + } +}