Kotlin / binary-compatibility-validator

Public API management tool
Apache License 2.0
759 stars 55 forks source link

Removal of default parameter is binary compatible but breaks runtime #216

Open osipxd opened 3 months ago

osipxd commented 3 months ago

If we remove default value for one of default parameters in function, this change doesn't affect API dump, however it can lead to unexpected behaviour or NPE in runtime (at least on JVM).

Is it the intended behaviour that default parameters are not marked somewhat in API dump? Or is it out of scope of BCV responsibility as soon as binary compatibility is not broken?

Example:

// for this declaration
fun foo(intParam: Int = -1,  stringParam: String = "")

// and these calls
foo(intParam = 42)
foo(stringParam = "value")
// will be generated this synthetic method
public static void foo$default(Foo var0, int var1, String var2, int var3, Object var4) {
    if ((var3 & 1) != 0) {
        var1 = -1;
    }

    if ((var3 & 2) != 0) {
        var2 = "";
    }

    var0.foo(var1, var2);
}

// and the calls will be
foo$default(this, 42, (String)null, 2, (Object)null);
foo$default(this, 0, "value", 1, (Object)null);

So if we remove default value for intParam, function call foo$default(this, 0, "value", 1, (Object)null); will still be valid, however it will lead to unexpected behaviour. intParam will be 0 instead of -1. In case we remove default value for stringParam, function call foo$default(this, 42, (String)null, 2, (Object)null); will still be valid, however it will lead to NPE due to intrinsic non-null check failure on stringParam.

fzhinkin commented 2 months ago

Sorry for the late reply. Indeed, it does make sense to capture information about parameters with default arguments in a generated dump.

osipxd commented 2 months ago

Great! I have the following questions:

fzhinkin commented 2 months ago
  • Should it be implemented only for new Klib format?
  • It will be a breaking change so should it be done before Klib finaly released?

In fact, Klib format already tracks parameters with default arguments, so it should be added only for JVM dumps. It indeed will be a breaking change, but the severity of the problem it solves justifies such a change.

Anyway, we can make a new behavior optional and, a few releases later, make it default.

  • Do you have an idea on how it could be done? Will it be some mark on each parameter having default value, or only the bitmask parameter will have some additional info?

I was thinking about printing all extra metadata near an affected declaration, but there is no particular proposal so far.

  • Do you think this change could be contributed by someone who is not familiar with the codebase? If so, I want to try to do it.

Sure! Feel free to implement and contribute it; let us just finalize how all the extra info should be presented in the dump first (I hope it'll happen within a few weeks).