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

EqualsHashCode: hashCode without equals is not a violation #3308

Closed
romani opened this Issue Jun 25, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Jun 25, 2016

Created: 2005-07-29
Creator: Christian Spreuer

http://checkstyle.sourceforge.net/config_coding.html#EqualsHashCode

EqualsHashCodeCheck checks that if equals is
overridden (either correctly with an Object parameter or
wrongly with some other Class), hashCode should also
be overridden.

But it does not check the other way round, that if
hashCode is overridden also equals is overridden.

Strictly speaking, this is harmless most times, but
usually it indicates that the programmer did make an
error.
Most programming rules I know strongly associate both
methods - either overload none or both. E.g.
http://gee.cs.oswego.edu/dl/html/javaCodingStd.html#secRec

I thought there was also a test that if equals is
overridden it has the right signature, but I do not find it
right now (this sentence could be a feature request).

/var/tmp $ cat TestClass.java 
public class InputEqualsHashCode {
    public int hashCode() {
        return 1;
    }

}
/var/tmp $ cat my_check.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name = "Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="EqualsHashCode"/>
    </module>
</module>

/var/tmp $ java -jar checkstyle-7.0-SNAPSHOT-all.jar -c my_check.xml TestClass.java 
Starting audit...
Audit done.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 5, 2016

Member

issue is fixed partly, not fixed case is at #3387 (comment)

Member

romani commented Aug 5, 2016

issue is fixed partly, not fixed case is at #3387 (comment)

@romani romani added the bug label Aug 5, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 5, 2016

Member

issue is fixed partly
Correct signatures for overriding: boolean equals(Object obj);

So what is the behavior we want if the code doesn't have the correct signature just for the parameter for equals, like they have String or the class instead of Object?

public class TestClass1 {
    public boolean equals(String o) { // violation?
        return true;
    }
}
public class TestClass2 {
    public int hashCode() { // will be a violation because no `boolean equals(Object o)`
        return 1;
    }
    public boolean equals(String o) { // violation?
        return true;
    }
}
public class TestClass3 {
    public int hashCode() {
        return 1;
    }
    public boolean equals(Object o) {
        return true;
    }
    public boolean equals(String o) { // no violation or still warn?
        return false;
    }
}

If there isn't a violation for this check, then users may not know they are using the wrong signature. When I first started using checkstyle, this check was the one that alerted me to this issue as I didn't know about it. I used the class as the parameter to equals without any hashCode method. But this is just for good practice, its not an actual issue unless something is calling boolean equals(Object o) expecting the result of boolean equals(XXX o).

I think we should have a new message just for the improper parameter of the equals.
Ex: Definition of 'equals()' must have 'Object' as its first and only parameter.

Member

rnveach commented Aug 5, 2016

issue is fixed partly
Correct signatures for overriding: boolean equals(Object obj);

So what is the behavior we want if the code doesn't have the correct signature just for the parameter for equals, like they have String or the class instead of Object?

public class TestClass1 {
    public boolean equals(String o) { // violation?
        return true;
    }
}
public class TestClass2 {
    public int hashCode() { // will be a violation because no `boolean equals(Object o)`
        return 1;
    }
    public boolean equals(String o) { // violation?
        return true;
    }
}
public class TestClass3 {
    public int hashCode() {
        return 1;
    }
    public boolean equals(Object o) {
        return true;
    }
    public boolean equals(String o) { // no violation or still warn?
        return false;
    }
}

If there isn't a violation for this check, then users may not know they are using the wrong signature. When I first started using checkstyle, this check was the one that alerted me to this issue as I didn't know about it. I used the class as the parameter to equals without any hashCode method. But this is just for good practice, its not an actual issue unless something is calling boolean equals(Object o) expecting the result of boolean equals(XXX o).

I think we should have a new message just for the improper parameter of the equals.
Ex: Definition of 'equals()' must have 'Object' as its first and only parameter.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2016

Member

@rnveach,

TestCase1 - TestCase1 should have NO violation , as it has no override of equals method
TestCase2 - violation is expected
TestCase3 - no violation as hashcode-equals contract is satisfied and Check should not care about other methods even they are looks similar.

I do not think that we need new message, we might need to update existing and clearly show what signatures we expect.

Member

romani commented Aug 7, 2016

@rnveach,

TestCase1 - TestCase1 should have NO violation , as it has no override of equals method
TestCase2 - violation is expected
TestCase3 - no violation as hashcode-equals contract is satisfied and Check should not care about other methods even they are looks similar.

I do not think that we need new message, we might need to update existing and clearly show what signatures we expect.

rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 8, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 16, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 16, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 28, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 28, 2016

rnveach added a commit to rnveach/checkstyle that referenced this issue Sep 29, 2016

romani added a commit that referenced this issue Sep 29, 2016

@romani romani added this to the 7.2 milestone Sep 29, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 30, 2016

Member

fix is merged

Member

romani commented Sep 30, 2016

fix is merged

@romani romani closed this Sep 30, 2016

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