8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode}

[jddarcy]

First pass at adding some quality of implementation discussions around the overridable methods of Object.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8336046 to be approved

Issues

• JDK-8336043: Add quality of implementation discussion to Object.{equals, toString, hashCode} (Enhancement - P4)
• JDK-8336046: Add quality of implementation discussion to Object.{equals, toString, hashCode} (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20128/head:pull/20128
$ git checkout pull/20128

Update a local copy of the PR:
$ git checkout pull/20128
$ git pull https://git.openjdk.org/jdk.git pull/20128/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20128

View PR using the GUI difftool:
$ git pr show -t 20128

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20128.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back darcy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@jddarcy The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[liach]

"may not support generating hash codes" sounds weird; maybe like "may or may not guard against cyclic data structures in recursive hash code generation."

I think the key is "recursive" here, as only recursive implementations are at risk of these faults.

[jddarcy]

Well, recursion is the easiest way to accidentally write an infinite loop, but I think a few other ones would be possible too ;-) I'll considering updating the wording to highlight the most likely hazards; thanks.

[stuart-marks]

Unfortunately the typical implementation pattern is to recur through the component fields of the object. Not recursion through this but rather the hashCode implementation of some object will typically invoke hashCode of its component fields. Of course this can result in infinite recursion (really, StackOverflowError) if the object graph contains cycles, but in general, implementations of these methods don't guard against this possibility. (Nor should they. Adding such guard code seems impractical.)

Note that the default implementations of these methods for records and for Collection classes doesn't guard against cycles. (The collections contain some special cases for collection that contains itself, but not cycles in general.)

I have a general concern about advice that isn't actionable, such as "may not support" or "care should be taken" etc. If we really want to say something about cyclic data structures, maybe we could say something like this. For data structures that may contain cycles, these implementations should avoid traversing component fields that may lead to cycles. A class could simply inherit the method implementations from Object. Alternatively, the class could contain a unique ID that's used for determining the result of equals/hashCode/toString. (Of course, determining uniqueness is up to the application.)

[mlbridge]

Webrevs

• 04: Full - Incremental (840ee4f2)
• 03: Full - Incremental (80cabee0)
• 02: Full - Incremental (95f890bf)
• 01: Full - Incremental (81245907)
• 00: Full (150713fe)

[jddarcy]

A note on terminology, I used "typical" for the new notes rather than "normal" to avoid conflating the discussion with the JLS concept of code "completing normally" (as opposed to completing abruptly due to an exception):

https://docs.oracle.com/javase/specs/jls/se22/html/jls-14.html#jls-14.1

[stuart-marks]

While it's not wrong, advising against "excessive" usage is so vague as not to be actionable. Given some piece of code it's hard to say whether or not it does anything excessive. Consider a List with a million elements; as a practical matter, any one of these methods requires visiting every element. Is that excessive? Why or why not?

[jddarcy]

While it's not wrong, advising against "excessive" usage is so vague as not to be actionable. Given some piece of code it's hard to say whether or not it does anything excessive. Consider a List with a million elements; as a practical matter, any one of these methods requires visiting every element. Is that excessive? Why or why not?

Hmm. In rare cases, I think providing not-quite-actionable advice is on net helpful.

My basic goal here is to convey "when you're implementing these methods, be considerate to your expected users." If that results in some extra contemplation during development or code review about whether or not the worst-case behavior of hashCode/toString/equals is excessive, I think that would be a reasonable outcome from this change.

[stuart-marks]

"Should not throw any exception or other throwable" is overly broad. However, there is a narrower sense where code that implements these methods "shouldn't" throw anything. I'd suggest focusing on precondition checking. Specifically, no object should ever be in a state such that calling one of these methods results in IllegalStateException or other exception based on the state of the object. In addition, no argument passed to equals() should ever cause IllegalArgumentException, ClassCastException, NullPointerException, or other exception based on the argument.

(This comment applies to other locations where the "excessive" wording is used.)

[kevinb9n]

I would hope to spend as little space on this as possible, perhaps "This method should avoid throwing or propagating any exceptions unless it legitimately cannot adhere to this contract."
(or shorter)

[jddarcy]

I would hope to spend as little space on this as possible, perhaps "This method should avoid throwing or propagating any exceptions unless it legitimately cannot adhere to this contract." (or shorter)

Pushed a version using that worked, but I expect discussion will continue.

[stuart-marks]

For these cases the recommendation should be to return false instead of throwing such exceptions.

[jddarcy]

Good point; updated accordingly.

[stuart-marks]

@kevinb9n You should take a look at this.

[rgiulietti]

Here's a classical example of a directed acyclic graph (DAG, no cycles) consisting of 100 ArrayLists, each of size 2.
It takes some 1000x the age of the universe to compute hashCode() or to compare two such DAGs with equals(), so no machine will ever come to completion.
And toString() throws an OOME because the results wouldn't fit in the limits of String.

This is to say that even without cycles and even with quite small data structures as here, we might encounter excessive resource usages in space or time by just invoking these methods.
(The same would hold by replacing ArrayList with a record of 2 components that does not override the default methods.)

import java.util.ArrayList;

public class DAG {

    private static final int DEPTH = 100;

    public static void main(String[] args) {
        ArrayList<Object> dag0 = createSmallDAG();
        ArrayList<Object> dag1 = createSmallDAG();
        System.out.println(dag0.equals(dag1));
        System.out.println(dag0.hashCode());
        System.out.println(dag0);
    }

    private static ArrayList<Object> createSmallDAG() {
        ArrayList<Object> n = growDAG(null);
        for (int i = 1; i < DEPTH; ++i) {
            n = growDAG(n);
        }
        return n;
    }

    private static ArrayList<Object> growDAG(ArrayList<Object> n) {
        ArrayList<Object> m = new ArrayList<>();
        m.add(n);
        m.add(n);
        return m;
    }

}

[stuart-marks]

Bloch's Effective Java, 3/e, Items 10, 11, and 12 have some useful background information on implementing equals, hashCode, and toString.

[jddarcy]

Bloch's Effective Java, 3/e, Items 10, 11, and 12 have some useful background information on implementing equals, hashCode, and toString.

Agreed; I reread over those items before posting the PR.

The "typical" wording was meant to cover guidance like Bloch's "have equals throw an AssertionError if the method should never be called if it escapes its intended domain."

[RogerRiggs]

"As as" typo.
"particular" is unnecessary. "a" -> "an"

[bridgekeeper]

@jddarcy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jddarcy]

Keep alive.

[bridgekeeper]

@jddarcy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jddarcy]

Keep alive again.

[liach]

Is this rendered as code font? I think only <-starting text is not rendered as code font.