Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
osipxd opened this issue Apr 5, 2024 · 3 comments
Open
Labels
enhancement New feature or request jvm

Comments

@osipxd
Copy link
Member

osipxd commented Apr 5, 2024

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 fzhinkin added the jvm label Apr 9, 2024
@fzhinkin
Copy link
Collaborator

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

@fzhinkin fzhinkin added the enhancement New feature or request label Apr 15, 2024
@osipxd
Copy link
Member Author

osipxd commented Apr 17, 2024

Great! I have the following questions:

  • Should it be implemented only for new Klib format?
  • It will be a breaking change so should it be done before Klib finaly released?
  • 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?
  • 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.

@fzhinkin
Copy link
Collaborator

  • 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request jvm
Projects
None yet
Development

No branches or pull requests

2 participants