Support tests written in kotlin with parameter types that are `@JvmInline` value classes

zach-klippenstein commented 1 year ago

Kotlin has a feature called "value classes" which currently desugar into primitive types. E.g. the following defines a class that will compile down to just being an float anywhere it's used:

@JvmInline value class Temp(val value: Float)

When such a class is used as the type of a constructor parameter, Kotlin generates an additional constructor with an extra parameter of type DefaultConstructorMarker. E.g. given the above class, this test class will actually have two constructors:

@RunWith(TestParameterInjector::class)
class TestClass(temp: Temp)

This will generate constructors (as described by IntelliJ's "Show Kotlin bytecode" view):

private <init>(F)V
public synthetic <init>(FLkotlin/jvm/internal/DefaultConstructorMarker;)V

When ran as a test, this library will see two constructors and give the following error message: "Expected exactly one constructor, but got {list of constructors}".

Use case

There are a number of reasons kotlin tests might need to be parameterized on value classes. E.g., the Jetpack Compose project frequently uses value classes to replicate enums but get around the various exhaustiveness checks that Kotlin performs for backwards compatibility. So we'd like to be able to parameterize tests on those types. We still need to explicitly list all the possible values, which is fine, but right now the runner will just crash.

nymanjens commented 1 year ago

So if I understand this correctly, this problem would be fixed with the following workaround:

Ignore all constructors where there is only argument of a type with name "DefaultConstructorMarker".

After this filter, there would only be one constructor left.

Would that fix your problem?

nymanjens commented 1 year ago

I added a unit test for Kotlin and noticed that @JvmInline value classes have multiple issues:

They mess up method names, and therefore the test names. This is fixed in ea2ccf7
They are ~impossible to assign as field names because they are not inlined as fields (they show up as the wrapper class in Java reflection). The only solution is to use a valueProvider. See https://github.com/google/TestParameterInjector/commit/8ace594a087a509e5eabf03e24886c1ae3ed825e for more info.
Constructors are duplicated (subject of this issue). See below

My findings so far about the constructor:

If you have a constructor like this:

class MyTest {
    constructor(
      @TestParameter("1", "2") width: Int,
      @TestParameter("5", "5.6") height: DoubleValueClass
    ) {...}
}
@JvmInline value class DoubleValueClass(val onlyValue: Double)

the Kotlin compiler will generate two Java constructors:

private MyTest(int, double) {...}
public MyTest(
    @TestParameter({"1", "2"}) int,
    @TestParameter({"5", "5.6"}) double,
    DefaultConstructorMarker) {...}

The first one is probably the one that TestParameterInjector would want to call, since it cannot construct a DefaultConstructorMarker. However, none of the parameters are annotated, so that constructor alone is invalid.

The second one has the right annotations, but has the unconstructable DefaultConstructorMarker.

A working (untested) solution would to combine the two:

Use the public constructor to get the annotations
Override the private visibility of the first constructor and call it with the desired values

Analysis on the required work

To avoid the whole codebase from needing to know about Kotlin hacks, it would be elegant IMO to pass along a 'fake' constructor that simulates the existence of a public MyTest(@TestParameter int, @TestParameter double) by delegating to both Kotlin-generated constructors.

It seems to be impossible to construct a custom java.lang.reflect.Constructor (ditto for the Guava wrapper: Invokable). Therefore, we'd need a custom type that wraps a constructor. However, this would then need a lot of changes in the codebase, including backwards incompatible change to interfaces that are used by other Google projects. This needs a lot of changes, just for working around a Kotlin feature that clearly was not intended to be used via Java reflection.

Even if all of this works, IIUC, there is no guarantee that this will keep working going forward as Kotlin changes. Also, when the JVM supports value classes, presumably @JvmInline will be deprecated, and Java and Kotlin can hopefully again interoperate without the need for these marker types.

In conclusion: I don't think supporting this feature justifies the immediate cost nor the long term maintenance cost.

JakeWharton commented 1 year ago

The second one has the right annotations, but has the unconstructable DefaultConstructorMarker.

Note that the value supplied for this argument when invoking is always null. This is true even for the bytecode emitted by the Kotlin compiler. See line 45 of the bytecode on this example: https://kotlin.godbolt.org/z/d5Gnj8zrq

You actually left out one more requirement to invoke this variant of the constructor. You need to call the "constructor-impl" function on the value class to ensure invariants are maintained. See https://kotlin.godbolt.org/z/8q737qT78. An alternative is to call the normal value class constructor which is akin to boxing and will allocate.

JakeWharton commented 1 year ago

Even if all of this works, IIUC, there is no guarantee that this will keep working going forward as Kotlin changes.

If this changed all existing bytecode would fail to link at runtime. Value classes are stable and Kotlin has a strong binary compatibility policy.

google / TestParameterInjector