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

Add Kotlin contracts to exposed Kotlin API #3259

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

awelless
Copy link

@awelless awelless commented Apr 17, 2023

Overview

Resolves #1866.
Introduced assertNull, assertNotNull and assertInstanceOf methods.
Introduced contracts for assertNull, assertNotNull, assertThrows and assertDoesNotThrow methods


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

kotlin {
sourceSets {
main {
languageSettings.optIn("kotlin.contracts.ExperimentalContracts")
Copy link
Member

Choose a reason for hiding this comment

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

Do consumers of junit-jupiter-api also have to opt-in? If so, what happens of they don't?

Copy link
Author

Choose a reason for hiding this comment

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

No, since we opt-in in the junit-jupiter-api, consumers don't need to specify any opt-ins in their code

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 fun fail(message: (() -> String)?) should also have a contract added to it?

Also, can contracts be added to vararg executables: () -> Unit?

Copy link
Author

Choose a reason for hiding this comment

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

callsInPlace supports only non-nullable arguments.
It would be great to have something like:

fun fail(message: (() -> String)?): Nothing {
    contract {
        if (message != null) {
            callsInPlace(message, EXACTLY_ONCE)
        }
    }

    Assertions.fail<Nothing>(message)
}

but if is not allowed in a contract definition.

Similar thing applies to vararg executables: () -> Unit

Copy link

@TWiStErRob TWiStErRob May 26, 2023

Choose a reason for hiding this comment

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

Could the if be partially resolved like this? So that more (most) calls end up using the contract description for analysis?

// Covers method references `fail(::foo)`, inline lambdas `fail { }`.
fun fail(message: () -> String): Nothing {
    contract {
        callsInPlace(message, EXACTLY_ONCE)
    }
    Assertions.fail<Nothing>(message)
}
// Covers backwards compatibility and potential edge cases like `fail(foo.bar)` where bar is a nullable functional type.
fun fail(message: (() -> String)?): Nothing {
    Assertions.fail<Nothing>(message)
}

Copy link
Author

@awelless awelless May 27, 2023

Choose a reason for hiding this comment

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

The method signatures are same from JVM perspective.

Platform declaration clash: The following declarations have the same JVM signature (fail(Lkotlin/jvm/functions/Function0;)Ljava/lang/Void;):
    fun fail(message: (() -> String)?): Nothing defined in org.junit.jupiter.api in file Assertions.kt
    fun fail(message: () -> String): Nothing defined in org.junit.jupiter.api in file Assertions.kt

Choose a reason for hiding this comment

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

Huh, I didn't get that when I tried. Anyway, @JvmName usually helps. The question is if this is a feasible solution to get contract in for most, and still keep supporting other edge cases. What's the use case for nullable lambda here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, creating a separate method with a contract and non-nullable lambda allows kotlin compiler to carry out contract checks when it's sure the passed lambda is not null.
I assume, lambda is made nullable to resemble existing java api where messageSupplier can be nullable

Copy link
Author

Choose a reason for hiding this comment

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

callsInPlace for fail method is also specified with InvocationKind.UNKNOWN .
If we set it to EXACTLY_ONCE the following code compiles:

val expectedValue = "value"
val value: String

try {
    fail {
        value = expectedValue
        "message"
    }
} catch (_: AssertionFailedError) {
    value = "another value"
}

assertEquals(expectedValue, value)

Here we reassign value albeit it is val. I reckon it isn't desirable to allow this code to compile

Assertions.assertTimeoutPreemptively(timeout, executable)
fun <R> assertTimeoutPreemptively(timeout: Duration, executable: () -> R): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
Copy link

@TWiStErRob TWiStErRob May 26, 2023

Choose a reason for hiding this comment

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

Is it actually true for things that can/expected to throw?
(I have a feeling we can't say "exactly once", but I might be misinterpreting the enum.)
Examples where it would not have compiled without the contract, and now it's a runtime error:

val value: String
// Swapped version of fun `assertThrows with value initialization in lambda`() in this PR
assertThrows<AssertionError> {
    Assertions.fail("message")
    value = "string"
}
assertEquals("string", value) // AssertionFailedError: expected: <string> but was: <null>
val x: String
assertThrows<IOException> {
    File("//").readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}
// Kotlin thinks x is initialized here, because of the EXACTLY_ONCE.
assertEquals(3, x.length) // Consistent NPE, due to "effectively unreachable code" above.
val x: String
assertThrows<AssertionError> {
    assertTimeoutPreemptively(Duration.seconds(1)) {
        Thread.sleep(2000)
        x = "" // Will never execute, because it always times out.
    }
}
// Kotlin thinks x is initialized here, because of the chain of EXACTLY_ONCE.
println(x.length) // Consistent NPE, due to "effectively unreachable code" above.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you write this in Kotlin with try/catch? I presume the compiler pitches a fit?

val x: String
try {
    File("//").readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
} catch (e: IOException) {
    
}
assertEquals(3, x.length)

What about this?

val x: String
File("//").apply {
	readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}

assertEquals(3, x.length)

If the apply case fails in the same way as your example, I think it's probably fine?

Copy link
Contributor

@JLLeitschuh JLLeitschuh May 26, 2023

Choose a reason for hiding this comment

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

This looks like it will compile fine:

import java.io.File

var x: String
File("//").apply {
    readText() // Deterministic IOException due to invalid name.
    x = "abc" // Will never execute, because it always throws.
}

if (3 == x.length) {
    
}

Choose a reason for hiding this comment

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

Huh, very good point! 😁

try-catch example will not compile, but because not all code paths initialize x, but the point is valid there too, you can resolve this thread (I can't).

Copy link
Contributor

Choose a reason for hiding this comment

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

You could theoretically reimplement this logic, in kotlin, and expose it as an inline function. Then the try/catch will be inlined. Then there will be no need for the contract

private static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable,
Object messageOrSupplier) {
try {
executable.execute();
}
catch (Throwable actualException) {
if (expectedType.isInstance(actualException)) {
return (T) actualException;
}
else {
UnrecoverableExceptions.rethrowIfUnrecoverable(actualException);
throw assertionFailure() //
.message(messageOrSupplier) //
.expected(expectedType) //
.actual(actualException.getClass()) //
.reason("Unexpected exception type thrown") //
.cause(actualException) //
.build();
}
}
throw assertionFailure() //
.message(messageOrSupplier) //
.reason(format("Expected %s to be thrown, but nothing was thrown.", getCanonicalName(expectedType))) //
.build();
}

Copy link
Author

Choose a reason for hiding this comment

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

Contracts don't work well when we suppress exception. Issues one and two.
To prevent any kind of errors and confusion, I suggest to replace callsInPlace(executable, EXACTLY_ONCE) with callsInPlace(executable) in all assertThrows methods.

What do you think?

Choose a reason for hiding this comment

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

Makes sense, I would be explicit with second param though, just for readability. If the Kotlin compiler is improved, is it easy to swap to a "better exactly once"? Or would it break users?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't noticed any improvements in contracts. InvocationKind.UNKNOWN is set for assertTimeoutPreemptively methods because of the same reasons as for assertThrows methods


assertThrows<AssertionError> {
value = "string"
Assertions.fail("message")

Choose a reason for hiding this comment

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

Should this just be

Suggested change
Assertions.fail("message")
fail("message")

considering this is Kotlin code and there's a : Nothing signature top level fail.

Copy link
Author

Choose a reason for hiding this comment

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

Contracts work really weird with Nothing return type. Compiler warns that expression after assertThrows is not reachable while it is.

In the thread above I proposed not to specify InvocationKind for assertThrows methods. If there's no InvocationKind specified, value assignment in lambda will be forbidden and this test will make no sense

Choose a reason for hiding this comment

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

In that case I agree with UNKNOWN for invocation kind, because not being able to call production code that declares Nothing as return type is a big restriction. The point of assertThrows is that it tests exceptions thrown from a method, and Nothing declares exactly that.

Copy link
Author

Choose a reason for hiding this comment

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

In such case I'll set InvocationKind.UNKNOWN for assertThrows methods

@awelless awelless requested a review from TWiStErRob June 3, 2023 21:39
Added assertNull and assertNotNull methods with contracts.
Added contracts for assertThrows and assertDoesNotThrow methods.

assertInstanceOf can be implemented only with kotlin 1.4, because
refined generics
[are not supported](https://youtrack.jetbrains.com/issue/KT-28298)
in contracts for kotlin 1.3 yet.

Issue: junit-team#1866
Kotlin assertNull and assertNotNull methods are marked as
experimental.

Issue: junit-team#1866
Lambda invoked in assertThrows is marked as being called UNKNOWN number
of times, since contracts do nothing with exception suppression

Issue: junit-team#1866
Created another fail method with a non-nullable lambda parameter

Issue: junit-team#1866
Copy link

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

Looks to be in a reasonable state. Have some thoughts though.

Assertions.assertTimeoutPreemptively(timeout, executable)
fun <R> assertTimeoutPreemptively(timeout: Duration, executable: () -> R): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)

Choose a reason for hiding this comment

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

Makes sense, I would be explicit with second param though, just for readability. If the Kotlin compiler is improved, is it easy to swap to a "better exactly once"? Or would it break users?

Because of kotlin smart casts, there is no need to return a
non-nullable value from `assertNotNull` methods

Issue: junit-team#1866
Api version is increased to 5.11 for `assertNull` and `assertNotNull`
methods

Issue: junit-team#1866
Added `assertInstanceOf` assertions with upcasting contracts

Issue: junit-team#1866
Copy link

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

Did a full review, wrote down what I noticed. Feel free to ignore some of them :)

Please resolve the conversations that are resolved, I can't do it, because only the PR Author and repo Members can.

And then we'll probably need a round of review from Marc or Jonathan.

* @see Assertions.fail
*/
@JvmName("fail_nonNullableLambda")
fun fail(message: () -> String): Nothing {

Choose a reason for hiding this comment

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

JUnit Experimental?

Comment on lines 338 to 341
inline fun <reified T : Throwable> assertThrows(message: String, executable: () -> Unit): T {
contract {
callsInPlace(executable, UNKNOWN)
}

Choose a reason for hiding this comment

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

Is it possible to test for this contract?
As in: if it's removed, something should stop compiling or fail a test, right?
Probably the one in the related convo: #3259 (comment)

(This is probably true for all of them.)

Copy link
Author

Choose a reason for hiding this comment

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

I haven't found a way to test callsInPlace(..., UNKNOWN) contracts.
I though having another function with a contract inside could work, but it seems, kotlin compiler doesn't check whether a function is really called in place in the functions it's passed to.

fun notWorkingTest(value: () -> Unit) {
    contract {
        callsInPlace(value, UNKNOWN)
    }

    assertThrows<AssertionError>(value) // works fine even when assertThrows doesn't have a contract
}

Comment on lines +249 to +251
val error = assertThrows<AssertionFailedError> {
assertInstanceOf<String>(null)
}

Choose a reason for hiding this comment

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

This feels meta, using a Kotlin assertion to test a Kotlin assertion, but it's probably ok, since they have different use cases for these two.

Copy link
Author

Choose a reason for hiding this comment

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

We have a few tests for assertThrows. Also, a similar approach is used for assertDoesNotThrow tests

Comment on lines 102 to 103
fun assertAll(heading: String?, vararg executables: () -> Unit) =
assertAll(heading, executables.toList().stream())

Choose a reason for hiding this comment

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

Reading this on GitHub makes me confused about the return type.
Again, not your code, but all your return types are nice and clean, probably worth aligning.

Copy link
Author

Choose a reason for hiding this comment

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

I've noticed that return type is not specified only for one-liners that return Unit. I assume, to resemble usual methods, where it isn't mandatory to specify Unit return type. For one-liners that return anything besides Unit, a return type is already specified

@@ -134,6 +355,11 @@ inline fun <reified T : Throwable> assertThrows(message: String, executable: ()
* @see Assertions.assertThrows
*/
inline fun <reified T : Throwable> assertThrows(noinline message: () -> String, executable: () -> Unit): T {
contract {
callsInPlace(executable, UNKNOWN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this unknown?

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Contracts doesn't work well as being discussed in this conversation.
Perhaps, not specifying contracts for assertThrows and assertTimeoutPreemptively is a better option. For example, kotlin's runCatching method doesn't specify any contracts.
What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Contracts have been removed for all lambdas that may throw an exception

Choose a reason for hiding this comment

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

Would it be worth adding a comment into those methods that they intentionally don't have contracts and why?

Copy link
Author

Choose a reason for hiding this comment

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

Imo, adding comments regarding a contract absence everywhere adds nothing but noise. A person who's concerned by the missing contracts can check the commits and see why they aren't in place

@awelless
Copy link
Author

Is there anything else that should be added/adjusted? Otherwise, I'd say the pr is in its final form

Copy link

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -211,6 +211,59 @@ class KotlinAssertionsTests {
assertMessageStartsWith(error, assertionMessage)
}

Choose a reason for hiding this comment

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

My only note is that there are 18 methods affected, but only 6 new tests, and this may be normal because some contract stuff is not testable directly.

Copy link
Author

Choose a reason for hiding this comment

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

Added missing tests for assertion method overloads with message and message supplier

Choose a reason for hiding this comment

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

Nice 👍

@awelless
Copy link
Author

awelless commented Oct 3, 2023

What do you think about the pr, @marcphilipp, @JLLeitschuh?

@awelless
Copy link
Author

Hi. I believe the change is still relevant and the pr is ready to be reviewed.

@sbrannen sbrannen changed the title Added Kotlin Contracts to Exposed Kotlin API Add Kotlin contracts to exposed Kotlin API Dec 16, 2023
@marcphilipp
Copy link
Member

We'll consider it for 5.11, just need to find some time to review.

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.

Add Kotlin contracts to exposed Kotlin API
4 participants