8290973: In AffineTransform, equals(Object) is inconsistent with hashCode()

[desruisseaux]

AffineTransform.equals(Object) and hashCode() break two contracts:

• A.equals(A) returns false if at least one affine transform coefficient is NaN.
• A.equals(B) should imply A.hashCode() == B.hashCode(), but it is not the case if a coefficient is zero with an opposite sign in A and B.

This patch preserves the current behaviour regarding 0 (i.e. -0 is considered equal to +0) for backward compatibility reason. Instead the hashCode() method is updated for being consistent with equals(Object) behaviour.

---

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

Issue

• JDK-8290973: In AffineTransform, equals(Object) is inconsistent with hashCode()

Reviewers

• Phil Race (@prrace - Reviewer) ⚠️ Review applies to 12abfde7

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9121

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

Using diff file

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

[bridgekeeper]

👋 Welcome back desruisseaux! 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]

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

• client

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.

[bridgekeeper]

@desruisseaux 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!

[desruisseaux]

Hello, this is a reminder for a pull request: in the AffineTransform class, hashCode() is inconsistent with equals(Object). The problem occurs with NaN and ±0 coefficients. In particular, the problem with zero values prevents the use of AffineTransorm as keys in HashMap, unless all zero values are forced to the same sign by AffineTransform construction or by pre-processing before using an AffineTransform instance as a key.

[mlbridge]

Webrevs

• 01: Full - Incremental (8ee3f7b8)
• 00: Full (12abfde7)

[mlbridge]

Mailing list message from Philip Race on client-libs-dev:

Hardly a "reminder". The PR is just 10 minutes old. Anything before the
rfr label was added does not count.

-phil.

On 7/25/22 10:38 AM, Martin Desruisseaux wrote:

[mlbridge]

Mailing list message from Martin Desruisseaux on client-libs-dev:

Hello Phil

Le 25/07/2022 ? 19:44, Philip Race a ?crit?:

Hardly a "reminder". The PR is just 10 minutes old. Anything before
the rfr label was added does not count.

Sorry for that noise, but I didn't sent that message today. That message
was written 14 days ago on GitHub, in reaction to the bot telling me
that the issue was inactive for 4 weeks and was going to be closed if I
didn't wrote something. The two last messages from "me" were actually
sent automatically by some bot, presumably the same one that added the
"rfr" label.

Have you read : https://openjdk.org/guide/#fixing-a-bug ?

I was not aware of that page. The last time that I contributed, I have
been told to read https://openjdk.org/contribute/.

??? Thanks,

??? ??? Martin

[prrace]

I've run all tests we have and not too surprised they pass since the affected values
probably aren't being exercised.

However I've been reading the docs about NaN and +/-0 equivalence at
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Double.html
since if we change anything it probably ought to align with what ever says there.

Since I recalled that NaN == NaN will always return false, it seemed what you have
was not consistent with that. Whilst this is true it seems Double.equals() does the opposite .. so OK ..
although it surprised me.

But then the docs say +0 and -0 should not be equal whereas for backwards compatibility
you say you extended them being treated as equal from equals() into hashCode() ..

Not sure why only in this case was the compatibility important.

I'd like to hear what @jddarcy (Joe Darcy) thinks about all of this.

[desruisseaux]

Indeed, in this proposed patch +0 is considered equal to -0 mainly for preserving current behaviour. The proposed new AffineTransform.equals(Object) has a behaviour different than the old one only regarding NaN, which should be rare in AffineTransform. So for practical purposes; the equals method would be basically unchanged in this patch proposal.

But there is also an additional reason. It is because two AffineTransform instances with the same coefficients except for the sign of zero will compute the same points when a transform(…) method is invoked (except when a result is zero, in which case the sign of that 0 may be different in some cases). So those two transforms can be considered as equal for the purpose of transforming points. There is a possibility that some existing codes rely on this equality (which is not mathematically wrong I think), especially since negative zeros appear easily in AffineTransform. Consider for example a translation on only one axis (line break added for clarity):

AffineTransform[[1.0, 0.0, 2.0],
                [0.0, 1.0, 0.0]]

A call to AffineTransform.createInverse() gives:

AffineTransform[[1.0, 0.0, -2.0],
                [0.0, 1.0, -0.0]]

Both translation terms become negative (as expected), including zero. But transforming points with that AffineTransform will give results strictly identical (except for the sign of some 0 coordinate values) to an AffineTransform created by getTranslateInstance(-2, 0), so the current behaviour of considering them as equal may not be wrong.

[AJ1062910]

Have you checked all tests ? I did not see it

[jddarcy]

Another idiom to accomplish this is

return Double.doubleToLongBits(m + 0.0); // Turn -0.0 to +0.0

[jddarcy]

I've run all tests we have and not too surprised they pass since the affected values probably aren't being exercised.

However I've been reading the docs about NaN and +/-0 equivalence at https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Double.html since if we change anything it probably ought to align with what ever says there.

Since I recalled that NaN == NaN will always return false, it seemed what you have was not consistent with that. Whilst this is true it seems Double.equals() does the opposite .. so OK .. although it surprised me.

But then the docs say +0 and -0 should not be equal whereas for backwards compatibility you say you extended them being treated as equal from equals() into hashCode() ..

Not sure why only in this case was the compatibility important.

I'd like to hear what @jddarcy (Joe Darcy) thinks about all of this.

For the equals contract, members of an equivalence class should all be equal to each other, meaning they are substitutable for each other in some sense. Different equivalence classes can be defined for the same set of values, one notable example being == (object identity) versus calling .equals().

From my reading of AffineTransform, it should be an acceptable substitution to use -0.0 rather +0.0. As noted a NaN value is "the same" as another NaN in the sense being tested in this method.

One way to express this would be to define

private static boolean equiv(double a, double b) {
return Double.compare(a + 0.0, b + 0.0);
}

Adding 0.0 turn -0.0 into +0.0 and has no other effect. Double.compare treats NaN inputs as the same. This could then do pair-wise comparison of the various components,

return equiv(m00, a.m00) && equiv(m01, a.m01) && ...

If this is the equals function, than the hash can be a combination of

Double.doubleToLongBits(m01 +0.0) ...

using adding 0.0 adding to remove the case of -0.0. As is already being done, using doubleToLongBits instead of doubleToRawLongBits is important so that all NaN values get mapped the same.

A different equals and hashCode could be constructed if all 90 degree rotations (of the same length) were considered the same, but that would require more work to construct.

HTH

[openjdk]

@desruisseaux This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290973: In AffineTransform, equals(Object) is inconsistent with hashCode()

Reviewed-by: prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 877 new commits pushed to the master branch:

• aa9b8f0: 8292043: Incorrect decoding near EOF for stateful decoders like UTF-16
• f95ee79: 8292566: Add reference to the java.nio.file package in java.nio package documentation
• 45c3e89: 8292316: Tests should not rely on specific JAR file names (jpackage)
• db77227: 8282684: Obsolete UseContainerCpuShares and PreferContainerQuotaForCPUCount flags
• 256b523: 8292381: java/net/httpclient/SpecialHeadersTest.java fails with "ERROR: Shutting down connection: HTTP/2 client stopped"
• e561933: 8292623: Reduce runtime of java.io microbenchmarks
• dcd7802: 8292708: Rename G1ParScanThreadState::flush to flush_stats
• 16593cf: 8292717: Clean up checking of testing requirements in configure
• c59f9b3: 8287828: Fix so that one can select jtreg test case by ID from make
• 476c484: 8292656: G1: Remove G1HotCardCache::_use_cache
• ... and 867 more: https://git.openjdk.org/jdk/compare/0901548833a0125f15fede64bc2e7dbe84fed42d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[desruisseaux]

Should I fix this trivial typo before the merge? ("into to" → "to").

[desruisseaux]

/integrate

[openjdk]

@desruisseaux
Your change (at version 8ee3f7b) is now ready to be sponsored by a Committer.

[bridgekeeper]

@desruisseaux 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!

[desruisseaux]

A ping for preventing this merge request to be automatically closed. This MR has been approved in August 11 and is ready for integration by a committer (if no more comment).

[jayathirthrao]

/sponsor

[openjdk]

Going to push as commit 5c030cc.
Since your change was applied there have been 1423 commits pushed to the master branch:

• f888aa9: 8293061: Combine CDSOptions and AppCDSOptions test utility classes
• 73f0646: 8294839: Disable StressLongCountedLoop in compiler/loopopts/TestRemoveEmptyLoop.java
• 2ceebf6: 8294456: Fix misleading-indentation warnings in core JDK libraries
• ad7b7d4: 8294697: java/lang/Thread/virtual/ThreadAPI.testGetStackTrace2 failed with non-empty stack trace
• e38ae8a: 8294759: Print actual lock/monitor ranking
• 7012d4b: 8294837: unify Windows 2019 version check in os_windows and java_props_md
• 8c15f77: 8270915: GIFImageReader disregards ignoreMetadata flag which causes memory exhaustion
• 6029120: 8278086: [REDO] ImageIO.write() method will throw IndexOutOfBoundsException
• 8f56115: 8294679: RISC-V: Misc crash dump improvements
• e986a97: 8292330: Update JCov version to 3.0.13
• ... and 1413 more: https://git.openjdk.org/jdk/compare/0901548833a0125f15fede64bc2e7dbe84fed42d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jayathirthrao @desruisseaux Pushed as commit 5c030cc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.