Skip to content

Relax Kotlin contracts for Executable parameters#4481

Closed
marcphilipp wants to merge 4 commits intodevelop/6.xfrom
marc/4371-relax-kotlin-contracts
Closed

Relax Kotlin contracts for Executable parameters#4481
marcphilipp wants to merge 4 commits intodevelop/6.xfrom
marc/4371-relax-kotlin-contracts

Conversation

@marcphilipp
Copy link
Copy Markdown
Member

  • Relax Kotlin contracts for Executable parameters
  • Treat Kotlin compiler warnings as errors again

Overview


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


Definition of Done

@marcphilipp marcphilipp self-assigned this Apr 14, 2025
@marcphilipp marcphilipp linked an issue Apr 14, 2025 that may be closed by this pull request
1 task
@marcphilipp marcphilipp changed the base branch from marc/jdk17 to develop/6.x May 10, 2025 12:41
Since `Executable` is allowed/expected to throw exceptions, the
semantics of being called `EXACTLY_ONCE` are blurry because it might not
be executed entirely when throwing an exception. Since the Kotlin 2.1
compiler emits a warning for these parameters, we're relaxing their
contracts to be called `AT_MOST_ONCE` instead.
@marcphilipp marcphilipp force-pushed the marc/4371-relax-kotlin-contracts branch from 79993fe to ab26f26 Compare May 10, 2025 12:43
Copy link
Copy Markdown
Contributor

@awelless awelless left a comment

Choose a reason for hiding this comment

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

The tests prevent me from approval.
The decision about assertDoesNotThrow I leave to you

@Test
fun `assertTimeout with value initialization in lambda`() {
val value: Int
fun `assertTimeout with value assignment in lambda`() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem is that this test will pass even when no contract is specified. The AT_LEAST_ONE contract allows us to assign vals in lamda body. A test like this would be a better candidate, as it will pass only with AT_LEAST_ONCE or EXACTLY_ONCE contracts. This applies to other changes in this file too.

       @Test
    fun `assertTimeout with value assignment in lambda`() {
        val value: Int

        assertTimeout(ofMillis(500)) {
            value = 10 // Val can be assigned in the message supplier lambda.
            assertEquals(10, value)
        }
    }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Applied in 5cac4e3

internal inline fun <R> evaluateAndWrap(executable: () -> R): ThrowingSupplier<R> {
contract {
callsInPlace(executable, EXACTLY_ONCE)
callsInPlace(executable, AT_MOST_ONCE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Although this makes the code compile, I'm still not entirely happy that we must mark this as AT_MOST_ONCE only to make kotlin assertDoesNotThrow compatible with its java counterpart.

If we were able to just run lambda in kotlin, we'd be able to mark all assertDoesNotThrow methods as EXACTLY_ONCE. That being set, this will cause duplication of AssertDoesNotThrow logic which doesn't sound optimal either.

internal inline fun <R> evaluateAndWrap(executable: () -> R): ThrowingSupplier<R> {
    contract {
        callsInPlace(executable, EXACTLY_ONCE)
    }

    return try {
        executable()
    } catch (throwable: Throwable) {
        // logic from java's AssertDoesNotThrow
        UnrecoverableExceptions.rethrowIfUnrecoverable(t)
        throw AssertDoesNotThrow.createAssertionFailedError(messageOrSupplier, t)
    }
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 0646eba

@marcphilipp marcphilipp requested a review from awelless May 15, 2025 08:35
@marcphilipp
Copy link
Copy Markdown
Member Author

@awelless Thanks for the review! Do you have time to take another look at my latest changes?

Copy link
Copy Markdown
Contributor

@awelless awelless left a comment

Choose a reason for hiding this comment

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

The changes look good, besides the contract in one assertDoesNotThrow.
Approved, assuming the comment will be addressed

executable: () -> R
): R {
contract {
callsInPlace(executable, EXACTLY_ONCE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This executable should be called EXACTLY_ONCE, as other assertDoesNotThrow methods.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! Addressed in ef78d82 (#4551)

@marcphilipp marcphilipp moved this from Todo to In Progress in Sovereign Tech Fund May 19, 2025
@marcphilipp marcphilipp mentioned this pull request May 19, 2025
5 tasks
@marcphilipp marcphilipp deleted the branch develop/6.x May 19, 2025 09:39
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Sovereign Tech Fund May 19, 2025
@marcphilipp
Copy link
Copy Markdown
Member Author

Superseded by #4551

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.

Resolve Kotlin 2.1 compiler warnings for contracts

2 participants