Skip to content

Fix for issue #1103.#1449

Merged
Raibaz merged 2 commits intomockk:masterfrom
sdetilly:issues-1103-mocking_function_with_optional_value_class
Nov 20, 2025
Merged

Fix for issue #1103.#1449
Raibaz merged 2 commits intomockk:masterfrom
sdetilly:issues-1103-mocking_function_with_optional_value_class

Conversation

@sdetilly
Copy link
Contributor

@sdetilly sdetilly commented Nov 19, 2025

The issue here was that innermostBoxedClass is recursive and was causing an infinite loop for nullable value classes, which are not inlined in JVM bytecode (they remain boxed), so it kept trying to unwrap something that shouldn't be unwrapped.

Also added a check to skip the boxedClass call entirely for nullable return types, since nullable value classes are not inlined and should be treated as regular boxed types.

Added 2 tests for this usecase and all tests pass using gradle check

Link to issue: #1103

val isReturnNullable = kFunction.returnType.isMarkedNullable
val isPrimitive = resultType.innermostBoxedClass.java.isPrimitive
// Use boxedClass (one level) instead of innermostBoxedClass (recursive) to avoid infinite loops (issue #1103)
val isPrimitive = resultType.boxedClass.java.isPrimitive
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usage of innermostBoxedClass was introduced in this commit to allow mocking complex value classes and fix #1308, so using boxedClass directly will likely cause a regression on it.

How about using a hybrid approach and recursively calling boxedClass with a limit on the number of recursions?

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 see, I would've expected some tests to be broken if my change would cause a regression. I'll look into the hybrid approach, thanks!

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 have added a recursion limit of 10. Do you think this is enough for all cases?

@Raibaz Raibaz merged commit b88bfce into mockk:master Nov 20, 2025
26 checks passed
@nishatoma
Copy link
Contributor

I believe this is causing another issue now:

#1475

When I try to add a || in JvmMockFactoryHelper line 181 to accept nullable value classes then it passes the test cases mentioned in #1475

/ For nullable value classes, we should NOT call boxedClass as they are not inlined in JVM bytecode
        // This prevents infinite loops in value class type resolution (issue #1103)
        val returnType: KClass<*> = if (returnTypeClass.isValue || !returnTypeNullable) returnTypeClass.boxedClass else returnTypeClass

Otherwise the current code in master will cause verification to fail this test case:

@Test
    fun `when mock with optional value then test passes`() {
        val mocked: Mocked = mockk()
        every { mocked.something(any()) } returns Id("bar")

        val tested = Tested(mocked)

        tested.test()

        verify { mocked.something(Id("foo")) }
    }

Where:

@JvmInline
        value class Id(val id: String)

        interface Mocked {
            fun something(data: Id): Id?
        }

        class Tested(
            private val mocked: Mocked
        ) {
            fun test() {
                val result = mocked.something(Id("foo"))
            }
        }

@nishatoma
Copy link
Contributor

Perhaps we just need to use the innermostBoxedClass() extension function that was introduced, within JvmMockFactoryHelper then it wouldn't matter if the value class was nullable or not, @sdetilly what are your thoughts on this?

@sdetilly
Copy link
Contributor Author

I was able to reproduce the issue. The problem occurs during verification with exact value matching when the return type is nullable.

I've implemented a fix and will a open a new PR:

  • Made innermostBoxedClass public in ValueClassSupport.kt
  • Updated JvmMockFactoryHelper.kt to use innermostBoxedClass() for all cases (nullable and non-nullable)
  • Added your test case to ValueClassTest.kt and it now passes (So I don't think I broke anything)

The innermostBoxedClass approach is better because it has built-in recursion protection while still properly handling both nullable and nested value classes. This resolves issues #1103, #1308, and #1475 together.

All value class tests pass with this fix. Thank you for the issue report and suggested solution 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants