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

[rickie]

Encountered this problem when testing downstream.

Without this rule this:

Collections.reverseOrder(String::compareTo)

Would be replaced with:

Collections.reverseOrder(Comparator.naturalOrder())

Which is incompatible so it would lead to non compilable code.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[Stephan202]

If you share a link on Slack to the code that gives trouble I'll try to have a look.

[Stephan202]

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

[rickie]

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 )'

[Stephan202]

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).

[rickie]

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

[rickie]

Moved it to the next milestone 0.7.0.