ronantakizawa/codereview-bench huggingface dataset:
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/test/channels/BufferedChannelTest.kt (Kotlin)

Code before:
package kotlinx.coroutines.channels

import kotlinx.coroutines.testing.*
import kotlinx.coroutines.*
import kotlin.test.*

class BufferedChannelTest : TestBase() {
    @Test
    fun testIterHasNextIsIdempotant() = runTest {
        val q = Channel<Int>()
        check(q.isEmpty)
        val iter = q.iterator()
        expect(1)
        val sender = launch {
            expect(4)
            q.send(1) // sent
            expect(10)
            q.close()
            expect(11)
        }
        expect(2)
        val receiver = launch {
            expect(5)
            check(iter.hasNext())
            expect(6)
            check(iter.hasNext())
            expect(7)
            check(iter.hasNext())
            expect(8)
            check(iter.next() == 1)
            expect(9)
            check(!iter.hasNext())
            expect(12)
        }

Review comment (suggestion):
```suggestion
    fun testIteratorHasNextIsIdempotent() = runTest {
```

Code after:
package kotlinx.coroutines.channels

import kotlinx.coroutines.testing.*
import kotlinx.coroutines.*
import kotlin.test.*

class BufferedChannelTest : TestBase() {
    @Test
    fun testIteratorHasNextIsIdempotent() = runTest {
        val q = Channel<Int>()
        check(q.isEmpty)
        val iter = q.iterator()
        expect(1)
        val sender = launch {
            expect(4)
            q.send(1) // sent
            expect(10)
            q.close()
            expect(11)
        }
        expect(2)
        val receiver = launch {
            expect(5)
            check(iter.hasNext())
            expect(6)
            check(iter.hasNext())
            expect(7)
            check(iter.hasNext())
            expect(8)
            check(iter.next() == 1)
            expect(9)
            check(!iter.hasNext())
            expect(12)
        }
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/src/Annotations.kt (Kotlin)

Code before:
            "should not be used from outside of kotlinx.coroutines. No compatibility guarantees are provided. " +
            "It is recommended to report your use-case of internal API to kotlinx.coroutines issue tracker, " +
            "so stable API could be provided instead"
)
public annotation class InternalCoroutinesApi

/**
 * Marks declarations that cannot be safely inherited from.
 */
@Target(AnnotationTarget.CLASS)
@RequiresOptIn(
    level = RequiresOptIn.Level.WARNING, message =
    "Inheriting from this kotlinx.coroutines API is unstable. " +
        "Either new methods may be added in the future, which would break the inheritance, " +
        "or correctly inheriting from it requires fulfilling contracts that may change in the future."
)
public annotation class BrittleForInheritanceCoroutinesApi

/**
 * Marks declarations that cannot be safely inherited from.
 */
@Target(AnnotationTarget.CLASS)
@RequiresOptIn(
    level = RequiresOptIn.Level.WARNING, message =
    "This is a kotlinx.coroutines API that is not intended to be inherited from, " +
        "as the library may handle predefined instances of this in a special manner."
)
public annotation class UnsuitableForInheritanceCoroutinesApi


Review comment (suggestion):
I would suggest keeping it consistent: `InternalForInheritance` (or `InternalForImplementation`) and adding the corresponding disclaimer to the message: `No compatibility guarantees are provided. It is recommended to report your use-case of internal API to kotlinx.coroutines issue tracker, so stable API could be provided instead`

Code after:
            "should not be used from outside of kotlinx.coroutines. No compatibility guarantees are provided. " +
            "It is recommended to report your use-case of internal API to kotlinx.coroutines issue tracker, " +
            "so stable API could be provided instead"
)
public annotation class InternalCoroutinesApi

/**
 * Marks declarations that cannot be safely inherited from.
 */
@Target(AnnotationTarget.CLASS)
@RequiresOptIn(
    level = RequiresOptIn.Level.WARNING, message =
    "Inheriting from this kotlinx.coroutines API is unstable. " +
        "Either new methods may be added in the future, which would break the inheritance, " +
        "or correctly inheriting from it requires fulfilling contracts that may change in the future."
)
public annotation class ExperimentalForInheritanceCoroutinesApi

/**
 * Marks declarations that cannot be safely inherited from.
 */
@Target(AnnotationTarget.CLASS)
@RequiresOptIn(
    level = RequiresOptIn.Level.WARNING, message =
    "This is a kotlinx.coroutines API that is not intended to be inherited from, " +
        "as the library may handle predefined instances of this in a special manner. " +
        "This will be an error in a future release. " +
        "If you need to inherit from this, please describe your use case in " +
        "https://github.com/Kotlin/kotlinx.coroutines/issues, so that we can provide a stable API for inheritance. "
)
public annotation class InternalForInheritanceCoroutinesApi
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/src/flow/operators/Transform.kt (Kotlin)

Code before:
 *     .collect {
 *         println(it) // Prints "ab", "cd", e"
 *     }
 * ```
 *
 * @throws IllegalArgumentException if [size] is not positive.
 */
@ExperimentalCoroutinesApi
public fun <T> Flow<T>.chunked(size: Int): Flow<List<T>> {
    require(size >= 1) { "Expected positive chunk size, but got $size" }
    return flow {
        var result: ArrayList<T>? = null // Do not preallocate anything
        collect { value ->
            // Allocate if needed
            val acc = result ?: ArrayList<T>(size).also { result = it }
            acc.add(value)
            if (acc.size == size) {
                emit(acc)
                // Cleanup, but don't allocate -- it might've been the case this is the last element
                result = null
            }
        }
        val acc = result ?: return@flow
        if (acc.isNotEmpty()) {
            emit(acc)
        }
    }
}


Review comment (suggestion):
A non-null `result` is always non-empty, right?
```suggestion
        result?.let { emit(it) }
```

Code after:
 *     .collect {
 *         println(it) // Prints "ab", "cd", e"
 *     }
 * ```
 *
 * @throws IllegalArgumentException if [size] is not positive.
 */
@ExperimentalCoroutinesApi
public fun <T> Flow<T>.chunked(size: Int): Flow<List<T>> {
    require(size >= 1) { "Expected positive chunk size, but got $size" }
    return flow {
        var result: ArrayList<T>? = null // Do not preallocate anything
        collect { value ->
            // Allocate if needed
            val acc = result ?: ArrayList<T>(size).also { result = it }
            acc.add(value)
            if (acc.size == size) {
                emit(acc)
                // Cleanup, but don't allocate -- it might've been the case this is the last element
                result = null
            }
        }
        result?.let { emit(it) }
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/jdk8/src/future/Future.kt (Kotlin)

Code before:
) : BiFunction<T?, Throwable?, Unit> {
    @Suppress("UNCHECKED_CAST")
    override fun apply(result: T?, exception: Throwable?) {
        val cont = this.cont ?: return // atomically read current value unless null
        if (exception == null) {
            // the future has completed normally
            cont.resume(result as T)
        } else {
            // the future has completed with an exception, unwrap it to provide consistent view of .await() result and to propagate only original exception
            cont.resumeWithException((exception as? CompletionException)?.cause ?: exception)
        }
    }
}

private class CancelFutureOnCompletion(
    private val future: Future<*>
) : JobNode() {
    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere.
        // We do not cancel the future if it's already completed in some way,
        // because `cancel` on a completed future won't change the state but is not guaranteed to behave well
        // on reentrancy. See https://github.com/Kotlin/kotlinx.coroutines/issues/4156
        if (cause != null && !future.isDone) future.cancel(false)
    }

    override val onCancelling get() = false
}

Review comment (bug):
> but is not guaranteed to behave well on reentrancy

Can you please mention what exactly goes wrong? Something like `Reentrant cancellation of a future may cause repeated cancellation of it's dependants`

Code after:
) : BiFunction<T?, Throwable?, Unit> {
    @Suppress("UNCHECKED_CAST")
    override fun apply(result: T?, exception: Throwable?) {
        val cont = this.cont ?: return // atomically read current value unless null
        if (exception == null) {
            // the future has completed normally
            cont.resume(result as T)
        } else {
            // the future has completed with an exception, unwrap it to provide consistent view of .await() result and to propagate only original exception
            cont.resumeWithException((exception as? CompletionException)?.cause ?: exception)
        }
    }
}

private class CancelFutureOnCompletion(
    private val future: Future<*>
) : JobNode() {
    override val onCancelling get() = false

    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere.
        // We do not cancel the future if it's already completed in some way,
        // because `cancel` on a completed future won't change the state but is not guaranteed to behave well
        // on reentrancy. See https://github.com/Kotlin/kotlinx.coroutines/issues/4156
        if (cause != null && !future.isDone) future.cancel(false)
    }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/jvm/src/Future.kt (Kotlin)

Code before:
@file:JvmMultifileClass
@file:JvmName("JobKt")

package kotlinx.coroutines

import java.util.concurrent.*

/**
 * Cancels a specified [future] when this job is cancelled.
 * This is a shortcut for the following code with slightly more efficient implementation (one fewer object created).
 * ```
 * invokeOnCancellation { if (it != null) future.cancel(false) }
 * ```
 */
@Deprecated(
    "This function does not do what its name implies: it will not cancel the future if just cancel() was called.",
    level = DeprecationLevel.WARNING,
    replaceWith = ReplaceWith("this.invokeOnCancellation { future.cancel(false) }")
)
public fun CancellableContinuation<*>.cancelFutureOnCancellation(future: Future<*>): Unit =
    invokeOnCancellation(handler = PublicCancelFutureOnCancel(future))

private class PublicCancelFutureOnCancel(private val future: Future<*>) : CancelHandler {
    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere
        if (cause != null)  future.cancel(false)
    }
    override fun toString() = "CancelFutureOnCancel[$future]"
}


Review comment (suggestion):
There are no tests for this method. Should we introduce one? I will become clearer which cases this method does not cover

Code after:
@file:JvmMultifileClass
@file:JvmName("JobKt")

package kotlinx.coroutines

import java.util.concurrent.*

/**
 * Cancels a specified [future] when this job is cancelled.
 * This is a shortcut for the following code with slightly more efficient implementation (one fewer object created).
 * ```
 * invokeOnCancellation { if (it != null) future.cancel(false) }
 * ```
 */
// Warning since 1.9.0, error in 1., hidden in 1.7
@Deprecated(
    "This function does not do what its name implies: it will not cancel the future if just cancel() was called.",
    level = DeprecationLevel.WARNING,
    replaceWith = ReplaceWith("this.invokeOnCancellation { future.cancel(false) }")
)
public fun CancellableContinuation<*>.cancelFutureOnCancellation(future: Future<*>): Unit =
    invokeOnCancellation(handler = PublicCancelFutureOnCancel(future))

private class PublicCancelFutureOnCancel(private val future: Future<*>) : CancelHandler {
    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere
        if (cause != null)  future.cancel(false)
    }
    override fun toString() = "CancelFutureOnCancel[$future]"
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/jvm/src/Future.kt (Kotlin)

Code before:
@file:JvmMultifileClass
@file:JvmName("JobKt")

package kotlinx.coroutines

import java.util.concurrent.*

/**
 * Cancels a specified [future] when this job is cancelled.
 * This is a shortcut for the following code with slightly more efficient implementation (one fewer object created).
 * ```
 * invokeOnCancellation { if (it != null) future.cancel(false) }
 * ```
 */
@Deprecated(
    "This function does not do what its name implies: it will not cancel the future if just cancel() was called.",
    level = DeprecationLevel.WARNING,
    replaceWith = ReplaceWith("this.invokeOnCancellation { future.cancel(false) }")
)
public fun CancellableContinuation<*>.cancelFutureOnCancellation(future: Future<*>): Unit =
    invokeOnCancellation(handler = PublicCancelFutureOnCancel(future))

private class PublicCancelFutureOnCancel(private val future: Future<*>) : CancelHandler {
    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere
        if (cause != null)  future.cancel(false)
    }
    override fun toString() = "CancelFutureOnCancel[$future]"
}


Review comment (suggestion):
> it will not cancel the future if just cancel() was called

Can you please specify the difference between the case when the method does work and does not work? For example, `The future will be cancelled only when the job is cancelled, but not the Continuation`

Code after:
@file:JvmMultifileClass
@file:JvmName("JobKt")

package kotlinx.coroutines

import java.util.concurrent.*

/**
 * Cancels a specified [future] when this job is cancelled.
 * This is a shortcut for the following code with slightly more efficient implementation (one fewer object created).
 * ```
 * invokeOnCancellation { if (it != null) future.cancel(false) }
 * ```
 */
// Warning since 1.9.0, error in 1., hidden in 1.7
@Deprecated(
    "This function does not do what its name implies: it will not cancel the future if just cancel() was called.",
    level = DeprecationLevel.WARNING,
    replaceWith = ReplaceWith("this.invokeOnCancellation { future.cancel(false) }")
)
public fun CancellableContinuation<*>.cancelFutureOnCancellation(future: Future<*>): Unit =
    invokeOnCancellation(handler = PublicCancelFutureOnCancel(future))

private class PublicCancelFutureOnCancel(private val future: Future<*>) : CancelHandler {
    override fun invoke(cause: Throwable?) {
        // Don't interrupt when cancelling future on completion, because no one is going to reset this
        // interruption flag and it will cause spurious failures elsewhere
        if (cause != null)  future.cancel(false)
    }
    override fun toString() = "CancelFutureOnCancel[$future]"
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/common/src/internal/DispatchedTask.kt (Kotlin)

Code before:
) {
    eventLoop.incrementUseCount(unconfined = true)
    try {
        block()
        while (true) {
            // break when all unconfined continuations where executed
            if (!eventLoop.processUnconfinedEvent()) break
        }
    } catch (e: Throwable) {
        /*
         * This exception doesn't happen normally, only if we have a bug in implementation.
         * Report it as a fatal exception.
         */
        handleFatalException(e)
    } finally {
        eventLoop.decrementUseCount(unconfined = true)
    }
}

@Suppress("NOTHING_TO_INLINE")
internal inline fun Continuation<*>.resumeWithStackTrace(exception: Throwable) {
    resumeWith(Result.failure(recoverStackTrace(exception, this)))
}

/**
 * This exception holds an exception raised in [CoroutineDispatcher.dispatch] method
 *
 * @see safeDispatch
 */
internal class DispatchException(
    override val cause: Throwable,
    dispatcher: CoroutineDispatcher,
    context: CoroutineContext,
) : Exception("Coroutine dispatcher $dispatcher threw an exception, context = $context", cause)


Review comment (bug):
Please leave a comment that helps readers/maintainers to understand **why** it is here without looking into git blame and/or corresponding tests.
For example:

```suggestion
 * This exception holds an exception raised in [CoroutineDispatcher.dispatch] method.
 * When dispatcher methods fail unexpectedly, it is likely a user-induced programmatic bug,
 * such as calling `executor.close()` prematurely. To avoid reporting such exceptions as fatal errors,
 * we handle them with a separate code path. See also #4091.
```

Code after:
) {
    eventLoop.incrementUseCount(unconfined = true)
    try {
        block()
        while (true) {
            // break when all unconfined continuations where executed
            if (!eventLoop.processUnconfinedEvent()) break
        }
    } catch (e: Throwable) {
        /*
         * This exception doesn't happen normally, only if we have a bug in implementation.
         * Report it as a fatal exception.
         */
        handleFatalException(e)
    } finally {
        eventLoop.decrementUseCount(unconfined = true)
    }
}

@Suppress("NOTHING_TO_INLINE")
internal inline fun Continuation<*>.resumeWithStackTrace(exception: Throwable) {
    resumeWith(Result.failure(recoverStackTrace(exception, this)))
}

/**
 * This exception holds an exception raised in [CoroutineDispatcher.dispatch] method.
 * When dispatcher methods fail unexpectedly, it is likely a user-induced programmatic bug,
 * such as calling `executor.close()` prematurely. To avoid reporting such exceptions as fatal errors,
 * we handle them with a separate code path. See also #4091.
 *
 * @see safeDispatch
 */
internal class DispatchException(
    override val cause: Throwable,
    dispatcher: CoroutineDispatcher,
    context: CoroutineContext,
) : Exception("Coroutine dispatcher $dispatcher threw an exception, context = $context", cause)
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-test/common/src/TestBuilders.kt (Kotlin)

Code before:
private fun <T: Any> systemProperty(
    name: String,
    parse: (String) -> T,
    default: T,
): T {
    val value = systemPropertyImpl(name) ?: return default
    return parse(value)
}

internal expect fun systemPropertyImpl(name: String): String?

@Deprecated(
    "This is for binary compatibility with the `runTest` overload that existed at some point",
    level = DeprecationLevel.HIDDEN
)
@JvmName("runTest\$default")
@Suppress("DEPRECATION", "UNUSED_PARAMETER")
public fun TestScope.runTestLegacy(
    dispatchTimeoutMs: Long,
    testBody: suspend TestScope.() -> Unit,
    marker: Int,
    unused2: Any?,
): TestResult = runTest(dispatchTimeoutMs = if (marker and 1 != 0) dispatchTimeoutMs else 60_000L, testBody)

private class AtomicBoolean(initial: Boolean) {
    private val container = atomic(initial)
    var value: Boolean
        get() = container.value
        set(value: Boolean) { container.value = value }
}


Review comment (suggestion):
```suggestion
// Remove after https://youtrack.jetbrains.com/issue/KT-62423/
private class AtomicBoolean(initial: Boolean) {
```

Code after:
private fun <T: Any> systemProperty(
    name: String,
    parse: (String) -> T,
    default: T,
): T {
    val value = systemPropertyImpl(name) ?: return default
    return parse(value)
}

internal expect fun systemPropertyImpl(name: String): String?

@Deprecated(
    "This is for binary compatibility with the `runTest` overload that existed at some point",
    level = DeprecationLevel.HIDDEN
)
@JvmName("runTest\$default")
@Suppress("DEPRECATION", "UNUSED_PARAMETER")
public fun TestScope.runTestLegacy(
    dispatchTimeoutMs: Long,
    testBody: suspend TestScope.() -> Unit,
    marker: Int,
    unused2: Any?,
): TestResult = runTest(dispatchTimeoutMs = if (marker and 1 != 0) dispatchTimeoutMs else 60_000L, testBody)

// Remove after https://youtrack.jetbrains.com/issue/KT-62423/
private class AtomicBoolean(initial: Boolean) {
    private val container = atomic(initial)
    var value: Boolean
        get() = container.value
        set(value: Boolean) { container.value = value }
}
Repository: Kotlin/kotlinx.coroutines
File: kotlinx-coroutines-core/native/src/EventLoop.kt (Kotlin)

Code before:
@file:OptIn(ObsoleteWorkersApi::class)

package kotlinx.coroutines

import kotlin.coroutines.*
import kotlin.native.concurrent.*
import kotlin.system.*

internal actual abstract class EventLoopImplPlatform : EventLoop() {

    private val current = Worker.current

    protected actual fun unpark() {
        current.executeAfter(0L, {})// send an empty task to unpark the waiting event loop
    }

    protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) {
        val delayTimeMillis = delayNanosToMillis(nanoTime() - now)
        DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext)
    }
}

internal class EventLoopImpl: EventLoopImplBase() {
    override fun invokeOnTimeout(timeMillis: Long, block: Runnable, context: CoroutineContext): DisposableHandle =
        DefaultDelay.invokeOnTimeout(timeMillis, block, context)
}

internal actual fun createEventLoop(): EventLoop = EventLoopImpl()

@Suppress("DEPRECATION")
internal actual fun nanoTime(): Long = getTimeNanos()


Review comment (suggestion):
I don't think that's quite right.
* `EventLoopImplBase.DelayedTask` has the `nanoTime` field that tells us when the task has to run.
* `now` should already be fairly close to `nanoTime()`. Passing `now` is a small optimization to avoid requerying nanoseconds all the time.

So, I think the correct version is
```suggestion
        val delayTimeMillis = delayNanosToMillis(delayedTask.nanoTime - now)
```

Code after:
@file:OptIn(ObsoleteWorkersApi::class)

package kotlinx.coroutines

import kotlin.coroutines.*
import kotlin.native.concurrent.*
import kotlin.time.*

internal actual abstract class EventLoopImplPlatform : EventLoop() {

    private val current = Worker.current

    protected actual fun unpark() {
        current.executeAfter(0L, {})// send an empty task to unpark the waiting event loop
    }

    protected actual fun reschedule(now: Long, delayedTask: EventLoopImplBase.DelayedTask) {
        val delayTimeMillis = delayNanosToMillis(delayedTask.nanoTime - now)
        DefaultExecutor.invokeOnTimeout(delayTimeMillis, delayedTask, EmptyCoroutineContext)
    }
}

internal class EventLoopImpl: EventLoopImplBase() {
    override fun invokeOnTimeout(timeMillis: Long, block: Runnable, context: CoroutineContext): DisposableHandle =
        DefaultDelay.invokeOnTimeout(timeMillis, block, context)
}

internal actual fun createEventLoop(): EventLoop = EventLoopImpl()

private val startingPoint = TimeSource.Monotonic.markNow()

internal actual fun nanoTime(): Long = (TimeSource.Monotonic.markNow() - startingPoint).inWholeNanoseconds