False positive: magicnumbers with bit shift expression

[ghernadi]

I have read check documentation: https://checkstyle.org/checks/coding/magicnumber.html I have downloaded the latest cli from: https://checkstyle.org/cmdline.html#Download_and_Run I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

$ javac Test.java
$

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
    "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
    "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <property name="severity" value="error"/>
  <property name="fileExtensions" value="java, properties, xml"/>
  <module name="TreeWalker">
    <module name="MagicNumber">
       <property name="ignoreNumbers" value="-2,-1,0,1,2,100"/>
       <property name="ignoreHashCodeMethod" value="true"/>
       <property name="ignoreFieldDeclaration" value="true"/>
       <property name="severity" value="warning"/>
       <property name="id" value="checkstyle:magicnumber"/>
    </module>
  </module>
</module>

public class Test {
    public static final int BIT0 = 1;
    public static final int BIT1 = 1 << 1;
    public static final int BIT2 = 1 << 2;
    public static final int BIT3 = 1 << 3;
    public static final int BIT4 = 1 << 4;
}

$ java -jar checkstyle-10.20.1-all.jar -c config.xml Test.java 
Starting audit...
[WARN] /var/tmp/Test.java:5:41: '3' is a magic number. [checkstyle:magicnumber]
[WARN] /var/tmp/Test.java:6:41: '4' is a magic number. [checkstyle:magicnumber]
Audit done.

I would have expected bit-shift operators to also be excluded from this check, just like 2 * 2 * 2 would work without checkstyle complaining...

[nrmancuso]

Few things:

I would have expected bit-shift operators to also be excluded from this check, just like 2 2 2 would work without checkstyle complaining...

This appears to be a bug; to be clear, the check claims that it:

Checks that there are no "magic numbers" where a magic number is a numeric literal that is not defined as a constant. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Constant definition is any variable/field that has 'final' modifier. It is fine to have one constant defining multiple numeric literals within one expression

So, since these fields are declared as constants and are defined by multiple numeric literal, I would expect that violations are not placed on them

<property name="ignoreFieldDeclaration" value="true"/>

This property being set to true would lead me to believe that NO fields should be checked, regardless; apparently this is not the case. The provided example in the docs confuses matters more: https://checkstyle.org/checks/coding/magicnumber.html#Example3-config

To configure the check so that it ignores magic numbers in field declarations

<property name="ignoreFieldDeclaration" value="false"/>

private static int myInt = 7; // ok, field declaration

If ignoreFieldDeclaration is "false", that means that we should NOT be ignoring field declarations :)

Here, I have extended your example to hopefully make the problem a bit more obvious:

➜  src cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
    <property name="severity" value="error"/>
    <property name="fileExtensions" value="java, properties, xml"/>
    <module name="TreeWalker">
        <module name="MagicNumber">
            <property name="ignoreNumbers" value="-2,-1,0,1,2,100"/>
            <property name="ignoreHashCodeMethod" value="true"/>
            <property name="ignoreFieldDeclaration" value="true"/>
            <property name="severity" value="warning"/>
            <property name="id" value="checkstyle:magicnumber"/>
        </module>
    </module>
</module>

 ➜  src cat Test.java 
public class Test {
    public static final int BIT0 = 1;
    public static final int BIT1 = 1 << 1;
    public static final int BIT2 = 1 << 2;
    public static final int BIT3 = 1 << 3;
    public static final int BIT4 = 1 << 4;

    public static final int MUL1 = 1;
    public static final int MUL2 = 1 * 1;
    public static final int MUL3 = 1 * 2;
    public static final int MUL4 = 1 * 3;
    public static final int MUL5 = 1 * 4;
}

➜  src javac Test.java 

➜  src java -jar checkstyle-10.17.0-all.jar -c config.xml Test.java
Starting audit...
[WARN] /home/nick/IdeaProjects/tester/src/Test.java:5:41: '3' is a magic number. [checkstyle:magicnumber]
[WARN] /home/nick/IdeaProjects/tester/src/Test.java:6:41: '4' is a magic number. [checkstyle:magicnumber]
Audit done.

We should also fix the docs while we are here.

[mahfouz72]

I am on it.

We don't have bitwise operators in constantWaiverParentToken.

Specify tokens that are allowed in the AST path from the number literal to the enclosing constant definition.

https://github.com/checkstyle/checkstyle/blob/1db5e7eb487d2801475e6d855c691a89fcb0f4f2/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java#L261-L272

when we have an AST with << (SL) in line 266 we don't find it in the constantWaiverParentToken so we break and return false.

It is not so obvious to me how we generate this default list. I think we are missing some operators in it (bitwise operators for example)

---

EDIT:

The acceptable value is any subset of TokenTypes. So we can override the default value and add bitwise operators.

D:\CS\test javac src/Test.java
D:\CS\test cat config.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="MagicNumber">
            <property name="ignoreNumbers" value="-2,-1,0,1,2,100"/>
            <property name="constantWaiverParentToken" value="ASSIGN, EXPR, STAR, SL "/>
            <property name="ignoreHashCodeMethod" value="true"/>
            <property name="ignoreFieldDeclaration" value="true"/>
            <property name="severity" value="warning"/>
            <property name="id" value="checkstyle:magicnumber"/>
        </module>
    </module>
</module>

D:\CS\test cat src/Test.java
public class Test {
    public static final int BIT0 = 1;
    public static final int BIT1 = 1 << 1;
    public static final int BIT2 = 1 << 2;
    public static final int BIT3 = 1 << 3;
    public static final int BIT4 = 1 << 4;

    public static final int MUL1 = 1;
    public static final int MUL2 = 1 * 1;
    public static final int MUL3 = 1 * 2;
    public static final int MUL4 = 1 * 3;
    public static final int MUL5 = 1 * 4;
}
D:\CS\test java  -jar checkstyle-10.17.0-all.jar -c config.xml src/Test.java
Starting audit...
Audit done.

[mahfouz72]

This is an expected behaviour according to the check documentation. I don't think this is an issue, The only thing I have here is why we don't expand the default value of constantWaiverParentToken to include all tokens, and let the users override it based on their specific needs.

IMO, the default value here is so strict, and makes no sense to me why we chose STAR but not LT for example

[romani]

Look at blame https://github.com/checkstyle/checkstyle/blame/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java

Updates should reference to issue, and our thoughts on this

[mahfouz72]

The commit message: Made MagicNumber check less aggressive, based on feedback on checkstyle-user list

looks like it is still aggressive

[romani]

Check is very aggressive by design. We are here to find practical cases where it should be less demanding or optionally less demanding.

[romani]

Initial set of tokens was 20 years ago.

But few recent: https://github.com/checkstyle/checkstyle/commit/262bcabc0712b649340a96a7229fba80858eaa03 https://github.com/checkstyle/checkstyle/commit/fabe67975252df0a7e2fe2036e4d50c273745c37

Have tracking of intend https://github.com/checkstyle/checkstyle/issues/1113 . All of them are for fields with expressions.

So I am ok to extend ignore list with binary operations. We will see in regression diff report , how it affects others.

Please send PR.