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

Child interfaces are not checked for default implementations #341

Closed zentol closed 2 years ago

zentol commented 2 years ago

I have stumbled on 2 scenarios where japicmp reports a source incompatiblity where none should exist.

They are both related to cases with an interface A declaring some method X, some interface B extending A with a default implementation for X.

In both cases the METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE is detected for CClass.

Scenario A:

public static class DefaultInSubInterface {
    public interface IInterface {}

    public interface IInterfaceWithDefault extends IInterface {}

    public class CCLass implements IInterfaceWithDefault {}
}

->

public static class DefaultInSubInterface {
    public interface IInterface {
        void foo();
    }

    public interface IInterfaceWithDefault extends IInterface {
        default void foo() {}
    }

    public class CCLass implements IInterfaceWithDefault {}
}

Scenario B:

public static class DefaultInSubInterfaceAddedSuperclass {
    public interface IInterface {
        void foo();
    }

    public interface IInterfaceWithDefault extends IInterface {
        @Override
        default void foo() {}
    }

    public static class CCLass implements IInterfaceWithDefault {}
}

->

public static class DefaultInSubInterfaceAddedSuperclass {
    public interface IInterface {
        void foo();
    }

    public interface IInterfaceWithDefault extends IInterface {
        @Override
        default void foo() {}
    }

    public static class CAddedSuperClass implements IInterfaceWithDefault {}

    public static class CCLass extends CAddedSuperClass {}
}

This additionally detects METHOD_DEFAULT_ADDED_IN_IMPLEMENTED_INTERFACE which seems wrong. It also doesn't detect SUPER_CLASS added, which is odd.

zentol commented 2 years ago

This is a bit annoying because we can't define a fine-grained exclusion for it. Either you exclude CClass or or IInterface#foo, but we can't say "ignore the issue with #foo in the context of CClass".

zentol commented 2 years ago

I made an attempt to fix this by adding this below the first loop in CompatibilityChanges#checkIfAbstractMethodAddedInSuperclass.

Essentially, after having looked up all abstract/default methods in the implemented interfaces of a class, we do another round over all found abstract methods and check the implemented interfaces for a default implementation of said method.

final List<JApiMethod> abstractMethodsWithDefaultInInterface = new ArrayList<>();
for (JApiMethod abstractMethod : abstractMethods) {
    for (JApiImplementedInterface jApiImplementedInterface : implementedInterfaces) {
        String fullyQualifiedName = jApiImplementedInterface.getFullyQualifiedName();
        JApiClass interfaceClass = classMap.get(fullyQualifiedName);

        for (JApiMethod method : interfaceClass.getMethods()) {
            if (method.getAbstractModifier().getNewModifier().get() != AbstractModifier.ABSTRACT) {
                // we have a default implementation for this method
                // double-check that we extend interface that the method comes from
                for (JApiImplementedInterface extendedInterface : interfaceClass.getInterfaces()) {
                    String extendedInterfaceFullyQualifiedName = extendedInterface.getFullyQualifiedName();
                    JApiClass extendedInterfaceClass = classMap.get(extendedInterfaceFullyQualifiedName);

                    if (abstractMethod.getjApiClass().equals(extendedInterfaceClass)) {
                        abstractMethodsWithDefaultInInterface.add(abstractMethod);
                    }
                }
            }
        }
    }
}
abstractMethods.removeAll(abstractMethodsWithDefaultInInterface);