search

lightbend-labs / mima

A tool for catching binary incompatibility in Scala
Apache License 2.0
459 stars 71 forks source link

False negatives for methods moving up in trait hierarchy #426

Closed travisbrown closed 4 years ago

travisbrown commented 4 years ago

This bug just hit us in the Cats 2.1.0-RC1 release, where we moved an instance method from the MonadError type class trait to its ApplicativeError supertype, which MiMa said was fine, but which actually breaks binary compatibility on both 2.12 and 2.13: https://github.com/typelevel/cats/pull/3162

The problem is the synthetic myMethod$ static implementation method, which no longer exists on the trait that the myMethod instance method was moved out of. Adding an override in this trait fixes the bincompat issue, but that doesn't work if for example the method was implicit and was moved in order to adjust prioritization.

It seems like this should be a pretty straightforward DirectMissingMethodProblem, and there's already a trait-pushing-up-concrete-methods-is-nok test that covers exactly this case—the problem is that the test states that there shouldn't be problems on 2.12+, when it definitely should be identifying problems.

I have a MiMa branch with a fix that does what I need to check this Cats release (it's a two-line change—I'm just filtering static methods out of the interface method lookup and not treating these synthetic methods as inaccessible), but someone who's more familiar with the MiMa implementation could probably do this in a more principled way.

dwijnand commented 4 years ago

Thanks for the detailed issue. If you want to share the patch we can have a look together.

dwijnand commented 4 years ago

Btw, I thought this sounded like #237, but re-reading that one's different.

travisbrown commented 4 years ago

@dwijnand Right, if these were classes I don't think this specific synthetic static method issue would be a problem.

Here's the change I was using to find these in Cats: https://github.com/travisbrown/mima/tree/topic/cats-3162-check

Thanks for taking a look!

dwijnand commented 4 years ago

Took a mini step forward today and ran that patch against the test suite and it broke the following tests:

[error] 'test-trait-pushing-up-concrete-methods-is-nok' failed.
[error] 'test-moving-method-upward-from-trait-to-class-nok' failed.
[error] 'test-trait-inheriting-from-class' failed.
[error] 'test-trait-moving-methods2-nok' failed.
[error] 'test-trait-moving-methods-nok' failed.
[error] 'test-trait-deleting-concrete-methods-is-nok' failed.

https://travis-ci.com/dwijnand/mima/jobs/259571256

I might come back to this, this weekend.

travisbrown commented 4 years ago

@dwijnand Thanks for looking! The problem is that at least some of the tests are wrong—e.g. test-trait-pushing-up-concrete-methods-is-nok is supposed to report no problems for 2.12 but the change definitely breaks bincompat.

dwijnand commented 4 years ago

Ah, yes, you'd already mentioned that. I'll try to study this tomorrow and push a patch for review.

szeiger commented 4 years ago

MiMa filters out most synthetic methods under the assumption that any compatibility error in those would also be caught in a non-synthetic method (and the latter one is the error that users should see). This clearly doesn't work for static trait implementation methods. I'm surprised that we didn't encounter this bug much sooner.

szeiger commented 4 years ago

On second thought, perhaps it is not that surprising after all. Our own use of MiMa for the Scala standard library would not be affected because such a change is not forward binary compatible anyway.