ronantakizawa/codereview-bench huggingface dataset:
Repository: JetBrains/kotlin
File: libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/targets/native/performance/PerformanceExtension.kt (Kotlin)

Code before:
/*
 * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package org.jetbrains.kotlin.gradle.plugin.performance

import org.gradle.api.Project
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.Optional
import org.jetbrains.kotlin.gradle.plugin.mpp.NativeBinary

enum class TrackableMetric { COMPILE_TIME, CODE_SIZE }
open class PerformanceExtension(private val project: Project) {
    @get:Input
    val version: String
        get() = project.version.toString()

    @Optional
    @Input
    var metrics = listOf(TrackableMetric.COMPILE_TIME, TrackableMetric.CODE_SIZE)

    @Input
    var trackedBinaries: List<NativeBinary> = emptyList()

    @Optional
    @Input
    var binaryNamesForReport = mapOf<NativeBinary, String>()
}

Review comment (suggestion):
May be add some DSL methods allowing one to add a binary along with its name for report? Say:
```
// Or `forBinary` or some other name, it's a subject for discussion.
fun binary(binary: NativeBinary, nameForReport: String = binary.name)
```

Code after:
package org.jetbrains.kotlin.gradle.plugin.performance

import org.gradle.api.Project
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.Optional
import org.jetbrains.kotlin.gradle.plugin.mpp.NativeBinary

enum class TrackableMetric { COMPILE_TIME, CODE_SIZE }
open class PerformanceExtension(private val project: Project) {
    val version: String
        get() = project.version.toString()

    var metrics = listOf(TrackableMetric.COMPILE_TIME, TrackableMetric.CODE_SIZE)

    val trackedBinaries = mutableListOf<NativeBinary>()

    val binaryNamesForReport = mutableMapOf<NativeBinary, String>()

    var includeAssociatedTasks = true

    @JvmOverloads
    fun binary(binary: NativeBinary, nameForReport: String = binary.name) {
        trackedBinaries.add(binary)
        binaryNamesForReport[binary] = nameForReport
    }
}
Repository: JetBrains/kotlin
File: libraries/stdlib/samples/test/samples/streams/streams.kt (Kotlin)

Code before:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package samples.streams

import samples.Sample
import samples.assertPrints
import java.util.stream.DoubleStream
import java.util.stream.IntStream
import java.util.stream.LongStream
import java.util.stream.Stream
import kotlin.streams.asSequence
import kotlin.streams.asStream
import kotlin.streams.toList

class Streams {

    @Sample
    fun streamAsSequence() {
        val stringStream = Stream.of("Never", "gonna", "give", "you", "up")
        val stringSequence = stringStream.asSequence()
        assertPrints(stringSequence.joinToString(" "), "Never gonna give you up")
    }

    @Sample
    fun intStreamAsSequence() {
        val intStream = IntStream.of(5, 6, 7)
        val intSequence = intStream.asSequence()
        assertPrints(intSequence.joinToString(", "), "5, 6, 7")
    }

    @Sample

Review comment (suggestion):
Please use a star-import from `samples` package, this is a requirement of the runnable sample converter.
For the other two packages, a star-import won't hurt either.

Code after:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package samples.streams

import samples.*
import java.util.stream.*
import kotlin.streams.*

class Streams {

    @Sample
    fun streamAsSequence() {
        val stringStream: Stream<String> = Stream.of("Never", "gonna", "give", "you", "up")
        val stringSequence: Sequence<String> = stringStream.asSequence()
        assertPrints(stringSequence.joinToString(" "), "Never gonna give you up")
    }

    @Sample
    fun intStreamAsSequence() {
        val intStream: IntStream = IntStream.of(5, 6, 7)
        val intSequence: Sequence<Int> = intStream.asSequence()
        assertPrints(intSequence.joinToString(", "), "5, 6, 7")
    }

    @Sample
    fun longStreamAsSequence() {
        val longStream: LongStream = LongStream.of(5_000_000_000, 6_000_000_000, 7_000_000_000)
        val longSequence: Sequence<Long> = longStream.asSequence()
        assertPrints(longSequence.joinToString(", "), "5000000000, 6000000000, 7000000000")
    }
Repository: JetBrains/kotlin
File: compiler/testData/debug/stepping/simpleInlineDefaultArg.kt (Kotlin)

Code before:
// FILE: test.kt

inline fun alsoInline() = "OK"

inline fun ifoo(s: String = alsoInline()): String {
    return s
}

fun box(): String {
    return ifoo()
}

// `alsoInline` is inlined in the `ifoo$default` stub. When exiting the inline
// codegen, the line number is reset. Therefore any code generated with line
// number information will emit a new line number so that stepping continues
// in the caller.
//
// For JVM none of the code after `alsoInline` leads to line number generation.
// This seems problematic as the line number for the inline function is then
// in effect for instructions outside the scope of the `$i$f$alsoInline`
// variable life time.
//
// For JVM_IR the parameter non-null check generation leads to line number
// generation. Therefore, we stop on line 5 before entering `alsoInline`
// and we step back to line 5 after `alsoInline` and before the body of
// `ifoo`. This seems reasonable and also help ensure that line numbers
// for the inline function are only used for instructions covered by
// the `$i$f$alsoInline` variable life time.

// LINENUMBERS
// test.kt:10 box
// test.kt:5 box
// test.kt:3 box
// LINENUMBERS JVM_IR
// test.kt:5 box
// LINENUMBERS
// test.kt:6 box
// test.kt:10 box

Review comment (suggestion):
Looks like this sentence is a bit misleading. The line number after the default value is not caused by the parameter non-null checks because if we suppress their generation by making `ifoo` private, we still generate that line number. I still think that JVM IR is correct, it's just that the comment should be slightly modified

Code after:
// FILE: test.kt

inline fun alsoInline() = "OK"

inline fun ifoo(s: String = alsoInline()): String {
    return s
}

fun box(): String {
    return ifoo()
}

// `alsoInline` is inlined in the `ifoo$default` stub. When exiting the inline
// codegen, the line number is reset. Therefore any code generated with line
// number information will emit a new line number so that stepping continues
// in the caller.
//
// For JVM none of the code after `alsoInline` leads to line number generation.
// This seems problematic as the line number for the inline function is then
// in effect for instructions outside the scope of the `$i$f$alsoInline`
// variable life time.
//
// For JVM_IR a line number is generated for the code following `alsoInline`.
// Therefore, we stop on line 5 before entering `alsoInline`
// and we step back to line 5 after `alsoInline` and before the body of
// `ifoo`. This seems reasonable and also help ensure that line numbers
// for the inline function are only used for instructions covered by
// the `$i$f$alsoInline` variable life time.

// LINENUMBERS
// test.kt:10 box
// test.kt:5 box
// test.kt:3 box
// LINENUMBERS JVM_IR
// test.kt:5 box
// LINENUMBERS
// test.kt:6 box
// test.kt:10 box
Repository: JetBrains/kotlin
File: compiler/ir/backend.jvm/src/org/jetbrains/kotlin/backend/jvm/JvmSymbols.kt (Kotlin)

Code before:
            // But we only need the following type-erased version to generate correct calls.
            klass.addFunction("valueOf", klass.defaultType, isStatic = true).apply {
                addValueParameter("enumType", javaLangClass.starProjectedType)
                addValueParameter("name", irBuiltIns.stringType)
            }
        }

    val enumValueOfFunction: IrSimpleFunctionSymbol =
        javaLangEnum.functionByName("valueOf")

    private val kotlinCoroutinesJvmInternalRunSuspendKt =
        createClass(FqName("kotlin.coroutines.jvm.internal.RunSuspendKt")) { klass ->
            klass.addFunction("runSuspend", irBuiltIns.unitType, isStatic = true).apply {
                addValueParameter(
                    "block",
                    getJvmSuspendFunctionClass(0).typeWith(
                        irBuiltIns.unitType
                    )
                )
            }
        }

    val runSuspendFunction: IrSimpleFunctionSymbol =
        kotlinCoroutinesJvmInternalRunSuspendKt.functionByName("runSuspend")

    val rawTypeAnnotationClass: IrClassSymbol =
        createClass(JvmGeneratorExtensions.RAW_TYPE_ANNOTATION_FQ_NAME, ClassKind.ANNOTATION_CLASS) { klass ->
            klass.addConstructor {
                isPrimary = true
            }
        }

    val rawTypeAnnotationConstructor: IrConstructorSymbol =
        rawTypeAnnotationClass.owner.primaryConstructor!!.symbol
}

private fun IrClassSymbol.functionByName(name: String): IrSimpleFunctionSymbol =
    functions.single { it.owner.name.asString() == name }

private fun IrClassSymbol.fieldByName(name: String): IrFieldSymbol =
    fields.single { it.owner.name.asString() == name }


Review comment (suggestion):
It would be great to use the same IR class symbol instance as in psi2ir, at least to avoid code duplication. I propose to pass the `JvmGeneratorExtensions` instance to `JvmBackendContext` (instead of `classNameOverride`), and use `context.generatorExtensions.rawTypeAnnotationConstructor` in `rawType`.

Code after:
            }
        }

    val enumValueOfFunction: IrSimpleFunctionSymbol =
        javaLangEnum.functionByName("valueOf")

    private val kotlinCoroutinesJvmInternalRunSuspendKt =
        createClass(FqName("kotlin.coroutines.jvm.internal.RunSuspendKt")) { klass ->
            klass.addFunction("runSuspend", irBuiltIns.unitType, isStatic = true).apply {
                addValueParameter(
                    "block",
                    getJvmSuspendFunctionClass(0).typeWith(
                        irBuiltIns.unitType
                    )
                )
            }
        }

    val runSuspendFunction: IrSimpleFunctionSymbol =
        kotlinCoroutinesJvmInternalRunSuspendKt.functionByName("runSuspend")
}

private fun IrClassSymbol.functionByName(name: String): IrSimpleFunctionSymbol =
    functions.single { it.owner.name.asString() == name }

private fun IrClassSymbol.fieldByName(name: String): IrFieldSymbol =
    fields.single { it.owner.name.asString() == name }
Repository: JetBrains/kotlin
File: libraries/stdlib/jdk8/src/kotlin/random/jdk8/PlatformThreadLocalRandom.kt (Kotlin)

Code before:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package kotlin.random.jdk8

import java.util.concurrent.ThreadLocalRandom

@Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER", "CANNOT_OVERRIDE_INVISIBLE_MEMBER")
internal object PlatformThreadLocalRandom : kotlin.random.AbstractPlatformRandom(), Serializable {
    private const val serialVersionUID = -8182665159126504406L

    // TODO no bridge generated for covariant override
    override val impl: java.util.Random
        get() = ThreadLocalRandom.current()

    override fun nextInt(from: Int, until: Int): Int = ThreadLocalRandom.current().nextInt(from, until)
    override fun nextLong(until: Long): Long = ThreadLocalRandom.current().nextLong(until)
    override fun nextLong(from: Long, until: Long): Long = ThreadLocalRandom.current().nextLong(from, until)
    override fun nextDouble(until: Double): Double = ThreadLocalRandom.current().nextDouble(until)

    //     do not delegate this, as it's buggy in JDK8+ (up to 11 at the moment of writing)
    //     override fun nextDouble(from: Double, until: Double): Double = ThreadLocalRandom.current().nextDouble(from, until)

    private fun readResolve(): Any = PlatformThreadLocalRandom
}


Review comment (suggestion):
I think such internal implementations should not be serializable at all. 
`Random.Default` manages its singleton serialization entirely and therefore it shouldn't expose any implementation details in a serialized form.

Code after:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package kotlin.random.jdk8

import java.util.concurrent.ThreadLocalRandom

@Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER", "CANNOT_OVERRIDE_INVISIBLE_MEMBER")
internal class PlatformThreadLocalRandom : kotlin.random.AbstractPlatformRandom() {
    // TODO no bridge generated for covariant override
    override val impl: java.util.Random get() = ThreadLocalRandom.current()

    override fun nextInt(from: Int, until: Int): Int = ThreadLocalRandom.current().nextInt(from, until)
    override fun nextLong(until: Long): Long = ThreadLocalRandom.current().nextLong(until)
    override fun nextLong(from: Long, until: Long): Long = ThreadLocalRandom.current().nextLong(from, until)
    override fun nextDouble(until: Double): Double = ThreadLocalRandom.current().nextDouble(until)

//     do not delegate this, as it's buggy in JDK8+ (up to 11 at the moment of writing)
//     override fun nextDouble(from: Double, until: Double): Double = ThreadLocalRandom.current().nextDouble(from, until)
}
Repository: JetBrains/kotlin
File: libraries/stdlib/jdk8/src/kotlin/random/jdk8/PlatformThreadLocalRandom.kt (Kotlin)

Code before:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package kotlin.random.jdk8

import java.util.concurrent.ThreadLocalRandom

@Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER", "CANNOT_OVERRIDE_INVISIBLE_MEMBER")
internal object PlatformThreadLocalRandom : kotlin.random.AbstractPlatformRandom() {
    // TODO no bridge generated for covariant override
    override val impl: java.util.Random get() = ThreadLocalRandom.current()

    override fun nextInt(from: Int, until: Int): Int = ThreadLocalRandom.current().nextInt(from, until)
    override fun nextLong(until: Long): Long = ThreadLocalRandom.current().nextLong(until)
    override fun nextLong(from: Long, until: Long): Long = ThreadLocalRandom.current().nextLong(from, until)
    override fun nextDouble(until: Double): Double = ThreadLocalRandom.current().nextDouble(until)

//     do not delegate this, as it's buggy in JDK8+ (up to 11 at the moment of writing)
//     override fun nextDouble(from: Double, until: Double): Double = ThreadLocalRandom.current().nextDouble(from, until)
}

Review comment (suggestion):
I don't see much sense in transforming it to `object`. It is instantiated once and then is kept in a private property of `Random` companion object. So no need to have another instance where its reference is stored.
Same for `FallbackThreadLocalRandom`

Code after:
/*
 * Copyright 2010-2018 JetBrains s.r.o. and Kotlin Programming Language contributors.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
 */

package kotlin.random.jdk8

import java.util.concurrent.ThreadLocalRandom

@Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER", "CANNOT_OVERRIDE_INVISIBLE_MEMBER")
internal class PlatformThreadLocalRandom : kotlin.random.AbstractPlatformRandom() {
    // TODO no bridge generated for covariant override
    override val impl: java.util.Random get() = ThreadLocalRandom.current()

    override fun nextInt(from: Int, until: Int): Int = ThreadLocalRandom.current().nextInt(from, until)
    override fun nextLong(until: Long): Long = ThreadLocalRandom.current().nextLong(until)
    override fun nextLong(from: Long, until: Long): Long = ThreadLocalRandom.current().nextLong(from, until)
    override fun nextDouble(until: Double): Double = ThreadLocalRandom.current().nextDouble(until)

//     do not delegate this, as it's buggy in JDK8+ (up to 11 at the moment of writing)
//     override fun nextDouble(from: Double, until: Double): Double = ThreadLocalRandom.current().nextDouble(from, until)
}
Repository: JetBrains/kotlin
File: compiler/testData/codegen/box/traits/abstractClassWithFakeOverride.kt (Kotlin)

Code before:
interface A {
    fun foo(): String = "OK"
}

abstract class B : A {
    abstract override fun foo(): String
}

abstract class C : B()

fun box() = "OK"

Review comment (bug):
Let's change the test case a little bit to use the classes from the hierarchy. (If the classes aren't loaded at runtime, they are not verified by the JVM and thus may contain invalid bytecode, which the test wouldn't catch)
```
interface A {
    fun foo(): String = "Fail"
}

abstract class B : A {
    abstract override fun foo(): String
}

abstract class C : B()

class D : C() {
    override fun foo(): String = "OK"
}

fun box() = D().foo()
```

Code after:
interface A {
    fun foo(): String = "Fail"
}

abstract class B : A {
    abstract override fun foo(): String
}

abstract class C : B()

class D : C() {
    override fun foo(): String = "OK"
}

fun box() = D().foo()
Repository: JetBrains/kotlin
File: ant/build.gradle.kts (Kotlin)

Code before:

description = "Kotlin Ant Tools"

plugins {
    kotlin("jvm")
}

dependencies {
    compile(commonDep("org.apache.ant", "ant"))
    compile(project(":kotlin-preloader"))
    compile(kotlinStdlib())
    compileOnly(project(":compiler:cli"))
}

sourceSets {
    "main" { projectDefault() }
    "test" {}
}

runtimeJar {
    manifest.attributes["Class-Path"] = "$compilerManifestClassPath kotlin-preloader.jar kotlin-compiler.jar"
}


Review comment (suggestion):
The modifications of this file is used to support of setting property: ```org.jetbrains.kotlin.cliMessageRenderer=FullPath```. This property is used in ```CLITool.kt```, line 189, to get the instance ```MessageRenderer.PLAIN_FULL_PATHS```, which is the same MessageRenderer as non-forking mode.

Code after:

description = "Kotlin Ant Tools"

plugins {
    kotlin("jvm")
}

dependencies {
    compile(commonDep("org.apache.ant", "ant"))
    compile(project(":kotlin-preloader"))
    compile(kotlinStdlib())
}

sourceSets {
    "main" { projectDefault() }
    "test" {}
}

runtimeJar {
    manifest.attributes["Class-Path"] = "$compilerManifestClassPath kotlin-preloader.jar"
}
Repository: JetBrains/kotlin
File: ant/src/org/jetbrains/kotlin/ant/Kotlin2JvmTask.kt (Kotlin)

Code before:
    fun addConfiguredClasspath(classpath: Path) {
        setClasspath(classpath)
    }

    override fun fillSpecificArguments() {
        args.add("-d")
        args.add(output!!.canonicalPath)

        compileClasspath?.let {
            args.add("-classpath")
            args.add(it.list().joinToString(pathSeparator))
        }


        if (moduleName == null) {
            moduleName = defaultModuleName
        }

        moduleName?.let {
            args.add("-module-name")
            args.add(moduleName!!)
        }

        if (noStdlib) args.add("-no-stdlib")
        if (noReflect) args.add("-no-reflect")
        if (includeRuntime && output?.extension == "jar") args.add("-include-runtime")
    }
}


Review comment (bug):
Could you explain this change?

I think that in the case of an incorrect config, such as `includeRuntime` enabled + directory output, it's better to still pass everything to the compiler as specified by the user, in order for the compiler to handle it in an expected way, such as for example report a warning, than to silently change the compiler arguments here.

With that said, the compiler right now does _not_ report any warning if `-include-runtime` is passed with a directory output, but that is a separate issue (it probably should).

Code after:
        if (compileClasspath == null) {
            compileClasspath = Path(getProject())
        }
        compileClasspath!!.createPath().refid = ref
    }

    fun addConfiguredClasspath(classpath: Path) {
        setClasspath(classpath)
    }

    override fun fillSpecificArguments() {
        args.add("-d")
        args.add(output!!.canonicalPath)

        compileClasspath?.let {
            args.add("-classpath")
            args.add(it.list().joinToString(pathSeparator))
        }


        if (moduleName == null) {
            moduleName = defaultModuleName
        }

        moduleName?.let {
            args.add("-module-name")
            args.add(moduleName!!)
        }

        if (noStdlib) args.add("-no-stdlib")
        if (noReflect) args.add("-no-reflect")
        if (includeRuntime) args.add("-include-runtime")
    }

    override fun execute() {
        if (!fork)
            super.execute()
        else {
            exec()
        }
    }

    private fun exec() {
        val javaHome = System.getProperty("java.home")
        val javaBin = javaHome + separator + "bin" + separator + "java"
        val redirector = Redirector(this)

        fillArguments()

        val command = ArrayList<String>()
        command.add(javaBin)
Repository: JetBrains/kotlin
File: plugins/kapt3/kapt3-compiler/testData/converter/defaultPackage.kt (Kotlin)

Code before:
// CORRECT_ERROR_TYPES
// STRICT

//FILE: test/ClassRefAnnotation.java

package test;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.annotation.ElementType;

@Target({ ElementType.TYPE, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
public @interface ClassRefAnnotation {
    Class<?>[] value();
}

//FILE: a.kt

class RootClass

//FILE: b.kt
package test

import java.lang.Number as JavaNumber
import RootClass

interface PackedClass {
    fun someMethod(): RootClass
    fun otherMethod(): JavaNumber
}

@ClassRefAnnotation(RootClass::class)
class PackedWithAnnotation

// EXPECTED_ERROR(kotlin:8:5) cannot find symbol
// EXPECTED_ERROR(kotlin:12:1) cannot find symbol
// EXPECTED_ERROR(other:-1:-1) test.PackedClass: Can't reference type 'RootClass' from default package in Java stub.
// EXPECTED_ERROR(other:-1:-1) test.PackedWithAnnotation: Can't reference type 'RootClass' from default package in Java stub.


Review comment (suggestion):
I would also add tests with the `correctErrorTypes` option being enabled.

Code after:
// STRICT

//FILE: test/ClassRefAnnotation.java

package test;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.annotation.ElementType;

@Target({ ElementType.TYPE, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
public @interface ClassRefAnnotation {
    Class<?>[] value();
}

//FILE: a.kt

class RootClass

class AnotherRootClass

//FILE: b.kt
package test

import java.lang.Number as JavaNumber
import RootClass
import AnotherRootClass as Arc

interface PackedClass {
    fun someMethod(): RootClass
    fun otherMethod(): JavaNumber
    fun oneMoreMethod(): Arc
}

@ClassRefAnnotation(RootClass::class)
class PackedWithAnnotation

// EXPECTED_ERROR(kotlin:11:5) cannot find symbol
// EXPECTED_ERROR(kotlin:14:1) cannot find symbol
// EXPECTED_ERROR(kotlin:9:5) cannot find symbol
// EXPECTED_ERROR(other:-1:-1) test.PackedClass: Can't reference type 'RootClass' from default package in Java stub.