[Java:S1206] Relax rule to accept super hashCode() implementations

Rules and Languages Report False-positive / False-negative...
1

This rule checks, if a class has either none or both of hashCode() and equals() implementations. I don’t think, this is always correct.

If a class overrides hashCode(), yes it should definitely override equals(), too.
But if it overrides equals() and a super class, that is not Object, already implements hashCode(), and the class itself deson’t, than the rule could accept this, because it is very likely, that the super hashCode() is sufficient, because hashCode() doesn’t need to be that specific.

Maybe there should be a configuration option to this rule. But the way it is now, it leads to many false positives, maybe even more than possible bugs.

2

Hi,

What language are we talking about?

 
Ann

3

Hi Ann,

it’s all Java.

Cheers
Marvin

5

Hi Marvin,

Welcome to the community and thanks a lot for your request.

According to Java Language Specification there is a contract between methods equals() and hashCode():

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results.
However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hashtables.

This means that all the situations where this contract can potentially be broken whould be marked as non-compliant.

Let’s consider this example:

class A {
   private int a;
  
  A(int a) {
    this.a = a;
  }

  @Override
  public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    A a1 = (A) o;
    return a == a1.a;
  }

  @Override
  public int hashCode() {
    return Objects.hash(a);
  }
}

class B extends A {
  private String b;

  B(String b, int a) {
    super(a);
    this.b = b;
  }

  @Override
  public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    B b1 = (B) o;
    return b == b1.b;
  }
  
  public static void main(String[] args) {
    B obj1 = new B("3", 4);
    B obj2 = new B("3", 2);

    System.out.println(obj1.equals(obj2));
    System.out.println(obj1.hashCode() == obj2.hashCode());
  }
}

You can see that we can override method equals() which will break the contract. This could be on purpose or by mistake in both cases it’s very important to override hashCode() too.

The purpose of static code analysis is to signal on possible bugs, so I truly believe that this rule is correct.

To sum up, you can still disagree with me and share your arguments or disable the rule.

And thanks a lot for your feedback, it made us consider different points of view. So feel free to share your thoughts and together we can make analyzers and code quality better.

7

Hi Margerita,

thanks for your detailed response. Well, actually I do disagree, well partly. :wink:

I agree, that the rule should at least have a mode, where all potential contract breakers are reported and this mode should be default. But I have strong rejections against having no other option, but to either “fix” correct code by adding nonsense code or NOSONARs just to make the rule quiet or to disable the rule completely.

I would consider your example a bit artificial. Ok, it stands for other actual bugs, that could cause misbehavior like that. But a much more common example would be this:

class A {
   private int id;
  
  A(int id) {
    this.id = id;
  }

  @Override
  public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof A)) return false;
    A that = (A) o;
    if (this.id != that.id) return false;
    return true;
  }

  @Override
  public int hashCode() {
    return Integer.hashCode(id);
  }
}

class B extends A {
  private String label;

  B(int a, String label) {
    super(a);
    this.label = label;
  }

  @Override
  public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof B)) return false;
    if (!super.equals(o)) return false;
    B that = (B) o;
    if (Objects.equals(this.label, that.label)) return false;
    return true;
  }
}

This code does in no way break the contract. B adds ‘label’ to the list of members. But ‘id’ is still sufficient to build a good hashcode. And this applies to many classes, where it is actually wrong to include more info in the hashcode. We had to realize this when implementing our object based locking.

So, to react on this rule we would have had to add this to all affected classes:

@Override
public int hashCode() {
   return super.hashCode();
}

In the end I disabled the rule, which is a pity. I would love this rule do do it’s job and report actual bugs. But in our case we would need a mode to be configured, that classes don’t match, that have an ancestor class, that is not Object and that implements hashCode().

I think, this would be handy and imprtant to many code bases and the wording of the contract actually implies it.

8

Hello Marvin, thanks for your detailed explanation. I understand what you mean and your words does make sense for me.

On the other hand, if you don’t implement hashCode explicitly, but rely on super.hashcode(), there might be issues. First of all, there is always a way that hashcode implementation will change in superclass, especially if it is not under your maintenance. And you won’t notice this.

Another thing, there could be bugs, when you try to rely on the super.hashcode(), which itself rely on the super.hashcode(), and finally they all are using Object.hashcode() which actually breaks the contract.

In my opinion, there are definitely cases when implementation of hashcode() from superclass is acceptable, but I think this should be done explicitly, so every developer will understand what’s going on. These 4 primitive lines of code can save you from many bugs and maintenance problems in the future.

To sum up, I understand your pain, but I truly believe, that having this rule parametrised will bring more complexity and misunderstanding than actual benefit.

And again thanks for your feedback. I might not absolutely agree with you, but you made us think about possible improvements, which is highly important.

Regards,
Margarita

9

I see. Well, the rule won’t hit, if there is a hashCode() implementation in the same class, that relies on super.hashCode(), will it? And not, if it does nothing more than return super.hashCode(). If the super implementation changes, the rule won’t notify (you). And atm. I have to add this super-calling implementtion to shut up the rule. Al in all nothing better than improving the rule.

Well, do you really suggest to always fully implement hashCode() in every sub class? This should be a code smell due to the code duplication rule, shouldn’t it?

Well, I think, this really goes into direction of taste or style or company rules or what ever. I really believe, this should be configurable.

I understand, that you made a decision and stick with it. This is ok. It is your project in the end. How could I not respect that? Keep up the good work! Thanks.