[java] SingularField false positive with switch in method that both assigns and reads field

[theneva]

Affects PMD Version: 5.5.1...6.55.0

Rule: SingularField

Description:

I have the following JAX-RS event listener that uses a Prometheus Summary to measure the time of all non-404 responses. It violates the java/design/SingularField rule.

Code Sample demonstrating the issue:

// package, imports

@Provider
public class LatencyRequestEventListener implements RequestEventListener {
    private static final Summary SUMMARY_LATENCY_SECONDS = …;

    private final ResourceInfo resourceInfo;

    private Summary.Timer timer; // BREAKS HERE

    LatencyRequestEventListener(final ResourceInfo resourceInfo) {
        this.resourceInfo = resourceInfo;
    }

    @Override
    public void onEvent(final RequestEvent event) {
        switch (event.getType()) {
            case RESOURCE_METHOD_START:
                final ResourceMethod matchedResourceMethod = event.getUriInfo().getMatchedResourceMethod();

                if (matchedResourceMethod == null) {
                    return;
                }

                timer = SUMMARY_LATENCY_SECONDS // ASSIGN FIELD
                    .labels(
                        matchedResourceMethod.getHttpMethod()
                        , resourceInfo.getResourceClass().getSimpleName() + "." + resourceInfo.getResourceMethod().getName()
                    )
                    .startTimer();
                break; // Don't keep going (both cases cannot be reached in the same call)
            case FINISHED:
                if (timer != null) {
                    timer.observeDuration(); // READ FIELD
                }
        }
    }
}

Expected outcome:

PMD reports a violation at line 25, but that's wrong. That's a false positive.

This results in the following error:

/path/to/project/…/listeners/LatencyRequestEventListener.java:25: Perhaps 'timer' could be replaced by a local variable.

timer can't be a local variable, so I don't think it's correct to report this rule violated.

I understand that the rule is violated if a field is only accessed by a single method. I also acknowledge that having the same method be called twice to first assign a field (the RESOURCE_METHOD_START case) and then read the field (FINISHED) may not be a good design. However, that's how the JAX-RS people designed the RequestEventListener class.

If I pull the code for the RESOURCE_METHOD_START case into a separate method, it passes with no problem since multiple methods access the field.

Is this a bug, or am I doing something wrong?

EDIT: As a funky side note, pulling the configuration of the Timer into a method makes the code legal, despite still only referencing the timer field in onEvent(…):

private static Summary.Child configureTimer(
    final Summary summary,
    final String httpMethod
    , final String handlerName
) {
    return summary
        .labels(httpMethod, handlerName);
}

@Override
public void onEvent(final RequestEvent event) {
    switch (event.getType()) {
        case RESOURCE_METHOD_START:
            final ResourceMethod matchedResourceMethod = event.getUriInfo().getMatchedResourceMethod();
            if (matchedResourceMethod != null) {
                timer = configureTimer(
                    SUMMARY_LATENCY_SECONDS,
                    matchedResourceMethod.getHttpMethod()
                    , resourceInfo.getResourceClass().getSimpleName() + "." + resourceInfo.getResourceMethod().getName()
                ).startTimer();
            }
            break;
        case FINISHED:
            if (timer != null) {
                timer.observeDuration();
            }
    }
}

[jsotuyod]

Hi @theneva and thanks for your report!

You are absolutely right, this is a false positive, mostly down to the fact PMD can't tell how calls will be performed in time.

However, I believe we could improve this scenario by checking, not only that the field is only being written / read on a single method, but is actually being written in all branches before being read (and therefore not relying on previously set values from other calls). This however, would probably need to rely on our Data Flow Analysis framework, which is in serious need of a revamp.

This is definitely in our backlog, but will take some time to get fixed. In the meantime you can either suppress the warning locally and document it as a false positive with a comment even linking back to this issue if you would like so.

[theneva]

Hey @jsotuyod, thanks for the reply! I just realised I was supposed to file the bug report on SourceForge, so I did that: https://sourceforge.net/p/pmd/bugs/1558/

I'll close this issue here. Thanks again :)

[jsotuyod]

@theneva don't worry, we have decided to move away from SourceForge issues and into GitHub. We will eventually move all issues here anyway.

[theneva]

Oh, cool! I'll leave it open in both places then.

[adangel]

This has been fixed in PMD 7.