search

siom79 / japicmp

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

Take changes to annotations into account #385

Closed guillermocalvo closed 3 months ago

guillermocalvo commented 5 months ago

Currently, the only annotation change that is reported is the addition of the @Deprecated annotation.

While adding or deleting annotations does not break compatibility with pre-existing binaries, changes in other types of annotations can also have a semantic impact in terms of backward compatibility. For example, adding or removing jakarta.transaction.Transactional to a method may affect client code invoking that method.

In any case, it would be a nice improvement if modifications to annotations were reported as PATCH changes.

siom79 commented 3 months ago

Released with 0.21.0.

siom79 commented 3 months ago

The change did cause issue #395.

For me the implementation of calling checkIfMethodsHaveChangedIncompatible() and checkIfFieldsHaveChangedIncompatible() in this context seems to be wrong, as these methods are supposed to be run on the methods and fields of a class (and an annotation is a class itself). Hence; these methods are anyhow called on an annotation, when the annotation itself is processed as "class".

What we want to track here are the changes of the values of an annotation, not the changes of the annotation itself. If a method is removed from an annotation in a new version, then the value of this method must also be removed.

Therewith we also don't need ANNOTATION_MODIFIED_INCOMPATIBLE, as the annotation class itself will already be changed incompatible, but not its usage as annotation.

guillermocalvo commented 3 months ago

@siom79 Sorry about the trouble my PR caused. I believe your latest changes should fix #395.

I agree that ANNOTATION_MODIFIED_INCOMPATIBLE is not really needed; especially since you introduced the compatibility change ANNOTATION_MODIFIED which checks annotation elements.

Therefore, I think this issue can be closed now, because the functionality is complete. However, I noticed that ANNOTATION_MODIFIED is not currently being tested, so I prepared a small PR to hopefully expand test coverage. Thanks!

siom79 commented 3 months ago

@guillermocalvo No problem, this can happen. While merging the PR I also haven't realized that annotating an annotation's element with the annotation itself will cause an endless recursion.

Thanks for the PR.

Version 0.21.1 is on its way.