METHOD_RETURN_TYPE_CHANGED reported for package-private method promoted to public that changes return type

[scordio]

The following package-private method:

Class<T> getType() {
  ...
}

has been promoted to public and changed to:

public Type getType() {
  ...
}

and japicmp reports it as binary incompatible:

Comparing binary compatibility of assertj-core-3.26.0-SNAPSHOT.jar against assertj-core-3.25.3.jar
***! MODIFIED CLASS: PUBLIC org.assertj.core.api.InstanceOfAssertFactory  (not serializable)
	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
	GENERIC TEMPLATES: === ASSERT:org.assertj.core.api.AbstractAssert<?,?><?,?>, === T:java.lang.Object
	***! MODIFIED METHOD: PUBLIC (<- PACKAGE_PROTECTED) java.lang.reflect.Type(<- <T>) (<-java.lang.Class(<- <T>)) getType()

Is it a false positive? If yes, I'm happy to submit a fix.

Commit: assertj/assertj@ff01677
CI result: https://github.com/assertj/assertj/actions/runs/8037768330/job/21952981201#step:4:202

[siom79]

But the method return type changed, didn't it? The change of the access modifier is not the problem.

[scordio]

Yes, it changed, but it shouldn't be a problem as it was originally package-private, or not?

Or do you have in mind split-package usage?

[garydgregory]

This seems to duplicate #265

[scordio]

@garydgregory the only reason I didn't mention it as a duplicate of #265 is the potential split-package usage of an existing package-private method (my case), while a private one (your case) wouldn't be affected.

[garydgregory]

OK. It is helpful to know that these two issues are related though.

[siom79]

It is a difficult decision.
If you only have the point of view of the users of your library, then every package protected method should not be used (although some users might do it) and it becomes visible for your users with public access.
From a theoretical point of view, changing the return type and the modifier from package protected to public is incompatible with respect to inner package usage.
Back then I have assumed that the accessModifier filter would solve the problem. But it looks like users only see the problem from their users point of view. Do we need a new option to solve this?

[scordio]

A separate error that I can exclude in my config would work for me 👍

If there are split-package users that are impacted by this change, it means they are already dancing on the edge of brokenness and we don't provide support for such usage 😉

[scordio]

Re-reading your last message, how is accessModifier supposed to be used?

I set it to protected (assuming it means it will analyze only elements that were protected and public before the change?) but I still received a METHOD_RETURN_TYPE_CHANGED error (see assertj/assertj@684063f).

[siom79]

I thought about an implementation and as it looks, it might introduce a new concept. The point is that currently the relation between type of change (like e.g. METHOD_RETURN_TYPE_CHANGED) and the incompatibility level was fix (see e.g. here).
Now we need something like METHOD_RETURN_TYPE_CHANGED but because the access modifier also changed to more visible it is not binary incompatible. Not sure how to implement this in a concise way.

[scordio]

My rough idea was to approach it in a way similar to #311.

I haven't looked at the code in detail yet, but I trust you can spot better than me whether this is feasible with such a strategy.

[siom79]

Unfortunately it is not that easy, because we need to change the incompatibility for each occurrence. I think we need to change the list of incompatibilities from a pure enum to an object, on which we can change the incompatibility level. :(

[siom79]

Committed the changes mentioned above to a new branch. Will have to sleep a night, before I merge it.

[scordio]

Thanks a lot for looking into it!

[siom79]

Fixed with 0.19.0.

[scordio]

Thank you!