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

Add check for Immutables reference equality #2210

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

pkoenig10
Copy link
Member

@pkoenig10 pkoenig10 commented Apr 19, 2022

The ReferenceEquality check does not apply when using an Immutables definition because the definition does not implement equals.

@changelog-app
Copy link

changelog-app bot commented Apr 19, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add ImmutablesReferenceEquality check that checks for comparison of Immutables values using reference equality.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -0,0 +1,49 @@
/*
* (c) Copyright 2020 Palantir Technologies Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* (c) Copyright 2020 Palantir Technologies Inc. All rights reserved.
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.

@@ -0,0 +1,42 @@
/*
* (c) Copyright 2020 Palantir Technologies Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* (c) Copyright 2020 Palantir Technologies Inc. All rights reserved.
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.

" interface Foo {}",
"}")
.doTest();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding a test case that shows the automatic fix? Should we apply suggested fixes by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we apply suggested fixes by default?

We should be consistent with the ReferenceEquality check. We don't currently apply that check by default, which is probably the right choice.

I would hope that there aren't (m)any existing places where we're using reference equality on immutables values, so I didn't apply this by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's a bit weird to test the automatic fixes here because the AbstractReferenceEquality check actually suggests two fixes.

[ImmutablesReferenceEquality] Comparison of Immutables value using reference equality instead of value equality.
  (see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)
Did you mean 'return Objects.equals(foo1, foo2);' or 'return foo1.equals(foo2);'?

When applying fixes, Error Prone will select the first one. In this case, that's Objects.equals(foo1, foo2); which probably isn't the one we want.

In tests you can specify a FixChooser to pick a specific fix, but using that feels a bit gross here.

FWIW the Error Prone ReferenceEquality tests don't test the automatic fixes, probably for this reason. I don't think it's that important to test the automatic fixes here, since the automatic fixes aren't part of baseline code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Objects.equals, but no strong opinion. Just don't want baseline upgrades to get stuck, but it's not likely many folks have this issue.

@bulldozer-bot bulldozer-bot bot merged commit 9d6f8b4 into develop Apr 20, 2022
@bulldozer-bot bulldozer-bot bot deleted the pkoenig10/immutablesReferenceEquality branch April 20, 2022 03:39
@svc-autorelease
Copy link
Collaborator

Released 4.109.0

This was referenced Apr 20, 2022
bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Apr 20, 2022
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 4.109.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Add `ImmutablesReferenceEquality` check that checks for comparison of Immutables values using reference equality. | palantir/gradle-baseline#2210 |



To enable or disable this check, please contact the maintainers of Excavator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants