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

EqualsAvoidsNull wrongly inverts a.compareTo(b) #442

Closed
Bananeweizen opened this issue Jan 13, 2025 · 2 comments
Closed

EqualsAvoidsNull wrongly inverts a.compareTo(b) #442

Bananeweizen opened this issue Jan 13, 2025 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@Bananeweizen
Copy link
Contributor

What is the smallest, simplest way to reproduce the problem?

EqualsAvoidsNull is intended to only improve comparisons with null. However, it unconditionally inverts a.compareTo(b) even for non-null arguments. That's completely wrong (because compareTo() implements an ordered comparison), but unfortunately the refactored code still compiles.

What did you expect to see?

Existing unit tests are wrong and would uncover this issue, if the expectation is fixed. See for example

System.out.println(s.compareTo("test"));
System.out.println(s.compareToIgnoreCase("test"));
System.out.println(s.contentEquals("test"));
}
}
""",
"""
public class A {
{
String s = null;
if("test".equals(s)) {}
if("test".equalsIgnoreCase(s)) {}
System.out.println("test".compareTo(s));
System.out.println("test".compareToIgnoreCase(s));
.
s.compareTo("test") is not the same as "test".compareTo(s).

Are you interested in contributing a fix to OpenRewrite?

I would just remove the 2 compareTo* matchers, not sure if that is the best fix.

@Bananeweizen Bananeweizen added the bug Something isn't working label Jan 13, 2025
@timtebeek
Copy link
Contributor

timtebeek commented Jan 13, 2025

Hmm interesting; this was first introduced by @punkratz312 I think in

Perhaps these cases should have been split off into a separate issue so as to not confuse the purpose of EqualsAvoidsNull, nor invert the comparison accidentally.

@timtebeek timtebeek moved this to Backlog in OpenRewrite Jan 13, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenRewrite Jan 13, 2025
@timtebeek timtebeek self-assigned this Jan 13, 2025
@timtebeek
Copy link
Contributor

I've pulled those two out; we could reintroduce those in a separate recipe which folks can then choose to apply or exclude as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

2 participants