`ignoreFieldDeclaration` property should have the highest priority in MagicNumberCheck #16101

mahfouz72 · 2024-12-31T12:21:14Z

I have read check documentation: https://checkstyle.sourceforge.io/checks/coding/magicnumber.html
I have downloaded the latest checkstyle 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

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">
  <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="constantWaiverParentToken" value="ARRAY_INIT , ASSIGN  , ELIST , EXPR"/>
    </module>
  </module>
</module>
D:\CS\test cat src/Test.java
public class Test {
    public final int radius = 10;
    public final double area = (22 / 7.0) * radius * radius;
    // 2 violations above, but it should be ok because
    // ignoreFieldDeclaration = true
}
D:\CS\test java  -jar checkstyle-10.21.1-all.jar -c config.xml src/Test.java
Starting audit...
[ERROR] D:\CS\test\src\Test.java:3:33: '22' is a magic number. [MagicNumber]
[ERROR] D:\CS\test\src\Test.java:3:38: '7.0' is a magic number. [MagicNumber]
Audit done.
Checkstyle ends with 2 errors.

Describe what you expect in detail.

Although constantWaiverParentToken doesn't have STAR or DIV in the config. but still, there should be no violation because these magic numbers are in a field declaration and ignoreFieldDeclaration is true

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

ignoreFieldDeclaration => Ignore magic numbers in field declarations.

The text was updated successfully, but these errors were encountered:

samr874 · 2025-01-02T16:06:40Z

@romani what is the status for this issue?? let me know?? are the contributions still active for this project??

romani · 2025-01-02T19:33:37Z

are the contributions still active for this project??

contributors are whole world Java community, please step in, make comment "I am on it" and simply send PR with fix.
Please read and watch videos at https://checkstyle.org/beginning_development.html#Starting_Development
If you are new to project we recommend to get easy issues first, https://github.com/checkstyle/checkstyle/blob/master/.github/CONTRIBUTING.md#getting-started

romani · 2025-01-02T21:16:54Z

@mahfouz72 , thank a lot for opening clear issue.

ShamithaReddy · 2025-01-06T04:23:16Z

I am on it

I have some questions related to my understanding @romani @mahfouz72

Current Behaviour:

Within visitToken method, first checks for constants (final, interface, enum)
If found, it checks against constantWaiverParentToken
If no constants found, then checks ignoreFieldDeclaration
The ignoreFieldDeclaration property is considered only if there are no constants found

As per the document , it mentions the magicnumbers are non-constants

where a magic number is a numeric literal that is not defined as a constant
Constant definition is any variable/field that has 'final' modifier.

According to this I suppose that ignoreFieldDeclaration flow neednt be modified:

If no constant is found, it then checks for field declarations and the ignoreFieldDeclaration property.
This ensures that only numeric literals that are not defined as constants (i.e., magic numbers) are reported, unless they are in a field declaration and ignoreFieldDeclaration is true.

But my question is why are we checking isMagicNumberExists if constants are found?

a magic number is a numeric literal that is not defined as a constant

MagicNumberCheck.java#L261-L280

If this is expected, ie. we have isMagicNumberExists check to verify that the number is being used in an approved way within that constant definition using constantWaiverParentToken, are we here required to enhance the functionality by adding the ignoreFieldDeclaration property on top of constantWaiverParentToken check?

ShamithaReddy · 2025-01-08T17:23:54Z

@romani could you please check the above comment

mahfouz72 · 2025-01-08T17:33:30Z

It is better to discuss implementation details in a PR, please push your code.

romani · 2025-01-26T02:03:57Z

Fix is merged .

Follow up to improve will be at #16228

### What changes were proposed in this pull request? This PR aims to upgrade `checkstyle` from 10.20.2 to 10.21.2. ### Why are the changes needed? To pick up bug fixes: - checkstyle-10.21.2 checkstyle/checkstyle#15939 - lineWrappingIndentation falsely detects incorrect indentation for text blocks checkstyle/checkstyle#16101 - ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck - checkstyle-10.21.1 checkstyle/checkstyle#11374 - UnusedLocalVariable: False Positive when inner class has same field as variable Full release notes: - https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.2 - https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.1 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually by: ``` bash ./dev/lint-java ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49899 from wayneguow/checkstyle. Authored-by: Wei Guo <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? This PR aims to upgrade `checkstyle` from 10.20.2 to 10.21.2. ### Why are the changes needed? To pick up bug fixes: - checkstyle-10.21.2 checkstyle/checkstyle#15939 - lineWrappingIndentation falsely detects incorrect indentation for text blocks checkstyle/checkstyle#16101 - ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck - checkstyle-10.21.1 checkstyle/checkstyle#11374 - UnusedLocalVariable: False Positive when inner class has same field as variable Full release notes: - https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.2 - https://github.com/checkstyle/checkstyle/releases/tag/checkstyle-10.21.1 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually by: ``` bash ./dev/lint-java ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#49899 from wayneguow/checkstyle. Authored-by: Wei Guo <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

mahfouz72 changed the title ~~ignoreFieldDeclaration property should Have the highest priority in MagicNumberCheck~~ ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck Dec 31, 2024

mahfouz72 mentioned this issue Dec 31, 2024

False positive: magicnumbers with bit shift expression #15950

Closed

romani added the approved label Dec 31, 2024

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 15, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

04d8687

Anmol202005 mentioned this issue Jan 15, 2025

Issue #16101: Fixed MagicNumber Check Bug #16172

Merged

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 15, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

bed4fa5

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 15, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

87fbce0

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

c98c987

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

707828e

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

cd3965d

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

4157820

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

51a890e

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

d56cc3c

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

23c35fd

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

59819c5

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

95575d4

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

3f75d5e

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

1766aee

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

2eed3c9

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

976f5fe

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

da3ba3f

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

3c80f9b

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

8f163e0

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

f34796e

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

0372467

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

f481279

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

695cd5c

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

c14a157

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 16, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

615497f

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 17, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

57fb51c

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 18, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

ad3326b

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 18, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

fff507f

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 18, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

bc03d07

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 23, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

43638ad

Anmol202005 added a commit to Anmol202005/checkstyle that referenced this issue Jan 24, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

f62620a

romani pushed a commit that referenced this issue Jan 24, 2025

Issue #16101: Fixed MagicNumber Check Bug

3096cfc

github-actions bot added this to the 10.21.2 milestone Jan 24, 2025

romani added the bug label Jan 24, 2025

rnveach mentioned this issue Jan 24, 2025

False positive Magic Number violations within the method declaration in class field #16228

Closed

romani closed this as completed Jan 26, 2025

wayneguow mentioned this issue Feb 12, 2025

[SPARK-51171][BUILD] Upgrade checkstyle to 10.21.2 apache/spark#49899

Closed

trinhnx pushed a commit to trinhnx/checkstyle that referenced this issue Feb 26, 2025

Issue checkstyle#16101: Fixed MagicNumber Check Bug

1c5f8d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ignoreFieldDeclaration` property should have the highest priority in MagicNumberCheck #16101

`ignoreFieldDeclaration` property should have the highest priority in MagicNumberCheck #16101

mahfouz72 commented Dec 31, 2024

samr874 commented Jan 2, 2025

romani commented Jan 2, 2025

romani commented Jan 2, 2025

ShamithaReddy commented Jan 6, 2025

ShamithaReddy commented Jan 8, 2025

mahfouz72 commented Jan 8, 2025

romani commented Jan 26, 2025 •

edited

Loading

ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck #16101

ignoreFieldDeclaration property should have the highest priority in MagicNumberCheck #16101

Comments

mahfouz72 commented Dec 31, 2024

samr874 commented Jan 2, 2025

romani commented Jan 2, 2025

romani commented Jan 2, 2025

ShamithaReddy commented Jan 6, 2025

ShamithaReddy commented Jan 8, 2025

mahfouz72 commented Jan 8, 2025

romani commented Jan 26, 2025 • edited Loading

`ignoreFieldDeclaration` property should have the highest priority in MagicNumberCheck #16101

`ignoreFieldDeclaration` property should have the highest priority in MagicNumberCheck #16101

romani commented Jan 26, 2025 •

edited

Loading