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

Warn on changes in method Signature (e.g. generics) #299

Merged
merged 4 commits into from
May 31, 2019

Conversation

raboof
Copy link
Contributor

@raboof raboof commented May 3, 2019

See #40

@raboof raboof changed the title Warn on changes in method Signature (e.g. generics, #40) Warn on changes in method Signature (e.g. generics) May 3, 2019
@raboof raboof closed this May 3, 2019
@raboof raboof reopened this May 3, 2019
Copy link

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

great, do we need a way to disable the feature (in case someone want the old behavior)?

@raboof
Copy link
Contributor Author

raboof commented May 3, 2019

The new behavior results in problems of the new type IncompatibleSignatureProblem, so filtering them out should be fairly simple. Perhaps wait until someone actually asks for this to be made optional?

The integration tests reveal this PR also introduces some new IncompatibleResultTypeProblem instances, but AFAICS those are actually valid.

Copy link
Collaborator

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Oh wow, didn't know this was accessible. Nice one, @raboof!

Is this PR blocked on something? It LGTM.

Not that it's particularly special, except of special interest to me, but I'd love to have the case class unapply test case from #40 in the test suite. I can send a follow-up PR, too, though.

@raboof raboof force-pushed the warnOnChangesInGenerics branch from d1b3a8c to 74bbe43 Compare May 20, 2019 13:03
Copy link
Collaborator

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM!

@raboof
Copy link
Contributor Author

raboof commented May 20, 2019

OK, I checked MiMa from this branch with akka-http, and while I had to add a bunch of new filters (akka/akka-http#2538), they do seem legitimate.

@jrudolph
Copy link
Contributor

👍, maybe some migration notes could be added somewhere that you can expect new problems that need to be fixed / excluded and how to exclude all if you don't care?

@raboof
Copy link
Contributor Author

raboof commented May 22, 2019

I think this would best fit in the GitHub 'Release'. Something like:

This release introduces a new problem type, the IncompatibleSignatureProblem. This problem is reported when a method's erased signature has remained the same, but its full signature has changed. Such a change is not necessarily binary incompatible, but typically worthwhile to manually investigate and explicitly confirm. You can exclude all such problems with a ProblemFilters.exclude[IncompatibleSignatureProblem]("*").

This release also fixes a problem where IncompatibleResultTypeProblem instances were missed. This means when performing this upgrade new filters for old (but previously undetected) incompatibilities may have to be added.

?

@jrudolph
Copy link
Contributor

Sounds good!

@dwijnand dwijnand force-pushed the warnOnChangesInGenerics branch from 74bbe43 to 83b6c6a Compare May 28, 2019 06:42
@dwijnand
Copy link
Collaborator

Thought we'd merged this already, so my repo structure changes broke this.

I also realised in doing that that this PR was deleting a problems.txt file in one of the functional tests, that I think might disable the test? So I undeleted it in the rebase.

@dwijnand
Copy link
Collaborator

[error] (test-tuple-parametric-type-change-doesnt-result-in-binary-incompatibility / testFunctional) com.typesafe.tools.mima.lib.TestFailed: test-tuple-parametric-type-change-doesnt-result-in-binary-incompatibility' failed.
[error] 	The following problems were not expected
[error] 	- method foo()scala.Tuple2 in class A has a different signature in new version, where it is ()Lscala/Tuple2<Ljava/lang/Object;Ljava/lang/String;>; rather than ()Lscala/Tuple2<Ljava/lang/String;Ljava/lang/Object;>;

@dwijnand
Copy link
Collaborator

79621dc:

The test should be failing once we have a fix for Re #40

😄

@dwijnand dwijnand force-pushed the warnOnChangesInGenerics branch from 83b6c6a to d73f674 Compare May 31, 2019 22:35
@dwijnand
Copy link
Collaborator

Folded into the first commit.

@mergify mergify bot merged commit 545d098 into lightbend-labs:master May 31, 2019
@raboof raboof deleted the warnOnChangesInGenerics branch June 3, 2019 07:30
@dwijnand dwijnand added this to the 0.4.0 milestone Jul 10, 2019
@SethTisue
Copy link
Collaborator

note that @dwijnand's #471 later made this opt-in, via a new mimaReportSignatureProblems setting

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

Successfully merging this pull request may close these issues.

5 participants