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.

Sign up for GitHub

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

Have ReverseOrder rule flag Collections.reverseOrder(String::compareTo) #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Have ReverseOrder rule flag Collections.reverseOrder(String::compareTo) #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ static final class ReverseOrder<T extends Comparable<? super T>> {
Comparator<T> before() {
return Refaster.anyOf(
Collections.reverseOrder(),
Collections.reverseOrder(T::compareTo),
Copy link
Member

Choose a reason for hiding this comment

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

The rule above is meant to replace T::compareTo with naturalOrder(), so this one shouldn't be necessary. 🤔 (And the build seems to agree 👀)

Copy link
Member Author

Choose a reason for hiding this comment

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

I shared it.

Still I think we should add this one. If we apply the rewrite suggested right now we would end up in a state where you can have:

  .isSortedAccordingTo(Collections.reverseOrder(Comparator.naturalOrder()));

It gives the following error:

'reverseOrder(java.util.Comparator )' in 'java.util.Collections' cannot be applied to '(java.util.Comparator )'

Copy link
Member

Choose a reason for hiding this comment

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

So the problem as I understand it is that:

  1. org.assertj.core.api.AbstractListAssert#isSortedAccordingTo accepts any Comparator<? super ELEMENT>. (For the code at hand ELEMENT is OffsetDateTime.)
  2. java.util.Collections#reverseOrder accepts any Comparable<T>.
  3. java.util.Comparator.naturalOrder() returns a Comparator<T extends Comparable<? super T>>.

As a result the compiler emits:

no suitable method found for reverseOrder(java.util.Comparator<T>)
    method java.util.Collections.<T>reverseOrder(java.util.Comparator<T>) is not applicable
      (inferred type does not conform to equality constraint(s)
        inferred: T
        equality constraints(s): T)
    method java.util.Collections.<T>reverseOrder() is not applicable
      (cannot infer type-variable(s) T
        (actual and formal argument lists differ in length))

This is a general problem with Refaster. Just adding a T::compareTo case here is not enough: the NaturalOrder rule would also "break" e.g. the following expressions:

  • abstractListAssert.isSortedAccordingTo(reverseOrder(comparing(identity())))
  • abstractListAssert.isSortedAccordingTo(reverseOrder(comparing(v -> v)))

We could of course just add those cases as well, but:

  1. That doesn't solve the general problem of rules such as NaturalOrder breaking code.
  2. There are likely other such problematic interactions between rules, and ideally we find a way to identify those, too. (Because otherwise a solution to (1) would prevent certain viable transformations from being applied.)

Yes, that's all much harder... :(

I did play around a bit with a @Placeholder @Matches(CustomMatcher.class) approach, but it appears that the VisitorState passed by Refaster's placeholder logic to the Matcher is "too fake" to infer the symbols and type information we may need to solve problem (1).

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, okay I didn't expect this. That's indeed a much bigger issue 🤔. Let's sync on this one offline tomorrow.

Collections.<T>reverseOrder(naturalOrder()),
Comparator.<T>naturalOrder().reversed());
}
Expand Down
1 change: 1 addition & 0 deletions ...rib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ ImmutableSet<Comparator<String>> testNaturalOrder() {
ImmutableSet<Comparator<String>> testReverseOrder() {
return ImmutableSet.of(
Collections.reverseOrder(),
Collections.reverseOrder(String::compareTo),
Collections.<String>reverseOrder(naturalOrder()),
Comparator.<String>naturalOrder().reversed());
}
Expand Down
5 changes: 4 additions & 1 deletion ...ib/src/test/resources/tech/picnic/errorprone/refasterrules/ComparatorRulesTestOutput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ ImmutableSet<Comparator<String>> testNaturalOrder() {

ImmutableSet<Comparator<String>> testReverseOrder() {
return ImmutableSet.of(
Comparator.reverseOrder(), Comparator.reverseOrder(), Comparator.reverseOrder());
Comparator.reverseOrder(),
Comparator.reverseOrder(),
Comparator.reverseOrder(),
Comparator.reverseOrder());
}

ImmutableSet<Comparator<String>> testCustomComparator() {
Expand Down