Refactored the catch block checking code to include try/catch/finally blocks.

This commit is contained in:
Oliver Burn 2002-06-05 13:17:20 +00:00
parent dc92fd1b89
commit 4018efa9fe
12 changed files with 247 additions and 101 deletions

View File

@ -74,9 +74,19 @@ This task is included in the checkstyle distribution.</p>
<td valign="top">Specifies the policy of how to pad parenthesises. The legal values are defined <a href="engine.html#parenpad">here</a>. Defaults to <span class="default">&quot;nospace&quot;</span>.</td>
<td align="center" valign="top">No</td>
</tr>
<tr>
<td valign="top">tryBlock</td>
<td valign="top">Specifies the policy of how check <span class="code">try</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;stmt&quot;</span>.</td>
<td align="center" valign="top">No</td>
</tr>
<tr>
<td valign="top">catchBlock</td>
<td valign="top">Specifies the policy of how check <span class="code">catch</span> blocks. The legal values are defined <a href="engine.html#catchBlockOpt">here</a>. Defaults to <span class="default">&quot;text&quot;</span>.</td>
<td valign="top">Specifies the policy of how check <span class="code">catch</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;text&quot;</span>.</td>
<td align="center" valign="top">No</td>
</tr>
<tr>
<td valign="top">finallyBlock</td>
<td valign="top">Specifies the policy of how check <span class="code">finally</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;stmt&quot;</span>.</td>
<td align="center" valign="top">No</td>
</tr>
<tr>

View File

@ -69,8 +69,16 @@ This command line tool is included in the checkstyle distribution.</p>
<td valign="top">Specifies the policy of how to pad parenthesises. The legal values are defined <a href="engine.html#parenpad">here</a>. Defaults to <span class="default">&quot;nospace&quot;</span>.</td>
</tr>
<tr>
<td valign="top">checkstyle.catchblock</td>
<td valign="top">Specifies the policy of how check <span class="code">catch</span> blocks. The legal values are defined <a href="engine.html#catchBlockOpt">here</a>. Defaults to <span class="default">&quot;text&quot;</span>.</td>
<td valign="top">checkstyle.block.try</td>
<td valign="top">Specifies the policy of how check <span class="code">try</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;stmt&quot;</span>.</td>
</tr>
<tr>
<td valign="top">checkstyle.block.catch</td>
<td valign="top">Specifies the policy of how check <span class="code">catch</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;text&quot;</span>.</td>
</tr>
<tr>
<td valign="top">checkstyle.block.finally</td>
<td valign="top">Specifies the policy of how check <span class="code">finally</span> blocks. The legal values are defined <a href="engine.html#blockOpt">here</a>. Defaults to <span class="default">&quot;text&quot;</span>.</td>
</tr>
<tr>
<td valign="top">checkstyle.allow.protected</td>

View File

@ -9,7 +9,7 @@
<body>
<h1>Checkstyle Core Engine - Version @CHECKSTYLE_VERSION@</h1>
<p>Author: <a href="mailto:checkstyle@puppycrawl.com">Oliver Burn</a></p>
<p>Authors: <a href="mailto:checkstyle@puppycrawl.com">Oliver Burn</a>, <a href="mailto:lars.kuehne@planet-interkom.de">Lars Kühne</a></p>
<h2>Introduction</h2>
<p>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 <a href="index.html#related">interfaces</a>. The standard distribution includes support for:
@ -378,22 +378,22 @@ line 5: /{71}
<p>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.</p>
</div>
<h3>Exception handling</h3>
<p>Checks for empty <span class="code">catch</span> blocks. The check can be configured with the following options:</p>
<h3>Empty Blocks</h3>
<p>Checks for empty <span class="code">try</span>, <span class="code">catch</span>, <span class="code">finally</span> blocks. The checks can be configured with the following options:</p>
<a name="catchBlockOpt"></a>
<table border="1" summary="catch block options">
<a name="blockOpt"></a>
<table border="1" summary="block options">
<tr class="header">
<td>Option</td>
<td>Definition</td>
</tr>
<tr>
<td><span class="code">ignore</span></td>
<td>Ignore empty <span class="code">catch</span> blocks.</td>
<td>Ignore empty blocks.</td>
</tr>
<tr>
<td><span class="code">text</span></td>
<td>Require that there is some text in the <span class="code">catch</span> block. For example:
<td>Require that there is some text in the block. For example:
<pre>
catch (Exception ex) {
// This is a bad coding practice
@ -403,10 +403,10 @@ line 5: /{71}
</tr>
<tr>
<td><span class="code">stmt</span></td>
<td>Require that there is a statement in the <span class="code">catch</span> block. For example:
<td>Require that there is a statement in the block. For example:
<pre>
catch (Exception ex) {
log.warn("Caught exception.", ex);
finally {
lock.release();
}
</pre>
</tr>

View File

@ -43,7 +43,9 @@
<ul>
<li class="body">Detect instantiations of classes that should not be instantiated (e.g. java.lang.Boolean) (request 550205).</li>
<li class="body">Check for line wrapping on operators (request 553160).</li>
<li class="body">Detect empty <span class="code">try</span> blocks.</li>
<li class="body">Detect empty <span class="code">catch</span> blocks (request 516255).</li>
<li class="body">Detect empty <span class="code">finally</span> blocks.</li>
<li class="body">Detect to-do comments (request 504275).</li>
<li class="body">Include column number in the XML output (request 555262).</li>
</ul>

View File

@ -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 <a href="mailto:oliver@puppycrawl.com">Oliver Burn</a>
*/
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 <code>CatchBlockOption</code> instance.
* Creates a new <code>BlockOption</code> 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 <code>CatchBlockOption</code> value represented by aStrRep,
* @return the <code>BlockOption</code> 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.
*

View File

@ -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);

View File

@ -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

View File

@ -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";
}

View File

@ -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.");
}
}
}
}
}
// }}}
}

View File

@ -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]);
}
)?
;

View File

@ -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);
}

View File

@ -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
}
}
}