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

New methods in interfaces... #201

Closed
twogee opened this issue Mar 11, 2018 · 17 comments
Closed

New methods in interfaces... #201

twogee opened this issue Mar 11, 2018 · 17 comments

Comments

@twogee
Copy link
Contributor

@twogee twogee commented Mar 11, 2018

... should count as incompatibility unless default implementation is provided (in Java 8+)

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented Mar 12, 2018

As of Chapter 13. of the JLS:

Adding a default method, or changing a method from abstract to default, does not break compatibility with pre-existing binaries, but may cause an IncompatibleClassChangeError if a pre-existing binary attempts to invoke the method.

Hence I decided to mark both changes (adding a new default method, changing a method from abstract to a default method) as incompatible, as both may be incompatible (depending on the inheritance hierarchy they are used in).

In general adding a method to an interface is only source incompatible but not binary incompatibly, as there is no existing code that does call the new method (see JLS).

siom79 added a commit that referenced this issue Mar 12, 2018
…sly abstract method to a default method is marked as incompatible
@siom79 siom79 closed this Mar 12, 2018
@bdemers

This comment has been minimized.

Copy link

@bdemers bdemers commented Apr 12, 2018

@siom79 I can open a new issue, but wanted to keep the context here.
What about adding a new parameter to allow default methods to be binary compatible?
This saves the user from needing to add excludes?

I guess writing a groovy script to post process the report is another option.

Thoughts or other recommendations ?

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented Apr 12, 2018

This may be a general topic as there are may be other changes that may be incompatible in a specific situation but in another not.

Why do you want to mark a new default method as binary compatible?

@bdemers

This comment has been minimized.

Copy link

@bdemers bdemers commented Apr 12, 2018

I want to extend an interface (and provide defaults). I really just want some easy way of configuring the japicmp plugin to not fail the build in this scenario.

@hkalina

This comment has been minimized.

Copy link

@hkalina hkalina commented May 2, 2018

I would welcome possibility to exclude this type of compatibility too.
If I understand correctly, pre-existing binary can invoke the new method (and cause IncompatibleClassChangeError) using reflection only.
(which is pretty uncommon and calling code should handle similar errors)

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 2, 2018

I have made it possible to configure whether certain changes are binary or source or even not incompatible at all (see #209).

@hkalina

This comment has been minimized.

Copy link

@hkalina hkalina commented May 2, 2018

Great thanks. Unfortunately, if I am looking correctly, it does not allow to allow adding methods iff they have default implementation...

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 3, 2018

Take a look here. The table for the overrideCompatibilityChangeParameters parameter lists the following two checks for default methods:

  • METHOD_NEW_DEFAULT
  • METHOD_ABSTRACT_NOW_DEFAULT

You can set them both to binary and source compatible, if you like:

<overrideCompatibilityChangeParameters>
	<overrideCompatibilityChangeParameter>
		<compatibilityChange>METHOD_NEW_DEFAULT</compatibilityChange>
		<binaryCompatible>true</binaryCompatible>
		<sourceCompatible>true</sourceCompatible>
	</overrideCompatibilityChangeParameter>
	<overrideCompatibilityChangeParameter>
		<compatibilityChange>METHOD_ABSTRACT_NOW_DEFAULT</compatibilityChange>
		<binaryCompatible>true</binaryCompatible>
		<sourceCompatible>true</sourceCompatible>
	</overrideCompatibilityChangeParameter>
</overrideCompatibilityChangeParameters>
@hkalina

This comment has been minimized.

Copy link

@hkalina hkalina commented May 3, 2018

Thanks!
Unfortunately, when default method is added into interface, both, METHOD_NEW_DEFAULT and METHOD_ADDED_TO_INTERFACE, are logged.
I cannot keep METHOD_ADDED_TO_INTERFACE checked and to ignore METHOD_NEW_DEFAULT only.

Before setting:
Breaking the build because there is at least one incompatibility: org.wildfly.security.password.interfaces.MaskedPassword.createRaw(java.lang.String,char[],int,byte[],byte[],byte[]):METHOD_NEW_DEFAULT,org.wildfly.security.password.interfaces.MaskedPassword.createRaw(java.lang.String,char[],int,byte[],byte[],byte[]):METHOD_ADDED_TO_INTERFACE,org.wildfly.security.password.interfaces.MaskedPassword.getInitializationVector():METHOD_NEW_DEFAULT,org.wildfly.security.password.interfaces.MaskedPassword.getInitializationVector():METHOD_ADDED_TO_INTERFACE

After setting METHOD_NEW_DEFAULT as compatible change:
Breaking the build because there is at least one incompatibility: org.wildfly.security.password.interfaces.MaskedPassword.createRaw(java.lang.String,char[],int,byte[],byte[],byte[]):METHOD_ADDED_TO_INTERFACE,org.wildfly.security.password.interfaces.MaskedPassword.getInitializationVector():METHOD_ADDED_TO_INTERFACE

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 3, 2018

Then you must also configure METHOD_ADDED_TO_INTERFACE as binary and source compatible:

<overrideCompatibilityChangeParameters>
	<overrideCompatibilityChangeParameter>
		<compatibilityChange>METHOD_NEW_DEFAULT</compatibilityChange>
		<binaryCompatible>true</binaryCompatible>
		<sourceCompatible>true</sourceCompatible>
	</overrideCompatibilityChangeParameter>
	<overrideCompatibilityChangeParameter>
		<compatibilityChange>METHOD_ABSTRACT_NOW_DEFAULT</compatibilityChange>
		<binaryCompatible>true</binaryCompatible>
		<sourceCompatible>true</sourceCompatible>
	</overrideCompatibilityChangeParameter>
	<overrideCompatibilityChangeParameter>
		<compatibilityChange>METHOD_ADDED_TO_INTERFACE</compatibilityChange>
		<binaryCompatible>true</binaryCompatible>
		<sourceCompatible>true</sourceCompatible>
	</overrideCompatibilityChangeParameter>
</overrideCompatibilityChangeParameters>
@hkalina

This comment has been minimized.

Copy link

@hkalina hkalina commented May 3, 2018

Yes, but that is something I would like to keep checked, as adding non-default method into interface is really breaking change. (If somebody implement that interface, he will have a problem to compile it against new version of library - as opposite to the default method, which allow to keep interface implementations unchanged. The mentioned IncompatibleClassChangeError looks more theoretical, as I dont see how could old code call the new default method.)

@bdemers

This comment has been minimized.

Copy link

@bdemers bdemers commented May 3, 2018

@hkalina Take a look at the groovy script support. It was pretty easy to add a single exclusion using the Maven plugin:
https://github.com/okta/okta-sdk-java/blob/628416d/pom.xml#L286

And the actual groovy script:
https://github.com/okta/okta-sdk-java/blob/628416d/src/japicmp/postAnalysisScript.groovy#L27-L29

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 3, 2018

@hkalina You can also use the <excludes/> element as shown here.

siom79 added a commit that referenced this issue May 3, 2018
…D_TO_INTERFACE are not set simultaneously
@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 3, 2018

@hkalina The last commit on this ticket changes the evaluation such that the two compatibility changes METHOD_NEW_DEFAULT and METHOD_ADDED_TO_INTERFACE are not set simultaneously.

@hkalina

This comment has been minimized.

Copy link

@hkalina hkalina commented May 3, 2018

Amazing, works perfectly! :)

@garydgregory

This comment has been minimized.

Copy link

@garydgregory garydgregory commented Apr 19, 2019

Can I add overrideCompatibilityChangeParameter for a specific class only?

@siom79

This comment has been minimized.

Copy link
Owner

@siom79 siom79 commented May 1, 2019

@garydgregory No, currently you can change the settings only globally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.