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

Incorrect BC error for METHOD_RETURN_TYPE_CHANGED when moving a method to a generic superclass #330

Closed joel-costigliola closed 2 years ago

joel-costigliola commented 2 years ago

japicmp reported this commit to break BC, some errors were correctly reported but the ones related to METHOD_RETURN_TYPE_CHANGED wer not. The report is https://github.com/assertj/assertj-core/runs/7263261276?check_suite_focus=true and the part I'm talking about is:

org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration$Builder.withIgnoredFields(java.lang.String[]):METHOD_RETURN_TYPE_CHANGED
org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration$Builder.withIgnoredFieldsMatchingRegexes(java.lang.String[]):METHOD_RETURN_TYPE_CHANGED
org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration$Builder.withIgnoredFieldsOfTypes(java.lang.Class[]):METHOD_RETURN_TYPE_CHANGED

What happened in this commit is a refactoring of the RecursiveComparisonConfiguration$Builder where the 3 methods above were moved to a superclass AbstractBuilder<BUILDER_TYPE extends AbstractBuilder<BUILDER_TYPE>>, ex:

    public BUILDER_TYPE withIgnoredFields(String... fieldsToIgnore) {
      this.ignoredFields = fieldsToIgnore;
      return thisBuilder;
    }

As the class RecursiveComparisonConfiguration$Builder specifies it extends AbstractBuilder<Builder> the above inherited method actually returns RecursiveComparisonConfiguration$Builder which is what it used to do before that commit.

There is a workaround to avoid this issue: overriding the methods in RecursiveComparisonConfiguration$Builder, ex:

    @Override
    public Builder withIgnoredFields(String... fieldsToIgnore) {
      return super.withIgnoredFields(fieldsToIgnore);
    }

This does not change the behavior of the code and I believe it demonstrates that the BC error is incorrectly reported. Having said that I'm not sure it is easy to address this issue as it involves generics which are erased at runtime.

siom79 commented 2 years ago

As you have already mentioned, the point here is the generic return type.

Generics in Java are implemented using a type erasure mechanism. The compiler translates all type parameters in the source code to their bounding type in the class file. A type parameter's bounding type is Object if a bound type is not specified.

The old method signature was

Builder withIgnoredFields(String... fieldsToIgnore)

The new method after type erasure will be (as BUILDER_TYPE extends AbstractBuilder<BUILDER_TYPE>):

AbstractBuilder withIgnoredFields(String... fieldsToIgnore)

Hence; the return type changed.

joel-costigliola commented 2 years ago

Alright thanks for the explanation, I guess I just need to override the methods then.

Feel free to close the ticket if you think there is not anything else to do here :)