diff --git a/docs/releasenotes.html b/docs/releasenotes.html index 25e0cd14e..4f4d62d7b 100644 --- a/docs/releasenotes.html +++ b/docs/releasenotes.html @@ -127,6 +127,9 @@ checkstyle-user). within the same file.
  • Unexpected char (bug 975346).
    • + +
  • MagicNumber check overly aggressive + (reported on user mailing list).

    • diff --git a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java index 4e2c50791..32f6b1c5b 100644 --- a/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java +++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java @@ -41,10 +41,29 @@ import java.util.Arrays; * </module> * * @author Rick Giles + * @author Lars Kühne */ public class MagicNumberCheck extends Check { - /** the numbers to ignore in the check, sorted */ + private static final int[] ALLOWED_PATH_TOKENTYPES = + { + TokenTypes.ASSIGN, + TokenTypes.ARRAY_INIT, + TokenTypes.EXPR, + TokenTypes.UNARY_PLUS, + TokenTypes.UNARY_MINUS, + TokenTypes.TYPECAST, + TokenTypes.ELIST, + TokenTypes.LITERAL_NEW, + TokenTypes.METHOD_CALL, + TokenTypes.STAR, + }; + + static { + Arrays.sort(ALLOWED_PATH_TOKENTYPES); + } + + /** the numbers to ignore in the check, sorted */ private double[] mIgnoreNumbers = {-1, 0, 1, 2}; /** @see com.puppycrawl.tools.checkstyle.api.Check */ @@ -61,26 +80,87 @@ public class MagicNumberCheck extends Check /** @see com.puppycrawl.tools.checkstyle.api.Check */ public void visitToken(DetailAST aAST) { - if (!inIgnoreList(aAST) && !isConstantDefinition(aAST)) { - String text = aAST.getText(); - final DetailAST parent = aAST.getParent(); - DetailAST reportAST = aAST; - if (parent.getType() == TokenTypes.UNARY_MINUS) { - reportAST = parent; - text = "-" + text; - } - else if (parent.getType() == TokenTypes.UNARY_PLUS) { - reportAST = parent; - text = "+" + text; - } - log(reportAST.getLineNo(), - reportAST.getColumnNo(), - "magic.number", - text); + if (inIgnoreList(aAST)) { + return; } + + DetailAST constantDefAST = findContainingConstantDef(aAST); + + if (constantDefAST == null) { + reportMagicNumber(aAST); + } + else { + DetailAST ast = aAST.getParent(); + while (ast != constantDefAST) { + int type = ast.getType(); + if (Arrays.binarySearch(ALLOWED_PATH_TOKENTYPES, type) < 0) { + reportMagicNumber(aAST); + break; + } + + ast = ast.getParent(); + } + } } /** + * Finds the constant definition that contains aAST. + * @param aAST the AST + * @return the constant def or null if aAST is not + * contained in a constant definition + */ + private DetailAST findContainingConstantDef(DetailAST aAST) { + DetailAST varDefAST = aAST; + while (varDefAST != null + && varDefAST.getType() != TokenTypes.VARIABLE_DEF) + { + varDefAST = varDefAST.getParent(); + } + + // no containing variable definition? + if (varDefAST == null) { + return null; + } + + // implicit constant? + if (ScopeUtils.inInterfaceBlock(varDefAST)) { + return varDefAST; + } + + // explicit constant + final DetailAST modifiersAST = + varDefAST.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersAST.branchContains(TokenTypes.FINAL)) { + return varDefAST; + } + + return null; + } + + /** + * Reports aAST as a magic number, includes unary operators as needed. + * @param aAST the AST node that contains the number to report + */ + private void reportMagicNumber(DetailAST aAST) + { + String text = aAST.getText(); + final DetailAST parent = aAST.getParent(); + DetailAST reportAST = aAST; + if (parent.getType() == TokenTypes.UNARY_MINUS) { + reportAST = parent; + text = "-" + text; + } + else if (parent.getType() == TokenTypes.UNARY_PLUS) { + reportAST = parent; + text = "+" + text; + } + log(reportAST.getLineNo(), + reportAST.getColumnNo(), + "magic.number", + text); + } + + /** * Decides whether the number of an AST is in the ignore list of this * check. * @param aAST the AST to check @@ -97,59 +177,6 @@ public class MagicNumberCheck extends Check return (Arrays.binarySearch(mIgnoreNumbers, value) >= 0); } - /** - * Decides whether the number of an AST is the RHS of a constant - * definition. - * @param aAST the AST to check. - * @return true if the number of aAST is the RHS of a constant definition. - */ - private boolean isConstantDefinition(DetailAST aAST) - { - if (ScopeUtils.inInterfaceBlock(aAST)) { - return true; - } - DetailAST parent = aAST.getParent(); - - if (parent == null) { - return false; - } - - //skip TYPECAST, UNARY_MINUS, UNARY_PLUS - while ((parent.getType() == TokenTypes.UNARY_MINUS) - || (parent.getType() == TokenTypes.UNARY_PLUS) - || (parent.getType() == TokenTypes.TYPECAST)) - { - parent = parent.getParent(); - } - - //expression? - if ((parent == null) || (parent.getType() != TokenTypes.EXPR)) { - return false; - } - - //array init? - parent = parent.getParent(); - if ((parent != null) && (parent.getType() == TokenTypes.ARRAY_INIT)) { - parent = parent.getParent(); - } - - //assignment? - if ((parent == null) || (parent.getType() != TokenTypes.ASSIGN)) { - return false; - } - - //variable definition? - parent = parent.getParent(); - if ((parent == null) || (parent.getType() != TokenTypes.VARIABLE_DEF)) { - return false; - } - - //final? - final DetailAST modifiersAST = - parent.findFirstToken(TokenTypes.MODIFIERS); - return modifiersAST.branchContains(TokenTypes.FINAL); - } - /** * Sets the numbers to ignore in the check. * BeanUtils converts numeric token list to double array automatically. diff --git a/src/testinputs/com/puppycrawl/tools/checkstyle/InputMagicNumber.java b/src/testinputs/com/puppycrawl/tools/checkstyle/InputMagicNumber.java index 5fb90cd51..aa9f872a1 100644 --- a/src/testinputs/com/puppycrawl/tools/checkstyle/InputMagicNumber.java +++ b/src/testinputs/com/puppycrawl/tools/checkstyle/InputMagicNumber.java @@ -83,6 +83,7 @@ class ArrayMagicTest { private static final int[] NONMAGIC = {3}; private int[] magic = {3}; + private static final int[][] NONMAGIC2 = {{1}, {2}, {3}}; } /** test long hex */ @@ -119,3 +120,25 @@ class Cast { public static final int TESTINTVAL = (byte) 0x80; } + +class ComplexAndFlagged +{ + public static final java.util.List MYLIST = new java.util.ArrayList() + { + public int size() + { + // should be flagged although technically inside const definition + return 378; + } + }; +} + +class ComplexButNotFlagged +{ + // according to user feedback this is typical code that should not be flagged + // (at least in the default configuration of MagicNumberCheck) + public static final Integer DEFAULT_INT = new Integer(27); + public static final int SECS_PER_DAY = 24 * 60 * 60; + public static final javax.swing.Border STD_BORDER = + javax.swing.BorderFactory.createEmptyBorder(3, 3, 3, 3); +} diff --git a/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java b/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java index 05c3af2f9..cc2e7e072 100644 --- a/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java +++ b/src/tests/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java @@ -33,15 +33,16 @@ public class MagicNumberCheckTest "71:29: '0x10L' is a magic number.", "72:29: '0X11l' is a magic number.", "85:28: '3' is a magic number.", - "91:14: '0xffffffffL' is a magic number.", - "99:30: '+3' is a magic number.", - "100:29: '-2' is a magic number.", - "101:35: '+3.5' is a magic number.", - "102:36: '-2.5' is a magic number.", - "110:35: '0x80000000' is a magic number.", - "111:36: '0x8000000000000000L' is a magic number.", - "114:37: '020000000000' is a magic number.", - "115:38: '01000000000000000000000L' is a magic number.", + "92:14: '0xffffffffL' is a magic number.", + "100:30: '+3' is a magic number.", + "101:29: '-2' is a magic number.", + "102:35: '+3.5' is a magic number.", + "103:36: '-2.5' is a magic number.", + "111:35: '0x80000000' is a magic number.", + "112:36: '0x8000000000000000L' is a magic number.", + "115:37: '020000000000' is a magic number.", + "116:38: '01000000000000000000000L' is a magic number.", + "131:32: '378' is a magic number.", }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); } @@ -68,18 +69,19 @@ public class MagicNumberCheckTest "64:30: '011l' is a magic number.", "69:24: '0X011' is a magic number.", "72:29: '0X11l' is a magic number.", - "91:14: '0xffffffffL' is a magic number.", - "100:29: '-2' is a magic number.", - "101:35: '+3.5' is a magic number.", - "102:36: '-2.5' is a magic number.", - "108:34: '0xffffffff' is a magic number.", - "109:36: '0xffffffffffffffffL' is a magic number.", - "110:35: '0x80000000' is a magic number.", - "111:36: '0x8000000000000000L' is a magic number.", - "112:36: '037777777777' is a magic number.", - "113:38: '01777777777777777777777L' is a magic number.", - "114:37: '020000000000' is a magic number.", - "115:38: '01000000000000000000000L' is a magic number.", + "92:14: '0xffffffffL' is a magic number.", + "101:29: '-2' is a magic number.", + "102:35: '+3.5' is a magic number.", + "103:36: '-2.5' is a magic number.", + "109:34: '0xffffffff' is a magic number.", + "110:36: '0xffffffffffffffffL' is a magic number.", + "111:35: '0x80000000' is a magic number.", + "112:36: '0x8000000000000000L' is a magic number.", + "113:36: '037777777777' is a magic number.", + "114:38: '01777777777777777777777L' is a magic number.", + "115:37: '020000000000' is a magic number.", + "116:38: '01000000000000000000000L' is a magic number.", + "131:32: '378' is a magic number.", }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); } @@ -133,19 +135,20 @@ public class MagicNumberCheckTest "71:29: '0x10L' is a magic number.", "72:29: '0X11l' is a magic number.", "85:28: '3' is a magic number.", - "91:14: '0xffffffffL' is a magic number.", - "99:30: '+3' is a magic number.", - "100:29: '-2' is a magic number.", - "101:35: '+3.5' is a magic number.", - "102:36: '-2.5' is a magic number.", - "108:34: '0xffffffff' is a magic number.", - "109:36: '0xffffffffffffffffL' is a magic number.", - "110:35: '0x80000000' is a magic number.", - "111:36: '0x8000000000000000L' is a magic number.", - "112:36: '037777777777' is a magic number.", - "113:38: '01777777777777777777777L' is a magic number.", - "114:37: '020000000000' is a magic number.", - "115:38: '01000000000000000000000L' is a magic number.", + "92:14: '0xffffffffL' is a magic number.", + "100:30: '+3' is a magic number.", + "101:29: '-2' is a magic number.", + "102:35: '+3.5' is a magic number.", + "103:36: '-2.5' is a magic number.", + "109:34: '0xffffffff' is a magic number.", + "110:36: '0xffffffffffffffffL' is a magic number.", + "111:35: '0x80000000' is a magic number.", + "112:36: '0x8000000000000000L' is a magic number.", + "113:36: '037777777777' is a magic number.", + "114:38: '01777777777777777777777L' is a magic number.", + "115:37: '020000000000' is a magic number.", + "116:38: '01000000000000000000000L' is a magic number.", + "131:32: '378' is a magic number.", }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); } @@ -176,13 +179,14 @@ public class MagicNumberCheckTest "71:29: '0x10L' is a magic number.", "72:29: '0X11l' is a magic number.", "85:28: '3' is a magic number.", - "91:14: '0xffffffffL' is a magic number.", - "99:30: '+3' is a magic number.", - "100:29: '-2' is a magic number.", - "110:35: '0x80000000' is a magic number.", - "111:36: '0x8000000000000000L' is a magic number.", - "114:37: '020000000000' is a magic number.", - "115:38: '01000000000000000000000L' is a magic number.", + "92:14: '0xffffffffL' is a magic number.", + "100:30: '+3' is a magic number.", + "101:29: '-2' is a magic number.", + "111:35: '0x80000000' is a magic number.", + "112:36: '0x8000000000000000L' is a magic number.", + "115:37: '020000000000' is a magic number.", + "116:38: '01000000000000000000000L' is a magic number.", + "131:32: '378' is a magic number.", }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); } @@ -214,9 +218,10 @@ public class MagicNumberCheckTest "71:29: '0x10L' is a magic number.", "72:29: '0X11l' is a magic number.", "85:28: '3' is a magic number.", - "91:14: '0xffffffffL' is a magic number.", - "99:30: '+3' is a magic number.", - "100:29: '-2' is a magic number.", + "92:14: '0xffffffffL' is a magic number.", + "100:30: '+3' is a magic number.", + "101:29: '-2' is a magic number.", + "131:32: '378' is a magic number.", }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); }