siom79 / japicmp

Comparison of two versions of a jar archive
https://siom79.github.io/japicmp
Apache License 2.0
712 stars 107 forks source link

METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE failure when both interface and implementer class is new #281

Closed gszadovszky closed 2 years ago

gszadovszky commented 3 years ago

I am not sure I completely understand what METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE means. I could not find a proper description for this (or the other) rules. Anyway this rule triggered for source incompatibility between two versions of parquet-mr. For the related difference both the interface and the implementer class was new. In this situation I cannot see any reason of incompatibility.

I'm happy to add any additional info or attach report files. I am not sure how to attach files to github issues, though.

gszadovszky commented 3 years ago

The issue is reproducible with the latest 0.15.2 japicmp-maven-plugin but is working fine with 0.14.2.

normanmaurer commented 3 years ago

I think I may suffer from the same problem... I added a new class (which extends an existing abstract class) and the plugin fails with METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE when using 0.15.3 while it works with 0.14.1

millems commented 3 years ago

+1, we had to roll back to 0.14.4 from 0.15.3, because we were getting failures on classes that weren't changed. Sorry, no easy repro I can provide at this time.

siom79 commented 3 years ago

Looks like the code does not check if the new method from the interface is already implemented in a superclass. Fixed that with the latest commit.

siom79 commented 3 years ago

Fixed: 0.15.4

ibauersachs commented 2 years ago

A new class still adds this error in 0.15.4.

siom79 commented 2 years ago

Can you provide an example?

Ingo Bauersachs @.***> schrieb am Sa., 25. Dez. 2021, 22:57:

A new class still adds this error in 0.15.4.

— Reply to this email directly, view it on GitHub https://github.com/siom79/japicmp/issues/281#issuecomment-1001075713, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4U7NH37WCDBVXJ42L5HULUSY43FANCNFSM4W6MQT5Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.***>

ibauersachs commented 2 years ago

The following branch in dnsjava fails unless that check is disabled in the Maven plugin: https://github.com/dnsjava/dnsjava/tree/dnssec

Here's a link to the commit just before adding the workaround that you could use: https://github.com/dnsjava/dnsjava/tree/0302a875575b4abb8882e9e7ecfdfaec89434eb8

joviegas commented 2 years ago

I am still seeing this issue in 0.15.6.

I reverted back to 0.14.4 and issue got resolved.

In my case japicmp report incompatibilty for ChildClass as below , the issue is seen when we are building for new release which has a new interace added B newArgs(NewArgs newArgs); the implementation is already there in Parent class.

Looks like False positive.

For now I have disable METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE since we need 0.15.6 to support JDK17 build, please let us know the long term fix for this issue,.

public class ParentClass {

    public interface Builder<B extends Builder> {
        // There are other existing methods.

        //This got added in the new release and japicmp is COMPLAINING because of this
        B newArgs(NewArgs newArgs);

        ParentClass build();
    }

    protected static class BuilderImpl<B extends Builder> implements Builder<B> {

        private NewArgs newArgs;

        protected BuilderImpl() {
        }

        // Parent class ALREADY has implemented the interface.
        @Override
        public B newArgs(NewArgs newArgs) {
            this.newArgs = newArgs;
            return (B) this;
        }

        @Override
        public ParentClass build() {
            return new ParentClass(this);
        }
    }
}
public final class ChildClass extends ParentClass {

    public interface Builder extends ParentClass.Builder<Builder> {

        Builder existingField(Boolean existingField);

        @Override
        ChildClass build();
    }

    private static final class BuilderImpl extends ParentClass.BuilderImpl<Builder> implements Builder {

        private Boolean existingField = false;

        private BuilderImpl() {
        }

        @Override
        public Builder existingField(Boolean existingField) {
            this.existingField = existingField;
            return this;
        }

        @Override
        public ChildClass build() {
            return new ChildClass(this);
        }
    }
}
joviegas commented 2 years ago

Hi @siom79 , let me know if I need to raise a new issue, since this issue is already closed.

siom79 commented 2 years ago

I have reopened the ticket and will have a look soon.

siom79 commented 2 years ago

@joviegas Thanks for the detailed report.

Fixed: 0.15.7