[java] ImmutableField false positive with multiple constructors

[AustinShalit]

Affects PMD Version: 6.3.0 and before

Rule: ImmutableField

Description:
The ImmutableField rule can detect false positives when you have multiple constructors that call this() on each other.

Code Sample demonstrating the false positive:

public class AnalogAccelerometer {
  private AnalogInput m_analogChannel;
  private boolean m_allocatedChannel; // Violation

  public AnalogAccelerometer(final int channel) {
    this(new AnalogInput(channel));
    m_allocatedChannel = true;
  }

  public AnalogAccelerometer(final AnalogInput channel) {
    m_allocatedChannel = false;
    m_analogChannel = channel;
  }
}

Code Sample demonstrating the issue if I listened to PMD's recommendation:

public class AnalogAccelerometer {
  private AnalogInput m_analogChannel;
  private final boolean m_allocatedChannel;

  public AnalogAccelerometer(final int channel) {
    this(new AnalogInput(channel));
    m_allocatedChannel = true; // error: variable m_allocatedChannel might already have been assigned
  }

  public AnalogAccelerometer(final AnalogInput channel) {
    m_allocatedChannel = false;
    m_analogChannel = channel;
  }
}

[jsotuyod]

@AustinShalit that's an interesting scenario...

Although it's technically correct that simply adding final would be illegal, it still seems the rule is right, and m_allocatedChannel is indeed intended to be immutable... This could be accomplished either by repeating the m_analogChannel assignment instead of calling this() or by using a third (private) constructor:

public class AnalogAccelerometer {
  private AnalogInput m_analogChannel;
  private final boolean m_allocatedChannel;

  public AnalogAccelerometer(final int channel) {
    this(new AnalogInput(channel), true);
  }

  public AnalogAccelerometer(final AnalogInput channel) {
    this(channel, false);
  }

  private  AnalogAccelerometer(final AnalogInput channel, final boolean allocatedChannel) {
    m_allocatedChannel = allocatedChannel;
    m_analogChannel = channel;
  }
}

After all, the rule states:

Identifies private fields whose values never change once they are initialized either in the declaration of the field or by a constructor. This helps in converting existing classes to becoming immutable ones.

Which holds true.

Why do you consider this a false positive?

[AustinShalit]

Your solution is what I went with to solve this violation.

I need to disagree that the rule description does not hold true. Not to lawyer the description but "never change once they are initialized" is clearly broken in the example I gave.

[AustinShalit]

If this is actually a false positive, I could see changing the description to match the implementation as a solution.

[jsotuyod]

@AustinShalit I'll rephrase that: "once object initialization ends"

We can change the description accordingly.

[rmartinus]

Hey @jsotuyod I'm happy to rephrase the documentation from "once they are initialized" to "once object initialization ends" as my first issue commit.