MagicNumberCheck NPE when ignoring field declarations

[rnveach]

Reported at checkstyle/sonar-checkstyle#519

$ cat TestClass.java
package uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.service.ddp.service;

import org.springframework.core.annotation.Order;
import org.springframework.stereotype.Component;
import uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.model.jpa.DeviceAttributeValue;
import uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.model.jpa.DeviceData;
import uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.model.dto.ddp.PointsEvent;

import java.util.LinkedHashMap;
import java.util.Map;

import static uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.service.ddp.service.AttributeUtil.parseDouble;
import static uk.co.XXX.msa.vpspointsmanager.vpspmvdpeventsservice.service.ddp.service.AttributeUtil.parseString;


/**
 * @author XXXX
 * @since 20/01/2024
 */
@Order(value = 3)
@Component
public class ActiveDurationEventRule implements PointsEventRule {

    @Override
    public LinkedHashMap<String, PointsEvent> createEvents(Map<Long, DeviceAttributeValue> deviceAttributeValueMap, DeviceData deviceData, LinkedHashMap<String, PointsEvent> pointsEventMap) {
        Double totalSessionDurationMS = parseDouble(deviceAttributeValueMap
                .get(AttributeId.ACTIVE_DURATION.getId()));
        String manufacturer = parseString(deviceAttributeValueMap
                .get(AttributeId.DEVICE_MANUFACTURER.getId()));

        if ((totalSessionDurationMS != null)  && (manufacturer != null && manufacturer.equalsIgnoreCase("Fiit"))) {
            double sessionDurationInMinutes = totalSessionDurationMS / 60;
            if (sessionDurationInMinutes >= 20) {
                pointsEventMap.put(
                        PointsEventId.FIIT_HOME_WORKOUT_PLAN.getMnemonic(),
                        new PointsEvent(PointsEventId.FIIT_HOME_WORKOUT_PLAN
                                .getMnemonic(), deviceData.getEntityNo(), deviceData.getEventDate().toDate()));
            }
        }
        return pointsEventMap;
    }
}

$ cat TestConfig.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="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">
    <module name="MagicNumber">
      <property name="ignoreAnnotationElementDefaults" value="false"/>
      <property name="ignoreNumbers" value="0.99"/>
      <property name="ignoreFieldDeclaration" value="true"/>
    </module>
    </module>
</module>

$ java -jar checkstyle-10.15.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:1: Got an exception - java.lang.NullPointerException
    at com.puppycrawl.tools.checkstyle.checks.coding.MagicNumberCheck.isFieldDeclaration(MagicNumberCheck.java:390)
    at com.puppycrawl.tools.checkstyle.checks.coding.MagicNumberCheck.visitToken(MagicNumberCheck.java:219)
    at com.puppycrawl.tools.checkstyle.TreeWalker.notifyVisit(TreeWalker.java:335)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processIter(TreeWalker.java:408)
    at com.puppycrawl.tools.checkstyle.TreeWalker.walk(TreeWalker.java:273)
    at com.puppycrawl.tools.checkstyle.TreeWalker.processFiltered(TreeWalker.java:154)
    at com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck.process(AbstractFileSetCheck.java:101)
    at com.puppycrawl.tools.checkstyle.Checker.processFile(Checker.java:340)
    at com.puppycrawl.tools.checkstyle.Checker.processFiles(Checker.java:299)
    at com.puppycrawl.tools.checkstyle.Checker.process(Checker.java:226)
    at com.puppycrawl.tools.checkstyle.Main.runCheckstyle(Main.java:415)
    at com.puppycrawl.tools.checkstyle.Main.runCli(Main.java:338)
    at com.puppycrawl.tools.checkstyle.Main.execute(Main.java:195)
    at com.puppycrawl.tools.checkstyle.Main.main(Main.java:130)
[Checker]
Audit done.
Checkstyle ends with 1 errors.

Expecting no exception.

Module code analysis at checkstyle/sonar-checkstyle#519 (comment)

[rnveach]

It should also be looked into why regression did not find this, and what we can do to ensure we try to find this in the future.

[Bananeweizen]

@rnveach I'm not sure if checkstyle uses either fuzzing or mutation testing in its build. Those techniques might find such or similar errors, but honestly I also don't have much practical experience with them (only used them in very small exploration projects).

[rnveach]

mutation testing in its build

We use pitest which is mutation testing, however, if anything it requires us to either to regress changes to find test cases or to remove code completely and wait for users to report issues. While it has its benefits, some of the maintainers don't see it as a good thing since we majority of the time just remove code it complains about. Some contributors can't do a deep dive on creating test cases because our tree is so complex and some checks are even more complex to understand what is needed.

[Lmh-java]

I am on this

[romani]

Do we have any signal from checker framework on this possibility of npe?

[rnveach]

@Bananeweizen If you look at e39e63e#diff-acd097abaf44b6edc9c81324f8c969963b793cbbc39487a470c6ead350f0dc67L528 which introduced this issue, you can see this code use to have a null check before it was merged. We have been using pitest for years, even before this. Even if we found some tool to report this as a possible issue, we would most likely ignore it since we would still need a test case to appease pitest.

[Lmh-java]

Do we have any signal from checker framework on this possibility of npe?

@romani, I didn't see any signal from checker-framework when fixing this bug. There is no corresponding checker suppressions either.

[romani]

I meant, maybe there is suppression already about this Check and around this code

[Bananeweizen]

@rnveach that kind of error would be almost impossible at my work. Most fields, return values and arguments are null annotated. The eclipse compiler would not compile the access with the missing null check then. Let me show you:
This is what I get by just adding the annotation library and enabling annotation based null analysis for the java project:

When I then try to "fix" that by declaring that variable as nonnull to make the dereference operation okay, the compiler rightfully complains about the return of getParent() (which I annoated as Nullable now. If I left away that annotation for the getParent(), the compiler would not recognize this error. This would be the most work for using that mechanism large scale, really annotating all the stuff that can be null).

So obviously the variable must be declared nullable instead, and then the compiler points right at this bug (again):

Annotating the getParent() method now causes 91 compiler errors in Eclipse for potential null pointer access.

I maintain a 7 digit Java code base at work, and null pointers in our own code happen like twice a year only. This demo was done using the Eclipse compiler and it's own null annotation library (org.eclipse.jdt.annotations). Using the checker framework library instead should work for 90% of the cases, however, the actual analysis is in the compiler, not in the library. Therefore I'm not sure how well that fits to your typical build and development tooling usage. I'm not sure if the checker framework alone can be used similarly.

[rnveach]

7 digit Java code base

Is this number of files or lines?

that kind of error would be almost impossible at my work

Until you add in pitest and have a requirement of no suppressions with pitest. These last 2 lead us to have only code we can prove we need. See #14788 (comment)

Most fields, return values and arguments are null annotated.

@nrmancuso @romani This method (getParent) is not annotated Nullable which we have started using in other places due to Checker.

Potentional null pointer access

I have seen this before. There have been times I enabled it and don't see how a null can naturally get in. I think we enabled this for CS eclipse analysis because of this. Again, going with how we maintain pitest, we would need proof before we actually could resolve such violation.

The mentality of this project is that if we can't prove the code is needed, we wait for the users to report it. So getting NPE like this (or other issues) would just be the natural outcome.

[romani]

One day in future we will focus on checker framework, to get more benefits.
But for now I see this problem as missed regression testing on specific config, I hope we will address it this summer.