Add check for Immutables reference equality #2210
Conversation
Generate changelog in
|
| @@ -0,0 +1,49 @@ | |||
| /* | |||
| * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. | |||
There was a problem hiding this comment.
| * (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. | |||
There was a problem hiding this comment.
| * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. | |
| * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. |
| " interface Foo {}", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
Mind adding a test case that shows the automatic fix? Should we apply suggested fixes by default?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Released 4.109.0 |
###### _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.
The
ReferenceEqualitycheck does not apply when using an Immutables definition because the definition does not implement equals.