New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] ImmutableField false positive with multiple constructors #1151

Closed
AustinShalit opened this Issue May 28, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@AustinShalit
Copy link
Contributor

AustinShalit commented May 28, 2018

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

This comment has been minimized.

Copy link
Member

jsotuyod commented May 28, 2018

@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

This comment has been minimized.

Copy link
Contributor

AustinShalit commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

AustinShalit commented May 28, 2018

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

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented May 28, 2018

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

We can change the description accordingly.

@jsotuyod jsotuyod added this to the 6.5.0 milestone May 28, 2018

@jsotuyod jsotuyod modified the milestones: 6.5.0, 6.6.0 Jun 26, 2018

@adangel adangel modified the milestones: 6.6.0, 6.7.0 Jul 29, 2018

@adangel adangel modified the milestones: 6.7.0, 6.8.0 Sep 1, 2018

@adangel adangel removed this from the 6.8.0 milestone Sep 29, 2018

@rmartinus

This comment has been minimized.

Copy link
Contributor

rmartinus commented Dec 2, 2018

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

@jsotuyod

This comment has been minimized.

Copy link
Member

jsotuyod commented Dec 2, 2018

@rmartinus please, feel free to do so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment