Skip to content
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

Avoid calling actual.hashCode() and expected.hashCode() in DualValue #3340

Closed
wants to merge 1 commit into from
Closed

Avoid calling actual.hashCode() and expected.hashCode() in DualValue #3340

wants to merge 1 commit into from

Conversation

davidboden
Copy link
Contributor

The hashCode of DualValue needs to match the object references of actual and expected, because that's what equals uses.

The current implementation caches the hashCode of both objects at construction, but this doesn't really match the requirement exactly.

The motivation for changing this is around avoiding calling hashCode unnecessarily on actual and expected objects, but I think it fixes potential bugs too. You could:

  • Create a DualValue.
  • Mutate a value in actual or expected that's present in the object's hashCode method.
  • Create another DualValue.

Both of the DualValues would be equal to each other according to equals() but would have different hashcodes.

We can continue to cache the hashCode if you prefer, but better to generate it based on the object references using System.identityHashCode.

Check List:

  • Fixes #??? (ignore if not applicable)
  • Unit tests : YES
  • Javadoc with a code example (on API only) : NA
  • PR meets the contributing guidelines

Following the contributing guidelines will make it easier for us to review and accept your PR.

@joel-costigliola
Copy link
Member

thanks for catching this up @davidboden, I commented the PR.

@davidboden
Copy link
Contributor Author

Thanks. All sounds good; I'll fix it up over the weekend.

The hashCode of `DualValue` needs to match the object references of actual and expected, because that's what `equals` uses.

The current implementation caches the hashCode of both objects at construction, but this doesn't really match the requirement exactly.

The motivation for changing this is around avoiding calling hashCode unnecessarily on actual and expected objects, but I think it fixes potential bugs too. You could:
* Create a DualValue.
* Mutate a value in actual or expected that's present in the object's hashCode method.
* Create another DualValue.

Both of the DualValues would be equal to each other according to equals() but would have different hashcodes.

We can continue to cache the hashCode if you prefer, but better to generate it based on the object references using `System.identityHashCode`.
@joel-costigliola
Copy link
Member

joel-costigliola commented Jan 21, 2024

I took a deeper look at the code and if we compute the hash code at construction time, I don't see a way that it could inconsistent with equals because:

  1. equals compares actual and expected fields reference
  2. we compute the hash code in the constructor
  3. as actual and expected are final, their references never change, thus even if their inner actual and expected are mutated the equals behavior stays the same and thus is still consistent with the computed hash code at construction time.

If we compute the hash code every time hashCode is called, then my last point does not hold as your test showed but your test succeeds if you compute the hash code only once at construction time.

Having said that, I think this PR is still relevant because the hash code computed based on actual and expected reference/identity is consistent with the equals implementation.

I'm happy to be proven wrong, so don't hesitate if you find a test case invalidating my reasoning.

@joel-costigliola
Copy link
Member

I'm taking over the PR, but you will be the commit author so full credit to you @davidboden

@joel-costigliola
Copy link
Member

Thanks @davidboden for your work, this is integrated..

@scordio scordio added the theme: recursive comparison An issue related to the recursive comparison label Jan 24, 2024
@scordio scordio added this to the 3.25.2 milestone Jan 24, 2024
@scordio scordio changed the title DualValue avoid calling actual.hashCode() and expected.hashCode() Avoid calling actual.hashCode() and expected.hashCode() in DualValue Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: recursive comparison An issue related to the recursive comparison
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants