From 4018efa9fefa66e599369ace1cb189b02ae64fa7 Mon Sep 17 00:00:00 2001 From: Oliver Burn Date: Wed, 5 Jun 2002 13:17:20 +0000 Subject: [PATCH] Refactored the catch block checking code to include try/catch/finally blocks. --- docs/anttask.html | 12 +- docs/cmdline.html | 12 +- docs/engine.html | 20 +-- docs/releasenotes.html | 2 + ...CatchBlockOption.java => BlockOption.java} | 31 +++-- .../tools/checkstyle/CheckStyleTask.java | 30 ++++- .../tools/checkstyle/Configuration.java | 59 +++++++-- .../com/puppycrawl/tools/checkstyle/Defn.java | 6 +- .../puppycrawl/tools/checkstyle/Verifier.java | 124 +++++++++++------- .../com/puppycrawl/tools/checkstyle/java.g | 10 +- .../tools/checkstyle/CheckerTest.java | 23 +++- .../tools/checkstyle/InputSemantic.java | 19 ++- 12 files changed, 247 insertions(+), 101 deletions(-) rename src/checkstyle/com/puppycrawl/tools/checkstyle/{CatchBlockOption.java => BlockOption.java} (68%) diff --git a/docs/anttask.html b/docs/anttask.html index cc27bbea5..9e95ab66b 100644 --- a/docs/anttask.html +++ b/docs/anttask.html @@ -74,9 +74,19 @@ This task is included in the checkstyle distribution.

Specifies the policy of how to pad parenthesises. The legal values are defined here. Defaults to "nospace". No + + tryBlock + Specifies the policy of how check try blocks. The legal values are defined here. Defaults to "stmt". + No + catchBlock - Specifies the policy of how check catch blocks. The legal values are defined here. Defaults to "text". + Specifies the policy of how check catch blocks. The legal values are defined here. Defaults to "text". + No + + + finallyBlock + Specifies the policy of how check finally blocks. The legal values are defined here. Defaults to "stmt". No diff --git a/docs/cmdline.html b/docs/cmdline.html index dc03dbac4..4b755e32a 100644 --- a/docs/cmdline.html +++ b/docs/cmdline.html @@ -69,8 +69,16 @@ This command line tool is included in the checkstyle distribution.

Specifies the policy of how to pad parenthesises. The legal values are defined here. Defaults to "nospace". - checkstyle.catchblock - Specifies the policy of how check catch blocks. The legal values are defined here. Defaults to "text". + checkstyle.block.try + Specifies the policy of how check try blocks. The legal values are defined here. Defaults to "stmt". + + + checkstyle.block.catch + Specifies the policy of how check catch blocks. The legal values are defined here. Defaults to "text". + + + checkstyle.block.finally + Specifies the policy of how check finally blocks. The legal values are defined here. Defaults to "text". checkstyle.allow.protected diff --git a/docs/engine.html b/docs/engine.html index c6e928f12..dfa1f605d 100644 --- a/docs/engine.html +++ b/docs/engine.html @@ -9,7 +9,7 @@

Checkstyle Core Engine - Version @CHECKSTYLE_VERSION@

-

Author: Oliver Burn

+

Authors: Oliver Burn, Lars Kühne

Introduction

This document describes the core engine of Checkstyle, including what checks it performs and what can be configured. The engine can be executed via different interfaces. The standard distribution includes support for: @@ -378,22 +378,22 @@ line 5: /{71}

Lines 1 and 5 demonstrate a more compact notation for 71 '/' characters. Line 3 enforces that the copyright notice includes a four digit year. Line 4 is an example how to enforce revision control keywords in a file header.

-

Exception handling

-

Checks for empty catch blocks. The check can be configured with the following options:

+

Empty Blocks

+

Checks for empty try, catch, finally blocks. The checks can be configured with the following options:

- - + +
- + - - diff --git a/docs/releasenotes.html b/docs/releasenotes.html index 5bdc6c8a7..729b4c0bc 100644 --- a/docs/releasenotes.html +++ b/docs/releasenotes.html @@ -43,7 +43,9 @@ diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/CatchBlockOption.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/BlockOption.java similarity index 68% rename from src/checkstyle/com/puppycrawl/tools/checkstyle/CatchBlockOption.java rename to src/checkstyle/com/puppycrawl/tools/checkstyle/BlockOption.java index e6170826d..f11b59a99 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/CatchBlockOption.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/BlockOption.java @@ -24,50 +24,49 @@ import java.util.Map; import java.util.HashMap; /** - * Represents the options for a catch block. + * Represents the options for a block. * * @author Oliver Burn */ -public final class CatchBlockOption implements Serializable +public final class BlockOption implements Serializable { /** maps from a string representation to an option **/ private static final Map STR_TO_OPT = new HashMap(); - /** represents ignoring catch blocks **/ - public static final CatchBlockOption IGNORE = - new CatchBlockOption("ignore"); - /** represents requiring some text in the catch block **/ - public static final CatchBlockOption TEXT = new CatchBlockOption("text"); - /** represents requiring a statement in the catch block **/ - public static final CatchBlockOption STMT = new CatchBlockOption("stmt"); + /** represents ignoring blocks **/ + public static final BlockOption IGNORE = new BlockOption("ignore"); + /** represents requiring some text in the block **/ + public static final BlockOption TEXT = new BlockOption("text"); + /** represents requiring a statement in the block **/ + public static final BlockOption STMT = new BlockOption("stmt"); /** the string representation of the option **/ private final String mStrRep; /** - * Creates a new CatchBlockOption instance. + * Creates a new BlockOption instance. * @param aStrRep the string representation */ - private CatchBlockOption(String aStrRep) + private BlockOption(String aStrRep) { mStrRep = aStrRep.trim().toLowerCase(); STR_TO_OPT.put(mStrRep, this); } /** - * Returns the CatchBlockOption specified by a string representation. If no + * Returns the BlockOption specified by a string representation. If no * option exists then null is returned. * @param aStrRep the String representation to parse - * @return the CatchBlockOption value represented by aStrRep, + * @return the BlockOption value represented by aStrRep, * or null if none exists. */ - public static CatchBlockOption decode(String aStrRep) + public static BlockOption decode(String aStrRep) { - return (CatchBlockOption) STR_TO_OPT.get(aStrRep.trim().toLowerCase()); + return (BlockOption) STR_TO_OPT.get(aStrRep.trim().toLowerCase()); } /** - * Ensures that we don't get multiple instances of one CatchBlockOption + * Ensures that we don't get multiple instances of one BlockOption * during deserialization. See Section 3.6 of the Java Object * Serialization Specification for details. * diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/CheckStyleTask.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/CheckStyleTask.java index 0813dcef2..9b709b0be 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/CheckStyleTask.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/CheckStyleTask.java @@ -669,6 +669,18 @@ public class CheckStyleTask }); } + /** @param aTo the try block option **/ + public void setTryBlock(final String aTo) + { + mOptionMemory.add(new Runnable() + { + public void run() + { + mConfig.setTryBlock(extractBlockOption(aTo)); + } + }); + } + /** @param aTo the catch block option **/ public void setCatchBlock(final String aTo) { @@ -676,7 +688,19 @@ public class CheckStyleTask { public void run() { - mConfig.setCatchBlock(extractCatchBlockOption(aTo)); + mConfig.setCatchBlock(extractBlockOption(aTo)); + } + }); + } + + /** @param aTo the finally block option **/ + public void setFinallyBlock(final String aTo) + { + mOptionMemory.add(new Runnable() + { + public void run() + { + mConfig.setFinallyBlock(extractBlockOption(aTo)); } }); } @@ -945,10 +969,10 @@ public class CheckStyleTask * @return the CatchBlockOption represented by aFrom * @throws BuildException if unable to decode aFrom */ - private CatchBlockOption extractCatchBlockOption(String aFrom) + private BlockOption extractBlockOption(String aFrom) throws BuildException { - final CatchBlockOption opt = CatchBlockOption.decode(aFrom); + final BlockOption opt = BlockOption.decode(aFrom); if (opt == null) { throw new BuildException("Unable to parse '" + aFrom + "'.", location); diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/Configuration.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/Configuration.java index ba3a2f6cf..43f80853d 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/Configuration.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/Configuration.java @@ -192,8 +192,12 @@ public class Configuration private LeftCurlyOption mLCurlyOther = LeftCurlyOption.EOL; /** where to place right curlies **/ private RightCurlyOption mRCurly = RightCurlyOption.SAME; + /** how to process try blocks **/ + private BlockOption mTryBlock = BlockOption.STMT; /** how to process catch blocks **/ - private CatchBlockOption mCatchBlock = CatchBlockOption.TEXT; + private BlockOption mCatchBlock = BlockOption.TEXT; + /** how to process finally blocks **/ + private BlockOption mFinallyBlock = BlockOption.STMT; /** how to pad parenthesis **/ private PadOption mParenPadOption = PadOption.NOSPACE; @@ -295,9 +299,15 @@ public class Configuration LeftCurlyOption.EOL, aLog)); setRCurly(getRightCurlyOptionProperty( aProps, RCURLY_PROP, RightCurlyOption.SAME, aLog)); + setTryBlock( + getBlockOptionProperty( + aProps, TRY_BLOCK_PROP, BlockOption.TEXT, aLog)); setCatchBlock( - getCatchBlockOptionProperty( - aProps, CATCH_BLOCK_PROP, CatchBlockOption.TEXT, aLog)); + getBlockOptionProperty( + aProps, CATCH_BLOCK_PROP, BlockOption.TEXT, aLog)); + setFinallyBlock( + getBlockOptionProperty( + aProps, FINALLY_BLOCK_PROP, BlockOption.TEXT, aLog)); setParenPadOption(getPadOptionProperty(aProps, PAREN_PAD_PROP, PadOption.NOSPACE, @@ -1021,18 +1031,42 @@ public class Configuration mRCurly = aTo; } + /** @return the try block option **/ + public BlockOption getTryBlock() + { + return mTryBlock; + } + + /** @param aTo set the try block option **/ + public void setTryBlock(BlockOption aTo) + { + mTryBlock = aTo; + } + /** @return the catch block option **/ - public CatchBlockOption getCatchBlock() + public BlockOption getCatchBlock() { return mCatchBlock; } /** @param aTo set the catch block option **/ - public void setCatchBlock(CatchBlockOption aTo) + public void setCatchBlock(BlockOption aTo) { mCatchBlock = aTo; } + /** @return the finally block option **/ + public BlockOption getFinallyBlock() + { + return mFinallyBlock; + } + + /** @param aTo set the finally block option **/ + public void setFinallyBlock(BlockOption aTo) + { + mFinallyBlock = aTo; + } + /** @return the parenthesis padding option **/ public PadOption getParenPadOption() { @@ -1159,19 +1193,18 @@ public class Configuration * @param aName the name of the property to parse * @param aDefault the default value to use. * - * @return the value of a CatchBlockOption property. If the property is not + * @return the value of a BlockOption property. If the property is not * defined or cannot be decoded, then a default value is returned. */ - private static CatchBlockOption getCatchBlockOptionProperty( - Properties aProps, - String aName, - CatchBlockOption aDefault, - PrintStream aLog) + private static BlockOption getBlockOptionProperty(Properties aProps, + String aName, + BlockOption aDefault, + PrintStream aLog) { - CatchBlockOption retVal = aDefault; + BlockOption retVal = aDefault; final String strRep = aProps.getProperty(aName); if (strRep != null) { - retVal = CatchBlockOption.decode(strRep); + retVal = BlockOption.decode(strRep); if (retVal == null) { aLog.println("Unable to parse " + aName + " property with value " + strRep diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/Defn.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/Defn.java index 00c970ce7..b0178736b 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/Defn.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/Defn.java @@ -105,6 +105,10 @@ public interface Defn /** property name for padding around parenthesis **/ String PAREN_PAD_PROP = "checkstyle.paren.pad"; + /** property name for try block options **/ + String TRY_BLOCK_PROP = "checkstyle.block.try"; /** property name for catch block options **/ - String CATCH_BLOCK_PROP = "checkstyle.catchblock"; + String CATCH_BLOCK_PROP = "checkstyle.block.catch"; + /** property name for finally block options **/ + String FINALLY_BLOCK_PROP = "checkstyle.block.finally"; } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/Verifier.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/Verifier.java index 74f292b17..160c8e84c 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/Verifier.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/Verifier.java @@ -869,6 +869,16 @@ class Verifier mMethodBlockLevel--; } + /** + * Report that the parser has found a try block. + * @param aBraces the start and end braces from the try block + * @param aNoStmt whether there are any statements in the block + */ + void reportTryBlock(MyCommonAST[] aBraces, boolean aNoStmt) + { + checkBlock("try", mConfig.getTryBlock(), aBraces, aNoStmt); + } + /** * Report that the parser has found a catch block. * @param aBraces the start and end braces from the catch block @@ -876,53 +886,17 @@ class Verifier */ void reportCatchBlock(MyCommonAST[] aBraces, boolean aNoStmt) { - if (aNoStmt && (mConfig.getCatchBlock() == CatchBlockOption.STMT)) { - log(aBraces[0].getLineNo(), - aBraces[0].getColumnNo(), - "Must have at least one statement."); - } - else if (mConfig.getCatchBlock() == CatchBlockOption.TEXT) { - if (aBraces[0].getLineNo() == aBraces[1].getLineNo()) { - // Handle braces on the same line - final String txt = mLines[aBraces[0].getLineNo() - 1] - .substring(aBraces[0].getColumnNo() + 1, - aBraces[1].getColumnNo()); - if (txt.trim().length() == 0) { - log(aBraces[0].getLineNo(), - aBraces[0].getColumnNo(), - "Empty catch block."); - } - } - else { - // check only whitespace of first & last lines - if ((mLines[aBraces[0].getLineNo() - 1] - .substring(aBraces[0].getColumnNo() + 1).trim().length() - == 0) - && (mLines[aBraces[1].getLineNo() - 1] - .substring(0, aBraces[1].getColumnNo()).trim().length() - == 0)) - { + checkBlock("catch", mConfig.getCatchBlock(), aBraces, aNoStmt); + } - // Need to check if all lines are also only whitespace - boolean isBlank = true; - for (int i = aBraces[0].getLineNo(); - i < (aBraces[1].getLineNo() - 1); - i++) - { - if (mLines[i].trim().length() > 0) { - isBlank = false; - break; - } - } - - if (isBlank) { - log(aBraces[0].getLineNo(), - aBraces[0].getColumnNo(), - "Empty catch block."); - } - } - } - } + /** + * Report that the parser has found a finally block. + * @param aBraces the start and end braces from the finally block + * @param aNoStmt whether there are any statements in the block + */ + void reportFinallyBlock(MyCommonAST[] aBraces, boolean aNoStmt) + { + checkBlock("finally", mConfig.getFinallyBlock(), aBraces, aNoStmt); } /** @@ -1636,5 +1610,63 @@ class Verifier } } + /** + * Check that a block conforms to the specified rule. + * @param aType the type of block to be used in error messages + * @param aOption the option to check the block against + * @param aBraces the start and end braces from the block + * @param aNoStmt whether there are any statements in the block + */ + void checkBlock(String aType, BlockOption aOption, + MyCommonAST[] aBraces, boolean aNoStmt) + { + if (aNoStmt && (aOption == BlockOption.STMT)) { + log(aBraces[0].getLineNo(), + aBraces[0].getColumnNo(), + "Must have at least one statement."); + } + else if (aOption == BlockOption.TEXT) { + if (aBraces[0].getLineNo() == aBraces[1].getLineNo()) { + // Handle braces on the same line + final String txt = mLines[aBraces[0].getLineNo() - 1] + .substring(aBraces[0].getColumnNo() + 1, + aBraces[1].getColumnNo()); + if (txt.trim().length() == 0) { + log(aBraces[0].getLineNo(), + aBraces[0].getColumnNo(), + "Empty " + aType + " block."); + } + } + else { + // check only whitespace of first & last lines + if ((mLines[aBraces[0].getLineNo() - 1] + .substring(aBraces[0].getColumnNo() + 1).trim().length() + == 0) + && (mLines[aBraces[1].getLineNo() - 1] + .substring(0, aBraces[1].getColumnNo()).trim().length() + == 0)) + { + + // Need to check if all lines are also only whitespace + boolean isBlank = true; + for (int i = aBraces[0].getLineNo(); + i < (aBraces[1].getLineNo() - 1); + i++) + { + if (mLines[i].trim().length() > 0) { + isBlank = false; + break; + } + } + + if (isBlank) { + log(aBraces[0].getLineNo(), + aBraces[0].getColumnNo(), + "Empty " + aType + " block."); + } + } + } + } + } // }}} } diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/java.g b/src/checkstyle/com/puppycrawl/tools/checkstyle/java.g index fe585807d..6ebe01476 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/java.g +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/java.g @@ -742,9 +742,10 @@ tryBlock final MethodSignature ignoreMS = new MethodSignature(); final boolean[] isEmpty = new boolean[1]; } - : t:"try"^ compoundStatement[stmtBraces, sIgnoreIsEmpty] + : t:"try"^ compoundStatement[stmtBraces, isEmpty] { ver.verifyWSAroundBegin(t.getLine(), t.getColumn(), t.getText()); + ver.reportTryBlock(stmtBraces, isEmpty[0]); ver.verifyLCurlyOther(t.getLine(), stmtBraces[0]); } @@ -761,8 +762,11 @@ tryBlock ( f:"finally"^ { ver.verifyRCurly(stmtBraces[1], f.getLine()); } - compoundStatement[stmtBraces, sIgnoreIsEmpty] - { ver.verifyLCurlyOther(f.getLine(), stmtBraces[0]); } + compoundStatement[stmtBraces, isEmpty] + { + ver.verifyLCurlyOther(f.getLine(), stmtBraces[0]); + ver.reportFinallyBlock(stmtBraces, isEmpty[0]); + } )? ; diff --git a/src/tests/com/puppycrawl/tools/checkstyle/CheckerTest.java b/src/tests/com/puppycrawl/tools/checkstyle/CheckerTest.java index 17441f36f..40cfeff0a 100644 --- a/src/tests/com/puppycrawl/tools/checkstyle/CheckerTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/CheckerTest.java @@ -87,7 +87,8 @@ public class CheckerTest { mConfig.setIgnoreCastWhitespace(false); mConfig.setParenPadOption(PadOption.NOSPACE); - mConfig.setCatchBlock(CatchBlockOption.IGNORE); + mConfig.setTryBlock(BlockOption.IGNORE); + mConfig.setCatchBlock(BlockOption.IGNORE); final Checker c = createChecker(); final String filepath = getPath("InputWhitespace.java"); assertNotNull(c); @@ -159,7 +160,8 @@ public class CheckerTest { mConfig.setIgnoreCastWhitespace(true); mConfig.setParenPadOption(PadOption.IGNORE); - mConfig.setCatchBlock(CatchBlockOption.IGNORE); + mConfig.setTryBlock(BlockOption.IGNORE); + mConfig.setCatchBlock(BlockOption.IGNORE); final Checker c = createChecker(); final String filepath = getPath("InputWhitespace.java"); assertNotNull(c); @@ -224,7 +226,8 @@ public class CheckerTest throws Exception { mConfig.setIgnoreWhitespace(true); - mConfig.setCatchBlock(CatchBlockOption.IGNORE); + mConfig.setTryBlock(BlockOption.IGNORE); + mConfig.setCatchBlock(BlockOption.IGNORE); final Checker c = createChecker(); final String filepath = getPath("InputWhitespace.java"); assertNotNull(c); @@ -765,7 +768,9 @@ public class CheckerTest throws Exception { mConfig.setJavadocScope(Scope.NOTHING); - mConfig.setCatchBlock(CatchBlockOption.STMT); + mConfig.setTryBlock(BlockOption.STMT); + mConfig.setCatchBlock(BlockOption.STMT); + mConfig.setFinallyBlock(BlockOption.STMT); mConfig.setIgnoreImports(true); mConfig.setIllegalInstantiations( "java.lang.Boolean," + @@ -788,6 +793,10 @@ public class CheckerTest filepath + ":70:38: Must have at least one statement.", filepath + ":71:52: Must have at least one statement.", filepath + ":72:45: Must have at least one statement.", + filepath + ":74:13: Must have at least one statement.", + filepath + ":76:17: Must have at least one statement.", + filepath + ":78:13: Must have at least one statement.", + filepath + ":81:17: Must have at least one statement.", }; verify(c, filepath, expected); } @@ -796,7 +805,9 @@ public class CheckerTest throws Exception { mConfig.setJavadocScope(Scope.NOTHING); - mConfig.setCatchBlock(CatchBlockOption.TEXT); + mConfig.setTryBlock(BlockOption.TEXT); + mConfig.setCatchBlock(BlockOption.TEXT); + mConfig.setFinallyBlock(BlockOption.TEXT); mConfig.setIgnoreImports(true); mConfig.setIllegalInstantiations(""); final Checker c = createChecker(); @@ -806,6 +817,8 @@ public class CheckerTest filepath + ":51:65: Empty catch block.", filepath + ":71:52: Empty catch block.", filepath + ":72:45: Empty catch block.", + filepath + ":74:13: Empty try block.", + filepath + ":76:17: Empty finally block.", }; verify(c, filepath, expected); } diff --git a/src/tests/com/puppycrawl/tools/checkstyle/InputSemantic.java b/src/tests/com/puppycrawl/tools/checkstyle/InputSemantic.java index 098c776c7..dbc8929a6 100644 --- a/src/tests/com/puppycrawl/tools/checkstyle/InputSemantic.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/InputSemantic.java @@ -46,7 +46,7 @@ class InputSemantic void exHandlerTest() { try { - // do stuff and don't handle exceptions in some cases + ; // do stuff and don't handle exceptions in some cases } catch (IllegalStateException emptyCatchIsAlwaysAnError) { } @@ -70,5 +70,22 @@ class InputSemantic catch (SecurityException ex) { /* hello */ } catch (StringIndexOutOfBoundsException ex) {} catch (IllegalArgumentException ex) { } + + try { + } + finally { + } + try { + // something + } + finally { + // something + } + try { + ; // something + } + finally { + ; // statement + } } }
Option Definition
ignoreIgnore empty catch blocks.Ignore empty blocks.
textRequire that there is some text in the catch block. For example: + Require that there is some text in the block. For example: 
     catch (Exception ex) {
         // This is a bad coding practice
@@ -403,10 +403,10 @@ line 5: /{71}
stmtRequire that there is a statement in the catch block. For example: + Require that there is a statement in the block. For example: 
-    catch (Exception ex) {
-        log.warn("Caught exception.", ex);
+    finally {
+        lock.release();
     }