siom79 / japicmp

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

When method signature changes japicmp does not recognise it as a change, it is a distinct add and a remove #319

Closed davidnewcomb closed 2 years ago

davidnewcomb commented 2 years ago

Same problem in 0.15.4 + 0.15.6

Old code:

@API.Public
public class JAPITestClass0005 {

   @API.Private
   public boolean methodPublicApi_1__NO(int p1) {
       return (p1==0);
   }

}

New code:

@API.Public
public class JAPITestClass0005 {

   @API.Private
   public boolean methodPublicApi_1__NO(String p1, int p2, int p3) {
       return "".equals(p1);
   }

}

The signature is the only change the method name remains the same. However, I get 2 changes. I put this code in the groovy script to help me see what is happening:

JApiClass jApiClass = jApiClasses.iterator().next()
String fqn = jApiClass.getFullyQualifiedName()
System.out.println(fqn +':# methods:' + jApiClass.getMethods().size() )

for (int i = 0 ; i < jApiClass.getMethods().size() ; ++i) {
        JApiMethod m = jApiClass.getMethods().get(i)
        System.out.println('method: ' + m.toString())
}

And get this out (I have added extra spaces for you to make it nice :) )

japicmptest.JAPITestClass0005:# methods:2
method:
        JApiMethod [
                oldMethod=japicmptest.JAPITestClass0005.methodPublicApi_1__NO(int),
                newMethod=n.a.,
                returnType=JApiReturnType [
                        oldReturnTypeOptional=boolean,
                        newReturnTypeOptional=value absent,
                        changeStatus=REMOVED
                ],
                getCompatibilityChanges()=[METHOD_REMOVED]
        ]

method:
        JApiMethod [
                oldMethod=n.a.,
                newMethod=japicmptest.JAPITestClass0005.methodPublicApi_1__NO(java.lang.String,int,int),
                returnType=JApiReturnType [
                        oldReturnTypeOptional=value absent,
                        newReturnTypeOptional=boolean,
                        changeStatus=NEW
                ],
                getCompatibilityChanges()=[METHOD_ADDED_TO_PUBLIC_CLASS]
        ]

I think that there is only 1 method with an old and new and so it should return something like this:

        JApiMethod [
                oldMethod=japicmptest.JAPITestClass0005.methodPublicApi_1__NO(int),
                newMethod=japicmptest.JAPITestClass0005.methodPublicApi_1__NO(java.lang.String,int,int),
                returnType=JApiReturnType [
                        oldReturnTypeOptional=boolean,
                        newReturnTypeOptional=boolean,
                        changeStatus=UNCHANGED
                ],
                changeStatus=MODIFED,
                getCompatibilityChanges()=[METHOD_MODIFED_PUBLIC_CLASS]
        ]

The old method has annotations which I need to know in order to work out if I can output this method in the results.

It's starting to look like I need some extra magic to find both sides of the modification equation and glue them together but really I think that work falls on your side of the fence ...as there is still just one method!

Does this look right to you? Am I missing an option to glue them together? Or are they supposed to be separate changes?

davidnewcomb commented 2 years ago

I just remembered about polymorphism, so maybe my request is impossible as there can be many different signatures to the same method. So it might be impossible to match them up. Grrrr! (but with understanding)

siom79 commented 2 years ago

You are right. When you have more than one method with the same name, then it is hard to say which one changed.

m(short s)
m(int i)

Turns to

m(double d)
m(float f)

Which one changed to which one?

David Newcomb @.***> schrieb am Do., 3. Feb. 2022, 18:55:

I just remembered about polymorphism, so maybe my request is impossible as there can be many different signatures to the same method. So it might be impossible to match them up. Grrrr! (but with understanding)

— Reply to this email directly, view it on GitHub https://github.com/siom79/japicmp/issues/319#issuecomment-1029249824, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4U7NHUL6EXAAHU5VYXNRDUZK6S3ANCNFSM5NPST7EQ . 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 are subscribed to this thread.Message ID: @.***>

davidnewcomb commented 2 years ago

I was thinking that if there was just one in the old and new then you could assume, but this is just my test case. In real life there are classes with loads of polymorphic signatures.

I'm closing this, as having given it a second thought, I don't think it's something that should be done. Having an 'add' and 'remove' does make more sense.