bnorm / kotlin-power-assert

Kotlin compiler plugin to enable diagrammed function calls in the Kotlin programming language
Apache License 2.0
575 stars 15 forks source link

Better Nullable Support #101

Closed jamesward closed 9 months ago

jamesward commented 11 months ago

I'm not sure if I'm using it wrong or if I hit an unsupported edge case but I was trying this:

        data class Role(val title: String)
        data class Person(val name: String, val role: Role)
        val people = listOf(
                Person("Joe", Role("Engineer")),
                Person("Sally", Role("Manager")),
                Person("Bob", Role("QA")),
        )

        assert(people.find { it.name == "Ralph" }!!.role.title.equals("qa", true))

I'd hoped to see the nice diagram of where I hit the null, but instead I just get:

MyTest > one FAILED
    java.lang.NullPointerException
        at MyTest.one(MyTest.kt:15)\

BTW, thanks for this awesome tool!

bnorm commented 11 months ago

We discussed something similar here https://github.com/bnorm/kotlin-power-assert/discussions/58. This plugin doesn't handle exceptions during the assertion expression. Handling exceptions could require lots of additional nontrivial code transformations, and I haven't found a good use-case for it that preserves code soundness.

I could see !! being a special case if found in the assert expression since we know what exception will be thrown and can specifically throw it with a custom message. Would this be enough for your needs? Or are you hoping for something more generic?

For now, in this case, you could use safe access. assert(people.find { it.name == "Ralph" }?.role?.title?.equals("qa", true) == true)

Or you could break out the find call into a local variable to smart-cast it during check. (Though this loses the diagram on people when it fails.)

val ralph = people.find { it.name == "Ralph" }
assert(ralph != null && ralph.role.title.equals("qa", true))
jamesward commented 11 months ago

Thanks for the details. Here is another idea... What if assert took a Boolean? - then could you figure out where in the call chain things went null and display it?

bnorm commented 11 months ago

could you figure out where in the call chain things went null and display it?

The plugin actually supports diagramming of any kind of parameter. This allows you to use any assertion library instead of just the language assert; like assertTrue from kotlin.test for example, so you gain the benefits of smart-casting from function contracts. So you could write your own version of assert which took Boolean? and it would diagram that parameter. The repository README contains some brief examples of how to write and configure custom functions for transformation.

jamesward commented 11 months ago

Thanks for the details. I tried this:

    fun assert(value: Boolean?) {
        if (value != null) {
            kotlin.assert(value)
        }
        else {
            throw AssertionError("value was null")
        }
    }

    @Test
    fun one() {
        data class Role(val title: String)
        data class Person(val name: String, val role: Role)
        val people = listOf(
                Person("Joe", Role("Engineer")),
                Person("Sally", Role("Manager")),
                Person("Bob", Role("QA")),
        )

        assert(people.find { it.name == "Ralph" }?.role?.title == "qa")
    }

Sure I'm missing something as I just got:

MyTest > one FAILED
    java.lang.AssertionError: assert(value)
           |
           false
        at MyTest.assert(MyTest.kt:7)
        at MyTest.one(MyTest.kt:24)

I looked at your example: https://github.com/bnorm/kotlin-power-assert/blob/master/sample/src/commonMain/kotlin/com/bnorm/power/AssertScope.kt

But wasn't sure how much of that I'd need to replicate to make this work. Maybe the easiest path is to add assert(value: Boolean?) to the stdlib or assertTrue(value: Boolean?) to kotlin.test then you can do your magic. :)

TWiStErRob commented 11 months ago

I think you might need to register your assert function like here: https://github.com/bnorm/kotlin-power-assert#gradle-plugin

bnorm commented 11 months ago

As TWiStErRob noted, you probably need to register your assert function with Gradle as mentioned in that part of the documentation. But you will also need to update your custom assert function to take an additional argument of type string.

fun assert(value: Boolean?) = assert(value, "default message")
fun assert(value: Boolean?, message: String) {
    // your other code here.
}

kotlin-power-assert works by transforming calls to the configured functions by adding a hard-coded diagram string parameter. Essentially, at the IR level, it transforms function calls to the configured functions from something like this:

assert(x == y)

to something like this:

val tmp = x == y
assert(tmp, """
    assert(x == y)
           | |  |
           | |  ${y}
           | ${tmp}
           ${x}
""".trimIndent())

So with your code, it did that for the assert call within your custom function, but it doesn't know to do that for calls to your custom function as well. And without the second string parameter, it wouldn't have a place to put the generated diagram string as well. So if you add the Gradle configuration to point at your own custom assert function, and create an overload with the needed message parameter, you should be able to make this work.

If you have a public repository you are attempting this within, I'd be happy to take a look and help get this working more directly.

Maybe the easiest path is to add assert(value: Boolean?) to the stdlib or assertTrue(value: Boolean?) to kotlin.test then you can do your magic. :)

The other option is you just use assert(nullableExpression == true) instead, and it will work that way. Then we aren't waiting for the Kotlin release cycle.

jamesward commented 11 months ago

Oh! Thank you! It works :)

MyTest > one FAILED
    java.lang.AssertionError: Assertion failed
    assertTrue(people.find { it.name == "Ralph" }?.role?.title == "qa")
               |      |                            |     |     |
               |      |                            |     |     false
               |      |                            |     null
               |      |                            null
               |      null
               [Person(name=Joe, role=Role(title=Engineer)), Person(name=Sally, role=Role(title=Manager)), Person(name=Bob, role=Role(title=QA))]
        at MyTestKt.assertTrue(MyTest.kt:5)
        at MyTest.one(MyTest.kt:24)

Code: https://github.com/jamesward/test-assertions/blob/main/gradleable/src/test/kotlin/MyTest.kt

I'm going to add this to my blog.

jamesward commented 11 months ago

Added to the blog. Thanks again!

bnorm commented 9 months ago

Thanks, James! I'm going to close this issue as resolved. Let me know if you find any other issues or have questions.

rocketraman commented 9 months ago

@jamesward FYI in your blog you ended up with this code example:

assertTrue(people.find { it.name == "Ralph" }?.role?.title == "qa")

which doesn't actually need your custom assertTrue function, as the result of that expression is Boolean i.e. null == "qa" is false.

And indeed doing that same call on a regular Kotlin language assert results in exactly the same output as shown in your blog:

java.lang.AssertionError: Assertion failed
assert(people.find { it.name == "Ralph" }?.role?.title == "qa")
       |      |                            |     |     |
       |      |                            |     |     false
       |      |                            |     null
       |      |                            null
       |      null
       [Person(name=Joe, role=Role(title=Engineer)), Person(name=Sally, role=Role(title=Manager)), Person(name=Bob, role=Role(title=QA))]

You probably want to update the blog post to stick with ?.equals(value, ignoreCase = true).

jamesward commented 9 months ago

Thanks @rocketraman! You are right. This is cleaner: https://github.com/jamesward/test-assertions/blob/main/gradleable/src/test/kotlin/MyTest.kt

Thank you! I'll get the blog updated.

rocketraman commented 9 months ago

Thanks @rocketraman! You are right. This is cleaner: https://github.com/jamesward/test-assertions/blob/main/gradleable/src/test/kotlin/MyTest.kt

That just does equals("qa") which would be cleaner to express in Kotlin as == "qa".

You really need the ignoreCase parameter in there to demo that particular problem ;-)

jamesward commented 9 months ago

Yeah, that makes sense. I've updated the code and the blog. Thanks!