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

Update sbt-mima-plugin to 0.4.0 #2934

Conversation

scala-steward
Copy link
Contributor

Updates com.typesafe:sbt-mima-plugin from 0.3.0 to 0.4.0.

I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.

If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention @scala-steward in the comments below.

Have a fantastic day writing Scala!

Ignore future updates

Add this to your .scala-steward.conf file to ignore future updates of this dependency:

updates.ignore = [{ groupId = "com.typesafe", artifactId = "sbt-mima-plugin" }]

labels: semver-minor

LukaJCB
LukaJCB previously approved these changes Jul 9, 2019
@kailuowang
Copy link
Contributor

kailuowang commented Jul 10, 2019

seems the new version reports on changes previously as bc
for example

method rethrow(cats.MonadError)java.lang.Object in class cats.syntax.MonadErrorRethrowOps has a different signature in current version, where it is (Lcats/MonadError<TF;-TE;>;)TF; rather than (Lcats/MonadError<TF;TE;>;)TF;

The actual change was from

  def rethrow[A](fa: F[Either[E, A]]): F[A]

to

  def rethrow[A, EE <: E](fa: F[Either[EE, A]]): F[A]

I am not sure if this is a Mima regression or it's a real BC breaking change, we better test with our own bin compat tests.

@codecov-io
Copy link

Codecov Report

Merging #2934 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2934   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files         370      370           
  Lines        7038     7038           
  Branches      186      186           
=======================================
  Hits         6618     6618           
  Misses        420      420

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07b2c21...72333aa. Read the comment docs.

@dwijnand
Copy link
Contributor

method rethrow(cats.MonadError)java.lang.Object in class cats.syntax.MonadErrorRethrowOps has a different signature in current version, where it is (Lcats/MonadError<TF;-TE;>;)TF; rather than (Lcats/MonadError<TF;TE;>;)TF;

So that's an IncompatibleSignatureProblem, where apparently the code diff of:

-  def rethrow[A](fa: F[Either[E, A]]): F[A]
+  def rethrow[A, EE <: E](fa: F[Either[EE, A]]): F[A]

resulted in the method signature diff of:

-(Lcats/MonadError<TF;TE;>;)TF;
+(Lcats/MonadError<TF;-TE;>;)TF;

Does that signature diff make sense? Is it a problem?

If you want to unblock the PR, as documented in https://github.com/lightbend/mima/releases/tag/0.4.0, you can ignore all IncompatibleSignatureProblems with ProblemFilters.exclude[IncompatibleSignatureProblem]("*").

@dwijnand
Copy link
Contributor

cc @raboof, FYI and in case you have more insights

@raboof
Copy link

raboof commented Jul 11, 2019

I am not sure if this is a Mima regression

The fact that MiMa now flags when the generic signature changes is intentional. Such a signature change is not necessarily incompatible, but flagged to err on the safe side and also catch unintentional changes.

For example, a change from def foo(o: Option[Any]) to def foo(o: Option[Foo]) may lead to runtime errors casting the input to Foo. A change from def foo(o: Option[Foo]) to def foo(o: Option[Any]) should be fine, but is flagged as well because it is still useful to verify it is intentional.

The rethrow case looks fine to me since it's just narrowing the type of the inner fa without allowing further assumptions about it, so I think it's safe to add the suggested ProblemFilters.exclude[IncompatibleSignatureProblem]("cats.syntax.MonadErrorRethrowOps.rethrow") (but perhaps someone more well-versed in cats should double-check).

You could also indeed just exclude all such warnings with ProblemFilters.exclude[IncompatibleSignatureProblem]("*"), but that might miss real problems - I'd recommend double-checking them all.

Perhaps we should expand on this a bit in https://github.com/lightbend/mima/blob/master/core/src/main/scala/com/typesafe/tools/mima/core/Problems.scala#L109 and make that easier to find?

@dwijnand
Copy link
Contributor

It looks like the diff change is actually:

-  def rethrow(implicit F: MonadError[F, E]): F[A] = F.rethrow(fea)
+  def rethrow(implicit F: MonadError[F, _ >: E]): F[A] =

(from 1745ea1)

So the TE to -TE change makes more sense now.

Is it safe for MiMa to assume that if a generic signature change is just going from invariant to covariant or contravariant then it's safe? The idea is that if the variance changed in the class (MonadError) then MiMa would warn there and if, instead, it's just a change in the declaration (like here) then it's safe because the typechecker passed it when it compiled it to bytecode.

Or maybe MiMa shouldn't get involved in the checking variance changes...

@raboof
Copy link

raboof commented Jul 11, 2019

Is it safe for MiMa to assume

Not generally - even if it could, I think mima should still flag it to manually verify it's an intentional change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants