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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

#152 support value classes #849

Merged
merged 22 commits into from Jul 26, 2022
Merged

#152 support value classes #849

merged 22 commits into from Jul 26, 2022

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Jul 20, 2022

Update:

any() and slot() now support value classes!

Summary

  • Added new 'expect' functions for 'unboxing' a value class to get its internal type, and to 'wrap' an object instance within a value class.
  • refactored RecordingState matcher to account for value classes
  • refactored the SignatureValueGenerator jvm impl to recursively generate values for value classes

resolves #152
resolves #791
resolves #683
resolves #847

Fix (maybe?) #778?

Notes:


This PR will hopefully provide some support for value classes #152

WIP - just some failing tests at the moment (credit to @qoomon)

This PR is blocked at the moment because the Kotlin language level is set to 1.4 - see #850 MockK is now on language level 1.5 馃帀

@Raibaz
Copy link
Collaborator

Raibaz commented Jul 20, 2022

I'm fine with upgrading Kotlin to 1.7.0 by default, just make sure we maintain backwards compatibility at least to a couple versions back, i.e. we can drop support to 1.4.* if needed but I'd keep at least support to 1.5.*.

@aSemy
Copy link
Contributor Author

aSemy commented Jul 22, 2022

Okay, I think I've cracked the code.

A quick summary:

To fix slots, TypedMatcher needs to account for value classes

/**
 * Checks if argument is of specific type
 */
interface TypedMatcher {
    val argumentType: KClass<*>

    fun checkType(arg: Any?): Boolean {
        return when {
            argumentType.simpleName === null -> true
            argumentType.isValue             -> {
                val unboxedClass = InternalPlatformDsl.unboxClass(argumentType)
                return unboxedClass.isInstance(arg)
            }
            else                             -> argumentType.isInstance(arg)
        }
    }
}

And to handle matching stubs to invocations, JvmSignatureValueGenerator also needs to account for value classes.

class JvmSignatureValueGenerator(val rnd: Random) : SignatureValueGenerator {
    override fun <T : Any> signatureValue(
        cls: KClass<T>,
        anyValueGeneratorProvider: () -> AnyValueGenerator,
        instantiator: AbstractInstantiator,
    ): T {

        if (cls.isValue) {
            val valueCls = InternalPlatformDsl.unboxClass(cls)
            val valueSig = signatureValue(valueCls, anyValueGeneratorProvider, instantiator)

            val constructor = cls.primaryConstructor!!.apply { isAccessible = true }
            return constructor.call(valueSig)
        }

        return cls.cast(
            when (cls) {
                java.lang.Boolean::class -> rnd.nextBoolean()
                java.lang.Byte::class -> rnd.nextInt().toByte()
                java.lang.Short::class -> rnd.nextInt().toShort()
                java.lang.Character::class -> rnd.nextInt().toChar()
                java.lang.Integer::class -> rnd.nextInt()
                java.lang.Long::class -> rnd.nextLong()
                java.lang.Float::class -> rnd.nextFloat()
                java.lang.Double::class -> rnd.nextDouble()
                java.lang.String::class -> rnd.nextLong().toString(16)

                else ->
                    @Suppress("UNCHECKED_CAST")
                    anyValueGeneratorProvider().anyValue(cls, isNullable = false) {
                        instantiator.instantiate(cls)
                    } as T
            }
        )
    }
}

In order to do this I created a common function on InternalPlatformDsl, and in JVM implemented it using the code from ValueClassSupport.kt

actual object InternalPlatformDsl {

    // ...

    actual fun unboxClass(cls: KClass<*>): KClass<*> {
        if (!cls.isValue) return cls

        // get backing field
        val backingField = cls.valueField()

        // get boxed value
        return backingField.returnType.classifier as KClass<*>
    }
}

I'll commit what I have. I think there are some edgecases if a value class' value is also a value class.

The test coverage for value classes is very low - basically every MockK API operation should verify it is compatible with value classes.

qoomon
qoomon approved these changes Jul 22, 2022
qoomon
qoomon approved these changes Jul 22, 2022
@aSemy aSemy marked this pull request as ready for review July 22, 2022 17:11
// TODO this is copy-pasted from ValueClassSupport
// I will try to move that class so it's available here

private val valueClassFieldCache = mutableMapOf<KClass<out Any>, KProperty1<out Any, *>>()

private fun <T : Any> KClass<T>.valueField(): KProperty1<out T, *> {
@Suppress("UNCHECKED_CAST")
return valueClassFieldCache.getOrPut(this) {
require(isValue) { "$this is not a value class" }

// value classes always have a primary constructor...
val constructor = primaryConstructor!!.apply { isAccessible = true }
// ...and exactly one constructor parameter
val constructorParameter = constructor.parameters.first()
// ...with a backing field
val backingField = declaredMemberProperties
.first { it.name == constructorParameter.name }
.apply { isAccessible = true }

backingField
} as KProperty1<out T, *>
}
Copy link
Contributor Author

@aSemy aSemy Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to resolve this

TODO this is from ValueClassSupport.kt - try and refactor to avoid copy-pasting

  • ValueClassSupport.kt is in project mockk-agent-jvm
  • InternalPlatformDsl is in project mockk-dsl-jvm

I'm looking for suggestions. Maybe it's okay just to have it copy and pasted?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep i think if they are in those two separate modules there's no better way and we can live with it being copypasted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment where the same functionality is implemented would help, i case that code ever needs to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, and I've made an issue to try and de-duplicated it later #857

@Raibaz
Copy link
Collaborator

Raibaz commented Jul 22, 2022

Thanks a lot for putting this together!

I'll take a look early next week.

@aSemy
Copy link
Contributor Author

aSemy commented Jul 23, 2022

There's a bug. I tried adding a test case for #729, but it causes MockK to hang indefinitely!

    /** https://github.com/mockk/mockk/issues/729 */
    @Test
    fun `verify extension function with UInt return can be stubbed`() {

        val fn = mockk<String.() -> UInt>()

        every { "string".fn() } returns 777u

        val result = "string".fn()

        assertEquals(777u, result)
    }

update: it gets stuck in a loop somewhere

2022-07-23 12:38:52 [Test worker] TRACE io.mockk.impl.JvmMockKGateway - Starting JVM MockK implementation. Java version = 1.8.0_332. 
2022-07-23 12:38:52 [Test worker] TRACE i.m.proxy.jvm.JvmMockKAgentFactory - Byte buddy agent installed
2022-07-23 12:38:52 [Test worker] TRACE i.m.p.jvm.dispatcher.BootJarLoader - Bootstrap class loaded io.mockk.proxy.jvm.dispatcher.JvmMockKDispatcher
2022-07-23 12:38:52 [Test worker] TRACE i.m.p.jvm.dispatcher.BootJarLoader - Bootstrap class loaded io.mockk.proxy.jvm.dispatcher.JvmMockKWeakMap
2022-07-23 12:38:52 [Test worker] TRACE i.m.p.jvm.dispatcher.BootJarLoader - Bootstrap class loaded io.mockk.proxy.jvm.dispatcher.JvmMockKWeakMap$StrongKey
2022-07-23 12:38:52 [Test worker] TRACE i.m.p.jvm.dispatcher.BootJarLoader - Bootstrap class loaded io.mockk.proxy.jvm.dispatcher.JvmMockKWeakMap$WeakKey
2022-07-23 12:39:19 [Test worker] DEBUG i.m.i.i.AbstractMockFactory - Creating mockk for Function1 name=#1
2022-07-23 12:39:25 [Test worker] TRACE i.m.i.i.AbstractMockFactory - Building proxy for Function1 hashcode=37efd131
2022-07-23 12:39:33 [Test worker] TRACE i.m.p.j.t.JvmInlineInstrumentation - Retransforming [Ljava.lang.Class;@5cbe877d
2022-07-23 12:39:33 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Building subclass proxy for interface kotlin.jvm.functions.Function1 with additional interfaces []
2022-07-23 12:39:40 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Instantiating proxy for interface kotlin.jvm.functions.Function1 via instantiator
2022-07-23 12:39:40 [Test worker] TRACE i.m.proxy.jvm.ObjenesisInstantiator - Skipping instantiation subclassing class kotlin.jvm.functions.Function1$Subclass0 because class is not abstract.
2022-07-23 12:39:40 [Test worker] TRACE i.m.proxy.jvm.ObjenesisInstantiator - Creating new empty instance of class kotlin.jvm.functions.Function1$Subclass0
2022-07-23 12:39:40 [Test worker] TRACE i.m.i.recording.CommonCallRecorder - Starting stubbing
2022-07-23 12:39:51 [Test worker] TRACE i.m.i.instantiation.JvmInstantiator - Building empty instance void
2022-07-23 12:39:51 [Test worker] TRACE i.m.proxy.jvm.ObjenesisInstantiator - Creating new empty instance of class java.lang.Void
2022-07-23 12:39:53 [Test worker] TRACE i.m.i.i.AbstractMockFactory - Building proxy for Any hashcode=2a3a299
2022-07-23 12:39:57 [Test worker] TRACE i.m.p.j.t.JvmInlineInstrumentation - Retransforming [Ljava.lang.Class;@219f4597
2022-07-23 12:39:57 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Taking instance of class java.lang.Object itself because it is not abstract and no additional interfaces specified.
2022-07-23 12:39:59 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Instantiating proxy for class java.lang.Object via instantiator
2022-07-23 12:40:01 [Test worker] TRACE i.mockk.impl.recording.JvmAutoHinter - Auto hint for 2-th call: class kotlin.UInt
2022-07-23 12:40:06 [Test worker] TRACE i.m.i.i.AbstractMockFactory - Building proxy for UInt hashcode=99a65d3
2022-07-23 12:40:13 [Test worker] TRACE i.m.p.j.t.JvmInlineInstrumentation - Retransforming [Ljava.lang.Class;@32fdec40
2022-07-23 12:40:17 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Taking instance of class kotlin.UInt itself because it is final.
2022-07-23 12:40:19 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Instantiating proxy for class kotlin.UInt via instantiator
2022-07-23 12:40:19 [Test worker] TRACE i.m.proxy.jvm.ObjenesisInstantiator - Creating new empty instance of class kotlin.UInt
2022-07-23 12:40:25 [Test worker] TRACE i.mockk.impl.recording.JvmAutoHinter - Auto hint for 4-th call: class kotlin.UInt
2022-07-23 12:40:28 [Test worker] TRACE i.m.i.i.AbstractMockFactory - Building proxy for UInt hashcode=99a65d3
2022-07-23 12:40:35 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Taking instance of class kotlin.UInt itself because it is final.
2022-07-23 12:40:37 [Test worker] TRACE io.mockk.proxy.jvm.ProxyMaker - Instantiating proxy for class kotlin.UInt via instantiator
2022-07-23 12:40:37 [Test worker] TRACE i.m.proxy.jvm.ObjenesisInstantiator - Creating new empty instance of class kotlin.UInt
2022-07-23 12:40:41 [Test worker] TRACE i.m.i.i.AbstractMockFactory - Building proxy for UInt hashcode=99a65d3

I guess this issue is related: #656

@qoomon
Copy link
Contributor

qoomon commented Jul 23, 2022

I think we can reduce the code of ValueClassSupport, have look at this. This looks so much cleaner to me.

package me.qoomon.enhancements.kotlin

import kotlin.reflect.KClass
import kotlin.reflect.KProperty1
import kotlin.reflect.full.declaredMemberProperties
import kotlin.reflect.jvm.isAccessible

/**
 * Underlying property value of a **`value class`** or self
 */
val <T : Any> T.boxedValue: Any?
    @Suppress("UNCHECKED_CAST")
    get() = if (!this::class.isValue) this
    else (this::class as KClass<T>).boxedProperty.get(this)

/**
 * Underlying property class of a **`value class`** or self
 */
val KClass<*>.boxedClass: KClass<*>
    get() = if (!this.isValue) this
    else this.boxedProperty.returnType.classifier as KClass<*>

/**
 * Underlying property of a **`value class`**
 */
private val <T : Any> KClass<T>.boxedProperty: KProperty1<T, *>
    get() = if (!this.isValue) throw UnsupportedOperationException("$this is not a value class")
    // value classes always have exactly one property
    else this.declaredMemberProperties.first().apply { isAccessible = true }

@aSemy
Copy link
Contributor Author

aSemy commented Jul 23, 2022

I think we can reduce the code of ValueClassSupport

Agreed - but fun <T : Any> KClass<T>.isValueClass() is necessary because isValue throws an exception when used from fun <T : Any> T.boxedValue(): Any?, I think when T is a KFunction

@qoomon
Copy link
Contributor

qoomon commented Jul 23, 2022

Is it the native isValue property that's throwing an exception or my polyfill implemention?

@qoomon
Copy link
Contributor

qoomon commented Jul 23, 2022

@aSemy I wasn't able to reproduce an exception by calling .isValue on a KFunction instance

@qoomon
Copy link
Contributor

qoomon commented Jul 23, 2022

@aSemy there was a bug in my reduced ValueClassSupport code in the previous comment. I fixed it already within the comment. .boxedClassneeds to check forif (this.isValue)instead ofif (this::class.isValue)`

refactor: simplify ValueClassSupport
鈥52-value-classes

# Conflicts:
#	agent/android/src/main/kotlin/io/mockk/ValueClassSupport.kt
#	agent/jvm/src/main/kotlin/io/mockk/ValueClassSupport.kt
@aSemy
Copy link
Contributor Author

aSemy commented Jul 24, 2022

Thanks @qoomon! Merged in, and I made some minor tweaks.

I also added some more tests for nested value classes - I'll disable them for now because they can be handled in a later PR.

However the local-extension function tests need to be resolved - they cause MockK to go into an infinite loop. Even if there's a way to make MockK throw an exception and say "this isn't supported yet" - is that possible?

@aSemy
Copy link
Contributor Author

aSemy commented Jul 24, 2022

The issue with tests like this

https://github.com/aSemy/mockk/blob/592bdaf710e1be674742afa61147dfad584d6072/mockk/common/src/test/kotlin/io/mockk/it/ValueClassTest.kt#L385-L410

failing is that value classes are handled inconsistently in MockK.

For example, if I comment out this line (or another one like it, I can't remember exactly)

https://github.com/aSemy/mockk/blob/592bdaf710e1be674742afa61147dfad584d6072/agent/android/src/main/kotlin/io/mockk/proxy/android/advice/Advice.kt#L84

then the extension function tests work

It's hard to keep track of all of the entry and exit points of MockK's stubbing and mocking and such.


I think a proper fix to this is to create a new internal KClassData type.

Whenever MockK has to handle a KClass, it needs to determine if it's a value class or not. If it's not, then no problem, continue as normal. Else, create a wrapper for the value class that can recursively unwrap the actual type (the one the compiler sees) and also wrap instances.

Creating a new type for this would ensure consistent behaviour across MockK.

sealed interface KClassData<T> {
  val cls: KClass<T>

  /** A regular [KClass] - treat as normal */
  @JvmInline
  value class Cls<T>(
    override val cls: KClass<T>
  ) : KClassData<T>
  
  /** A value class - must be wrapped/unwrapped as required */
  data class VCls<T, E>(
    override val cls: KClass<T>,
  ): KClassData<T> {
    val unwrappedType: KClass<E> = ...
    
    fun wrap(any: Any?): T = // recursively wrap

    fun unwrap(instance: T): E = // recursively unwrap
  }
}

I'm not going to do it in this ticket - just throwing out ideas. It would be a big refactor.

@aSemy
Copy link
Contributor Author

aSemy commented Jul 24, 2022

@qoomon @Raibaz can you cancel the checks? I think they're stuck. I don't want to burn through all your GitHub Action hours :)

build.gradle Outdated
tasks.withType(Test).configureEach {
timeout.set(Duration.ofMinutes(2))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this timeout for all test tasks - I think 2 minutes should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃槑馃憤

build.gradle Outdated
tasks.withType(Test).configureEach {
timeout.set(Duration.ofMinutes(2))
}
Copy link
Contributor Author

@aSemy aSemy Jul 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this timeout to prevent the tests from hanging. I think 2 5 minutes should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

qoomon
qoomon approved these changes Jul 24, 2022
build.gradle Outdated
tasks.withType(Test).configureEach {
timeout.set(Duration.ofMinutes(2))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃槑馃憤

@Raibaz
Copy link
Collaborator

Raibaz commented Jul 24, 2022

Thanks a lot to both of you for looking so deep into this!

I'll review the code over the next days.

@qoomon
Copy link
Contributor

qoomon commented Jul 25, 2022

Thank you for this great project.

@Raibaz
Copy link
Collaborator

Raibaz commented Jul 26, 2022

Ok, this is really cool, thanks a lot!

Merging this as soon as the build finishes.

@Raibaz Raibaz merged commit 02a6d45 into mockk:master Jul 26, 2022
44 checks passed
@danieldisu
Copy link

Thank you!

@dybarsky
Copy link

Thank you @Raibaz

@qoomon
Copy link
Contributor

qoomon commented Jul 29, 2022

@Raibaz I would have liked to be mentioned in the release changelog as well 馃槄.

@Raibaz
Copy link
Collaborator

Raibaz commented Jul 29, 2022

Damn you're right, sorry!

I edited the changelog.

@maciejtulaza
Copy link

maciejtulaza commented Jul 30, 2022

hey, I still notice issues with any() and value classes.

I described two problems I found here: #847 (comment)

Not sure if this is related to this issue, but I encountered it because I first had the issue described here, then upgraded the version of MockK, error was gone but another two came up to life 馃槄 (you can see it in the issue I linked)

@qoomon
Copy link
Contributor

qoomon commented Jul 30, 2022

@aSemy @Raibaz I found the bug causing Duration and Result errors.
Because declaredMemberProperties includes properties without backing fields as well just .first() is not sufficient . We need to add following filter .first { it.javaField != null }

Fixed code. WDYT?

private val <T : Any> KClass<T>.boxedProperty: KProperty1<T, *>
        get() = if (!this.isValue_safe) {
            throw UnsupportedOperationException("$this is not a value class")
        } else {
            // value classes always have exactly one property with a backing field
            this.declaredMemberProperties.first { it.javaField != null }.apply { isAccessible = true }
        }

@qoomon
Copy link
Contributor

qoomon commented Jul 30, 2022

I'll create a PR this evening, including some tests.

@qoomon
Copy link
Contributor

qoomon commented Jul 30, 2022

Have a look at #872

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment