Skip to content

Commit

Permalink
#289: added METHOD_NEW_STATIC_ADDED_TO_INTERFACE, METHOD_NON_STATIC_I…
Browse files Browse the repository at this point in the history 
…N_INTERFACE_NOW_STATIC and METHOD_STATIC_IN_INTERFACE_NO_LONGER_STATIC
  • Loading branch information
@siom79
siom79 committed Sep 8, 2023
1 parent 064789e commit a941c34
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 4 deletions.
8 changes: 8 additions & 0 deletions japicmp-testbase/japicmp-test-v1/src/main/java/japicmp/test/Interfaces.java
View file
Expand Up @@ -77,4 +77,12 @@ public interface InterfaceAddMethod {
public static class ClassImplementsComparable {

}

public interface InterfaceWithStaticMethod {

}

public interface InterfaceWithDefaultMethod {

}
}
12 changes: 12 additions & 0 deletions japicmp-testbase/japicmp-test-v2/src/main/java/japicmp/test/Interfaces.java
View file
Expand Up @@ -83,4 +83,16 @@ public int compareTo(ClassImplementsComparable o) {
return 0;
}
}

public interface InterfaceWithStaticMethod {
static void test() {

}
}

public interface InterfaceWithDefaultMethod {
default void test() {

}
}
}
19 changes: 15 additions & 4 deletions japicmp/src/main/java/japicmp/compat/CompatibilityChanges.java
View file
Expand Up @@ -402,7 +402,7 @@ public Integer callback(JApiClass superclass, Map<String, JApiClass> classMap, J
}
}
// section 13.4.18 of "Java Language Specification" SE7
if (isNotPrivate(method)) {
if (isNotPrivate(method) && !isInterface(method.getjApiClass())) {
if (method.getStaticModifier().hasChangedFromTo(StaticModifier.NON_STATIC, StaticModifier.STATIC)) {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_NOW_STATIC);
}
Expand Down Expand Up @@ -478,8 +478,12 @@ private void checkAbstractMethod(JApiClass jApiClass, Map<String, JApiClass> cla
List<JApiMethod> methodsWithSameSignature = getMethodsInImplementedInterfacesWithSameSignature(jApiClass, classMap, method);
if (methodsWithSameSignature.size() == 0) {
// new default method in interface
if (method.getAbstractModifier().hasChangedTo(AbstractModifier.NON_ABSTRACT)) {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_NEW_DEFAULT);
if (method.getAbstractModifier().getNewModifier().get() == AbstractModifier.NON_ABSTRACT) {
if (method.getStaticModifier().getNewModifier().get() == StaticModifier.STATIC) {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_NEW_STATIC_ADDED_TO_INTERFACE);
} else {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_NEW_DEFAULT);
}
} else {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_ADDED_TO_INTERFACE);
}
Expand All @@ -488,6 +492,7 @@ private void checkAbstractMethod(JApiClass jApiClass, Map<String, JApiClass> cla
for (JApiMethod jApiMethod : methodsWithSameSignature) {
if (jApiMethod.getChangeStatus() != JApiChangeStatus.NEW) {
allNew = false;
break;
}
}
if (allNew) {
Expand All @@ -496,11 +501,17 @@ private void checkAbstractMethod(JApiClass jApiClass, Map<String, JApiClass> cla
}
} else if (method.getChangeStatus() == JApiChangeStatus.MODIFIED || method.getChangeStatus() == JApiChangeStatus.UNCHANGED) {
JApiModifier<AbstractModifier> abstractModifier = method.getAbstractModifier();
// method changed from abstract to default
if (abstractModifier.getOldModifier().isPresent() && abstractModifier.getOldModifier().get() == AbstractModifier.ABSTRACT &&
abstractModifier.getNewModifier().isPresent() && abstractModifier.getNewModifier().get() == AbstractModifier.NON_ABSTRACT) {
// method changed from abstract to default
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_ABSTRACT_NOW_DEFAULT);
}
JApiModifier<StaticModifier> staticModifier = method.getStaticModifier();
if (staticModifier.hasChangedFromTo(StaticModifier.NON_STATIC, StaticModifier.STATIC)) {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_NON_STATIC_IN_INTERFACE_NOW_STATIC);
} else if (staticModifier.hasChangedFromTo(StaticModifier.STATIC, StaticModifier.NON_STATIC)) {
addCompatibilityChange(method, JApiCompatibilityChange.METHOD_STATIC_IN_INTERFACE_NO_LONGER_STATIC);
}
}
}
} else {
Expand Down
3 changes: 3 additions & 0 deletions japicmp/src/main/java/japicmp/model/JApiCompatibilityChange.java
View file
Expand Up @@ -43,8 +43,11 @@ public enum JApiCompatibilityChange {
METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE(true, false, JApiSemanticVersionLevel.MINOR),
METHOD_DEFAULT_ADDED_IN_IMPLEMENTED_INTERFACE(true, true, JApiSemanticVersionLevel.MINOR),
METHOD_NEW_DEFAULT(true, true, JApiSemanticVersionLevel.MINOR),
METHOD_NEW_STATIC_ADDED_TO_INTERFACE(true, true, JApiSemanticVersionLevel.MINOR),
METHOD_MOVED_TO_SUPERCLASS(true, true, JApiSemanticVersionLevel.PATCH),
METHOD_ABSTRACT_NOW_DEFAULT(false, false, JApiSemanticVersionLevel.MAJOR),
METHOD_NON_STATIC_IN_INTERFACE_NOW_STATIC(true, true, JApiSemanticVersionLevel.MINOR),

This comment has been minimized.

Sign in to view

Copy link
@Marcono1234

Marcono1234 Sep 26, 2023

Contributor

Is changing a non-static method to static really source and binary compatible?
For example the following does not compile:

interface I {
    // Let's assume this was `default` before being made `static`
    static void doSomething() {
    }
}

class C implements I {}

// Does not compile
new C().doSomething()

And if only I was recompiled but C not, then this would fail with:

NoSuchMethodError: 'void C.doSomething()'

And when changing from abstract to static it can cause issues as well, for example:

I i = ...;
i.doSomething();

If doSomething() is changed to static this causes

  • Source incompatibility: 
    illegal static interface method call
  • Binary incompatibility: 
    IncompatibleClassChangeError: Expected instance not static method 'void I.doSomething()'

This comment has been minimized.

Sign in to view

Copy link
@siom79

siom79 Sep 27, 2023

Author Owner

If the method was a default method and was changed to static, then you are right. It is source incompatible ("Static method may be invoked on containing interface class only") and binary incompatible ("cannot find symbol, method doSomething()").
In case it was abstract and changed to static, then the Class C must have an implementation of the method (as it was abstract) and therefore it is still source compatible (new C().doSomething() calls the method in C) and binary compatible (the method in C gets called).
Looks like I have to distinguish the two cases. :(

This comment has been minimized.

Sign in to view

Copy link
@siom79

siom79 Sep 27, 2023

Author Owner

I have created a new issue (#363) and committed some changes.
METHOD_STATIC_IN_INTERFACE_NO_LONGER_STATIC(false, false, JApiSemanticVersionLevel.MAJOR),
FIELD_STATIC_AND_OVERRIDES_STATIC(false, false, JApiSemanticVersionLevel.MAJOR),
FIELD_LESS_ACCESSIBLE_THAN_IN_SUPERCLASS(false, false, JApiSemanticVersionLevel.MAJOR),
FIELD_NOW_FINAL(false, false, JApiSemanticVersionLevel.MAJOR),
Expand Down
92 changes: 92 additions & 0 deletions japicmp/src/test/java/japicmp/compat/CompatibilityChangesTest.java
View file
Expand Up @@ -1124,6 +1124,98 @@ public List<CtClass> createNewClasses(ClassPool classPool) throws Exception {
assertThat(jApiMethod.isSourceCompatible(), is(false));
}

@Test
public void testMethodStaticAddedToInterface() throws Exception {
JarArchiveComparatorOptions options = new JarArchiveComparatorOptions();
options.setIncludeSynthetic(true);
options.setAccessModifier(AccessModifier.PRIVATE);
List<JApiClass> jApiClasses = ClassesHelper.compareClasses(options, new ClassesHelper.ClassesGenerator() {
@Override
public List<CtClass> createOldClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
return Collections.singletonList(ctClass);
}

@Override
public List<CtClass> createNewClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
CtMethodBuilder.create().publicAccess().staticAccess().name("method").addToClass(ctClass);
return Collections.singletonList(ctClass);
}
});
JApiClass jApiClass = getJApiClass(jApiClasses, "japicmp.Test");
assertThat(jApiClass.getChangeStatus(), is(JApiChangeStatus.MODIFIED));
assertThat(jApiClass.isBinaryCompatible(), is(true));
assertThat(jApiClass.isSourceCompatible(), is(true));
JApiMethod jApiMethod = getJApiMethod(jApiClass.getMethods(), "method");
assertThat(jApiMethod.getChangeStatus(), is(JApiChangeStatus.NEW));
assertThat(jApiMethod.getCompatibilityChanges(), hasItem(JApiCompatibilityChange.METHOD_NEW_STATIC_ADDED_TO_INTERFACE));
assertThat(jApiMethod.isBinaryCompatible(), is(true));
assertThat(jApiMethod.isSourceCompatible(), is(true));
}

@Test
public void testMethodNotStaticChangesToStaticInInterface() throws Exception {
JarArchiveComparatorOptions options = new JarArchiveComparatorOptions();
options.setIncludeSynthetic(true);
options.setAccessModifier(AccessModifier.PRIVATE);
List<JApiClass> jApiClasses = ClassesHelper.compareClasses(options, new ClassesHelper.ClassesGenerator() {
@Override
public List<CtClass> createOldClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
CtMethodBuilder.create().publicAccess().name("method").addToClass(ctClass);
return Collections.singletonList(ctClass);
}

@Override
public List<CtClass> createNewClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
CtMethodBuilder.create().publicAccess().staticAccess().name("method").addToClass(ctClass);
return Collections.singletonList(ctClass);
}
});
JApiClass jApiClass = getJApiClass(jApiClasses, "japicmp.Test");
assertThat(jApiClass.getChangeStatus(), is(JApiChangeStatus.MODIFIED));
assertThat(jApiClass.isBinaryCompatible(), is(true));
assertThat(jApiClass.isSourceCompatible(), is(true));
JApiMethod jApiMethod = getJApiMethod(jApiClass.getMethods(), "method");
assertThat(jApiMethod.getChangeStatus(), is(JApiChangeStatus.MODIFIED));
assertThat(jApiMethod.getCompatibilityChanges(), hasItem(JApiCompatibilityChange.METHOD_NON_STATIC_IN_INTERFACE_NOW_STATIC));
assertThat(jApiMethod.isBinaryCompatible(), is(true));
assertThat(jApiMethod.isSourceCompatible(), is(true));
}

@Test
public void testMethodStaticChangesToNotStaticInInterface() throws Exception {
JarArchiveComparatorOptions options = new JarArchiveComparatorOptions();
options.setIncludeSynthetic(true);
options.setAccessModifier(AccessModifier.PRIVATE);
List<JApiClass> jApiClasses = ClassesHelper.compareClasses(options, new ClassesHelper.ClassesGenerator() {
@Override
public List<CtClass> createOldClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
CtMethodBuilder.create().publicAccess().staticAccess().name("method").addToClass(ctClass);
return Collections.singletonList(ctClass);
}

@Override
public List<CtClass> createNewClasses(ClassPool classPool) throws Exception {
CtClass ctClass = CtInterfaceBuilder.create().name("japicmp.Test").addToClassPool(classPool);
CtMethodBuilder.create().publicAccess().name("method").addToClass(ctClass);
return Collections.singletonList(ctClass);
}
});
JApiClass jApiClass = getJApiClass(jApiClasses, "japicmp.Test");
assertThat(jApiClass.getChangeStatus(), is(JApiChangeStatus.MODIFIED));
assertThat(jApiClass.isBinaryCompatible(), is(false));
assertThat(jApiClass.isSourceCompatible(), is(false));
JApiMethod jApiMethod = getJApiMethod(jApiClass.getMethods(), "method");
assertThat(jApiMethod.getChangeStatus(), is(JApiChangeStatus.MODIFIED));
assertThat(jApiMethod.getCompatibilityChanges(), hasItem(JApiCompatibilityChange.METHOD_STATIC_IN_INTERFACE_NO_LONGER_STATIC));
assertThat(jApiMethod.isBinaryCompatible(), is(false));
assertThat(jApiMethod.isSourceCompatible(), is(false));
}

@Test
public void testMethodAddedToPublicClass() throws Exception {
JarArchiveComparatorOptions options = new JarArchiveComparatorOptions();
Expand Down
3 changes: 3 additions & 0 deletions src/site/markdown/MavenPlugin.md
View file
Expand Up @@ -300,8 +300,11 @@ for each check. This allows you to customize the following verifications:
| METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE | true | false | MINOR |
| METHOD_DEFAULT_ADDED_IN_IMPLEMENTED_INTERFACE | true | true | MINOR |
| METHOD_NEW_DEFAULT | true | true | MINOR |
| METHOD_NEW_STATIC_ADDED_TO_INTERFACE | true | true | MINOR |
| METHOD_ABSTRACT_NOW_DEFAULT | false | false | MAJOR |
| METHOD_MOVED_TO_SUPERCLASS | true | true | PATCH |
| METHOD_NON_STATIC_IN_INTERFACE_NOW_STATIC | true | true | MINOR |
| METHOD_STATIC_IN_INTERFACE_NO_LONGER_STATIC | false | false | MAJOR |
| FIELD_STATIC_AND_OVERRIDES_STATIC | false | false | MAJOR |
| FIELD_LESS_ACCESSIBLE_THAN_IN_SUPERCLASS | false | false | MAJOR |
| FIELD_NOW_FINAL | false | false | MAJOR |
Expand Down

0 comments on commit a941c34

Please sign in to comment.