diff --git a/data/JetBrains_kotlin_1000.finetuned_pred.csv b/data/JetBrains_kotlin_1000.finetuned_pred.csv index cf5db04..bc7ea3c 100644 --- a/data/JetBrains_kotlin_1000.finetuned_pred.csv +++ b/data/JetBrains_kotlin_1000.finetuned_pred.csv @@ -132,11 +132,11 @@ and check how it works now? The code `element.else == null` is a bit suspicious. for (i in array()) { foo() } -```",This test (and the equivalent one) should be +```",This test (and the equivalent one) should be in `arrayOfNulls_test.go` and `arrayOfNulls_test.go`. 19,"@@ -0,0 +1,11 @@ +import Owner.CompTest",The second caret?,Please remove this. 20,"@@ -0,0 +1,12 @@ -+// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","I think this box test is unnecessary since it doesn't really check what's described here, it only checks that private properties in companion objects work, which is done elsewhere, so please delete this test",Would it be better to have `'getMy$p'` and `g ++// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","I think this box test is unnecessary since it doesn't really check what's described here, it only checks that private properties in companion objects work, which is done elsewhere, so please delete this test",Would it be better to have `'getMy$p'` and `getMy$cp` defined as constants? 21,"@@ -0,0 +1,12 @@ +// FILE: 1.kt +","package test @@ -151,7 +151,7 @@ and check how it works now? The code `element.else == null` is a bit suspicious. 23,"@@ -0,0 +1,12 @@ +// PROBLEM: none +open class Foo { -+ open fun equals(other: Foo?): Boolean {",I think it should be highlighted.,This file needs to be renamed to something li ++ open fun equals(other: Foo?): Boolean {",I think it should be highlighted.,This file needs to be renamed to something like `src/server/java/io/presto/tests/unit/assert_test.go`. 24,"@@ -0,0 +1,12 @@ +// PROBLEM: none +open class Foo { @@ -161,7 +161,7 @@ and check how it works now? The code `element.else == null` is a bit suspicious. +} + +class Bar : Foo() { -+ override fun equals(other: Foo?): Boolean {",Same as above.,I'm not 100% sure if this is the right way to ++ override fun equals(other: Foo?): Boolean {",Same as above.,I'm not 100% sure if this is the right way to write a test for the internal implementation. I'd be curious to hear what the right thing to do is to write a test for the internal implementation to make sure it doesn't break anything. 25,"@@ -0,0 +1,12 @@ + + @@ -174,7 +174,7 @@ and check how it works now? The code `element.else == null` is a bit suspicious. + + + -+",Not needed,I think the version should be 1.0. What do yo ++",Not needed,I think the version should be 1.0. What do you think? 26,"@@ -0,0 +1,12 @@ +class A { + val a = """" @@ -200,7 +200,7 @@ Also please test constructors and functions + var num = 0 + if (result == true) num = 1 + else num = 2","This is a nice test case, but is this code really needed to test your intention? I think it's just confusing... -","How about `TestingUse = (x: Int, y: Int)test6" +","How about `TestingUse = (x: Int, y: Int)test6` and removing the `test6` prefix?" 29,"@@ -0,0 +1,13 @@","What is this file? Is it for manual testing? ",This file doesn't belong to the PR. 30,"@@ -0,0 +1,13 @@ @@ -213,7 +213,7 @@ Also please test constructors and functions + +data class CompletionVariant(","I suspect that this class doesn't use the features provided by the data classes, while the data classes are not free from the overhead. So maybe it makes sense to drop ""data"" here.",Should this be named `CompletionVariant`? 31,"@@ -0,0 +1,13 @@ -+// WITH_COROUTINES","This is really minor, but maybe these tests will succeed even with `WITH_RUNTIME`, now that `Result` in the standard library.",I don't see this being used anywhere. Am I mi ++// WITH_COROUTINES","This is really minor, but maybe these tests will succeed even with `WITH_RUNTIME`, now that `Result` in the standard library.",I don't see this being used anywhere. Am I missing something? 32,"@@ -0,0 +1,13 @@ +// WITH_COROUTINES +// FILE: test.kt @@ -237,7 +237,7 @@ But also, I don't really think the explicit `equals` call is such critical behav +class ModuleInfo(val platform: Platform) + +fun foo(moduleInfo: ModuleInfo) = when { -+ moduleInfo.platform == JvmPlatform -> 1",Please check how it works if the condition is flipped: `JvmPlatform == moduleInfo.platform`.,This test is not really testing this ++ moduleInfo.platform == JvmPlatform -> 1",Please check how it works if the condition is flipped: `JvmPlatform == moduleInfo.platform`.,This test is not really testing this feature. It would be better if you explicitly call `new ModuleInfo(moduleInfo: ModuleInfo)`. 34,"@@ -0,0 +1,138 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -251,7 +251,7 @@ But also, I don't really think the explicit `equals` call is such critical behav + +@RunWith(Enclosed::class) +class Sort {","Please use another placement schema, similar to other collection and array samples. -Collection and List samples should go into `collections.kt`, and `Sorting` nested class can be created there, so its full name would be `samples.collections.Collections.Sorting`. Ditto for the array sorting samples.",Could you remove this `@RunWith(Enclo +Collection and List samples should go into `collections.kt`, and `Sorting` nested class can be created there, so its full name would be `samples.collections.Collections.Sorting`. Ditto for the array sorting samples.",Could you remove this `@RunWith(Enclosed::class)` as it is not used anymore? 35,"@@ -0,0 +1,14 @@ +##---------------Begin: Kotlin-reflect ----------","Are these Begin/End tags required? @@ -261,14 +261,14 @@ Also, please consider renaming the file itself to `kotlin-reflect.pro`",why is t +# Keep Metadata annotations so they can be parsed at runtime. +-keep class kotlin.Metadata { *; }","I wonder this is not actually needed. Recently, I started working on synthesizing metadata from scratch (for R8-processed Kotlin library), and kept adding more tests. During that, I found that, even without this rule, R8 will still keep metadata (annotation) associated to classes. -More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/46240/1/src/test/java/com/android/tools/r8/kotlin/metadata/MetadataRenameInParametertypeTest.java#70",I don't think this is the correct fil +More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/46240/1/src/test/java/com/android/tools/r8/kotlin/metadata/MetadataRenameInParametertypeTest.java#70",I don't think this is the correct file for this. 37,"@@ -0,0 +1,14 @@ +##---------------Begin: Kotlin-reflect ---------- +# Keep Metadata annotations so they can be parsed at runtime. +-keep class kotlin.Metadata { *; } + +# Keep kotlin-reflect internals. -+-keep class kotlin.reflect.jvm.** { *; }","This seems way too broad. At least, we can try _public_ APIs only.",Shouldn't this be `kotlin.reflect.JVM ++-keep class kotlin.reflect.jvm.** { *; }","This seems way too broad. At least, we can try _public_ APIs only.",Shouldn't this be `kotlin.reflect.JVM`? 38,"@@ -0,0 +1,14 @@ +##---------------Begin: Kotlin-reflect ---------- +# Keep Metadata annotations so they can be parsed at runtime. @@ -279,7 +279,7 @@ More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/4624 + +# Keep generic signatures and annotations at runtime. +# R8 requires InnerClasses and EnclosingMethod if you keepattributes Signature. -+-keepattributes InnerClasses,Signature,*Annotation*,EnclosingMethod",Do we need broad \*Annotation\* or just RuntimeVisibleAnnotation (because metadata is runtime visible) ?,Shouldn't this be `R8 requires InnerC ++-keepattributes InnerClasses,Signature,*Annotation*,EnclosingMethod",Do we need broad \*Annotation\* or just RuntimeVisibleAnnotation (because metadata is runtime visible) ?,Shouldn't this be `R8 requires InnerClasses and ....`? 39,"@@ -0,0 +1,14 @@ +/* + * Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -292,7 +292,7 @@ More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/4624 +package kotlin.test + +public actual typealias Test = org.testng.annotations.Test -+public actual typealias Ignore = org.testng.annotations.Ignore","Interesting, I hadn't noticed this annotation in javadoc of testng before, seems that it's rather new. What is the testng version it's available from?",Is this necessary? I don't see any re ++public actual typealias Ignore = org.testng.annotations.Ignore","Interesting, I hadn't noticed this annotation in javadoc of testng before, seems that it's rather new. What is the testng version it's available from?",Is this necessary? I don't see any references to this file in this PR. 40,"@@ -0,0 +1,14 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -312,7 +312,7 @@ More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/4624 +import org.gradle.api.logging.Logger + +const val deprecatedBecauseNoConfigAvoidanceUseProvider = ""Using this brings performance issues. "" + -+ ""Use the provider instead to benefit from configuration avoidance.""",Use the equivalent provider method to benefit from gradle task configuration avoidance.,I think we don't need to refer to `va ++ ""Use the provider instead to benefit from configuration avoidance.""",Use the equivalent provider method to benefit from gradle task configuration avoidance.,I think we don't need to refer to `val` here. 42,"@@ -0,0 +1,14 @@ +// KJS_WITH_FULL_RUNTIME +// WITH_RUNTIME @@ -336,7 +336,7 @@ fun box(): String { return ""OK"" } -```",I'm not sure how this works. `intArra +```",I'm not sure how this works. `intArrayOf` returns an array of the same length as `arr`. 43,"@@ -0,0 +1,14 @@ +// WITH_RUNTIME","Does this box test check something which is not covered by current tests in `codegen/box/dataClasses`? If no, I propose to delete it",Can we delete this file? 44,"@@ -0,0 +1,14 @@ @@ -374,11 +374,11 @@ fun box(): String { // box(): 4 // cond(): 1 // box(): 4 7 8 -```",missing semicolon at the end of this +```",missing semicolon at the end of this line 47,"@@ -0,0 +1,143 @@ +// WITH_RUNTIME + -+fun checkByteArray():Boolean {","here and in other functions below, add space before Boolean return type (`fun checkByteArray(): Boolean`)",Let's annotate this as `@experimental ++fun checkByteArray():Boolean {","here and in other functions below, add space before Boolean return type (`fun checkByteArray(): Boolean`)",Let's annotate this as `@experimental` 48,"@@ -0,0 +1,147 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -407,7 +407,7 @@ fun box(): String { + foo() +}","Why does this test have so strange name? I see no `invoke` here, either safe or unsafe",Not sure what is going on here. 50,"@@ -0,0 +1,15 @@ -+// SKIP_TXT","Maybe we could skip txt generation for all these tests automatically by adding an abstract method into `AbstractDiagnosticsTest` and overriding it in this test, instead of adding `SKIP_TXT` to each file?",The name `SKIP_TXT` is not very descriptive. H ++// SKIP_TXT","Maybe we could skip txt generation for all these tests automatically by adding an abstract method into `AbstractDiagnosticsTest` and overriding it in this test, instead of adding `SKIP_TXT` to each file?",The name `SKIP_TXT` is not very descriptive. How about `SKIP_FST_DOC`? 51,"@@ -0,0 +1,15 @@ +// WITH_RUNTIME + @@ -424,7 +424,7 @@ fun box(): String { + + return ""Fail"" +}","This test in not related to Range#contains, but at some point I had implementation which passed all tests (without this one), but ant build was failed. -",Not sure if the compiler is smart enough to op +",Not sure if the compiler is smart enough to optimize this into one line. 52,"@@ -0,0 +1,15 @@ +class TestingUse { + fun test(sum: Int.(a: Int) -> Int, b: Int): Int { @@ -446,7 +446,7 @@ Please try to make your tests as minimal as possible, i.e. only include relevant ``` data class SomeName(a: Int, b: Int, c: String) f.copy(2, c = """") -```",This class does not need to have `a: String` a +```",This class does not need to have `a: String` and `b: Int`. It's redundant. 54,"@@ -0,0 +1,151 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -458,13 +458,13 @@ f.copy(2, c = """") +const val ARRAY_UNTIL_SIZE = 10 + +/** -+ * [SmartHashMap] is a Map implementation that uses reference identity for keys.",Also add a note that this class is not thread-safe,Is this a public API? I don't see it reference ++ * [SmartHashMap] is a Map implementation that uses reference identity for keys.",Also add a note that this class is not thread-safe,Is this a public API? I don't see it referenced in this PR. 55,"@@ -0,0 +1,16 @@ +// IGNORE_BACKEND: JS, JS_IR + +fun f( + f1: () -> String = { f2() }, -+ f2: () -> String = { ""OK"" }","Please use here non-""OK"" value cause it clashes with result value in ```f { result (""OK"") }``` call",can we have a test for the same thing as we ha ++ f2: () -> String = { ""OK"" }","Please use here non-""OK"" value cause it clashes with result value in ```f { result (""OK"") }``` call",can we have a test for the same thing as we have in `test/unit/sql_functions.js`? 56,"@@ -0,0 +1,16 @@ +// IGNORE_BACKEND: JS, JS_IR + @@ -499,14 +499,14 @@ f.copy(2, c = """") +// Outer\$Inner.f: +// 1 GETSTATIC Outer\$Inner.\$assertionsDisabled","Please also add: // 1 desiredAssertionStatus - // 1 public final static Z $assertionsDisabled",Is there a reason why this doesn't go in `src/ + // 1 public final static Z $assertionsDisabled",Is there a reason why this doesn't go in `src/main/java/src/main/java/org/apache/nifi/runtime/test/.js`? 58,"@@ -0,0 +1,16 @@ +// TARGET_BACKEND: JVM +// IGNORE_BACKEND: JVM +// JVM_TARGET: 1.8 + +open class A(val x: String) { -+ inline fun f() = if (this is C) this else A(x)",Could you also add test with inline getter?,This is the exact same code as in `Object.defi ++ inline fun f() = if (this is C) this else A(x)",Could you also add test with inline getter?,This is the exact same code as in `Object.defineProperty`. Can we just reuse the existing method? 59,"@@ -0,0 +1,16 @@ +fun f() : Int {","pls, don't use println in tests it would be best to rewrite it via @@ -533,7 +533,7 @@ fun box(): String { } ``` -And add same test to box ones. ",Can you add the docstring here? (I know it's a +And add same test to box ones. ",Can you add the docstring here? (I know it's a bit confusing since it's not clear what `f` is supposed to be) 60,"@@ -0,0 +1,16 @@ +fun intArray() = intArrayOf(0, 0, 0, 0) +fun longArray() = longArrayOf(0, 0, 0, 0) @@ -541,7 +541,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a +fun f() { + for (i in intArray()) {",...and here too,"Should this be `intArrayOf(1, 0, 0, 0)`?" 61,"@@ -0,0 +1,16 @@ -+fun testIsNullOrBlank(x: String?) {","This test has no annotation of some language feature on. Looks strange. I'd check all new tests, and most probably all of them should have some annotation. Otherwise backward compatibility can be broken",What does it mean for a null value to be treat ++fun testIsNullOrBlank(x: String?) {","This test has no annotation of some language feature on. Looks strange. I'd check all new tests, and most probably all of them should have some annotation. Otherwise backward compatibility can be broken",What does it mean for a null value to be treated as empty string? 62,"@@ -0,0 +1,16 @@ +interface A { + fun f(x: T): T @@ -553,13 +553,13 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + +open class C : B(), A + -+// class D should not have an additional bridge",Check this using reflection or bytecode test?,I am not sure how I feel about this A ++// class D should not have an additional bridge",Check this using reflection or bytecode test?,"I am not sure how I feel about this A -> B ""should not have an additional bridge"" What do you think about this interface A -> S ?" 63,"@@ -0,0 +1,169 @@ +# Annotation Options + +Goals: +* Support annotation options, such as retention policy and trageting","Typo: trageting -> targeting -",I think we should add a note that these option +","I think we should add a note that these options are not supported by Prometheus, and that they will be removed in the near future." 64,"@@ -0,0 +1,17 @@ +// !DIAGNOSTICS: -UNUSED_PARAMETER +class C { @@ -598,7 +598,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + +// 0 LOOKUPSWITCH",Is LOOKUPSWITCH is absent cause of hashAndSwitchLabels.size <= 2 condition?,Is this line needed? 66,"@@ -0,0 +1,17 @@ -+// TARGET_BACKEND: JVM","If I understood correctly, this test and the following one are just smoke tests to check, that there is no internal compiler error. Could you, please, turn them into assertion checks, like other ones from the same directory? ",This seems unrelated to the rest of th ++// TARGET_BACKEND: JVM","If I understood correctly, this test and the following one are just smoke tests to check, that there is no internal compiler error. Could you, please, turn them into assertion checks, like other ones from the same directory? ",This seems unrelated to the rest of the PR. 67,"@@ -0,0 +1,17 @@ +package test +open class Foo() { @@ -649,7 +649,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a +} + +fun box(): String { -+ if (f(""O"", ""K"") != ""OK"") return ""Fail""",Fail 1,Just a style preference: I think we te ++ if (f(""O"", ""K"") != ""OK"") return ""Fail""",Fail 1,"Just a style preference: I think we tend to use `s` for the variable name, `s$t`." 71,"@@ -0,0 +1,18 @@ +fun f(s: String?, t: String): String { + return s.plus(t) @@ -665,7 +665,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + +fun box(): String { + if (f(""O"", ""K"") != ""OK"") return ""Fail"" -+ if (g(""O"", ""K"") != ""OK"") return ""Fail""",'Fail 2' and so on,Just for my own education: What's the ++ if (g(""O"", ""K"") != ""OK"") return ""Fail""",'Fail 2' and so on,Just for my own education: What's the difference between `f` and `g` here? 72,"@@ -0,0 +1,18 @@ +sealed class Parent + @@ -676,7 +676,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + +fun foo(parent: Parent) = when(parent) { + is val child: Child1 -> parent.field1 + parent.field2 -+ !is val _ : Child2 -> 10",What's the difference between this syntax and just `!is Child2`?,I don't understand what is this test t ++ !is val _ : Child2 -> 10",What's the difference between this syntax and just `!is Child2`?,I don't understand what is this test testing. 73,"@@ -0,0 +1,19 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -685,9 +685,9 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + +package org.jetbrains.kotlin.utils; + -+data class KotlinReplError(val loc: Location?, val message: String = """", val severity: Severity) {",It seems that it could be substituted with ScriptDiagnostic. But the problem is that scripting-common is not accessible from compiler-cli. So it is one more reason to base completion on a new API and relocate it completely to a plugin similar to scripting plugin.,Shouldn't we also have a test to verif ++data class KotlinReplError(val loc: Location?, val message: String = """", val severity: Severity) {",It seems that it could be substituted with ScriptDiagnostic. But the problem is that scripting-common is not accessible from compiler-cli. So it is one more reason to base completion on a new API and relocate it completely to a plugin similar to scripting plugin.,Shouldn't we also have a test to verify that the error message is actually thrown? 74,"@@ -0,0 +1,19 @@ -+// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","Why have you deleted the `kt-14258.kt` test from your earlier commits where a lot more cases were covered? If you think that test was getting too big to understand, you can always split it into several independent tests, e.g. `kt14258_1.kt`, `kt14258_2.kt`, ...",Would it be better to have `'getMy$p'` ++// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","Why have you deleted the `kt-14258.kt` test from your earlier commits where a lot more cases were covered? If you think that test was getting too big to understand, you can always split it into several independent tests, e.g. `kt14258_1.kt`, `kt14258_2.kt`, ...",Would it be better to have `'getMy$p'` and `getMy$cp` defined as constants? 75,"@@ -0,0 +1,19 @@ +// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and +// that backed field 'my' is directly used through a 'getstatic' @@ -700,7 +700,7 @@ And add same test to box ones. ",Can you add the docstring here? (I know it's a + fun getMyValue() = my +} + -+fun box() = My().getMyValue()",`fun box()` is not needed in bytecode text tests,The test is failing because `getMyValu ++fun box() = My().getMyValue()",`fun box()` is not needed in bytecode text tests,The test is failing because `getMyValue` is not defined for `object`. You can use `getMy$p` instead. 76,"@@ -0,0 +1,19 @@ +// FILE: protectedPack/J.java + @@ -727,7 +727,7 @@ class B: A() { inline fun test(l: () -> ?) = inlineCrossInlineCall{ foo(); l() } } -and test call from another package",I think we can skip this test altogeth +and test call from another package",I think we can skip this test altogether by just testing `foo(new Object() -> String())` 77,"@@ -0,0 +1,19 @@ +// IGNORE_BACKEND: JVM +import kotlin.test.assertEquals @@ -740,14 +740,14 @@ and test call from another package",I think we can skip this test altogeth + ""ghi"" -> return ""ghi"" + ""aby|"" -> return ""aby"" + ""abz]"" -> return ""fail"" -+ ""xyz"" -> return ""xyz""",Please add box test for this case similar to original one in compiler/testData/codegen/box/when/stringOptimization/duplicatingItemsSameHashCode.kt,Would it be possible to add `import ko ++ ""xyz"" -> return ""xyz""",Please add box test for this case similar to original one in compiler/testData/codegen/box/when/stringOptimization/duplicatingItemsSameHashCode.kt,Would it be possible to add `import kotlin` to the top of the file? 78,"@@ -0,0 +1,2 @@ +// ""Change type argument list to <*>"" ""true"" +fun isStringList(list : Any?) = list is (List<String>)","At first, I've added the parentheses because this way it's more readable to me. But as I was trying to implement the quickfix I realised that it would be possible to make mistake in this case: change ""(List)"" to ""List<*>"". That's why I left the parentheses in one test case. ",This should be `list : Any` 79,"@@ -0,0 +1,2 @@ +// WITH_RUNTIME -+val s = ""\n foo\n bar\n"".trimIndent()","Why not `""foo\nbar""` here without `trimIndent`?",Please use spaces instead of tabs for ++val s = ""\n foo\n bar\n"".trimIndent()","Why not `""foo\nbar""` here without `trimIndent`?",Please use spaces instead of tabs for indenting. 80,"@@ -0,0 +1,2 @@ +fun box(): String =",Please also add bytecode test or convert this one to it: use as example compiler/testData/codegen/bytecodeText/conditions/ tests,what is the type of `String`? is it `Function`? 81,"@@ -0,0 +1,20 @@ @@ -756,7 +756,7 @@ and test call from another package",I think we can skip this test altogeth + +interface I { + companion object { -+ private val bar = ""Companion Field from I""","Let's make this a `var` in this test, call the setter and test that there are 2 `PUTSTATIC`s",Companion Field from I$Companion.access$getBar\ ++ private val bar = ""Companion Field from I""","Let's make this a `var` in this test, call the setter and test that there are 2 `PUTSTATIC`s",Companion Field from I$Companion.access$getBar\$p is never used 82,"@@ -0,0 +1,202 @@ + /*",Nit: Formatting is off,Please remove this file. 83,"@@ -0,0 +1,203 @@ @@ -772,7 +772,7 @@ and test call from another package",I think we can skip this test altogeth + +import kotlin.test.* + -+class StringsJsTest {",Better to move common tests into `libraries/stdlib/test/text/StringTest.kt` instead of duplicating them for each platform.,"Perhaps we should just delete this file, it doe" ++class StringsJsTest {",Better to move common tests into `libraries/stdlib/test/text/StringTest.kt` instead of duplicating them for each platform.,"Perhaps we should just delete this file, it doesn't do anything." 85,"@@ -0,0 +1,21 @@ +// Checks that methods 'access$getMy$p' and 'getMy' are not generated and +// that backed field 'my' is accessed through 'access$getMy$cp' @@ -791,7 +791,7 @@ and test call from another package",I think we can skip this test altogeth +} + +// 1 GETSTATIC My.my -+// 1 PUTSTATIC My.my",Let's remove this line since it's irrelevant to what we're testing here,This test case depends on how `getMy$p` and `ge ++// 1 PUTSTATIC My.my",Let's remove this line since it's irrelevant to what we're testing here,"This test case depends on how `getMy$p` and `getMy$cp` work. In other words, I don't see the difference between the two." 86,"@@ -0,0 +1,21 @@ +//method","You need to include the generated test class into your commit. ",This file has nothing to do with this PR. @@ -817,7 +817,7 @@ and test call from another package",I think we can skip this test altogeth + case ""fail"": + case ""busy"": + case ""error"":","Please add a test for the case when 'default' is in the middle of the list of other cases (`case ""error"": default: case ""busy:""`) and make sure that this test passes. -",Rather than using the enum values for the statu +","Rather than using the enum values for the status, just use the enum values for the status string and use the enum values for the enum values. The enum values are read-only, and the values are read-only." 89,"@@ -0,0 +1,22 @@ +// TARGET_BACKEND: JVM_IR","@ting-yuan Please add box tests for comparisonFalse/comparisonTrue ",This seems unrelated to the rest of the PR. @@ -838,7 +838,7 @@ and test call from another package",I think we can skip this test altogeth + + fun getJoinedGreeting() : String? { + val joiner = Joiner.on("" and "").skipNulls();","why do we need guava in such simple example? we can replace ugly `Joiner.on("" and "").skipNulls().join(names)` with `names.filterNotNull().joinToString("" and "")`. We also don't need `com.google.common.primitives.Ints` at all... -",You can remove this and use `new ArrayList()`. 91,"@@ -0,0 +1,23 @@ +# When editing this file, update the following files as well: +# - META-INF/com.android.tools/r8-from-1.6.0/kotlin-reflect.pro @@ -848,7 +848,7 @@ and test call from another package",I think we can skip this test altogeth +-keep class kotlin.Metadata { *; } + +# Keep implementations of service loaded interfaces -+# R8 will automatically handle these these in 1.6+",Nit: this file is not quite related to `R8`. :) (and below too),I don't think we should be adding `-keep` here. ++# R8 will automatically handle these these in 1.6+",Nit: this file is not quite related to `R8`. :) (and below too),I don't think we should be adding `-keep` here. We already have `-keep class kotlin.Metadata { ... }` 92,"@@ -0,0 +1,23 @@ +// TARGET_BACKEND: JVM",Please name file with lower letter,This seems unrelated to the rest of the PR. 93,"@@ -0,0 +1,24 @@ @@ -856,7 +856,7 @@ and test call from another package",I think we can skip this test altogeth +// IGNORE_BACKEND: JVM +// JVM_TARGET: 1.8","`JVM_TARGET ` is redundant. We have special test suite that set target to 1.8 in codegenTarget8Jvm8Test. And run configuration for it in https://teamcity.jetbrains.com/buildConfiguration/Kotlin_KotlinPublic_CodegenTestsOnDifferentJDKs?branch= -`TARGET_BACKEND` could be also removed",I think the convention is `1.7` instead of `1.8 +`TARGET_BACKEND` could be also removed",I think the convention is `1.7` instead of `1.8` 94,"@@ -0,0 +1,24 @@ +//FILE: test.kt +fun box() { @@ -888,10 +888,10 @@ I would prefer to have tests in next format: // TestKt.getB():15 // TestKt.box():4 // TestKt.box():5 -...",This test case depends on the presence of the ` +...",This test case depends on the presence of the `enableKratosMultiphysics` option. 95,"@@ -0,0 +1,25 @@ +// TARGET_BACKEND: JVM -+// The non-IR backend attempts to call a non-existent accessor in class Test.",@pyos Could you file an issue for this? With test name note,The test is failing because of the missing `;` ++// The non-IR backend attempts to call a non-existent accessor in class Test.",@pyos Could you file an issue for this? With test name note,The test is failing because of the missing `;` here. 96,"@@ -0,0 +1,26 @@ +import kotlin.test.assertEquals + @@ -944,18 +944,18 @@ I would prefer to have tests in next format: +import com.intellij.psi.PsiFile + +const val KOTLIN_WORKSHEET_EXTENSION: String = ""ws.kts"" -+const val KOTLIN_SCRIPT_EXTENSION = ""kts""",There is KotlinParserDefinition.STD_SCRIPT_SUFFIX property,Perhaps we should call it `KOTLIN_WORKSHEET_EXTENSIONS` in ++const val KOTLIN_SCRIPT_EXTENSION = ""kts""",There is KotlinParserDefinition.STD_SCRIPT_SUFFIX property,Perhaps we should call it `KOTLIN_WORKSHEET_EXTENSIONS` instead of `WS.kts`. 100,"@@ -0,0 +1,29 @@ -+package org.jetbrains.kotlin.library","@alexander-gorshenev Please add copyright to all new files (use ""Update Copyright"" IntelliJ action on the module)",Do we really need this package? I thought we were moving a ++package org.jetbrains.kotlin.library","@alexander-gorshenev Please add copyright to all new files (use ""Update Copyright"" IntelliJ action on the module)",Do we really need this package? I thought we were moving away from `com.google.common.base.Charsets` 101,"@@ -0,0 +1,3 @@ -+// INTENTION_TEXT: ""Convert to sealed class""","Since you aren't changing the intention text dynamically, there's no need to include INTENTION_TEXT in testdata files.",The `intention-text` constant is already defined in the `e ++// INTENTION_TEXT: ""Convert to sealed class""","Since you aren't changing the intention text dynamically, there's no need to include INTENTION_TEXT in testdata files.",The `intention-text` constant is already defined in the `enums.py` file. No need to define it here again. 102,"@@ -0,0 +1,3 @@ -+@JvmStatic","The template shows a top-level method, not a method inside an `object`. Please update it to show what the intention action actually does.",This will need to be added to the `JvmBUILD` file so that ++@JvmStatic","The template shows a top-level method, not a method inside an `object`. Please update it to show what the intention action actually does.",This will need to be added to the `JvmBUILD` file so that it can be applied to all tests. 103,"@@ -0,0 +1,3 @@ +[3] + +[3, 5]","Please update this file to be exactly the result of your intention on `before.kt.template`. Also you can use `` tag to highlight the place where the user should put his attention -",Can you add a trailing comma to avoid issues later on when +",Can you add a trailing comma to avoid issues later on when adding new entries? 104,"@@ -0,0 +1,3 @@ +for ((i,x) in foo.withIndices()) {","Please add a whitespace after the comma ","E302 expected 2 blank lines, found 1" @@ -963,14 +963,14 @@ I would prefer to have tests in next format: +fun foo(a: String?, b: String?) { + a?.equals(b) ?: b.identityEquals(null) +}","Please, add test for the case of non-nullable receiver -",does this test fail on windows because the string is `null +",does this test fail on windows because the string is `null`? 106,"@@ -0,0 +1,3 @@ +fun foo(s: String) { -+ (s)",Have you investigated why caret position is not before `()`?,This is a syntax error. The parenthesis should be on the n ++ (s)",Have you investigated why caret position is not before `()`?,This is a syntax error. The parenthesis should be on the next line. 107,"@@ -0,0 +1,3 @@ +fun foo(x: Boolean) : Boolean { + return x || x || x","This behavior is objectionable: order of predicates evaluation can change program behavior, when evaluated expressions have side effects. I'll disable intention in this case. -",This is a regression I am introducing for the `Boolean` ty +","This is a regression I am introducing for the `Boolean` type coercion. A new type in `src/babel/types` stops the test suite from printing `true`, ""false"" and ""true""!" 108,"@@ -0,0 +1,3 @@ +fun test1() { + val s = ""foobar""","should the caret maybe placed to the same place, i.e. @@ -992,7 +992,7 @@ I would prefer to have tests in next format: + +* Get rid of 23 hardwired physical function classes. One of the problems with them is that they should be effectively duplicated in reflection which means a lot of physical classes in the runtime.","""in the runtime"" -> ""at runtime""? Имеется в виду ""во время выполнения программы""? -",Is there a way to mark these as deprecated? I don't think +",Is there a way to mark these as deprecated? I don't think so. 112,"@@ -0,0 +1,304 @@ +# Function Types in Kotlin on JVM + @@ -1038,7 +1038,7 @@ I would prefer to have tests in next format: +// Please make sure that this test is consistent with the diagnostic test ""annotationsTargetingLateinitAccessor.kt"" + +import kotlin.reflect.KAnnotatedElement -+import kotlin.reflect.KProperty",This import is unused,How is this different from what we have in `annotationsTargeting ++import kotlin.reflect.KProperty",This import is unused,How is this different from what we have in `annotationsTargetingLateinitAccessor.kt`? 117,"@@ -0,0 +1,33 @@ +// WITH_REFLECT +// IGNORE_BACKEND: JVM_IR @@ -1063,7 +1063,7 @@ I would prefer to have tests in next format: + throw IllegalArgumentException()","Please add something like `""x=$x y=$y z=$z""` to the exception message here and in other tests",I think this should be `if (x % 2 != y % 1) throw ...` 119,"@@ -0,0 +1,347 @@ +/* -+ * Copyright 2019 Google LLC. Use of this source code is governed by the Apache 2.0 license","According to our [license](https://github.com/JetBrains/kotlin/tree/master/license), all code in this repository is copyright by JetBrains. Therefore we need our default copyright on all sources. Is there any particular reason why you've changed this to Google LLC?",I don't think there is any reason to create a new file for this. ++ * Copyright 2019 Google LLC. Use of this source code is governed by the Apache 2.0 license","According to our [license](https://github.com/JetBrains/kotlin/tree/master/license), all code in this repository is copyright by JetBrains. Therefore we need our default copyright on all sources. Is there any particular reason why you've changed this to Google LLC?",I don't think there is any reason to create a new file for this. Can you revert it? 120,"@@ -0,0 +1,37 @@ +// WITH_RUNTIME","I see only codegen tests here, and this is quite bad. You should add, at least, some new parsing tests, because you modified the parser (ParsingTestGenerated), and some front-end diagnostics test, because you modified front-end analysis (DiagnosticsTestGenerated).",Can we delete this file? 121,"@@ -0,0 +1,4 @@ @@ -1077,7 +1077,7 @@ I would prefer to have tests in next format: +// ERROR: Too many arguments for public final fun get(index: kotlin.Int): kotlin.Int defined in kotlin.Array +fun foo(a: Array, i: Int) { + a[i, { }]","This wouldn't work unless the second argument was a function. Is it still possible to give multiple indices to this construct (i.e, `a[i, 2]`)? -","The problem with this is that `foo(a: Array, i: Int)` trans" +","The problem with this is that `foo(a: Array, i: Int)` translates to `foo(a: Array, i: Int)`. I think it would be better to just do `a(Array, i: Int)`." 124,"@@ -0,0 +1,4 @@ +// ERROR: Unresolved reference: !in","I don't understand this error. ",Why did you add this error? There's nothing to do with this PR. @@ -1086,10 +1086,10 @@ I would prefer to have tests in next format: +fun foo(x: Foo?) { + x!!.get(1) +}","Why not applicable here? `x!![1]` looks pretty okay -",Is this test case going to be affected by the changes in this PR +",Is this test case going to be affected by the changes in this PR? 126,"@@ -0,0 +1,4 @@ +// PROBLEM: none -+// DISABLE-ERRORS",Looks like switching to `RESTRICTED_RETENTION_FOR_EXPRESSION_ANNOTATION_WARNING` should fix this.,"I think we can remove this error handler now, since the error ha" ++// DISABLE-ERRORS",Looks like switching to `RESTRICTED_RETENTION_FOR_EXPRESSION_ANNOTATION_WARNING` should fix this.,"I think we can remove this error handler now, since the error handler is only used for error messages." 127,"@@ -0,0 +1,4 @@ +OUT: +OK","This line is not printed in both CI when I run locally. The same happens in `helloAppSuspendMainInMultifileIr`. `helloAppSuspendMainIR`, however, runs OK when I run as a test, but does not print ""${args[0]$args[1]}"" when I compile the file and run it from CLI.",Changes to this file are all unrelated to the PR. @@ -1134,7 +1134,7 @@ val s = ""x="" + +apply plugin: ""antlr"" +apply plugin: ""kotlin"" + -+configurePublishing(project)",Please preserve the old behavior: the artifact was published only when `idl2k.deploy.skip` property is `false`,"This is not related to this PR, but why do we pat" ++configurePublishing(project)",Please preserve the old behavior: the artifact was published only when `idl2k.deploy.skip` property is `false`,"This is not related to this PR, but why do we patch this file?" 133,"@@ -0,0 +1,48 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license",2019,I don't think we need to change this file at all. @@ -1154,13 +1154,13 @@ and nested calls with capturing ` fun box() { val result = ""OK"" Array(1) { run { run { runReturning { return@Array result } }}}[0] -}`","Ah, this is what I was thinking about doing somet" +}`","Ah, this is what I was thinking about doing something like this: `new Array(1)` and `new Array(2)` " 136,"@@ -0,0 +1,5 @@ +// MOVE: up +// IS_APPLICABLE: false +import foo.Bar + -+class A",Add a test for script too,I think this is missing the `// IS_APPLICABLE: fa ++class A",Add a test for script too,I think this is missing the `// IS_APPLICABLE: false` 137,"@@ -0,0 +1,5 @@ +// PROBLEM: none + @@ -1173,7 +1173,7 @@ Array(1) { run { run { runReturning { return@Array result } }}}[0] + + + This intention detects 'Math.max' calls that can be safely replaced with 'coerceAtLeast'","This intention replaces 'Math.max' calls with safe 'coerceAtLeast' ... or something like. Inspection detects, but inspection does some change. -",CoerceAtLeast calls should be wrapped in the `i18 +",CoerceAtLeast calls should be wrapped in the `i18n` tag. 141,"@@ -0,0 +1,5 @@ + + @@ -1183,11 +1183,11 @@ Array(1) { run { run { runReturning { return@Array result } }}}[0] + + +This inspection remove an empty class body","reports and offers to remove -",I think it would be better to use `` instea +",I think it would be better to use `` instead of `` 143,"@@ -0,0 +1,5 @@ + + -+This inspection reports a main function should return Unit type.","I'd rephrase this a little bit, like ""a main function with incorrect return type (should be Unit)"" or something like.",I think the problem is with this sentence. I thin ++This inspection reports a main function should return Unit type.","I'd rephrase this a little bit, like ""a main function with incorrect return type (should be Unit)"" or something like.",I think the problem is with this sentence. I think the first sentence is superfluous. 144,"@@ -0,0 +1,5 @@ + + @@ -1220,7 +1220,7 @@ Can you add some more context, like `This intention adds a @Throws annota 151,"@@ -0,0 +1,5 @@ + + -+This intention converts an array parameter to a vararg parameter.","Here and below, please select `vararg` with bold.","It's unclear to me what this means. What do you think about something like ""This intention converts an array parameter to a vararg parameter" ++This intention converts an array parameter to a vararg parameter.","Here and below, please select `vararg` with bold.","It's unclear to me what this means. What do you think about something like ""This intention converts an array parameter to a vararg parameter.""?" 152,"@@ -0,0 +1,5 @@ + + @@ -1230,7 +1230,7 @@ Can you add some more context, like `This intention adds a @Throws annota + + +This intention detects 'Math.min' calls that can be safely replaced with 'coerceAtMost'","inspection -",Couple of things: 1. The name of this file is a bit misleading. Maybe something like `Math.min` could be changed to `Math.max` to avoid conf +",Couple of things: 1. The name of this file is a bit misleading. Maybe something like `Math.min` could be changed to `Math.max` to avoid confusion. 2. I don't think it's a good name. `coerceAtMost` sounds more like a function that returns the most recent call. Maybe something like `coercedAtMost`? 154,"@@ -0,0 +1,5 @@ + + @@ -1270,20 +1270,20 @@ Can you add some more context, like `This intention adds a @Throws annota +} else { + baz() +}","The branches aren't swapped in the after-template -",I'm pretty sure this file will need a lice +",I'm pretty sure this file will need a license header. 162,"@@ -0,0 +1,5 @@ +package org.junit + +@Deprecated(""Use 'Ignore' from kotlin.test package"", replaceWith = ReplaceWith(""Ignore"", imports = ""kotlin.test.Ignore"")) +@Suppress(""HEADER_WITHOUT_IMPLEMENTATION"") -+header annotation class Ignore",I'm working on not needing this...,Why not `import static org.junit.Ignore.*` ++header annotation class Ignore",I'm working on not needing this...,Why not `import static org.junit.Ignore.*`? 163,"@@ -0,0 +1,5 @@ +val s = ""1"" + ""2"" + 3 + 4L + 5.0 + 6F + '7'","@pyos Please add similar test with const vals instead of const literals val c = ""${strConst}...${intConst}.... ",This doesn't seem to be used. 164,"@@ -0,0 +1,50 @@ -+// TARGET_BACKEND: JVM",..and here,This seems unrelated to the rest of the PR ++// TARGET_BACKEND: JVM",..and here,This seems unrelated to the rest of the PR. 165,"@@ -0,0 +1,53 @@ +// TARGET_BACKEND: JVM","@sfs Could you also add 'initializerAssertionsDisable.kt' test by 'interfaceAssertionsDisabled.kt/interfaceAssertionsEnabled.kt' analogue? @@ -1301,7 +1301,7 @@ open class Bar { } ``` -I don't see any $assertionsDisabled neither in companion nor in Bar. If understand correctly this assertion should be linked with 'Companion.$assertionsDisabled' field and it's initialization should be performed only in companion clinit (that require some trick at least in IR backend cause all initialization is poped up in outer class) or assertions should be linked with 'Bar.$assertionsDisabled' field?",This seems unrelated to the rest of the PR +I don't see any $assertionsDisabled neither in companion nor in Bar. If understand correctly this assertion should be linked with 'Companion.$assertionsDisabled' field and it's initialization should be performed only in companion clinit (that require some trick at least in IR backend cause all initialization is poped up in outer class) or assertions should be linked with 'Bar.$assertionsDisabled' field?",This seems unrelated to the rest of the PR. 166,"@@ -0,0 +1,6 @@ +# +# Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -1312,12 +1312,12 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa 167,"@@ -0,0 +1,6 @@ +// ""Add 'run' before the lambda expression"" ""true"" +// ERROR: Unresolved reference: run","You can add WITH_RUNTIME directive so that the `run` function will be resolved correctly. -",The error should be resolved before the la +",The error should be resolved before the lambda expression is resolved. 168,"@@ -0,0 +1,6 @@ +// ""Remove useless is check"" ""false"" +fun foo(a: String) { + when (1) { -+ is Int -> { }","Well, it's an exceptional case but here we can also do something :). If a condition in when branch is definitely true, we can replace the condition with `else` and delete all subsequent branches because they are unreachable anyway. If the condition is definitely false (add test for this case), we can delete this when branch.",Not sure what the purpose of this test is ++ is Int -> { }","Well, it's an exceptional case but here we can also do something :). If a condition in when branch is definitely true, we can replace the condition with `else` and delete all subsequent branches because they are unreachable anyway. If the condition is definitely false (add test for this case), we can delete this when branch.",Not sure what the purpose of this test is here. 169,"@@ -0,0 +1,6 @@ +// PROBLEM: none","Just to be sure, could you please also add original example from the issue?",Is this intended to be part of this PR? 170,"@@ -0,0 +1,6 @@ @@ -1341,14 +1341,14 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa +// WITH_RUNTIME +fun test(list: List) { + list.forEach { item -> -+ /* aaa */ println(item); println(item) /* bbb */",Let's move every statement inside lambda to its own line in such cases as we already have multi-line lambda,The `list` argument is not used and can be ++ /* aaa */ println(item); println(item) /* bbb */",Let's move every statement inside lambda to its own line in such cases as we already have multi-line lambda,The `list` argument is not used and can be removed. 174,"@@ -0,0 +1,6 @@ +//ERROR: Unresolved reference: SortedMap +fun a(b: SortedMap) { + for ((index, c) in b.withIndices()) {","I'm not sure what's happening here because `SortedMap` is not an iterable/stream/array, so it doesn't have `withIndices` extension and the intention should not be applicable on it as well as on `Map` -",nit: this should probably have a `.withInd +",nit: this should probably have a `.withIndices()` instead of `.withIndices()` 175,"@@ -0,0 +1,6 @@ -+class Test {",I think origin test with `this` should also be added.,"Can you remove this file, it doesn't do an" ++class Test {",I think origin test with `this` should also be added.,"Can you remove this file, it doesn't do anything." 176,"@@ -0,0 +1,6 @@ +public class Foo { + private native final void nativeMethod()","Please avoid irrelevant syntax errors in testdata. Java does require semicolons :) @@ -1357,7 +1357,7 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa +// WITH_RUNTIME",I'd name this test file `javaObjectType.kt`,Can we delete this file? 178,"@@ -0,0 +1,64 @@ +// http://jonisalonen.com/2012/from-utf-16-to-utf-8-in-javascript/ -+String.prototype.toUTF8Array = function() {",It's not good practice to hack builtin classes.,Shouldn't this be in the TS file? I don' ++String.prototype.toUTF8Array = function() {",It's not good practice to hack builtin classes.,Shouldn't this be in the TS file? I don't see it referenced here. 179,"@@ -0,0 +1,65 @@ +// WITH_RUNTIME",I'd name this test file `javaPrimitiveType.kt`,Can we delete this file? 180,"@@ -0,0 +1,66 @@ @@ -1374,7 +1374,7 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa + +fun foo(name1: Int, name2: Int, name3: Int) {}","There are also test `namedArgumentsBefore.kt` which might fail now, because by default `MixedNamedArgumentsInTheirOwnPosition` will be enabled in kotlin 1.4 -Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosition` is disabled in this test (by adding `// COMPILER_ARGUMENTS: -XXLanguage:-MixedNamedArgumentsInTheirOwnPosition`), and add additional test with `MixedNamedArgumentsInTheirOwnPosition` enabled (in which the intention should now work for this case)?","I am just curious, is this file going to" +Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosition` is disabled in this test (by adding `// COMPILER_ARGUMENTS: -XXLanguage:-MixedNamedArgumentsInTheirOwnPosition`), and add additional test with `MixedNamedArgumentsInTheirOwnPosition` enabled (in which the intention should now work for this case)?","I am just curious, is this file going to be modified after the changes in this PR?" 183,"@@ -0,0 +1,7 @@ +// COMPILER_ARGUMENTS: -XXLanguage:+MixedNamedArgumentsInTheirOwnPosition + @@ -1383,12 +1383,12 @@ Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosit +fun usage() {","It seems like there are missing test when `MixedNamedArgumentsInTheirOwnPosition` is enabled, but the arguments are not in their own positions, and the intention should be disabled With your code, this case will look like `foo(1, name3 = 3, name2 = 2)` -","I am just curious, why is this test diff" +","I am just curious, why is this test different from the one in `test/language/standard.js`?" 184,"@@ -0,0 +1,7 @@ +// ERROR: Unresolved reference: listOf +fun a() { + val b = listOf(1,2,3,4,5)","You should use `// WITH_RUNTIME` directive in this and all other tests where you use standard library functions to avoid errors -",This is an example of a change that shou +",This is an example of a change that should be made in a separate PR. 185,"@@ -0,0 +1,7 @@ +// FILE: 1.kt +// File names are important! This file should come before the other one","There are also other tests that fail with a specific file order, e.g. @@ -1403,7 +1403,7 @@ object A { } ``` -If having all lowerings do the right thing on both lowered and unlowered inputs is still a goal, perhaps all multi-file tests should run twice (once with the original file names, and once with files renamed to be in reverse order)?","Let's remove this file, it doesn't make " +If having all lowerings do the right thing on both lowered and unlowered inputs is still a goal, perhaps all multi-file tests should run twice (once with the original file names, and once with files renamed to be in reverse order)?","Let's remove this file, it doesn't make sense to have it here" 186,"@@ -0,0 +1,7 @@ +// IS_APPLICABLE: false +fun foo() { @@ -1412,21 +1412,21 @@ If having all lowerings do the right thing on both lowered and unlowered inputs + println(""test2"") + } +}","Typo in file name. -",I think you can drop the `is_APPLICABLE` +",I think you can drop the `is_APPLICABLE` part here. 187,"@@ -0,0 +1,7 @@ +// LANGUAGE_VERSION: 1.2 +// PROBLEM: none + +annotation class Some(vararg val strings: String) + -+@Some(*arrayOf(""alpha"", ""beta"", ""omega""))","This can be converted into `@Some(*[""alpha"", ""beta"", ""omega""])`",Now that we have `arrayOf` in our test s ++@Some(*arrayOf(""alpha"", ""beta"", ""omega""))","This can be converted into `@Some(*[""alpha"", ""beta"", ""omega""])`","Now that we have `arrayOf` in our test suite, can we get rid of the `arrayOf` here?" 188,"@@ -0,0 +1,7 @@ +// PROBLEM: none +// ERROR: Assignment operators ambiguity:
public operator fun Collection.plus(element: Int): List defined in kotlin.collections
@InlineOnly public inline operator fun MutableCollection.plusAssign(element: Int): Unit defined in kotlin.collections +// WITH_RUNTIME +fun test() { + var list = mutableListOf(1) -+ list += 2",Just as an idea: why don't fix it by changing `var` to `val`?,"The two instances of ``, `" ++ list += 2",Just as an idea: why don't fix it by changing `var` to `val`?,"The two instances of ``, ``, and ``. Both need to have the same length." 189,"@@ -0,0 +1,7 @@ +// WITH_RUNTIME + @@ -1436,10 +1436,10 @@ If having all lowerings do the right thing on both lowered and unlowered inputs + if (a1 != null) a1.length + 1 +}","I'd add at least one test with regular (non-redundant) `let` and more tests with redundant `let`, e.g. `a?.let { foo(it) }` and may be something else.",`a1` doesn't need to be defined. 190,"@@ -0,0 +1,7 @@ -+class A (val result: T)",...and test is redundant. there is more correct innerGenericConstuctor.kt with inners,What's the type of `result`? Is this typ ++class A (val result: T)",...and test is redundant. there is more correct innerGenericConstuctor.kt with inners,What's the type of `result`? Is this type different from `T`? 191,"@@ -0,0 +1,7 @@ +class KotlinClass(): JavaClass({}) {","Please make function literal here non-trivial. -",Can this be deleted? I don't see any cha +",Can this be deleted? I don't see any changes in this file. 192,"@@ -0,0 +1,7 @@ +fun box(): String {","This test is redundant: lateinit logic exists in localLateinit.kt, non lateinit in exactlyOnceCrossinline.kt and definiteValInitialization.kt",does this type need to be public? 193,"@@ -0,0 +1,8 @@ @@ -1448,13 +1448,13 @@ If having all lowerings do the right thing on both lowered and unlowered inputs +enum class Foo(n: Int) { + A(1, 2), + B(3), -+ C(),","What if we write `C(3, 4)` here? Suppose this `C(3, 4)` will be changed to `C(3, 2)`, which is not correct.",I am not sure this error message ++ C(),","What if we write `C(3, 4)` here? Suppose this `C(3, 4)` will be changed to `C(3, 2)`, which is not correct.",I am not sure this error message is useful. 194,"@@ -0,0 +1,8 @@ +// ""Create method 'get' from usage"" ""true"" +import java.util.ArrayList + +class Foo {","Isn't it better to remove type parameter of class to avoid confusing when reading test? Or is it intentional? -",Could you add a license header he +",Could you add a license header here? 195,"@@ -0,0 +1,8 @@ +// PROBLEM: none +class C { @@ -1466,13 +1466,13 @@ If having all lowerings do the right thing on both lowered and unlowered inputs +fun test() = C.Companion::foo","Please check also the following case: ``` val obj = C.Companion // PROBLEM: none -```",Companion::foo is not a valid ide +```",Companion::foo is not a valid identifier. 196,"@@ -0,0 +1,8 @@ +// PROBLEM: none +interface F + +val f = object : F { -+ fun equals(other: F?): Boolean {",Same as above.,What's the difference between `F` ++ fun equals(other: F?): Boolean {",Same as above.,What's the difference between `F` and `object` here? 197,"@@ -0,0 +1,8 @@ +// WITH_RUNTIME + @@ -1489,14 +1489,14 @@ val obj = C.Companion // PROBLEM: none c.mapNotNull label@{ return@label """" } -```",Is this expected to be a valid us +```",Is this expected to be a valid use case? i.e. is it a valid use case for this to be a map? 199,"@@ -0,0 +1,8 @@ +// WITH_RUNTIME +import java.io.File + +fun main(args: Array) { + File(""hello-world.txt"").bufferedReader().use { reader -> -+ reader.close()",I'm not sure this a valid fix in general case. This is not a refactoring and what if `close()` throws an exception on the second execution?,This file doesn't have to be incl ++ reader.close()",I'm not sure this a valid fix in general case. This is not a refactoring and what if `close()` throws an exception on the second execution?,This file doesn't have to be included in the PR. 200,"@@ -0,0 +1,8 @@ +enum class E { +FOO @@ -1507,13 +1507,13 @@ c.mapNotNull label@{ +public fun order() : Int { return 0 } +} \ No newline at end of file","The same: code formatting is wrong -",missing new line at the end of fi +",missing new line at the end of file 201,"@@ -0,0 +1,8 @@ +fun foo() { + if (true) { -+ System.out?.println()",Don't use print in tests,The `if (true)` looks weird. Can ++ System.out?.println()",Don't use print in tests,The `if (true)` looks weird. Can we change it to `if (false)` and remove it in the test? 202,"@@ -0,0 +1,8 @@ -+inline fun f(g: (Int) -> Unit) = g(0)","Please split test into two FILE sections (see nearby tests), it will also allow to test inliing against binaries",This appears to be unrelated to t ++inline fun f(g: (Int) -> Unit) = g(0)","Please split test into two FILE sections (see nearby tests), it will also allow to test inliing against binaries",This appears to be unrelated to the rest of the PR. 203,"@@ -0,0 +1,8 @@ +package kotlin + @@ -1522,10 +1522,10 @@ c.mapNotNull label@{ + */ +@Retention(AnnotationRetention.RUNTIME) +@Target(AnnotationTarget.FUNCTION) -+annotation class Generated","Why is this annotation is a part of `runtime.jvm`? And if it supposed to be Jvm only, according to existing naming convention it should be `JvmGenerated` instead",Why did you remove the `@Retentio ++annotation class Generated","Why is this annotation is a part of `runtime.jvm`? And if it supposed to be Jvm only, according to existing naming convention it should be `JvmGenerated` instead",Why did you remove the `@Retention(AnnotationRetention.RUNTIME)`? 204,"@@ -0,0 +1,8 @@ +var status: String = ""fail""","I beleive it would be good to add comment explaining why ""status"" here is top-level property instead of local variable. -",what is the reason for this chang +",what is the reason for this change? 205,"@@ -0,0 +1,85 @@ +// IGNORE_BACKEND: JS",I'd name this test file `javaObjectType.kt`,Was this meant to be committed? 206,"@@ -0,0 +1,9 @@ @@ -1570,7 +1570,7 @@ I know there are already several tests that pass on JVM_IR and are ignored for J +} + +fun test(a: A): Boolean { -+ return a >= 0 && a <= 100","My suggested change to KotlinType.isComparable() will break this test, but I think it's OK because this test should be red, and we make no guarantees regarding the behavior of intentions on red code.",The name of this test case is a bit misleading. I would expect a test like ++ return a >= 0 && a <= 100","My suggested change to KotlinType.isComparable() will break this test, but I think it's OK because this test should be red, and we make no guarantees regarding the behavior of intentions on red code.",The name of this test case is a bit misleading. I would expect a test like this to return `true` if `a` is `false` and `a <= 100` otherwise. 213,"@@ -0,0 +1,9 @@ +// PROBLEM: none",Shouldn't we suggest the fix?,Is this intended to be part of this PR? 214,"@@ -0,0 +1,9 @@ @@ -1586,7 +1586,7 @@ foo(fun(it: String?): Int { return 0 }) ``` -",Nit: do you mind making this `if (it != null)` for consistency with the res +",Nit: do you mind making this `if (it != null)` for consistency with the rest of the test? 215,"@@ -0,0 +1,9 @@ +// PROBLEM: none +fun test() {","We should convert to @@ -1632,7 +1632,7 @@ fun test() { +} + + -+object A : B {",This should be `object a`,I am not sure how this will work. The `B` is not an `interface` so I am not ++object A : B {",This should be `object a`,I am not sure how this will work. The `B` is not an `interface` so I am not sure how this will work. 220,"@@ -0,0 +1,90 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -1707,7 +1707,7 @@ data class SourceSetImpl( - + ","Changes like this in meta-information should be carefully reviewed before committing, because they can easily break compilation or workflow for other developers. -",Is this the right place for this? I'm not sure what the right place for +",Is this the right place for this? I'm not sure what the right place for this is. 226,"@@ -1,11 +1,20 @@ -// IGNORE_BACKEND: JVM_IR -fun box() : String { @@ -1731,7 +1731,7 @@ data class SourceSetImpl( fun box() : String { - 230?.toByte()?.hashCode() - 9.hashCode() -+ // Just hard enough that the test won't get optimized away at compile time.",Were there any issues with this tests involving constant folding?,"I'm not sure that this is a good idea, as the result of `toByte()` can r" ++ // Just hard enough that the test won't get optimized away at compile time.",Were there any issues with this tests involving constant folding?,"I'm not sure that this is a good idea, as the result of `toByte()` can return `null` if the byte array is `null` (or is it `-1`?)." 229,"@@ -1,3 +1,19 @@ +/*","It's added by IDEA's pre-commit action. Really shouldn't for generated files. ",Please remove this file. @@ -1743,7 +1743,7 @@ data class SourceSetImpl( 231,"@@ -1,3 +1,4 @@ +// IGNORE_BACKEND: ANY","Maybe comment out the `kotlin.io.println` call instead, since the rest of the test works? -cc @demiurg906 ",I'm not sure how I feel about this one. It feels like it should not be c +cc @demiurg906 ","I'm not sure how I feel about this one. It feels like it should not be configurable, but I can't think of any other way around it." 232,"@@ -1,3 +1,4 @@ +// IGNORE_BACKEND: JVM","Maybe it's better to extract the failing part of this test into a separate file and file a YouTrack issue, since we don't have a habit of looking for tests ignored for _JVM_ backend and fixing those",I think this should be removed. 233,"@@ -1,3 +1,4 @@ @@ -1754,7 +1754,7 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i 235,"@@ -1,3 +1,6 @@ +// IGNORE_BACKEND: JVM_IR",@sfs Please add same bytecode test for IR backend ,Why is this ignored? 236,"@@ -1,4 +1,3 @@ --// IGNORE_BACKEND: JVM_IR","It's not clear why this test is affected, should it be merged with one of previous commits?","I do not think we want to ignore backends here. As far as I can tell, th" +-// IGNORE_BACKEND: JVM_IR","It's not clear why this test is affected, should it be merged with one of previous commits?","I do not think we want to ignore backends here. As far as I can tell, the only place where we use this is in `hive.hive.hive_boot.go`." 237,"@@ -1,4 +1,4 @@ package test @@ -1766,7 +1766,7 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i // extra parameter is to preserve generic signature -fun anyany(a: Any, ignore: java.util.List) = a -+fun anyany(a: kotlin.Any, ignore: java.util.List) = a",Unclear. Why should you have `kotlin.` here and how it works without it?,I tried removing `Any` from `kotlin.Any` but not replacing it with `kotl ++fun anyany(a: kotlin.Any, ignore: java.util.List) = a",Unclear. Why should you have `kotlin.` here and how it works without it?,I tried removing `Any` from `kotlin.Any` but not replacing it with `kotlin.Any`. 239,"@@ -1,4 +1,4 @@ -// ""Change type of overriden property 'A.x' to '(Int) -> Int'"" ""false"" +// ""Change type of overridden property 'A.x' to '(Int) -> Int'"" ""false""",Isn't this a part of our testdata markup? @mglukhikh ,Does this need a full stop at the end of the sentence? @@ -1785,7 +1785,7 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i +@Deprecated(""Use 'Test' from kotlin.test package"", replaceWith = ReplaceWith(""Test"", imports = ""kotlin.test.Test""))",👍 ,Why do you need to replace the import here? 244,"@@ -1,4 +1,7 @@ // KJS_WITH_FULL_RUNTIME -+// TODO: muted automatically, investigate should it be ran for JVM_IR or not","Do you understand why these two tests are now failing. If so, can you replace this auto mute comment with an actual comment explaining what is going on?",Maybe `TODO(guomingz): investigate should it be run f ++// TODO: muted automatically, investigate should it be ran for JVM_IR or not","Do you understand why these two tests are now failing. If so, can you replace this auto mute comment with an actual comment explaining what is going on?",Maybe `TODO(guomingz): investigate should it be run for JVM_IR or not.` 245,"@@ -1,5 +1,3 @@ -// IGNORE_BACKEND: JVM_IR -// WITH_RUNTIME","Please return `WITH_RUNTIME` in these tests: @@ -1814,11 +1814,11 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i +// ACTION: Disable 'Make Types Implicit In Lambda' +// ACTION: Edit intention settings +// ACTION: Make types implicit in lambda","Note that this test is now failing again due to obvious reasons :) -",Perhaps add a note here that types implicit in lambda +",Perhaps add a note here that types implicit in lambda are not allowed. 250,"@@ -1,6 +1,6 @@ // !LANGUAGE: +AllowContractsForCustomFunctions +UseCallsInPlaceEffect +ReadDeserializedContracts // !USE_EXPERIMENTAL: kotlin.contracts.ExperimentalContracts --// IGNORE_BACKEND: JVM_IR, NATIVE, JS_IR",Could you also please unmute JS_IR tests? ,Was this intended to be committed with the other chan +-// IGNORE_BACKEND: JVM_IR, NATIVE, JS_IR",Could you also please unmute JS_IR tests? ,Was this intended to be committed with the other changes? 251,"@@ -1,6 +1,7 @@ // !LANGUAGE: +FunctionTypesWithBigArity","What happens to this test? Do we not want to support this kind of code any more? @@ -1828,19 +1828,19 @@ If so, the test should probably just be removed.",I do not think we should edit companion object { - private var r: Int = 1; + private var r: Int = 1 -+ // Custom getter is needed, otherwise no need to generate getTest",I think this test is no longer necessary and can be safely deleted instead,"Not sure why this is needed. Custom getter is needed," ++ // Custom getter is needed, otherwise no need to generate getTest",I think this test is no longer necessary and can be safely deleted instead,"Not sure why this is needed. Custom getter is needed, otherwise we would have a compiler warning." 253,"@@ -1,7 +1,6 @@ // Even before any IR lowerings, the type of `when` is determined to be // Unit even though the outer `if` still returns `Int?`. This results // in a ClassCastException when that Unit is converted into a Number. --// IGNORE_BACKEND: JVM_IR",The comment above is probably also not needed after this change?,Are you sure this is safe to remove? The problem seem +-// IGNORE_BACKEND: JVM_IR",The comment above is probably also not needed after this change?,Are you sure this is safe to remove? The problem seems to be that `when` returns `Number` instead of `Int?`. It seems to be that `when` returns `Int?` instead of `Int?`. 254,"@@ -1,7 +1,6 @@ fun foo() { - val a: kotlin.test.Asserter? - if () { - a = null + val a = if () {","It's not OK that you're losing the variable type here. -","The `else` clause is unnecessary here, because you al" +","The `else` clause is unnecessary here, because you already return in the `if` block." 255,"@@ -1,7 +1,7 @@ fun foo() { - Loop@ while (true) { @@ -1856,14 +1856,14 @@ If so, the test should probably just be removed.",I do not think we should edit - println(Integer.TYPE) - println(java.lang.Double.TYPE) + println(Unit::class.javaPrimitiveType)","This doesn't actually work: `Unit` has no `javaPrimitiveType`. For `void.class`, the previous variant of the code should be used. -",I believe this line can be remove +","I believe this line can be removed, since the `@JvmStatic` annotation does the same thing." 258,"@@ -1,8 +1,9 @@ -// IGNORE_BACKEND: JVM_IR // IGNORE_BACKEND: JS_IR // TODO: muted automatically, investigate should it be ran for JS or not // IGNORE_BACKEND: JS, NATIVE -+// FULL_JDK",Why FULL_JDK directive is required? Current backend works withoit it,This seems to be the only real ch ++// FULL_JDK",Why FULL_JDK directive is required? Current backend works withoit it,This seems to be the only real change in this file. 259,"@@ -10,6 +10,12 @@ class BasicAssertionsTest { } @@ -1877,7 +1877,7 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko /** + * Builds new string by populating newly created [StringBuilder] initialized with the given capacity using provided [builderAction] and then converting it to [String]. + */","Will fix formatting upon rebase. -",This method should return `String +","This method should return `StringBuilder` object, not `Unit`." 261,"@@ -10,6 +10,7 @@ import org.gradle.api.Named import org.gradle.api.file.SourceDirectorySet @@ -1887,11 +1887,11 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko * Executes the given [block] and returns elapsed time in milliseconds. */ public inline fun measureTimeMillis(block: () -> Unit): Long { -+ contract {",I believe this requires `kotlin.contracts.*` import.,Wouldn't be good to have an expli ++ contract {",I believe this requires `kotlin.contracts.*` import.,Wouldn't be good to have an explicit test for the contract type here? 263,"@@ -100,10 +101,10 @@ open class ConstraintSystemBuilderImpl(private val mode: Mode = ConstraintSystem } - for ((_, typeVariable) in typeParameters.zip(typeVariables)) {",nit: `for (typeVariable in typeVariables)`,Could you please update the docst + for ((_, typeVariable) in typeParameters.zip(typeVariables)) {",nit: `for (typeVariable in typeVariables)`,Could you please update the docstring as well? 264,"@@ -100,10 +101,10 @@ open class ConstraintSystemBuilderImpl(private val mode: Mode = ConstraintSystem } @@ -1901,7 +1901,7 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko } - for ((typeVariable, _) in allTypeParameterBounds) { -+ for (typeVariable in allTypeParameterBounds.keys) {",nit: `for (typeVariable in typeVariables)`. Probably combine with the above.,It looks like this change (and th ++ for (typeVariable in allTypeParameterBounds.keys) {",nit: `for (typeVariable in typeVariables)`. Probably combine with the above.,"It looks like this change (and the similar one below) could use `for (typeVariable, _) in enumerate(allTypeParameterBounds.items())`" 265,"@@ -101,7 +106,29 @@ class KotlinUFunctionCallExpression( } @@ -1914,34 +1914,34 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko // (step > 0 && inductionVar <= last) || (step < 0 || last <= inductionVar) val stepKotlinType = progressionType.stepType(builtIns).toKotlinType() - val zero = if (progressionType == ProgressionType.LONG_PROGRESSION) irLong(0) else irInt(0) -+ val isLong = progressionType == ProgressionType.LONG_PROGRESSION;",Semicolon may be considered a nostalgic reminiscence here.,Don't use `if` without curly brac ++ val isLong = progressionType == ProgressionType.LONG_PROGRESSION;",Semicolon may be considered a nostalgic reminiscence here.,Don't use `if` without curly braces. 267,"@@ -102,6 +101,22 @@ class JvmBuiltinOptimizationLowering(val context: JvmBackendContext) : FileLower expression.branches.removeIf() { it.condition.isFalseConst() && isCompilerGenerated } + if (expression.origin == IrStatementOrigin.ANDAND) { -+ assert(expression.type.isBoolean()","Please always add messages with additional information to asserts, including a description of what's wrong and any relevant variables (e.g. `expression.dump()` here)",Shouldn't this be `IrStatementOri ++ assert(expression.type.isBoolean()","Please always add messages with additional information to asserts, including a description of what's wrong and any relevant variables (e.g. `expression.dump()` here)",Shouldn't this be `IrStatementOrigin.ANDAND` rather than `IrStatementOrigin.CONDITION`? 268,"@@ -102,6 +102,20 @@ class JvmBuiltinOptimizationLowering(val context: JvmBackendContext) : FileLower expression.branches.removeIf() { it.condition.isFalseConst() && isCompilerGenerated } + // Replace conjunction condition with intrinsic and function call -+ if (expression.type.isBoolean() && expression.branches.size == 2) {",@neetopia Please add '// a && b == if (a) b else false' clarification comment,Nit: simplify to `if (expression. ++ if (expression.type.isBoolean() && expression.branches.size == 2) {",@neetopia Please add '// a && b == if (a) b else false' clarification comment,Nit: simplify to `if (expression.type.isBoolean() && expression.branches.size == 1) {` 269,"@@ -102,6 +103,7 @@ class ReplaceSingleLineLetIntention : SelfTargetingOffsetIndependentIntention right.text != parameterName is KtDotQualifiedExpression -> !right.hasLambdaExpression() && !right.nameUsed(parameterName) -+ is KtBinaryExpression -> right.operationToken == KtTokens.RANGE",Don't quite understand why only `RANGE` is allowed here. Please consider other binary operators. It seems to me (may be I miss something) that it should be allowed for most of them.,We should also check that the typ ++ is KtBinaryExpression -> right.operationToken == KtTokens.RANGE",Don't quite understand why only `RANGE` is allowed here. Please consider other binary operators. It seems to me (may be I miss something) that it should be allowed for most of them.,We should also check that the type of the expression is binary and doesn't match the type of the parameter. 270,"@@ -102,7 +103,7 @@ abstract class AbstractKotlinCompilation( // To configure a task that may have not yet been created at this point, use 'withType-matching-all`: .withType(AbstractKotlinCompile::class.java) - .matching { it.name == compileKotlinTaskName }","we can use `named(String, Class)` ",Would it make sense to add `.with + .matching { it.name == compileKotlinTaskName }","we can use `named(String, Class)` ",Would it make sense to add `.withType(KotlinCompile::class.getName())`? 271,"@@ -102,7 +103,7 @@ private class VarargLowering(val context: JvmBackendContext) : FileLoweringPass, context.createJvmIrBuilder(currentScope!!.scope.scopeOwnerSymbol, startOffset, endOffset) private val IrFunctionSymbol.isArrayOf: Boolean - get() = this == context.ir.symbols.arrayOf || owner.isPrimitiveArrayOf -+ get() = this == context.ir.symbols.arrayOf || owner.isArrayOf","Looks like it is already checking is this is `kotlin.Array`, so it is no need to check it by name",Why did you remove `isArrayOf` he ++ get() = this == context.ir.symbols.arrayOf || owner.isArrayOf","Looks like it is already checking is this is `kotlin.Array`, so it is no need to check it by name",Why did you remove `isArrayOf` here? 272,"@@ -102,7 +105,12 @@ class DefaultArgumentsConversion(context: NewJ2kConverterContext) : RecursiveApp return JKFieldAccessExpression(newSymbol) } @@ -2000,14 +2000,14 @@ private const val SPACES = "" "" ${SPACES + SPACES} """""" + SPACES + ).trimIndent() -```",I don't see this method being called anywhere. Am I missing somethin +```",I don't see this method being called anywhere. Am I missing something? 277,"@@ -107,6 +107,7 @@ val jvmPhases = namedIrFilePhase( jvmTypeOperatorLoweringPhase then foldConstantLoweringPhase then flattenStringConcatenationPhase then + computeStringTrimPhase then","I wanted to add this for JS but I don't know how to test it. Would appreciate a pointer so I can follow-up. -And for native I assume I wait until they update to a version of compiler that contains this phase and then I can update their list?",I don't see this method being called anywhere. Am I missing somethin +And for native I assume I wait until they update to a version of compiler that contains this phase and then I can update their list?",I don't see this method being called anywhere. Am I missing something? 278,"@@ -107,7 +108,9 @@ open class InsertImplicitCasts( override fun visitReturn(expression: IrReturn): IrExpression = @@ -2024,14 +2024,14 @@ class C() { ``` The artificial example above is considered well-formed, and contains smart cast to Unit. However. this smart cast will not be represented in IR. -Actually should cast constructor return value to `kotlin.Unit`.",why don't you move the `returnType` from `IrReturn` to `IrConstructo +Actually should cast constructor return value to `kotlin.Unit`.",why don't you move the `returnType` from `IrReturn` to `IrConstructorSymbol` instead? that way you wouldn't need to add the `IrConstructorSymbol` to the return type. 279,"@@ -108,10 +109,11 @@ fun KtExpression.hasSuspendCalls(bindingContext: BindingContext = analyze(BodyRe } } else -> { - val resolvedCall = getResolvedCall(bindingContext) - if ((resolvedCall?.resultingDescriptor as? FunctionDescriptor)?.isSuspend == true) true -+ val target = (this as? KtParenthesizedExpression)?.getStrictParentOfType() ?: this","This is a very hacky way to solve the problem. The root of problem here is the fact that the considered call expression is not a valid candidate (see check at line 88), so I believe this place should be fixed instead.",Fixes an issue when we don't have a resulting descriptor. If there i ++ val target = (this as? KtParenthesizedExpression)?.getStrictParentOfType() ?: this","This is a very hacky way to solve the problem. The root of problem here is the fact that the considered call expression is not a valid candidate (see check at line 88), so I believe this place should be fixed instead.","Fixes an issue when we don't have a resulting descriptor. If there is a resulting descriptor, the target will be the one from the original expression." 280,"@@ -108,15 +115,18 @@ internal class InterfaceLowering(val context: JvmBackendContext) : IrElementTran Visibilities.isPrivate(function.visibility)","It's true for private non-JvmDefault, private JvmDefault should be keeped in interface",I think visibility should be `public` 281,"@@ -109,11 +111,16 @@ class ErasedInlineClassBodyCodegen( @@ -2055,7 +2055,7 @@ Actually should cast constructor return value to `kotlin.Unit`.",why don't you m for (stackFrame in currentStackTrace()) { - @Suppress(""PLATFORM_CLASS_MAPPED_TO_KOTLIN"") - val className = stackFrame.className as java.lang.String -+ val className = stackFrame.className as String",isn't `className` already String?,I can't get why this change is being made. I don't think it's releva ++ val className = stackFrame.className as String",isn't `className` already String?,I can't get why this change is being made. I don't think it's relevant for this PR. 284,"@@ -110,7 +149,10 @@ fun createContainerForLazyResolveWithJava( useImpl() } @@ -2068,7 +2068,7 @@ Actually should cast constructor return value to `kotlin.Unit`.",why don't you m sealed class ReplEvalResult : Serializable { class ValueResult(val value: Any?) : ReplEvalResult() { - override fun toString(): String = ""Result: $value"" -+ val type = LastInferredTypeHolder.inferredType.get()",I think you can obtain this type from `compileResult` at `GenericReplEvaluator.eval` and pass to `ValueResult`'s constructor here,Please don't make any changes to the `ValueResult` class. The only r ++ val type = LastInferredTypeHolder.inferredType.get()",I think you can obtain this type from `compileResult` at `GenericReplEvaluator.eval` and pass to `ValueResult`'s constructor here,Please don't make any changes to the `ValueResult` class. The only reason to use `LastInferredTypeHolder` is that the `.inferredType.get()` method is going to return `null` if the `ValueResult` had not been initialized. This is because the `.inferredType` would have already been set and the `.inferredType` would have already been set. 286,"@@ -111,8 +112,13 @@ private class InheritedDefaultMethodsOnClassesLowering(val context: JvmBackendCo irFunction.body = irBlockBody { +irReturn( @@ -2119,7 +2119,7 @@ Because it looks like something similar required here #1200",I don't think this } } + -+internal fun isZero(value: Any?): Boolean {",private,This feels a bit awkward. `isZero` sounds like a function that checks if the value is z ++internal fun isZero(value: Any?): Boolean {",private,"This feels a bit awkward. `isZero` sounds like a function that checks if the value is zero, not whether it is zero or not. What about `isNaN` or `isPositive` or `isNegative`?" 294,"@@ -1168,3 +1122,96 @@ fun CompileTimeConstant<*>.isStandaloneOnlyConstant(): Boolean { else -> return false } @@ -2133,7 +2133,7 @@ Because it looks like something similar required here #1200",I don't think this + } +} + -+internal fun typeStrToCompileTimeType(str: String) = when (str) {",private,Not sure what the best way to do this is. `value is Number` -> `isNaN` -> `isPositiveNu ++internal fun typeStrToCompileTimeType(str: String) = when (str) {",private,Not sure what the best way to do this is. `value is Number` -> `isNaN` -> `isPositiveNumber` 295,"@@ -118,6 +134,18 @@ internal class ProgressionHeaderInfo( } constLimitAsLong == lastValueAsLong @@ -2261,7 +2261,7 @@ Those could be utilized, but this is not a correct way to do that.",You can use constantValueGenerator.generateConstantValueAsExpression(UNDEFINED_OFFSET, UNDEFINED_OFFSET, it) ) } -+ parent = generateParentStub(descriptor)",Seems like it overrides parent for deserialised IrField as well.,"This is going to cause all of the tests to fail, since `generateParen" ++ parent = generateParentStub(descriptor)",Seems like it overrides parent for deserialised IrField as well.,"This is going to cause all of the tests to fail, since `generateParentStub` is called before `constantValueGenerator` is initialized. I think it's better to move this to before the `constantValueGenerator` declaration." 307,"@@ -120,6 +121,14 @@ class SingleAbstractMethodLowering(val context: CommonBackendContext) : FileLowe } +irIfNull(superType, irGet(invokableVariable), irNull(), instance) @@ -2297,23 +2297,23 @@ Those could be utilized, but this is not a correct way to do that.",You can use } -+ private fun JKExpression.addThisReceiverIfNeeded(","Also, please use `applyRecursive` for creating recursive conversions as it will not unnecessary recreate tree elements ",It looks like `addThisReceiverIfNeeded` and `addThisReceiverIfNeeded` ++ private fun JKExpression.addThisReceiverIfNeeded(","Also, please use `applyRecursive` for creating recursive conversions as it will not unnecessary recreate tree elements ",It looks like `addThisReceiverIfNeeded` and `addThisReceiverIfNeeded` are only used in `visitUnaryOperator`. Can we refactor into a static method there and remove the duplication? 314,"@@ -125,9 +125,6 @@ class EnumClassConstructorTransformer(val context: CommonBackendContext, private private val loweredEnumConstructors = HashMap() fun transform(): List { -- // Make sure InstanceInitializer exists",@skuzmich Please take a look for js/lower/EnumClassLowering.kt changes,"Can you remove the `IrConstructorSymbol` from the transform, since it" +- // Make sure InstanceInitializer exists",@skuzmich Please take a look for js/lower/EnumClassLowering.kt changes,"Can you remove the `IrConstructorSymbol` from the transform, since it's now unused?" 315,"@@ -126,6 +131,12 @@ fun IrFunctionBuilder.buildConstructor(): IrConstructor { } } -+inline fun buildFunWithDescriptor(originalDescriptor: FunctionDescriptor, b: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Please rename 'b' to something meaningful: builder?,Would it be better to have this return a `Unit` instead of `FunctionD ++inline fun buildFunWithDescriptor(originalDescriptor: FunctionDescriptor, b: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Please rename 'b' to something meaningful: builder?,Would it be better to have this return a `Unit` instead of `FunctionDescriptor`? 316,"@@ -127,7 +127,9 @@ open class ClassCodegen protected constructor( val shortName = File(fileEntry.name).name visitor.visitSource(shortName, null) } - -+ if (irClass in context.suspendFunctionContinuations.values) {",This is a left-over from a previous hack. It can be removed.,"If the goal is to get rid of `suspendFunctionContinuations`, why do w" ++ if (irClass in context.suspendFunctionContinuations.values) {",This is a left-over from a previous hack. It can be removed.,"If the goal is to get rid of `suspendFunctionContinuations`, why do we need `irClass`? Couldn't we just use `context.getClass().getSimpleName()`?" 317,"@@ -12710,6 +12865,8 @@ public inline fun Array.reduceRight(operation: (T, acc: S) -> /** @@ -2321,7 +2321,7 @@ Those could be utilized, but this is not a correct way to do that.",You can use + * + * @sample samples.collections.Collections.Aggregates.reduceRightOrNull","This sample is for `reduceRightOrNull` and doesn't relate to `reduceRight`. Perhaps it was misplaced? -Note that the samples for `reduceRight` have been already submitted in the PR #2867.",Maybe it would be more clear and explicit to call this `reduceRight` +Note that the samples for `reduceRight` have been already submitted in the PR #2867.",Maybe it would be more clear and explicit to call this `reduceRight` instead of `reduceRightOrNull`. 318,"@@ -128,6 +128,8 @@ extra[""versions.jsr305""] = ""1.3.9"" extra[""versions.jansi""] = ""1.16"" extra[""versions.jline""] = ""3.3.1"" @@ -2371,14 +2371,14 @@ With tailrec optimization foo$default call from foo should be avoided",This seem .dropWhile { !it.isTransformationOrTermination(context) } .takeWhile { it.isTransformationOrTermination(context) && !it.hasReturn() } .toList() -+ .dropLastWhile { it.calleeExpression?.text == ""groupingBy"" }","Why you did not want just to remove `groupingBy` from termination list, is there some reason I do not see?",Any reason why we can't use `it.callee ++ .dropLastWhile { it.calleeExpression?.text == ""groupingBy"" }","Why you did not want just to remove `groupingBy` from termination list, is there some reason I do not see?",Any reason why we can't use `it.calleeExpression` here? 325,"@@ -1314,4 +1314,19 @@ ${"" ""} assertEquals("" ABC\n \n 123"", ""ABC\n \n123"".prependIndent("" "")) assertEquals("" "", """".prependIndent("" "")) } + + @Test fun charArrayToStringFullSlice() { -+ val chars: CharArray = charArrayOf().plus(""Kotlin"".toList())",I wonder if it was tested on strings with non-ASCII characters and surrogate pairs,Please add a case for non-ascii charac ++ val chars: CharArray = charArrayOf().plus(""Kotlin"".toList())",I wonder if it was tested on strings with non-ASCII characters and surrogate pairs,Please add a case for non-ascii characters. 326,"@@ -132,7 +132,10 @@ map.platform.class.to.kotlin=Change all usages of ''{0}'' in this file to ''{1}' map.platform.class.to.kotlin.multiple=Change all usages of ''{0}'' in this file to a Kotlin class map.platform.class.to.kotlin.advertisement=Choose an appropriate Kotlin class @@ -2397,26 +2397,26 @@ With tailrec optimization foo$default call from foo should be avoided",This seem + def getExtraSourcesMethod = variantData.getMetaClass().getMetaMethod(""getExtraGeneratedSourceFolders"") + if (getExtraSourcesMethod.returnType.metaClass == List.metaClass) { + result.addAll(variantData.getExtraGeneratedSourceFolders())","variantData.getExtraGeneratedSourceFolders() is Nullable, so you should check for null, before calling addAll -",Is this the correct logic? Why isn't t +",Is this the correct logic? Why isn't the return type always a `List`? 328,"@@ -134,10 +135,24 @@ class ExpressionCodegen( mv.areturn(returnType) } } - writeLocalVariablesInTable(info) + val endLabel = writeLocalVariablesInTable(info) -+ createLocalVariablesForParameters(startLabel, endLabel)",It would be cleaner to create new label here,What happens if startLabel is null? Sh ++ createLocalVariablesForParameters(startLabel, endLabel)",It would be cleaner to create new label here,What happens if startLabel is null? Shouldn't we check for startLabel == null? 329,"@@ -134,7 +134,8 @@ open class KotlinCocoapodsPlugin: Plugin { if (requestedTargetName == KOTLIN_TARGET_FOR_DEVICE) { // We create a fat framework only for device platforms: iosArm64 and iosArm32. - val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32) -+ val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32,",The comment above also needs to be updated.,This doesn't seem right. The list of p ++ val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32,",The comment above also needs to be updated.,This doesn't seem right. The list of platforms is the same as the one in `KonanTarget`The `IOS_TARGET_FOR_DEVICE` list. The only target we care about is `KonanTarget.IOS_ARM64` 330,"@@ -135,4 +136,23 @@ public static KotlinSingleIntentionActionFactory createFactory() { } }; } + -+ public static KotlinSingleIntentionActionFactory createLateInitFactory() {","Please convert this file to Kotlin if you're adding substantial new code to it. (Rename to .kt, commit, then rename back to .java, run J2K, make any changes needed for the project to compile, commit again, then make your changes.)",Would it make sense to rename this fac ++ public static KotlinSingleIntentionActionFactory createLateInitFactory() {","Please convert this file to Kotlin if you're adding substantial new code to it. (Rename to .kt, commit, then rename back to .java, run J2K, make any changes needed for the project to compile, commit again, then make your changes.)",Would it make sense to rename this factory to createLateInitActionFactory? 331,"@@ -1354,6 +1370,72 @@ public IElementType parseProperty(PropertyParsingMode mode) { return multiDeclaration ? DESTRUCTURING_DECLARATION : PROPERTY; } @@ -2427,7 +2427,7 @@ With tailrec optimization foo$default call from foo should be avoided",This seem public String[] scriptResolverEnvironment; + // Javac options -+ @Argument(value = ""-Xuse-javac"", description = ""Use Javac analysis"")","Consider something like `""Use javac for analysis of Java source and class files""`",I don't think we should add `-Xuse-jav ++ @Argument(value = ""-Xuse-javac"", description = ""Use Javac analysis"")","Consider something like `""Use javac for analysis of Java source and class files""`",I don't think we should add `-Xuse-javac` to the public API of javac. I don't think we should add this to the public API. 333,"@@ -137,8 +137,8 @@ public inline fun String.Companion.format(format: String, vararg args: Any?): St public inline fun String.format(locale: Locale, vararg args : Any?) : String = java.lang.String.format(locale, this, *args) @@ -2441,7 +2441,7 @@ Could you improve these two overloads' docs as well?",Why did you remove the def override fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?): LoopReplacement { with(builder) { - var (newLoop, replacementExpression) = if (headerInfo.canOverflow) { -+ return if (headerInfo.canOverflow) {","Nit, but could you use an expression body here?",Why remove `newLoop` from the function ++ return if (headerInfo.canOverflow) {","Nit, but could you use an expression body here?",Why remove `newLoop` from the function signature? 335,"@@ -139,29 +140,34 @@ public fun File.readLines(charset: Charset = Charsets.UTF_8): List { return result } @@ -2471,7 +2471,7 @@ Please add such test anyway. ",The result is ignored if `expression.isInSingleLi -+ { doc { f -> ""Returns `true` if [element] is found in the ${f.collection}."" } typeParam(""@kotlin.internal.OnlyInputTypes T"") @@ -2504,7 +2504,7 @@ Please add such test anyway. ",The result is ignored if `expression.isInSingleLi + if (irFunction.origin == JvmLoweredDeclarationOrigin.CLASS_STATIC_INITIALIZER) { + return + } -+ if (irFunction is IrConstructor && irFunction.isPrimary) {","This method called after function body generation (irFunction.body!!.accept(this, info)). Should we use markLineNumber(startOffset = FALSE)?","Nit: can you move this to a separate method, so that it can be s" ++ if (irFunction is IrConstructor && irFunction.isPrimary) {","This method called after function body generation (irFunction.body!!.accept(this, info)). Should we use markLineNumber(startOffset = FALSE)?","Nit: can you move this to a separate method, so that it can be shared between `visitStart` and `visitEnd`?" 345,"@@ -141,7 +139,7 @@ private class ToArrayLowering(private val context: JvmBackendContext) : ClassLow toArrayName, Visibilities.PUBLIC, @@ -2528,21 +2528,21 @@ Please add such test anyway. ",The result is ignored if `expression.isInSingleLi val deserializedModuleFragments = sortedImmediateDependencies.map { val moduleFile = File(it.klibPath, moduleHeaderFileName) - deserializer.deserializeIrModuleHeader(depsDescriptors.getModuleDescriptor(it), moduleFile.readBytes(), File(it.klibPath), DeserializationStrategy.ONLY_REFERENCED) -+ deserializer.DeserializeIrModuleHeader(depsDescriptors.getModuleDescriptor(it))",deserializer.~D~deserializeIrModuleHeader,Move the `deserializer` call into `deserializeIrModuleHeader` me ++ deserializer.DeserializeIrModuleHeader(depsDescriptors.getModuleDescriptor(it))",deserializer.~D~deserializeIrModuleHeader,Move the `deserializer` call into `deserializeIrModuleHeader` method. 348,"@@ -142,7 +176,7 @@ public inline fun > Map.mapKeysTo(destinatio } /** - * Puts all the entries into this [[MutableMap]] with the first value in the pair being the key and the second the value + * Puts all the entries into this [MutableMap] with the first value in the pair being the key and the second the value","""Puts given values"", and again value vs component for Pair. -",Not sure this is correct. It should be like `with the first valu +",Not sure this is correct. It should be like `with the first value in the pair being the key and the second value in the map` 349,"@@ -143,6 +143,9 @@ public IntrinsicMethods(JvmTarget jvmTarget, boolean shouldThrowNpeOnExplicitEqu declareIntrinsicFunction(FQ_NAMES.string, ""plus"", 1, new Concat()); declareIntrinsicFunction(FQ_NAMES.string, ""get"", 1, new StringGetChar()); + intrinsicsMap.registerIntrinsic(TEXT_PACKAGE_FQ_NAME, FQ_NAMES.string, ""trimMargin"", 1, new TrimMargin());","We've discussed internally the idea to apply the intrinsic unconditionally and figured that it might lead to compatibility problems in case we decide to change the behavior of `trimMargin`/`trimIndent` on some corner case in a future release. It seems that these kinds of problems will only arise if the compiler of version X is compiling code against standard library of version Y where Y > X: the user expects behavior of version Y at runtime, but will actually observe behavior of version X because that's what was executed in the compiler at compilation time. -Therefore we'd like to restrict these intrinsics to be applicable only if the version of the standard library in dependencies is _not greater_ than the version of the compiler itself. This can be achieved by checking if the API version (can be obtained via `languageVersionSettings.apiVersion` at the call site) is <= than the version of the standard library in the runtime of the compiler (can be obtained via `KotlinVersion.CURRENT`). (Sorry if this sounds confusing, I can do it myself if it's unclear, just wanted to notify you that we'd like it to work that way -- maybe you have some other related concerns.)",Why did you add `intrinsicsMap` as an intrinsic? AFAIK `declareI +Therefore we'd like to restrict these intrinsics to be applicable only if the version of the standard library in dependencies is _not greater_ than the version of the compiler itself. This can be achieved by checking if the API version (can be obtained via `languageVersionSettings.apiVersion` at the call site) is <= than the version of the standard library in the runtime of the compiler (can be obtained via `KotlinVersion.CURRENT`). (Sorry if this sounds confusing, I can do it myself if it's unclear, just wanted to notify you that we'd like it to work that way -- maybe you have some other related concerns.)","Why did you add `intrinsicsMap` as an intrinsic? AFAIK `declareIntrinsicFunction` does not add an intrinsic to `intrinsicsMap`, and `intrinsicsMap` is immutable." 350,"@@ -145,10 +145,22 @@ private static JetElement overrideFunction(Project project, JetFile file, Simple StringBuilder bodyBuilder = new StringBuilder(""override fun ""); bodyBuilder.append(descriptor.getName()); @@ -2564,13 +2564,13 @@ Therefore we'd like to restrict these intrinsics to be applicable only if the ve } } -+ private fun IrStatementsBuilder<*>.generateIrIfNull(","I'm unsure if this has consequences for non-jvm backends? On the JVM, introducing a temporary for constants makes little sense as the constant can just be reloaded. Is that a reasonable assumption for the other backends as well?",doesn't look like this method is c ++ private fun IrStatementsBuilder<*>.generateIrIfNull(","I'm unsure if this has consequences for non-jvm backends? On the JVM, introducing a temporary for constants makes little sense as the constant can just be reloaded. Is that a reasonable assumption for the other backends as well?",doesn't look like this method is called anywhere. Can we remove? 354,"@@ -148,4 +149,19 @@ class ReplaceCallWithBinaryOperatorInspection : AbstractApplicabilityBasedInspec else -> OperatorConventions.BINARY_OPERATION_NAMES.inverse()[identifier] } } + -+ private fun KtDotQualifiedExpression.isFloatingPointNumberEquals(): Boolean {","This implementation is very strange. You focus here on a given example, but there are some simpler cases which probably will not be detected by this implementation correctly. I'd suggest you should find (via `resolveToCall()`) type of `equals` receiver and type of `equals` argument, and in case at least one of them is FP type (`Float` or `Double`) and another of them is numeric type (FP or `Byte`, `Short`, `Int`, `Long`), the function should return `true`, otherwise `false`.",This does not look like it is inte ++ private fun KtDotQualifiedExpression.isFloatingPointNumberEquals(): Boolean {","This implementation is very strange. You focus here on a given example, but there are some simpler cases which probably will not be detected by this implementation correctly. I'd suggest you should find (via `resolveToCall()`) type of `equals` receiver and type of `equals` argument, and in case at least one of them is FP type (`Float` or `Double`) and another of them is numeric type (FP or `Byte`, `Short`, `Int`, `Long`), the function should return `true`, otherwise `false`.",This does not look like it is intended to be a public API. It can be a breaking change. 355,"@@ -148,6 +148,10 @@ You can now run the various Run/Debug Configurations such as * All Compiler Tests * All IDEA Plugin Tests @@ -2596,37 +2596,37 @@ If you don't`,` every test",I'm guessing this is a leftover? + +If you need to debug a specific generated test ensure that you have the `Working directory` your IntellJ run configuration set +to the root directory of this project. If you don't every test you try to run will fail with a `No such file or directory` exception.","Basically change the test run config from `$MODULE_DIR$` to `$PROJECT_DIR$`, right? -Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit default run config` to it?",I don't think this is specific to +Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit default run config` to it?",I don't think this is specific to IntellJ. I think it would be better to just say `If you need to run a specific generated test ensure that you have the `Working directory of this project` in the root directory of your project. 357,"@@ -148,7 +148,7 @@ abstract class AbstractKotlinCompile() : AbstractKo protected val additionalClasspath = arrayListOf() @get:Internal // classpath already participates in the checks -- protected val compileClasspath: Iterable","We'd like to keep the visibility of this `val` restricted, maybe use `internal` if it is not needed outside the module.",Please annotate with `@VisibleForT +- protected val compileClasspath: Iterable","We'd like to keep the visibility of this `val` restricted, maybe use `internal` if it is not needed outside the module.",Please annotate with `@VisibleForTesting`. 358,"@@ -149,6 +249,10 @@ open class KotlinUField( override val psi = javaPsi + override fun acceptsAnnotationTarget(target: AnnotationUseSiteTarget?): Boolean = + target == AnnotationUseSiteTarget.FIELD || -+ (sourcePsi is KtProperty) && (target == null || target == AnnotationUseSiteTarget.PROPERTY)",The `@delegate:` target also maps to fields.,I think you can simplify this to ` ++ (sourcePsi is KtProperty) && (target == null || target == AnnotationUseSiteTarget.PROPERTY)",The `@delegate:` target also maps to fields.,I think you can simplify this to `Boolean = target == AnnotationUseSiteTarget.FIELD || (target == AnnotationUseSiteTarget.PROPERTY || target == AnnotationUseSiteTarget.FIELD)` 359,"@@ -149,7 +150,7 @@ class MemoizedInlineClassReplacements { } private fun buildReplacement(function: IrFunction, body: IrFunctionImpl.() -> Unit) = - buildFun { -+ buildFunWithDescriptor(function.descriptor) {",Could you clarify why descriptor is reuqired here?,The `function.descriptor` is not a ++ buildFunWithDescriptor(function.descriptor) {",Could you clarify why descriptor is reuqired here?,The `function.descriptor` is not available in this case. The only case where `function.descriptor` is `undefined` is the scenario where `function.descriptor` is `undefined`. 360,"@@ -15,3 +17,20 @@ public static void runTest(doGenerateParamAssertions a) { throw new AssertionError(""Fail: IllegalArgumentException expected""); } } + +// FILE: B.kt -+import test.doGenerateParamAssertions as C","Would be nice to rename the class itself now that it's possible. Previously the Java class was named this way because its file was compiled directly by javac, but box tests copy everything to tmpdir with new names, so we can use the short name for convenience","Unrelated to this PR, but this tes" ++import test.doGenerateParamAssertions as C","Would be nice to rename the class itself now that it's possible. Previously the Java class was named this way because its file was compiled directly by javac, but box tests copy everything to tmpdir with new names, so we can use the short name for convenience","Unrelated to this PR, but this test class is going to be deprecated and should be removed." 361,"@@ -15,6 +15,7 @@ public abstract class Mine : java.util.AbstractList { public open /*fake_override*/ fun clear(): kotlin.Unit public open /*fake_override*/ fun contains(/*0*/ T!): kotlin.Boolean public open /*fake_override*/ fun containsAll(/*0*/ kotlin.collections.Collection): kotlin.Boolean -+ public open /*fake_override*/ fun forEach(/*0*/ java.util.function.Consumer!): kotlin.Unit","You'd better not to change existing tests, but to create a new test group with the same test data and your flag enabled.",You might be able to use `java.uti ++ public open /*fake_override*/ fun forEach(/*0*/ java.util.function.Consumer!): kotlin.Unit","You'd better not to change existing tests, but to create a new test group with the same test data and your flag enabled.",You might be able to use `java.util.function.Consumer`. 362,"@@ -151,15 +156,19 @@ abstract class AbstractWriteSignatureTest : CodegenTestCase() { } @@ -2639,7 +2639,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau /** - * Puts all the entries into this [[MutableMap]] with the first value in the pair being the key and the second the value + * Puts all the entries into this [MutableMap] with the first value in the pair being the key and the second the value","""Puts elements of the given collection"" -",Not sure this is correct. It shoul +",Not sure this is correct. It should be like `with the first value in the pair being the key and the second value in the map` 364,"@@ -152,23 +147,14 @@ class IrExpressionLambdaImpl( } @@ -2649,14 +2649,14 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau } - override val lambdaClassType: Type = Type.getObjectType(""test${counter++}"") -+ override val lambdaClassType: Type = Type.getObjectType(""$typePrefix\$lambda-${counter++}"")",Maybe best to use precalculated old name ```codegen.context.getLocalClassInfo(reference)```,the indent is wrong here (should b ++ override val lambdaClassType: Type = Type.getObjectType(""$typePrefix\$lambda-${counter++}"")",Maybe best to use precalculated old name ```codegen.context.getLocalClassInfo(reference)```,the indent is wrong here (should be spaces instead of tabs) 365,"@@ -1520,6 +1520,15 @@ language=""kotlin"" /> + @@ -2667,13 +2667,13 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + enabledByDefault=""true"" + cleanupTool=""true"" + level=""WARNING""","This should be a WEAK_WARNING at most. There's nothing wrong with the original code - it's simply not as concise as it could be. -",Do we want this to be an error? Or +",Do we want this to be an error? Or should it be a warning? 367,"@@ -153,7 +153,7 @@ private class EnumClassLowering(val context: JvmBackendContext) : ClassLoweringP } }) - body = enumConstructor.body // will be transformed later -+ body = enumConstructor.body?.patchDeclarationParents(this)",Is this really needed? If so I think there is another issue with lost parent,"Same as above, `this` shouldn't be" ++ body = enumConstructor.body?.patchDeclarationParents(this)",Is this really needed? If so I think there is another issue with lost parent,"Same as above, `this` shouldn't be needed." 368,"@@ -153,7 +154,13 @@ class IrSourceCompilerForInline( get() = callElement.descriptor as FunctionDescriptor @@ -2714,7 +2714,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau - KOTLIN_NATIVE_CURRENT_ABI_VERSION, - metadataReader = CachingIdeMetadataReaderImpl - ) -+ private val nativeLibrary = createKotlinLibrary(root)",But here `CachingIdeKonanLibraryMetadataLoader` must be present in order to cache contents of any KLIB loaded in IDE.,Why do we need to pass `cachingIdeMetadataReaderImpl` here? Can't we just use `c ++ private val nativeLibrary = createKotlinLibrary(root)",But here `CachingIdeKonanLibraryMetadataLoader` must be present in order to cache contents of any KLIB loaded in IDE.,Why do we need to pass `cachingIdeMetadataReaderImpl` here? Can't we just use `createKonanLibrary` directly? 374,"@@ -158,6 +146,12 @@ internal class CallableReferenceLowering(val context: JvmBackendContext) : FileL }) } @@ -2752,7 +2752,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + * This class is intended to be used in 'for' loops, and the JVM backend suggests efficient + * bytecode generation for it. Progressions with a step of -1 can be created through the + * `downTo` method on classes representing primitive types.","Link to `downTo` method. -",The enum value is `-1` and the enum value is `-1`. I think it would be clearer t +",The enum value is `-1` and the enum value is `-1`. I think it would be clearer to say something like `The step of -1 can be used in ...`. 379,"@@ -16,10 +16,25 @@ package kotlin @@ -2784,7 +2784,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau +/** + * Returns true if the receiver and the [other] object are ""equal"" to each other, or if they are","Link to Any.equals() on ""equal"" would be good, to refer to explanation of equality. -","If the [other] object are the same object instance, then it is considered ""equal" +","If the [other] object are the same object instance, then it is considered ""equal"" to each other." 382,"@@ -16,18 +16,25 @@ package kotlin.script.templates.standard @@ -2826,7 +2826,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau +/** + * Annotates the parameter of a function annotates as [inline] and forbids inlining of","Typo: ""annotates as [inline]"" -> ""annotated as [inline]"" -","Perhaps should be ""annotates the parameter of a fun" +","Perhaps should be ""annotates the parameter of a function as [inline] and forbids inlining of the parameter.""" 387,"@@ -16,8 +16,18 @@ package kotlin @@ -2853,7 +2853,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau writeOutput(state.configuration, state.factory, null) } + -+ if (chunk.size == 1 && projectConfiguration.getBoolean(JVMConfigurationKeys.USE_JAVAC)) {","If the chunk contains multiple modules and `-Xuse-javac` has been specified, maybe we should report a warning saying that we won't in fact use javac for compilation",I think we can remove this check. It's sufficient t ++ if (chunk.size == 1 && projectConfiguration.getBoolean(JVMConfigurationKeys.USE_JAVAC)) {","If the chunk contains multiple modules and `-Xuse-javac` has been specified, maybe we should report a warning saying that we won't in fact use javac for compilation","I think we can remove this check. It's sufficient to check for ""chunk.size == 1"" when we have ""USE_JAVAC""" 390,"@@ -161,6 +162,14 @@ object KotlinToJVMBytecodeCompiler { ProgressIndicatorAndCompilationCanceledStatus.checkCanceled() writeOutput(state.configuration, state.factory, null) @@ -2882,12 +2882,12 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau val maybeSingleParagraph = markdownNode.children.singleOrNull { it.type != MarkdownTokenTypes.EOL } return if (maybeSingleParagraph != null && !allowSingleParagraph) { - maybeSingleParagraph.children.joinToString("""") { it.toHtml() } -+ val lineSeparator = LineSeparator.getSystemLineSeparator().separatorString",Source code in IntelliJ always uses `\n` as a line separator. No need to access the system line separator here.,Please remove the `LineSeparator.getSystemLineSepar ++ val lineSeparator = LineSeparator.getSystemLineSeparator().separatorString",Source code in IntelliJ always uses `\n` as a line separator. No need to access the system line separator here.,Please remove the `LineSeparator.getSystemLineSeparator()` method. 394,"@@ -164,6 +165,7 @@ class CodeToInlineBuilder( } if (expression.getReceiverExpression() == null) { -+ val targetCallable = (targetCallable as? FunctionImportedFromObject)?.callableFromObject ?: targetCallable",Should we use `ImportedFromObjectCallableDescriptor<*>` instead?,"If `targetCallable` is null, then `null` should be " ++ val targetCallable = (targetCallable as? FunctionImportedFromObject)?.callableFromObject ?: targetCallable",Should we use `ImportedFromObjectCallableDescriptor<*>` instead?,"If `targetCallable` is null, then `null` should be returned instead of throwing an exception." 395,"@@ -164,7 +164,8 @@ internal class CallableReferenceLowering(val context: JvmBackendContext) : FileL val irFunctionReference: IrFunctionReference ) { @@ -2900,7 +2900,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau - +@PublishedApi -+@SinceKotlin(""1.3"")","Should this be `""1.4""`, or some other value?",Why did you remove the `since Kotlin` annotation he ++@SinceKotlin(""1.3"")","Should this be `""1.4""`, or some other value?",Why did you remove the `since Kotlin` annotation here? 397,"@@ -165,6 +165,9 @@ fun IrDeclarationWithVisibility.getVisibilityAccessFlag(kind: OwnerKind? = null) ?: visibilityToAccessFlag[visibility] ?: throw IllegalStateException(""$visibility is not a valid visibility in backend for ${ir2string(this)}"") @@ -2912,19 +2912,19 @@ when (visibility) { JavaVisibilities.PACKAGE_VISIBILITY -> AsmUtil.NO_FLAG_PACKAGE_PRIVATE else -> error(""Default argument stub should be either public or package private: ${ir2string(this)}"") } -```",is there a reason for not using `visibilityToAccess +```",is there a reason for not using `visibilityToAccessFlag` directly? 398,"@@ -167,7 +166,7 @@ class ExpressionCodegen( irFunction.markLineNumber(startOffset = irFunction is IrConstructor && irFunction.isPrimary) } val returnType = typeMapper.mapReturnType(irFunction) - result.coerce(returnType).materialize() -+ result.coerce(returnType, if (irFunction !is IrConstructor) irFunction.returnType else null).materialize()",I'm wondering if the `null` here could be the unit type instead? ,I don't think this is the right fix. The code below ++ result.coerce(returnType, if (irFunction !is IrConstructor) irFunction.returnType else null).materialize()",I'm wondering if the `null` here could be the unit type instead? ,I don't think this is the right fix. The code below calls `typeMapper.mapReturnType(irFunction)` when IrConstructor is not IrConstructor. `irFunction.returnType` will never be null. 399,"@@ -168,14 +170,14 @@ abstract class AbstractKotlinCompile() : AbstractKo get() = (classpath + additionalClasspath) .filterTo(LinkedHashSet(), File::exists) - private val sourceFilesExtensionsSources: MutableList> = mutableListOf() -+ private val sourceFilesExtensionsSources = project.objects.listProperty(String::class.java)","`listProperty` is introduced in Gradle `4.3`, but we support all versions starting from `4.1`",Can't we use `sourceFilesExtensionsSources` directl ++ private val sourceFilesExtensionsSources = project.objects.listProperty(String::class.java)","`listProperty` is introduced in Gradle `4.3`, but we support all versions starting from `4.1`",Can't we use `sourceFilesExtensionsSources` directly? 400,"@@ -169,6 +169,28 @@ class Maps { // map.containsValue(""string"") // cannot call extension when the argument type and the map value type are unrelated at all @@ -2981,7 +2981,7 @@ See comments below. - * Nothing has no instances + * Nothing has no instances. You can use Nothing as the return type of a function to indicate + * that it never returns (always throws an exception).","There are more use-cases for Nothing, e.g. Iterable is ok to exist, but it will never enter ""for"" loop, etc. Needs link to lang docs. -","It's not clear what ""always returns"" means." +","It's not clear what ""always returns"" means. Can you tell me what that means?" 405,"@@ -17,8 +16,16 @@ fun test() { JFoo.foo2({}, runnable()) } @@ -2992,7 +2992,7 @@ See comments below. // @TestKt.class // 2 NEW ``` -could be extracted in common part",Remove this line since it is unnecessary no +could be extracted in common part",Remove this line since it is unnecessary now. 406,"@@ -17,8 +21,6 @@ fun test(u1: UInt, u2: UInt, us: Array) { // 2 INVOKESTATIC UInt\.box // 0 INVOKEVIRTUAL UInt.unbox @@ -3012,13 +3012,13 @@ could be extracted in common part",Remove this line since it is unnecessary no } + + private val Visibility.admitsFakeOverride: Boolean -+ get() = this != Visibilities.PRIVATE && this != Visibilities.PRIVATE_TO_THIS && this != Visibilities.INVISIBLE_FAKE",Looks like this can be simplified to `!Visibilities.isPrivate(this)` and inlined because invisible fakes have been filtered out in the first line of `generateFakeOverrideProperty`?,You don't need the `get()` call here. If `v ++ get() = this != Visibilities.PRIVATE && this != Visibilities.PRIVATE_TO_THIS && this != Visibilities.INVISIBLE_FAKE",Looks like this can be simplified to `!Visibilities.isPrivate(this)` and inlined because invisible fakes have been filtered out in the first line of `generateFakeOverrideProperty`?,"You don't need the `get()` call here. If `visible` is true, `admitsFakeOverride` will already be true." 408,"@@ -172,6 +202,33 @@ abstract class KaptTask : ConventionTask(), TaskWithLocalState { } } + private fun findClasspathChanges(changedClasspath: Iterable): KaptClasspathChanges { -+ incAptCache!!.mkdirs()","Minor, style: I find creating a local val with the same or using `lateinit` modifier for property more readable than using `!!` operator multiple times.",Is there a reason we need to create the cac ++ incAptCache!!.mkdirs()","Minor, style: I find creating a local val with the same or using `lateinit` modifier for property more readable than using `!!` operator multiple times.",Is there a reason we need to create the cache here? Wouldn't this make it harder to just create it when necessary? 409,"@@ -1730,14 +1730,14 @@ public fun Stream.lastOrNull(): T? { } @@ -3033,14 +3033,14 @@ could be extracted in common part",Remove this line since it is unnecessary no /** - * Returns last element matching the given *predicate*, or null if element was not found + * Returns the last element matching the given [predicate], or *null* if no such element was found.","_null_ -> `null` -",This worries me a bit. It's not the last el +","This worries me a bit. It's not the last element, but the last character. It's the last element of the collection that was found. It's the last element of the collection." 410,"@@ -175,6 +175,23 @@ class KotlinQuickDocumentationProvider : AbstractDocumentationProvider() { // element is not an KtReferenceExpression, but KtClass of enum return renderEnum(element, originalElement, quickNavigation) } + else if (element is KtEnumEntry && !quickNavigation) { + val desc = element.resolveToDescriptorIfAny() -+ val ordinal =",It's better to calculate ordinal with `val ordinal = element.containingClassOrObject?.run { getChildrenOfType().indexOf(element) }`,When will the element be a KtEnumEntry? If ++ val ordinal =",It's better to calculate ordinal with `val ordinal = element.containingClassOrObject?.run { getChildrenOfType().indexOf(element) }`,"When will the element be a KtEnumEntry? If it's not a KtEnumEntry, should it be an error?" 411,"@@ -175,7 +195,9 @@ class ExpressionCodegen( override fun visitBlockBody(body: IrBlockBody, data: BlockInfo): StackValue { @@ -3051,14 +3051,14 @@ could be extracted in common part",Remove this line since it is unnecessary no exp.accept(this, data).also { (exp as? IrExpression)?.markEndOfStatementIfNeeded() } -```","Unrelated to this PR, but this doesn't seem" +```","Unrelated to this PR, but this doesn't seem like the best place to markEndOfStatementIfNeeded. Should the markEndOfStatementIfNeeded be in some other place, or should the markEndOfStatementIfNeeded be in BlockInfoResolver?" 412,"@@ -175,8 +188,8 @@ internal class ConstraintSystemImpl( override fun toBuilder(filterConstraintPosition: (ConstraintPosition) -> Boolean): ConstraintSystem.Builder { val result = ConstraintSystemBuilderImpl() - for ((typeParameter, typeBounds) in allTypeParameterBounds) { - result.allTypeParameterBounds.put(typeParameter, typeBounds.filter(filterConstraintPosition)) -+ for (typeParameter in allTypeParameterBounds.entries) {","Since `entries` returns a destructurable class, I suppose you can keep the `for ((typeParameter, typeBounds) ...` syntax here",This was the only place where we used the ` ++ for (typeParameter in allTypeParameterBounds.entries) {","Since `entries` returns a destructurable class, I suppose you can keep the `for ((typeParameter, typeBounds) ...` syntax here",This was the only place where we used the ``ConstraintSystem.Builder.withTypeParameterBounds()`` method. It's not clear why we need to change anything here. 413,"@@ -176,6 +174,18 @@ class FoldConstantLowering(private val context: JvmBackendContext) : IrElementTr else -> expression } @@ -3073,7 +3073,7 @@ expression.arguments.takeWhile {it is IrConst<*> && it.value is String } and . vmLoop@ while (true) { - val eventSet = virtualMachine.eventQueue().remove(1000) -+ val eventSet = virtualMachine.eventQueue().remove(1000) ?: continue",Seems it's equals to remove without timeout,This is the fix for the bug described in th ++ val eventSet = virtualMachine.eventQueue().remove(1000) ?: continue",Seems it's equals to remove without timeout,This is the fix for the bug described in the PR description. 415,"@@ -1793,6 +1793,15 @@ language=""kotlin"" /> @@ -3083,7 +3083,7 @@ expression.arguments.takeWhile {it is IrConst<*> && it.value is String } and . + groupPath=""Kotlin"" + groupName=""Style issues"" + enabledByDefault=""true"" -+ level=""INFO""","`INFO` is deprecated, use `WEAK WARNING` instead.",I'm not sure if we should include the name ++ level=""INFO""","`INFO` is deprecated, use `WEAK WARNING` instead.","I'm not sure if we should include the name of the class as part of the JIRA ticket, but if we do, I think we should include it as part of the JIRA ticket as well." 416,"@@ -1794,7 +1794,7 @@ QUnit.diff = (function() { }; }()); @@ -3101,7 +3101,7 @@ expression.arguments.takeWhile {it is IrConst<*> && it.value is String } and . - -+",Please avoid commiting changes in all configuration files without a reason,Why did you move $PROJECT_DIR into the Vcs directo ++",Please avoid commiting changes in all configuration files without a reason,Why did you move $PROJECT_DIR into the Vcs directory mapping component? 463,"@@ -20,6 +20,7 @@ private val DIAGNOSTIC_FACTORY_TO_RENDERER by lazy { ErrorsNative.INCOMPATIBLE_THROWS_INHERITED, ""Member inherits different @Throws filters from {0}"", Renderers.commaSeparated(Renderers.NAME) ) + put(ErrorsNative.INCOMPATIBLE_SHARED_IMMUTABLE, ""@SharedImmutable isn't applicable to mutable variable"")","> variable -property (or field) actually.",`@SharedImmutable` isn't applicable to mutable var +property (or field) actually.",`@SharedImmutable` isn't applicable to mutable variables. 464,"@@ -20,6 +20,8 @@ object ErrorsNative { val INCOMPATIBLE_THROWS_OVERRIDE = DiagnosticFactory1.create(Severity.ERROR) @JvmField @@ -3485,7 +3485,7 @@ declarations",Why is this applied only to `@SharedImmutable` entities? val typeReference = property.typeReference ?: return val initializer = property.initializer ?: return - -+ if ((typeReference.typeElement as? KtUserType)?.referenceExpression?.mainReference?.resolve() is KtTypeAlias) return","I'm almost sure this can be done easier, without additional reference resolve. Probably `type` should be `WrappedType` for type alias (not sure exactly).",`typeReference.typeElement as KtUserType` will always be n ++ if ((typeReference.typeElement as? KtUserType)?.referenceExpression?.mainReference?.resolve() is KtTypeAlias) return","I'm almost sure this can be done easier, without additional reference resolve. Probably `type` should be `WrappedType` for type alias (not sure exactly).",`typeReference.typeElement as KtUserType` will always be non-null. 467,"@@ -200,4 +202,14 @@ public static PsiElement createPrimaryConstructor(Project project) { JetClass aClass = createClass(project, ""class A()""); return aClass.findElementAt(7).getParent(); @@ -3517,18 +3517,18 @@ declarations",Why is this applied only to `@SharedImmutable` entities? /** - * Appends all elements matching the given *predicate* into the given *destination* + * Appends all entries matching the given [predicate] into the given [destination] mutable map.","""[destination] mutable map."", looks confusing a bit. -",Why not just `Appends all entries matching the given predi +",Why not just `Appends all entries matching the given predicate into the given destination`? 472,"@@ -207,8 +207,6 @@ open class FunctionCodegen( } else { frameMap.enterTemp(parameter.asmType) } -- } else if (parameter.kind != JvmMethodParameterKind.VALUE) {",Please add assertion that there are only VALUE or RECEIVER kinds,I don't understand why this change is being made. It seems +- } else if (parameter.kind != JvmMethodParameterKind.VALUE) {",Please add assertion that there are only VALUE or RECEIVER kinds,I don't understand why this change is being made. It seems like the rest of the changes are just removing redundant code. 473,"@@ -209,7 +209,7 @@ class ForConverter( return if (reversed) DownToExpression(startConverted, convertBound(bound, if (inclusiveComparison) 0 else +1)) else - RangeExpression(startConverted, convertBound(bound, if (inclusiveComparison) 0 else -1)) -+ UntilExpression(startConverted, convertBound(bound, if (inclusiveComparison) +1 else 0))",I don't think this is correct. `until` should be used only in the case where the original code used `-1`. Cases where `0..9` was generated should remain unchanged.,Since `RangeExpression` and `UntilExpression` are identica ++ UntilExpression(startConverted, convertBound(bound, if (inclusiveComparison) +1 else 0))",I don't think this is correct. `until` should be used only in the case where the original code used `-1`. Cases where `0..9` was generated should remain unchanged.,"Since `RangeExpression` and `UntilExpression` are identical, I'm wondering if it's worth having two methods. - `UntilExpression(bound, if (inclusiveComparison) ..)` - `UntilExpression(bound, if (inclusiveComparison) ..)` - `UntilExpression(bound, if (inclusiveComparison) ..)` - `UntilExpression(bound, if (inclusiveComparison) ..)` - `UntilExpression(bound, if (inclusiveComparison) ..)`" 474,"@@ -21,15 +21,18 @@ private object EmptyMap : Map { /** Returns an empty read-only map of specified type */ public fun emptyMap(): Map = EmptyMap as Map @@ -3538,7 +3538,7 @@ declarations",Why is this applied only to `@SharedImmutable` entities? + * Returns a new read-only map with the specified contents, given as a list of pairs + * where the first value is the key and the second is the value. + */","specify, that it behaves as sequential `put` calls, so that if conflicting keys exist in source sequence, last one wins. -",`with the specified contents` -> `with the specified keys +",`with the specified contents` -> `with the specified keys and values` 475,"@@ -21,29 +35,29 @@ public enum class FileWalkDirection { /** Depth-first search, directory is visited BEFORE its files */ TOP_DOWN, @@ -3550,7 +3550,7 @@ declarations",Why is this applied only to `@SharedImmutable` entities? -+ ","Try to avoid light classes, some explanation below","Hmmm, do we really need this? I'm not very familiar with t" ++ ","Try to avoid light classes, some explanation below","Hmmm, do we really need this? I'm not very familiar with this part of the code" 477,"@@ -21,6 +21,24 @@ package kotlin.script.dependencies import java.io.File import kotlin.script.dependencies.DependenciesResolver.ResolveResult @@ -3562,7 +3562,7 @@ declarations",Why is this applied only to `@SharedImmutable` entities? +// +// Some concerns on naming: +// Environment -> ScriptEnvironment (top level class with too common a name) -+// ResolveResult -> ResolutionResult",+1 for ResolutionResult,"The package name is `kotlin.script.environment`, but the p" ++// ResolveResult -> ResolutionResult",+1 for ResolutionResult,"The package name is `kotlin.script.environment`, but the package name is `kotlin.script`. I would suggest to rename the package to `kotlin.script`." 478,"@@ -21,6 +21,8 @@ package kotlin * represented as values of the primitive type `boolean`. */ @@ -3573,7 +3573,7 @@ declarations",Why is this applied only to `@SharedImmutable` entities? fun infer(x: T, y: Y): Outer.Inner = null!! -val infered = infer("""", 1) -+val inferred = infer("""", 1)","Here and below: this change affects descriptors-dump (corresponding `.txt`-files). I think, the easiest way to check that nothing else is affected is to launch all tests. ",Let's use `val inferred = ...` for consistency with the re ++val inferred = infer("""", 1)","Here and below: this change affects descriptors-dump (corresponding `.txt`-files). I think, the easiest way to check that nothing else is affected is to launch all tests. ",Let's use `val inferred = ...` for consistency with the rest of the file. 480,"@@ -21,7 +21,7 @@ class ProcessorLoader( ) : Closeable { private var annotationProcessingClassLoader: URLClassLoader? = null @@ -3591,12 +3591,12 @@ You probably want something more isolated that only have `tools.jar` classes",wh public final FqNameUnsafe intRange = rangesFqName(""IntRange""); public final FqNameUnsafe longRange = rangesFqName(""LongRange""); -+ public final FqNameUnsafe stringsKt = textFqName(""StringsKt"");","This is a JVM-specific name, so I prefer not to declare it here, but rather store it as a constant at the use site",Should this be `StringsKt` rather than `Strings ++ public final FqNameUnsafe stringsKt = textFqName(""StringsKt"");","This is a JVM-specific name, so I prefer not to declare it here, but rather store it as a constant at the use site",Should this be `StringsKt` rather than `StringsKt` ? 483,"@@ -211,11 +211,11 @@ fun IrBuilderWithScope.irEquals(arg1: IrExpression, arg2: IrExpression, origin: arg1, arg2 ) -+fun IrBuilderWithScope.irNot(arg: IrExpression, origin: IrStatementOrigin? = null) =",There's already an irNot in LowerUtils.kt.,Is this used? Can we just use `IrBuilderWithSco ++fun IrBuilderWithScope.irNot(arg: IrExpression, origin: IrStatementOrigin? = null) =",There's already an irNot in LowerUtils.kt.,Is this used? Can we just use `IrBuilderWithScope` directly? 484,"@@ -213,7 +213,7 @@ private static String renderTypeConstructorVerboseDebugInformation(TypeConstruct } assert order != null; @@ -3608,7 +3608,7 @@ You probably want something more isolated that only have `tools.jar` classes",wh - private static void removeTypeAnnotation(@NotNull JetNamedDeclaration property, @Nullable JetTypeReference typeReference) { + private static void removeTypeAnnotation(@NotNull PsiElement removeAfter, @Nullable JetTypeReference typeReference) {","I don't love name 'removeAfter' but I didn't come up with anything better. -",We need to keep the old method signature as wel +","We need to keep the old method signature as well, and deprecate the new one." 486,"@@ -215,5 +215,19 @@ class IrBuiltIns( companion object { @@ -3626,13 +3626,13 @@ class IrBuiltIns { } } ``` -cc @udalov ",This should be `lessThan` instead of `lessOrEqu +cc @udalov ",This should be `lessThan` instead of `lessOrEqual`. 487,"@@ -2165,6 +2183,9 @@ public fun Iterable.sum(): Int { */ @kotlin.jvm.JvmName(""sumOfLong"") public fun Iterable.sum(): Long { + if (this is LongProgression) { -+ return (this.first + this.last) * this.count() / 2",Is this as overflow safe as the old approach? `(this.first + this.last) * this.count()` might overflow when `(this.first + this.last) * (this.count() / 2)` wouldn't.,Could we use `this.sum` here instead of the har ++ return (this.first + this.last) * this.count() / 2",Is this as overflow safe as the old approach? `(this.first + this.last) * this.count()` might overflow when `(this.first + this.last) * (this.count() / 2)` wouldn't.,Could we use `this.sum` here instead of the hard-coded `2`? 488,"@@ -217,14 +255,16 @@ public inline fun > Map.filterTo(destination: C } @@ -3657,7 +3657,7 @@ cc @udalov ",This should be `lessThan` instead of `lessOrEqu } } ``` -","Unrelated to this PR, but I'm curious: what's t" +","Unrelated to this PR, but I'm curious: what's the difference between `javaFiles` and `VirtualFile.javaFiles`?" 490,"@@ -217,6 +217,7 @@ public inline fun C.ifEmpty(defaultValue: () -> R): R where C : Collectio @kotlin.internal.InlineOnly public inline fun <@kotlin.internal.OnlyInputTypes T> Collection.containsAll(elements: Collection): Boolean = this.containsAll(elements) @@ -3668,13 +3668,13 @@ cc @udalov ",This should be `lessThan` instead of `lessOrEqu /> + @@ -3724,19 +3724,19 @@ In such cases, always refer to existing UI elements, so the new features will be klass.addFunction(""invokeSuspend"", irBuiltIns.anyNType, Modality.ABSTRACT).apply { - addValueParameter(""result"", resultClassStub.typeWith(irBuiltIns.anyNType)) + addValueParameter(""\$result"", resultClassStub.typeWith(irBuiltIns.anyNType))","I somehow missed it. Thanks! -Minor: there is `SUSPEND_CALL_RESULT_NAME`, which, in fact, I should've used.",Shouldn't this be something like `...TypeWith(irBuilt +Minor: there is `SUSPEND_CALL_RESULT_NAME`, which, in fact, I should've used.",Shouldn't this be something like `...TypeWith(irBuiltIns.anyType).result`? 501,"@@ -221,9 +221,18 @@ class AnnotationChecker( }.toSet() } - fun getDeclarationSiteActualTargetList(annotated: KtElement, descriptor: ClassDescriptor?, trace: BindingTrace): -+ fun getDeclarationSiteActualTargetList(","I don't like an idea to change front-end code in this commit: as result we get an additional parameter used only from IDE code. I think it's better to call `getActualTargetList` from fix (make it public for this), and then take necessary targets from this list (effectively inlining your function here). As alternative idea, you can write separate function in IDE for a case with `all = true`.",What's the difference between this and `getDeclaratio ++ fun getDeclarationSiteActualTargetList(","I don't like an idea to change front-end code in this commit: as result we get an additional parameter used only from IDE code. I think it's better to call `getActualTargetList` from fix (make it public for this), and then take necessary targets from this list (effectively inlining your function here). As alternative idea, you can write separate function in IDE for a case with `all = true`.",What's the difference between this and `getDeclarationSiteActualTargetList`? Can we just inline it? 502,"@@ -222,7 +225,7 @@ internal val interfaceDefaultCallsPhase = makeIrFilePhase( ) private class InterfaceDefaultCallsLowering(val context: JvmBackendContext) : IrElementTransformerVoid(), FileLoweringPass { - -+ // TODO If there are no default _implementations_ we can avoid generating defaultImpls class entirely by moving default arg dispatchers to the interface class",NB: if any annotation on property is present it's stored on synthetic methods in DI (and we can support interface as storage class),This is not correct. We don't generate defaultImplsCl ++ // TODO If there are no default _implementations_ we can avoid generating defaultImpls class entirely by moving default arg dispatchers to the interface class",NB: if any annotation on property is present it's stored on synthetic methods in DI (and we can support interface as storage class),This is not correct. We don't generate defaultImplsClass when there is no default implementation. The idea is that we don't want to generate defaultImplsClass when there is no default implementation. We can fix this by moving default arg dispatchers to the interface. 503,"@@ -223,6 +223,10 @@ private val jvmFilePhases = typeAliasAnnotationMethodsPhase then stripTypeAliasDeclarationsPhase then @@ -3774,20 +3774,20 @@ fun foo(b: B) {} fun bar(c: C) = foo(c as A) // <-- incorrect warning here, the cast is not useless ``` -",The constant could be renamed to `USELESS_CAST_IS_FIN +",The constant could be renamed to `USELESS_CAST_IS_FINE`. 507,"@@ -2278,6 +2278,14 @@ language=""kotlin"" /> + ` rather than the implementation class (i.e. `org.jetbrains.inspector.redundantProgressionInspection`). 508,"@@ -228,6 +228,8 @@ fun createSpacingBuilder(settings: CodeStyleSettings, builderUtil: KotlinSpacing betweenInside(REFERENCE_EXPRESSION, LAMBDA_ARGUMENT, CALL_EXPRESSION).spaces(1) betweenInside(TYPE_ARGUMENT_LIST, LAMBDA_ARGUMENT, CALL_EXPRESSION).spaces(1) + around(IS_KEYWORD).spaces(1)","This is really minor, but still: to make the code easier to follow, I'd move this line next to the rule for the AS keyword. -","I'm not sure how I feel about this being better, but " +","I'm not sure how I feel about this being better, but I think I'd rather have an `IS_KEYWORD` around the `betweenInside` calls rather than have an `IS_KEYWORD` around each call." 509,"@@ -229,7 +229,7 @@ class Kapt3ComponentRegistrar : ComponentRegistrar { try { Class.forName(JAVAC_CONTEXT_CLASS) @@ -3819,7 +3819,7 @@ What about caching fqnames for descriptors? Will it waste memory? + + override fun afterEach(name: String, afterFn: () -> Any?) { + afterFn() -+ }",This way `beforeEach` hook gets executed once for the suite.,does this override the `beforeEach` and `afterEach` o ++ }",This way `beforeEach` hook gets executed once for the suite.,does this override the `beforeEach` and `afterEach` of `TestFn` as well? 512,"@@ -230,6 +231,7 @@ project(':compiler:ir.psi2ir').projectDir = ""$rootDir/compiler/ir/ir.psi2ir"" as project(':compiler:ir.ir2cfg').projectDir = ""$rootDir/compiler/ir/ir.ir2cfg"" as File project(':compiler:ir.backend.common').projectDir = ""$rootDir/compiler/ir/backend.common"" as File @@ -3866,7 +3866,7 @@ What about caching fqnames for descriptors? Will it waste memory? /> + @@ -3881,7 +3881,7 @@ What about caching fqnames for descriptors? Will it waste memory? /** - * Returns a map containing all key-value pairs matching the given *predicate* + * Returns a map containing all key-value pairs not matching the given [predicate].","Returns a _new_ map -","I think we should be consistent about how we refer to ""predicate"" vs ""value pai" +","I think we should be consistent about how we refer to ""predicate"" vs ""value pairs""." 520,"@@ -239,6 +240,14 @@ open class ClassCodegen protected constructor( } } @@ -3926,7 +3926,7 @@ Then we are looking for a line that starts with ""// "" with a number of digits val usedAsExpression = expression.isUsedAsExpression(expression.analyze()) -- holder.registerProblem(expression,",I think we shouldn't change this file.,I'm not sure if we should remove this. I'm not sure if there is a use-case for +- holder.registerProblem(expression,",I think we shouldn't change this file.,I'm not sure if we should remove this. I'm not sure if there is a use-case for it. 524,"@@ -24,6 +24,16 @@ import com.sun.tools.javac.util.List as JavacList import org.jetbrains.kotlin.kapt3.base.plus @@ -3953,7 +3953,7 @@ val someIterable = Iterable { buildIterator { ... } } // now you can call extensions on the Iterable val result: List = someIterable.mapIndexed { index, value -> ""$index => $value"" } assertPrints(result, ""..."") -```",Hi! Why did you choose to replace `yieldAll` with `buildIterator`? There's no n +```","Hi! Why did you choose to replace `yieldAll` with `buildIterator`? There's no need to change this, just curious." 526,"@@ -24,7 +24,7 @@ fileAnnotations ; @@ -3961,7 +3961,7 @@ assertPrints(result, ""..."") - : ""@"" ""file"" "":"" (""["" annotationEntry+ ""]"" | annotationEntry) + : ""@"" ""file"" "":"" (""["" annotation+ ""]"" | annotation)","I think it should be `unescapedAnnotation` instead of `annotation`. Probably we should also remove `("":"" ""file"")?` part from `annotationPrefix`. -",Prefer single-quoted strings when you don't need string interpolation or specia +",Prefer single-quoted strings when you don't need string interpolation or special symbols. 527,"@@ -24,7 +26,7 @@ open class NodeJsSetupTask : DefaultTask() { init { @Suppress(""LeakingThis"") @@ -4045,12 +4045,12 @@ Maybe we need to wrap `env.nodeExecutable` in constructor call of File?",Why is + override fun getParameterList(): PsiAnnotationParameterList = KtLightAnnotationParameterList(super.getParameterList()) + + inner class KtLightAnnotationParameterList(private val list: PsiAnnotationParameterList) : KtLightElementBase(this), -+ PsiAnnotationParameterList {",Formatting: `{` should be on the next line.,This needs to be implemented as `PsiAnnotationParameterList = KtLightAnnotationParameterList(. ++ PsiAnnotationParameterList {",Formatting: `{` should be on the next line.,This needs to be implemented as `PsiAnnotationParameterList = KtLightAnnotationParameterList(...)` 539,"@@ -256,6 +255,16 @@ public KotlinType getBodyExpressionType( DataFlowInfo beforeJumpInfo = newContext.dataFlowInfo; boolean jumpOutPossible = false; for (Iterator iterator = block.iterator(); iterator.hasNext(); ) { -+ // Use filtering trace to keep effect system cache only for one statement","For me this place is not very clear, check it with @erokhins ",This feels a little bit like a hack. Can't we just filter out the affected elements instead of ++ // Use filtering trace to keep effect system cache only for one statement","For me this place is not very clear, check it with @erokhins ",This feels a little bit like a hack. Can't we just filter out the affected elements instead of doing it in every iteration of the loop? 540,"@@ -258,29 +140,45 @@ class KotlinUastLanguagePlugin : UastLanguagePlugin { else -> false } @@ -4081,7 +4081,7 @@ Maybe we need to wrap `env.nodeExecutable` in constructor call of File?",Why is + groupPath=""Kotlin"" + groupName=""Probable bugs"" + enabledByDefault=""true"" -+ cleanupTool=""true""","Please do not set `cleanupTool = true` if you are not absolutely sure that your inspection should be included into ""Code Cleanup"" action.",I don't think we should have a separate group for probable bugs. Kotlin doesn't have any knowl ++ cleanupTool=""true""","Please do not set `cleanupTool = true` if you are not absolutely sure that your inspection should be included into ""Code Cleanup"" action.","I don't think we should have a separate group for probable bugs. Kotlin doesn't have any knowledge about it, so we can just make it enabled by default and then remove the group for probable bugs." 544,"@@ -2633,6 +2633,16 @@ language=""kotlin"" /> @@ -4097,7 +4097,7 @@ Maybe we need to wrap `env.nodeExecutable` in constructor call of File?",Why is */ @kotlin.internal.InlineOnly -public inline fun String(chars: CharArray, offset: Int, length: Int): String = -+public actual inline fun String(chars: CharArray, offset: Int, length: Int): String =",It's worth to provide common `String(chars: CharArray)` too.,I don't know if this is correct. It looks like the `Enumerable` type is `internal.InlineO ++public actual inline fun String(chars: CharArray, offset: Int, length: Int): String =",It's worth to provide common `String(chars: CharArray)` too.,"I don't know if this is correct. It looks like the `Enumerable` type is `internal.InlineOnly`, not `internal.InlineOnly`." 546,"@@ -269,7 +271,10 @@ fun KtDeclaration.implicitVisibility(): KtModifierKeywordToken? = } } @@ -4121,7 +4121,7 @@ It seems that this change makes Jasmine and Jest use wrong API, even for non-asy One way to solve this would be using different adapters for Mocha and Jasmine/Jest. Not sure there is a reliable way to detect which one it is though. -P.S. `beforeEach('', function() {});` results in `Error: beforeEach expects a function argument; received [object String]` in Jasmine","Is there a reason why you couldn't just do `kotlin.test.adapters.beforeEach(name, beforeF" +P.S. `beforeEach('', function() {});` results in `Error: beforeEach expects a function argument; received [object String]` in Jasmine","Is there a reason why you couldn't just do `kotlin.test.adapters.beforeEach(name, beforeFn)`?" 548,"@@ -27,6 +27,27 @@ class SmartIdentityTable { val size: Int get() = keysArray?.size ?: largeMap!!.size @@ -4139,13 +4139,13 @@ I suggest either providing a view even if the table is small (more preferably, f typeProducer: () -> PsiType, val ktInitializer: KtExpression?, - val psiParent: PsiElement?, -+ psiParentProvider: () -> PsiElement?,",Minor nit: we're using 'producer' and 'provider' for two parameters that work in exactly the same way. Better be consistent.,"Please rename this to `psiParentProvider`, this is confusing with the term ""parent provid" ++ psiParentProvider: () -> PsiElement?,",Minor nit: we're using 'producer' and 'provider' for two parameters that work in exactly the same way. Better be consistent.,"Please rename this to `psiParentProvider`, this is confusing with the term ""parent provider"" below." 550,"@@ -27,7 +27,8 @@ class ClosureGenerationStrategy( ) : FunctionGenerationStrategy.FunctionDefault(state, declaration) { override fun doGenerateBody(codegen: ExpressionCodegen, signature: JvmMethodSignature) { - initializeVariablesForDestructuredLambdaParameters(codegen, codegen.context.functionDescriptor.valueParameters) -+ initializeVariablesForDestructuredLambdaParameters(","Minor. Can you, please, remove default argument from `initializeVariablesForDestructuredLambdaParameters`, since this was the only place, which used it?","Unrelated to this PR, but it seems like we should just pass the `valueParameters` to `han" ++ initializeVariablesForDestructuredLambdaParameters(","Minor. Can you, please, remove default argument from `initializeVariablesForDestructuredLambdaParameters`, since this was the only place, which used it?","Unrelated to this PR, but it seems like we should just pass the `valueParameters` to `handleFunctionBody` instead of exposing the implementation of `initializeVariablesForDestructuredLambdaParameters`." 551,"@@ -27,7 +28,7 @@ class ScratchRunLineMarkerContributor : RunLineMarkerContributor() { val ktFile = element.containingFile as? KtFile if (ktFile?.isScript() != true) return null @@ -4171,7 +4171,7 @@ There's no reason to require it to be inline in the expectation.",I'd recommend return false } -+ if (!ReferencesSearch.search(declaration, useScope).forEach(::checkReference)) return true","It's Ok but I don't like that we now invoke references search twice, it can affect performance. I'd consider here to do this only for particular case with `JvmName`. Will fix myself.","I would skip the loop if `ReferencesSearch.search(declaration, useScope).anyMatch(::check" ++ if (!ReferencesSearch.search(declaration, useScope).forEach(::checkReference)) return true","It's Ok but I don't like that we now invoke references search twice, it can affect performance. I'd consider here to do this only for particular case with `JvmName`. Will fix myself.","I would skip the loop if `ReferencesSearch.search(declaration, useScope).anyMatch(::checkReference)`" 554,"@@ -271,10 +269,7 @@ fun KtDeclaration.implicitVisibility(): KtModifierKeywordToken? = } } @@ -4198,7 +4198,7 @@ There's no reason to require it to be inline in the expectation.",I'd recommend + groupPath=""Kotlin"" + groupName=""Style issues"" + enabledByDefault=""true"" -+ level=""WEAK WARNING""","Not quite sure but I'd prefer `INFORMATION` level here. I think these two kinds of syntax are more or less equivalent. May be `to` looks a bit nicer, but not in every case.",It might be worth adding a warning similar to the one in `src/main/java/src/main/java/org ++ level=""WEAK WARNING""","Not quite sure but I'd prefer `INFORMATION` level here. I think these two kinds of syntax are more or less equivalent. May be `to` looks a bit nicer, but not in every case.",It might be worth adding a warning similar to the one in `src/main/java/src/main/java/org/jetbrains/kotlin/idea/convertPairConstructorToToFunctionInspection.java`. 557,"@@ -2713,6 +2713,15 @@ language=""kotlin"" /> @@ -4206,7 +4206,7 @@ There's no reason to require it to be inline in the expectation.",I'd recommend + @@ -4216,12 +4216,12 @@ There's no reason to require it to be inline in the expectation.",I'd recommend + groupPath=""Kotlin"" + groupName=""Style issues"" + enabledByDefault=""true"" -+ level=""WEAK WARNING""",I'd upgrade it to WARNING,I'm not sure if this is the group name or the group name of the inspection. @jetbrains do ++ level=""WEAK WARNING""",I'd upgrade it to WARNING,I'm not sure if this is the group name or the group name of the inspection. @jetbrains do you have any opinion on this? 559,"@@ -274,6 +272,7 @@ private val jvmFilePhases = singleAbstractMethodPhase then assertionPhase then returnableBlocksPhase then -+ sharedVariablesPhase then",Why this move is required?,I am not sure if this is the right place for the shared variables phase. They are not rel ++ sharedVariablesPhase then",Why this move is required?,"I am not sure if this is the right place for the shared variables phase. They are not related to the request from the client, but they are related to the response from the client. I assume they are needed for the request from the client." 560,"@@ -278,7 +279,12 @@ open class ConvertLambdaToReferenceIntention(text: String) : } } @@ -4234,7 +4234,7 @@ There's no reason to require it to be inline in the expectation.",I'd recommend withLineBreak() return ""FAIL 0"" } catch (e: RuntimeException) { -- if (e.stackTrace[0].lineNumber != 19) return ""FAIL 1 ${e.stackTrace[0].lineNumber}""",I would prefer to avoid unecessery test data changes: so it's best to add new line after // FULL_JDK,@choumx do you know why this had the +- if (e.stackTrace[0].lineNumber != 19) return ""FAIL 1 ${e.stackTrace[0].lineNumber}""",I would prefer to avoid unecessery test data changes: so it's best to add new line after // FULL_JDK,@choumx do you know why this had the line break in the first place? 562,"@@ -28,41 +28,47 @@ import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode class UnnecessaryJavaUsageInspection : AbstractKotlinInspection(), CleanupLocalInspectionTool { @@ -4243,30 +4243,30 @@ There's no reason to require it to be inline in the expectation.",I'd recommend - ""java.io.PrintStream.print"" to ""print($0)"", - ""java.util.Collections.sort"" to ""$0.sort()"" + ""java.io.PrintStream.println"" to Pair(""println($0)"", false),","This boolean value is quite unclear. Much better to use an enum instead. -","This change seems unrelated, can you" +","This change seems unrelated, can you explain?" 563,"@@ -28,6 +26,7 @@ class Dummy fun enableAssertions(): CheckerJvmAssertInlineFunctionAssertionsEnabled { val loader = Dummy::class.java.classLoader loader.setClassAssertionStatus(""CheckerJvmAssertInlineFunctionAssertionsEnabled"", true) -+ loader.setClassAssertionStatus(""InlineKt"", false)",Looks like redundant.,Please remove all the changes in thi ++ loader.setClassAssertionStatus(""InlineKt"", false)",Looks like redundant.,Please remove all the changes in this file. 564,"@@ -28,6 +30,7 @@ import org.jetbrains.kotlin.ir.util.transformDeclarationsFlat import org.jetbrains.kotlin.ir.visitors.IrElementTransformerVoid import org.jetbrains.kotlin.ir.visitors.transformChildrenVoid -+// This pass has to run after LocalDeclarationsLowering, since we don't handle nested functions.","Now, instead of documenting the dependency in a comment, you could specify it in the `prerequisite` field of the phase.","Nit: ""this pass has to run after Loc" ++// This pass has to run after LocalDeclarationsLowering, since we don't handle nested functions.","Now, instead of documenting the dependency in a comment, you could specify it in the `prerequisite` field of the phase.","Nit: ""this pass has to run after LocalDeclarationsLowering, since we don't handle nested functions""" 565,"@@ -28,60 +28,60 @@ public inline fun TODO(reason: String): Nothing = throw NotImplementedError(""An * Calls the specified function [block] and returns its result. */ @kotlin.internal.InlineOnly -public inline fun run(block: () -> R): R = block() -+public inline fun run(@kotlin.internal.CalledInPlace(kotlin.internal.InvocationCount.EXACTLY_ONCE) block: () -> R): R = block()",Can we annotate also both `TODO` functions?,It seems that the invocation count i ++public inline fun run(@kotlin.internal.CalledInPlace(kotlin.internal.InvocationCount.EXACTLY_ONCE) block: () -> R): R = block()",Can we annotate also both `TODO` functions?,It seems that the invocation count is not restricted to `EXACTLY_ONCE`. 566,"@@ -28,7 +28,7 @@ private fun Name.shouldBeEscaped(): Boolean { if (isSpecial) return false val string = asString() - return string in KeywordStringsGenerated.KEYWORDS || string.any { !Character.isLetterOrDigit(it) && it != '_' } + return string in KeywordStringsGenerated.KEYWORDS || string.any { !Character.isLetterOrDigit(it) }","Look at https://teamcity.jetbrains.com/viewLog.html?buildId=2319666 -This is incorrect fix.",The `asString` method already checks +This is incorrect fix.","The `asString` method already checks for underscores, so this is redundant." 567,"@@ -28,8 +28,10 @@ private fun assertFailsWithImpl(exceptionClass: Class, messag block() } catch (e: Throwable) { @@ -4284,7 +4284,7 @@ This is incorrect fix.",The `asString` method already checks + origin: IrDeclarationOrigin, + descriptor: PropertyDescriptor, + type: IrType, -+ visibility: Visibility,","Similarly, would be nice to have a default value in `declareField` instead. Also consider specifying the actual argument value for clarity (when it's for example obviously private or public) in as many call sites as possible",Consider adding this new line to 80 ++ visibility: Visibility,","Similarly, would be nice to have a default value in `declareField` instead. Also consider specifying the actual argument value for clarity (when it's for example obviously private or public) in as many call sites as possible",Consider adding this new line to 80 chars to break up the long line. 569,"@@ -285,8 +285,7 @@ internal class HeaderProcessor( progressionType.elementCastFunctionName ), @@ -4296,13 +4296,13 @@ This is incorrect fix.",The `asString` method already checks // TODO: use this implementation instead of any other fun IrSimpleFunction.resolveFakeOverride(): IrSimpleFunction? { - return collectRealOverrides().singleOrNull { it.modality != Modality.ABSTRACT } -+ return collectRealOverrides().firstOrNull { it.modality != Modality.ABSTRACT }",I think there is an assumption that we could have only one real implementation for a fake override and it would be nice to keep it. I'd suggest adding another implementation with Deprecated annotation instead of modifying this one.,wouldn't this make more sense as `it ++ return collectRealOverrides().firstOrNull { it.modality != Modality.ABSTRACT }",I think there is an assumption that we could have only one real implementation for a fake override and it would be nice to keep it. I'd suggest adding another implementation with Deprecated annotation instead of modifying this one.,wouldn't this make more sense as `it.modality == Modality.ABSTRACT`? 571,"@@ -2863,6 +2863,15 @@ language=""kotlin"" /> + `? + @@ -4405,7 +4405,7 @@ Not critical, of course.",shouldn't this be `Array`? /> + @@ -4469,7 +4469,7 @@ Not critical, of course.",shouldn't this be `Array`? import org.jetbrains.kotlin.gradle.dsl.KotlinJvmCompile +tasks.withType { -+ maxParallelForks = (Runtime.getRuntime().availableProcessors() / 2).takeIf { it > 0 } ?: 1",@sfs Please use Math.max,"You can use `Math.max(Runtime.availableProcessors(), 1" ++ maxParallelForks = (Runtime.getRuntime().availableProcessors() / 2).takeIf { it > 0 } ?: 1",@sfs Please use Math.max,"You can use `Math.max(Runtime.availableProcessors(), 1)` instead." 594,"@@ -3,7 +3,7 @@ class My { @@ -4481,14 +4481,14 @@ Not critical, of course.",shouldn't this be `Array`? - -+ ",Why? :(,Why do we want to disable proguard run on kotlin-compi ++ ",Why? :(,Why do we want to disable proguard run on kotlin-compiler.jar? 596,"@@ -3,7 +3,7 @@ package kotlin import java.util.Enumeration /** - * Helper to make java.util.Enumeration usable in for + * Helper to make [Enumeration] usable in `for` loops.","""Creates `Iterator` for an `Enumeration`"" -",I don't think we need the `in` part given that it's no +",I don't think we need the `in` part given that it's not used in the `for` loop. 597,"@@ -3,7 +3,9 @@ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. */ @@ -4569,13 +4569,13 @@ For current kotlin plugin converts them into override fun isApplicable(file: PsiFile): Boolean { - val languageVersionSettings = file.project.getLanguageVersionSettings() -+ val languageVersionSettings = file.module?.languageVersionSettings ?: file.project.getLanguageVersionSettings()","I think here you should write just `file.languageVersionSettings` without these hacks. If you look inside, you see it handles your case, as well as some other cases.",Please annotate with `@pragma warning disable` as well ++ val languageVersionSettings = file.module?.languageVersionSettings ?: file.project.getLanguageVersionSettings()","I think here you should write just `file.languageVersionSettings` without these hacks. If you look inside, you see it handles your case, as well as some other cases.",Please annotate with `@pragma warning disable` as well. 602,"@@ -302,7 +302,8 @@ public static boolean isAbstractMethod(FunctionDescriptor functionDescriptor, Ow } public static boolean isStaticMethod(OwnerKind kind, CallableMemberDescriptor functionDescriptor) { - return isStaticKind(kind) || -+ return isStaticDeclaration(functionDescriptor) ||","Purpose of this change: Without this, the generated functions are not invoked with the correct descriptor; the enclosing class is included in the descriptor as the dispatch receiver parameter. This causes a stack underflow error during verification.",is isStaticDeclaration(kind) the right thing to do her ++ return isStaticDeclaration(functionDescriptor) ||","Purpose of this change: Without this, the generated functions are not invoked with the correct descriptor; the enclosing class is included in the descriptor as the dispatch receiver parameter. This causes a stack underflow error during verification.",is isStaticDeclaration(kind) the right thing to do here? 603,"@@ -303,6 +302,12 @@ class Maps { assertPrints(props.getProperty(""z"", ""fail""), ""fail"") } @@ -4590,7 +4590,7 @@ For current kotlin plugin converts them into /> + @@ -4598,7 +4598,7 @@ For current kotlin plugin converts them into + (taskName) { it.onlyIf { kotlinTask.get().kotlinOptions.sourceMap }","I am wondering here when the `onlyIf` statement is gonna be evaluated as calling `get()` will realize the task. -TBH, it would be nice to have someone from Gradle having a look at it.",Method `registerTask` has 5 arguments (exceeds 4 allow +TBH, it would be nice to have someone from Gradle having a look at it.",Method `registerTask` has 5 arguments (exceeds 4 allowed). Consider refactoring. 607,"@@ -3078,6 +3078,15 @@ language=""kotlin"" /> @@ -4616,7 +4616,7 @@ TBH, it would be nice to have someone from Gradle having a look at it.",Method ` + groupPath=""Kotlin"" + groupName=""Style issues"" + enabledByDefault=""true"" -+ level=""INFO""",I confess I'm not sure about default level it should have. Will discuss it with my colleagues.,The convention is to use a class name like `Idea.Boole ++ level=""INFO""",I confess I'm not sure about default level it should have. Will discuss it with my colleagues.,The convention is to use a class name like `Idea.BooleanLiteralArgumentInspection`. 608,"@@ -31,6 +31,7 @@ class RedundantGotoMethodTransformer : MethodTransformer() { * (1) subsequent labels * ... @@ -4626,25 +4626,25 @@ TBH, it would be nice to have someone from Gradle having a look at it.",Method ` import org.jetbrains.kotlin.ir.visitors.IrElementTransformerVoid import org.jetbrains.kotlin.ir.visitors.transformChildrenVoid import org.jetbrains.kotlin.load.java.JvmAbi.JVM_FIELD_ANNOTATION_FQ_NAME -+import org.jetbrains.kotlin.synthetic.isVisibleOutside",Redundant import,Please move this import to the `o ++import org.jetbrains.kotlin.synthetic.isVisibleOutside",Redundant import,Please move this import to the `org.jetbrains.kotlin.synthetic` package 610,"@@ -31,7 +31,7 @@ import java.io.InputStream class CliVirtualFileFinder( private val index: JvmDependenciesIndex, private val scope: GlobalSearchScope -) : VirtualFileFinder() { -+) : JavacVirtualFileFinder() {",Check attentively that nothing is changed without `use-javac` option,"Unrelated to this PR, but I'm cur" ++) : JavacVirtualFileFinder() {",Check attentively that nothing is changed without `use-javac` option,"Unrelated to this PR, but I'm curious why we decided to make this a class, instead of just having a constructor. Can you elaborate?" 611,"@@ -31,7 +32,7 @@ import org.jetbrains.org.objectweb.asm.Type object IteratorNext : IntrinsicMethod() { override fun toCallable(expression: IrFunctionAccessExpression, signature: JvmMethodSignature, context: JvmBackendContext): IrIntrinsicFunction { - val type = AsmUtil.unboxType(signature.returnType) -+ val type = if (expression.type.isPrimitiveType()) signature.returnType else AsmUtil.unboxType(signature.returnType)","@juan-chen The aim of this change is not clear, could you explain a bit?",It would be good to have tests fo ++ val type = if (expression.type.isPrimitiveType()) signature.returnType else AsmUtil.unboxType(signature.returnType)","@juan-chen The aim of this change is not clear, could you explain a bit?",It would be good to have tests for this as well. 612,"@@ -31,7 +32,8 @@ import org.jetbrains.org.objectweb.asm.Type object IteratorNext : IntrinsicMethod() { override fun toCallable(expression: IrFunctionAccessExpression, signature: JvmMethodSignature, context: JvmBackendContext): IrIntrinsicFunction { - val type = AsmUtil.unboxType(signature.returnType) -+ // If the array element type is unboxed primitive, do not unbox. Otherwise AsmUtil.unbox throws exception",This change breaks 16 IR black box codegen tests.,If the array element type is unbo ++ // If the array element type is unboxed primitive, do not unbox. Otherwise AsmUtil.unbox throws exception",This change breaks 16 IR black box codegen tests.,"If the array element type is unboxed, does it mean that the array element type is unboxed as well? If so, does this mean that the array element type is unboxed as well?" 613,"@@ -3107,6 +3108,18 @@ private StackValue generateEquals( ); } @@ -4659,7 +4659,7 @@ enum class Test { A, B } fun eq(x: Test?, y: Test) = x == y fun box() = if (!eq(null, A)) ""OK"" else ""Fail"" -```",You can simplify this to `opToken +```",You can simplify this to `opToken == KtTokens.EQEQ || opToken == KtTokens.EXCEQ`. 614,"@@ -311,6 +311,28 @@ class Collections { assertPrints(moreFrequencies, ""{o=1, t=4, f=2, s=2, e=2, n=1}"") } @@ -4677,44 +4677,44 @@ fun box() = if (!eq(null, A)) ""OK"" else ""Fail"" + val numbers = listOf(1, 2, 3) + assertPrints(numbers.joinTo(sb, prefix = ""["", postfix = ""]"").toString(), ""An existing string and a list:[1, 2, 3]"") + -+ val lotOfNumbers = (1..100).asIterable()",`IntRange` is already an `Iterable`. What does this conversion intend to show?,This test needs to be in a separa ++ val lotOfNumbers = (1..100).asIterable()",`IntRange` is already an `Iterable`. What does this conversion intend to show?,This test needs to be in a separate test file. 616,"@@ -314,6 +314,7 @@ object Mapping : TemplateGroupBase() { specialFor(ArraysOfUnsigned) { inlineOnly() } doc { ""Appends all elements yielded from results of [transform] function being invoked on each ${f.element} of original ${f.collection}, to the given [destination]."" } + sample(""samples.collections.Collections.Transformations.flatMap"")","The sample is for `flatMap` function, but it's placed into **`flatMapTo`**'s function template. -But no worries: I'll fix it before merging.",Looks like this sample needs to b +But no worries: I'll fix it before merging.",Looks like this sample needs to be put in the samples folder. 617,"@@ -319,9 +319,6 @@ FILE fqName: fileName:/enum.kt public final fun (): kotlin.Int declared in .TestEnum4 $this: VALUE_PARAMETER name: type:kotlin.Enum<.TestEnum4> PROPERTY FAKE_OVERRIDE name:x visibility:public modality:FINAL [val] -- FIELD FAKE_OVERRIDE name:x type:kotlin.Int visibility:public [final]",Is it expected that this fake field override was removed?,Why did you remove the visibility +- FIELD FAKE_OVERRIDE name:x type:kotlin.Int visibility:public [final]",Is it expected that this fake field override was removed?,Why did you remove the visibility attribute here? 618,"@@ -32,15 +32,49 @@ public annotation class noinline */ public annotation class inline(public val strategy: InlineStrategy = InlineStrategy.AS_FUNCTION) +/** + * Specifies the strategy for JVM bytecode generation for an inline function.","It's not JVM specific. -",specify the strategy for JVM byte +",specify the strategy for JVM bytecode generation for an inline function 619,"@@ -32,4 +30,11 @@ open class GeneratorExtensions { companion object Instance : SamConversion() } + + open fun computeFieldVisibility(descriptor: PropertyDescriptor): Visibility = -+ when {","Suggestion to move this logic inside the `PropertyDescriptor.fieldVisibility` in `PropertyGenerator` and return null in this method, to have only truly platform-specific code in `GeneratorExtensions` implementations",This will need to be updated to u ++ when {","Suggestion to move this logic inside the `PropertyDescriptor.fieldVisibility` in `PropertyGenerator` and return null in this method, to have only truly platform-specific code in `GeneratorExtensions` implementations","This will need to be updated to use the new syntax `when(condition, condition, condition)`." 620,"@@ -32,6 +32,7 @@ interface IrDeclarationOrigin { object FILE_CLASS : IrDeclarationOriginImpl(""FILE_CLASS"") object GENERATED_DATA_CLASS_MEMBER : IrDeclarationOriginImpl(""GENERATED_DATA_CLASS_MEMBER"") object GENERATED_INLINE_CLASS_MEMBER : IrDeclarationOriginImpl(""GENERATED_INLINE_CLASS_MEMBER"") -+ object SYNTHETIC_INLINE_CLASS_MEMBER : IrDeclarationOriginImpl(""SYNTHETIC_INLINE_CLASS_MEMBER"", isSynthetic = true)",Let's add this to `JvmLoweredDeclarationOrigin` instead. These origins are used in IR serialization format in Native and JS (`KotlinIr.proto`) and it's probably better to avoid adding JVM specifics here at this point,please add a trailing comma to av ++ object SYNTHETIC_INLINE_CLASS_MEMBER : IrDeclarationOriginImpl(""SYNTHETIC_INLINE_CLASS_MEMBER"", isSynthetic = true)",Let's add this to `JvmLoweredDeclarationOrigin` instead. These origins are used in IR serialization format in Native and JS (`KotlinIr.proto`) and it's probably better to avoid adding JVM specifics here at this point,please add a trailing comma to avoid difficult diffs when adding new entries 621,"@@ -32,6 +32,7 @@ public open class CharProgression ) : Iterable { init { if (step == 0) throw kotlin.IllegalArgumentException(""Step must be non-zero"") + else if (step == Int.MIN_VALUE) throw kotlin.IllegalArgumentException(""Step must have an inverse"")","The exception message isn't correct because `Int.MIN_VALUE` does have an inverse: it's `Int.MAX_VALUE`. -I'll reword the message.",nit: should we change the message +I'll reword the message.","nit: should we change the message to say ""Step must have an inverse"" or similar?" 622,"@@ -32,6 +50,7 @@ interface DependenciesResolver : @Suppress(""DEPRECATION"") ScriptDependenciesReso sealed class ResolveResult {","I'd simplify this by assuming that the result is ""success"" iff there are no errors: @@ -4726,12 +4726,12 @@ data class ResolutionResult( val isSuccess: Boolean get() = diagnostics.none { it.severity == ERROR } // maybe an extension } ``` -",This file doesn't have anything t +",This file doesn't have anything to do with the PR title. 623,"@@ -32,6 +50,7 @@ interface DependenciesResolver : @Suppress(""DEPRECATION"") ScriptDependenciesReso sealed class ResolveResult { abstract val dependencies: ScriptDependencies? -+ // reports -> diagnostics",+1 for `val diagnostics: List`,FMI: What is the significance of ++ // reports -> diagnostics",+1 for `val diagnostics: List`,FMI: What is the significance of this? 624,"@@ -32,7 +33,11 @@ class KotlinIfElseExpressionSurrounder(private val withBraces: Boolean) : Kotlin } @@ -4744,7 +4744,7 @@ data class ResolutionResult( } if (containingClass != null && !isStatic) { val thisType = containingClass.thisReceiver!!.type -+ val descriptor = WrappedReceiverParameterDescriptor()","If you make similar changes in `declareDefaultSetterParameter`, this also fixes `testData/codegen/box/traits/inheritedVar.kt` 🙂 (sorry GitHub won't let me comment on line 281)",This is not correct. This method returns the **descriptor** of the **containingClass** and **thisType** of the **receiver**. The only place where this variable is used is inside the ++ val descriptor = WrappedReceiverParameterDescriptor()","If you make similar changes in `declareDefaultSetterParameter`, this also fixes `testData/codegen/box/traits/inheritedVar.kt` 🙂 (sorry GitHub won't let me comment on line 281)",This is not correct. This method returns the **descriptor** of the **containingClass** and **thisType** of the **receiver**. The only place where this variable is used is inside the `if (containingClass != null && !isStatic) {` block. The only case where `thisType` can be null is when `isStatic` is true. The only case where `thisType` can be null is when `isStatic == true`. The only case where `thisType` can be null is when `isStatic == true`. The only case where `isStatic == true` is when `is 626,"@@ -326,15 +333,13 @@ internal class KotlinCommonSourceSetProcessor( project.tasks.findByName(kotlinCompilation.target.artifactsTaskName)?.dependsOn(kotlinTask)","findByName triggers all task creations. There is no direct equivalent, named is the closest but might fail, so you would have to introduce a try catch.",Could you explain why this change is necessary? 627,"@@ -33,10 +33,18 @@ public class StringBuilder(content: String = """") : Appendable, CharSequence { @@ -4880,7 +4880,7 @@ Without this overload we would have to split the name string twice or use an int @JvmField val HAS_NEXT = Name.identifier(""hasNext"") @JvmField val COMPONENT_REGEX = Regex(""component\\d+"") -+ @JvmField val DECONSTRUCT = Name.identifier(""deconstruct"")","Don't quite understand why this is needed. If you write a design document, please mention this moment there.","There's no need to use the identifier ""deconstruct"" here. ""deconstruct"" is not a va" ++ @JvmField val DECONSTRUCT = Name.identifier(""deconstruct"")","Don't quite understand why this is needed. If you write a design document, please mention this moment there.","There's no need to use the identifier ""deconstruct"" here. ""deconstruct"" is not a valid component name." 644,"@@ -3404,7 +3404,16 @@ level=""WEAK WARNING"" language=""kotlin"" @@ -4895,7 +4895,7 @@ Without this overload we would have to split the name string twice or use an int (if (isFinal) Opcodes.ACC_FINAL else 0) or - (if (isStatic) Opcodes.ACC_STATIC else 0) + (if (isStatic) Opcodes.ACC_STATIC else 0) or -+ (annotations.findAnnotation(VOLATILE_ANNOTATION_FQ_NAME)?.let { Opcodes.ACC_VOLATILE } ?: 0) or",Maybe `if` with `hasAnnotation`?,"This will crash if `isStatic` is false, since `findAnnotation` will return `null` i" ++ (annotations.findAnnotation(VOLATILE_ANNOTATION_FQ_NAME)?.let { Opcodes.ACC_VOLATILE } ?: 0) or",Maybe `if` with `hasAnnotation`?,"This will crash if `isStatic` is false, since `findAnnotation` will return `null` if the annotation is not found." 646,"@@ -344,6 +343,15 @@ class KotlinCompletionContributor : CompletionContributor() { super.handleInsert(context) @@ -4916,7 +4916,7 @@ Without this overload we would have to split the name string twice or use an int var minElem = iterator.next() + if (!iterator.hasNext()) return minElem var minValue = selector(minElem) - while (iterator.hasNext()) {",Condition check now can be moved to the end of the loop by replacing `while(...) { }` loop with `do { } while(...)` to avoid checking `iterator.hasNext()` twice on the first iteration.,The docs say that `iterator.next()` will return `null` if there is no next element. + while (iterator.hasNext()) {",Condition check now can be moved to the end of the loop by replacing `while(...) { }` loop with `do { } while(...)` to avoid checking `iterator.hasNext()` twice on the first iteration.,The docs say that `iterator.next()` will return `null` if there is no next element. But `iterator.hasNext()` will return `false` if there is no next element. Is this intentional? 649,"@@ -35,12 +36,18 @@ class SetterBackingFieldAssignmentInspection : AbstractKotlinInspection(), Clean arg.text == parameter?.text && arg.getArgumentExpression().getResolvedCall(context)?.resultingDescriptor == parameterDescriptor @@ -4939,13 +4939,13 @@ Without this overload we would have to split the name string twice or use an int /** + * Specifies the name for the target platform element (Java method, JavaScript function)","Documentation should probably not mention JavaScript anymore -",This should mention that the target platform is managed by one of the target platfo +",This should mention that the target platform is managed by one of the target platforms. 653,"@@ -35,6 +35,7 @@ import org.jetbrains.kotlin.util.ExtensionProvider interface DiagnosticSuppressor { fun isSuppressed(diagnostic: Diagnostic): Boolean + fun isSuppressed(diagnostic: Diagnostic, bindingContext: BindingContext?): Boolean = isSuppressed(diagnostic)","Why `bindingContext` is nullable here? -Note that we have `BindingContext.EMPTY` that can be used when there is no binding context",I am not convinced the bindingContext is optional here. Do you have an answer to th +Note that we have `BindingContext.EMPTY` that can be used when there is no binding context",I am not convinced the bindingContext is optional here. Do you have an answer to this? 654,"@@ -36,10 +39,11 @@ class AddWhenElseBranchFix(element: KtWhenExpression) : KotlinQuickFixAction -+ ",This string must be removed due to licensing problems,"Could you add `src=""${dependencies}/" ++ ",This string must be removed due to licensing problems,"Could you add `src=""${dependencies}/javaslang-2.0.6.jar""` to the other fileset? Thanks!" 665,"@@ -38,6 +38,21 @@ class FriendPathsTest : TestCaseWithTmpdir() { ) } @@ -5027,7 +5027,7 @@ In that mode I believe the parts should _not_ be synthetic, since otherwise meth + fun testArchive_relativePath() {"," ```suggestion fun testArchiveWithRelativePath() { -```",This is a temporary test until the r +```",This is a temporary test until the rest of the changes in this PR have been merged. 666,"@@ -38,6 +41,13 @@ interface IrComposite : IrContainerExpression { interface IrReturnableBlock : IrBlock, IrSymbolOwner, IrReturnTarget { @@ -5056,7 +5056,7 @@ In that mode I believe the parts should _not_ be synthetic, since otherwise meth - if (descriptor is ConstructorDescriptor && kind !== OwnerKind.ERASED_INLINE_CLASS) { + if (descriptor is ConstructorDescriptor && (kind !== OwnerKind.ERASED_INLINE_CLASS || isIrBackend)) {","It looks like the checking on ir backend is a little all over the place here. Looks like the IrBackend does a lot less here. Would it be simpler to just do separate methods to implement this for IR and non-IR and then share most of it via methods with reasonable names? -That could maybe polute the current type mapper less and maybe make it easier to reuse this in a new type mapper for the JVM_IR backend?","same, we can make this more readable" +That could maybe polute the current type mapper less and maybe make it easier to reuse this in a new type mapper for the JVM_IR backend?","same, we can make this more readable by just doing `kind !== OwnerKind.ERASED_INLINE_CLASS`" 669,"@@ -3833,6 +3833,9 @@ public Unit invoke(InstructionAdapter v) { v.mark(nonnull); } @@ -5067,7 +5067,7 @@ That could maybe polute the current type mapper less and maybe make it easier to else { v.dup(); generateInstanceOfInstruction(rightType);","Similar code is present in `generateInstanceOf` which probably means `Unit is Int?` is also affected? -",I am not sure why this is needed? Do +",I am not sure why this is needed? Do you know why this is needed? 670,"@@ -386,6 +386,10 @@ else if (containingDeclaration instanceof PackageFragmentDescriptor) { @Override public void visitCallExpression(@NotNull JetCallExpression expression) { @@ -5076,13 +5076,13 @@ That could maybe polute the current type mapper less and maybe make it easier to + } + + public void checkSamCall(@NotNull JetCallElement expression) {","public? -",I think this should be in `visitCall +",I think this should be in `visitCallExpression`. 671,"@@ -387,13 +417,23 @@ private class SyntheticAccessorLowering(val context: JvmBackendContext) : IrElem return false val contextDeclarationContainer = allScopes.lastOrNull { it.irElement is IrDeclarationContainer }?.irElement + // context for functions in an interface is DefaultImpls -+ val adjustedContext = contextDeclarationContainer.let {","Nitpick: maybe rename to `adjustedContextContainer`, to avoid `JvmBackendContext` associations.",What is the difference between `val` ++ val adjustedContext = contextDeclarationContainer.let {","Nitpick: maybe rename to `adjustedContextContainer`, to avoid `JvmBackendContext` associations.",What is the difference between `val` and `var` here? Can we use `var` instead? 672,"@@ -3871,12 +3871,11 @@ private StackValue genCmpWithNull(KtExpression exp, IElementType opToken, @Nulla Type type = expressionType(exp); StackValue argument = pregeneratedExpr != null ? pregeneratedExpr : gen(exp); @@ -5103,7 +5103,7 @@ That could maybe polute the current type mapper less and maybe make it easier to - if (ctor === iface) return true; - +function getAllInterfaces(ctor) { -+ if (ctor.$metadata$ == null) ctor.$metadata$ = {};",Why do we need this check? ,I don't think we should be modifying the metadata of a constructor. I think we should just make this an ++ if (ctor.$metadata$ == null) ctor.$metadata$ = {};",Why do we need this check? ,"I don't think we should be modifying the metadata of a constructor. I think we should just make this an empty object instead, like we did with `getOwnPropertyNames`. What do you think?" 675,"@@ -39,22 +39,35 @@ Kotlin.callSetter = function (thisObject, klass, propertyName, value) { Kotlin.callSetter(thisObject, Object.getPrototypeOf(klass), propertyName, value); }; @@ -5120,7 +5120,7 @@ That could maybe polute the current type mapper less and maybe make it easier to fun createUsageContexts(): Set -+ fun publication(action: Closure)","Maybe a better naming scheme would provide more concrete understanding that this publication comes from the `maven-publish` plugin, like `mavenPublication { ... }` (probably not the best option, since there's also the older `maven` plugin).",This feels like it should be on the `Unit` interface. It feels like it should be on the `Unit` interface ++ fun publication(action: Closure)","Maybe a better naming scheme would provide more concrete understanding that this publication comes from the `maven-publish` plugin, like `mavenPublication { ... }` (probably not the best option, since there's also the older `maven` plugin).",This feels like it should be on the `Unit` interface. It feels like it should be on the `Unit` interface. 677,"@@ -39,54 +39,111 @@ import kotlin.concurrent.read import kotlin.concurrent.write @@ -5327,7 +5327,7 @@ On the other hand, small formatting changes in comparison with the functional on Type.getArgumentTypes(node.desc) + parameters.capturedTypes } ``` -and then...","I don't think you need to check for `null` here. If `node` is `None`, then the argument list should be initialized wit" +and then...","I don't think you need to check for `null` here. If `node` is `None`, then the argument list should be initialized with an empty list." 704,"@@ -414,6 +414,14 @@ class CollectionTest { expect(arrayList(2, 3, 1)) { list } } @@ -5342,22 +5342,22 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non val libraries: FileCollection @InputFiles get() = compilation.compileDependencyFiles -+ val friendModule: FileCollection?",Consider restricting access,Consider using `FileCollection` instead of `FileCollection?`. 706,"@@ -42,6 +42,7 @@ interface IrDeclarationOrigin { object IR_BUILTINS_STUB : IrDeclarationOriginImpl(""IR_BUILTINS_STUB"") object BRIDGE : IrDeclarationOriginImpl(""BRIDGE"", isSynthetic = true) object BRIDGE_SPECIAL: IrDeclarationOriginImpl(""BRIDGE_SPECIAL"") -+ object GENERATED_PROPERTY_REFERENCE : IrDeclarationOriginImpl(""GENERATED_PROPERTY_REFERECE"", isSynthetic = true)",Please move to `JvmLoweredDeclarationOrigin`,isn't `GENERATED_PROPERTY_REFERECE ++ object GENERATED_PROPERTY_REFERENCE : IrDeclarationOriginImpl(""GENERATED_PROPERTY_REFERECE"", isSynthetic = true)",Please move to `JvmLoweredDeclarationOrigin`,isn't `GENERATED_PROPERTY_REFERECE` covered by the test already? 707,"@@ -42,6 +42,8 @@ interface IrDeclarationOrigin { object IR_BUILTINS_STUB : IrDeclarationOriginImpl(""IR_BUILTINS_STUB"") object BRIDGE : IrDeclarationOriginImpl(""BRIDGE"", isSynthetic = true) object BRIDGE_SPECIAL: IrDeclarationOriginImpl(""BRIDGE_SPECIAL"") -+ object SYNTHETIC_METHOD_FOR_TYPEALIAS_ANNOTATIONS :",As it's only jvm-specific please move it to JvmLoweredDeclarationOrigin,Why is this here? This is not used ++ object SYNTHETIC_METHOD_FOR_TYPEALIAS_ANNOTATIONS :",As it's only jvm-specific please move it to JvmLoweredDeclarationOrigin,Why is this here? This is not used in this file. 708,"@@ -420,6 +420,15 @@ class ExpressionCodegen( } override fun visitFieldAccess(expression: IrFieldAccessExpression, data: BlockInfo): PromisedValue { -+ classCodegen.fieldConstantValue(expression.symbol.owner)?.let {","@pyos Is it possible to move this part to ConstLowering? In such case additional optimization would be applied during lowers phase, e.g. foldConstantLoweringPhase ",Nit: We usually don't put spaces a ++ classCodegen.fieldConstantValue(expression.symbol.owner)?.let {","@pyos Is it possible to move this part to ConstLowering? In such case additional optimization would be applied during lowers phase, e.g. foldConstantLoweringPhase ",Nit: We usually don't put spaces around parentheses. 709,"@@ -420,6 +420,15 @@ class ExpressionCodegen( } @@ -5368,7 +5368,7 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non ""$originalDescriptor has ${originalDescriptor.typeParameters}"" } -+ val partialSam = context.languageVersionSettings.supportsFeature(LanguageFeature.SamConversionPerArgument)","Minor: give it some meaningful name, like `isPartialSamConversionSupported` or something.",Shouldn't this be `!context.langua ++ val partialSam = context.languageVersionSettings.supportsFeature(LanguageFeature.SamConversionPerArgument)","Minor: give it some meaningful name, like `isPartialSamConversionSupported` or something.",Shouldn't this be `!context.languageVersionSettings.supportsFeature(LanguageFeature.SamConversionPerArgument)`? 711,"@@ -424,6 +425,24 @@ class Converter private constructor( } } @@ -5376,14 +5376,14 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non + + private fun specialAnnotationPropertyCases(field: PsiField): Annotations { + val javaSerializableInterface = PsiType.getTypeByName(""java.io.Serializable"", project, GlobalSearchScope.allScope(project)).resolve()","It's a minor issue, but the correct way is to use `field.getResolveScope()` instead of `allScope` here. Otherwise you may get incorrect results if the project uses multiple JDKs in different modules. -","PsiType.getTypeByName(""java.io.Ser" +","PsiType.getTypeByName(""java.io.Serializable"", project).resolve() can be deleted." 712,"@@ -424,8 +424,14 @@ public inline fun String.toPattern(flags: Int = 0): java.util.regex.Pattern { * * @sample samples.text.Strings.capitalize */ -public actual fun String.capitalize(): String { - return if (isNotEmpty() && this[0].isLowerCase()) substring(0, 1).toUpperCase() + substring(1) else this -+public actual fun String.capitalize() = capitalize(Locale.getDefault())",Better to leave the previous implementation to avoid accidentally changing behavior in 1.3.x release.,Should we remove the `toUpperCase( ++public actual fun String.capitalize() = capitalize(Locale.getDefault())",Better to leave the previous implementation to avoid accidentally changing behavior in 1.3.x release.,Should we remove the `toUpperCase()` call here? 713,"@@ -427,6 +434,36 @@ class JvmSymbols( } @@ -5395,7 +5395,7 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non } + val dslJavacOptions = kaptExtension.getJavacOptions().toMutableMap() -+ if (javaCompile != null && ""-source"" !in dslJavacOptions) {","Probably need to check for `--release N` too which combines `-source N`, `-target N` and `-bootclasspath ` http://openjdk.java.net/jeps/247",Avoid using a mutable object. Inst ++ if (javaCompile != null && ""-source"" !in dslJavacOptions) {","Probably need to check for `--release N` too which combines `-source N`, `-target N` and `-bootclasspath ` http://openjdk.java.net/jeps/247","Avoid using a mutable object. Instead, create a new object in the extension class." 715,"@@ -428,3 +429,8 @@ fun BodyResolveComponents.transformQualifiedAccessUsingSmartcastInfo(qualifiedAc } return FirExpressionWithSmartcastImpl(qualifiedAccessExpression, intersectedTypeRef, typesFromSmartCast) @@ -5403,7 +5403,7 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non + +fun CallableId.isInvoke() = + packageName == KotlinBuiltIns.BUILT_INS_PACKAGE_FQ_NAME -+ && className?.asString()?.startsWith(""Function"") == true",How does it work if `className` is e.g. `KFunction*`?,Can we move this to `FirExpression ++ && className?.asString()?.startsWith(""Function"") == true",How does it work if `className` is e.g. `KFunction*`?,Can we move this to `FirExpressionWithSmartcastImpl`? 716,"@@ -428,4 +428,37 @@ class KotlinGradleIT: BaseGradleIT() { assertSuccessful() } @@ -5412,23 +5412,23 @@ and then...","I don't think you need to check for `null` here. If `node` is `Non + @Test + fun testGradleJavaIcWorking() { + val project = Project(""kotlinJavaProject"", ""2.14.1"")","What about newer Gradle versions? -Btw, it would be interesting to test if Java compile avoidance works in this case with the Gradle 3.4 (in other words that Java is not compiled if Kotlin public API is not changed). ",Please create a follow-up JIRA so +Btw, it would be interesting to test if Java compile avoidance works in this case with the Gradle 3.4 (in other words that Java is not compiled if Kotlin public API is not changed). ",Please create a follow-up JIRA so we remember to do this in a later PR. 717,"@@ -43,13 +46,13 @@ class ReplCodeAnalyzer(environment: KotlinCoreEnvironment) { val module: ModuleDescriptorImpl - val trace: BindingTraceContext = NoScopeRecordCliBindingTrace() -+ val trace: BindingTraceContext = CliBindingTrace()",I'm not sure that unconditional change is good here. I suspect that CliBindingsTrace is more expensive than NoScopeRecordCliBindingTrace. I'd suggest using either one depending on whether the completion is requested.,Please rename to `CliBindingTraceC ++ val trace: BindingTraceContext = CliBindingTrace()",I'm not sure that unconditional change is good here. I suspect that CliBindingsTrace is more expensive than NoScopeRecordCliBindingTrace. I'd suggest using either one depending on whether the completion is requested.,Please rename to `CliBindingTraceContext`. 718,"@@ -43,6 +43,40 @@ class Arrays { assertPrints(matrix.contentDeepToString(), ""[[3, 7, 9], [0, 1, 0], [2, 4, 8]]"") } -+ @Sample","Better to introduce another nested class, since `ContentOperations` class is to group samples for `content*` array extensions.",Could you also add a `@Category(Sa ++ @Sample","Better to introduce another nested class, since `ContentOperations` class is to group samples for `content*` array extensions.",Could you also add a `@Category(Samples.class)` here? 719,"@@ -43,6 +44,7 @@ class RedundantUnitReturnTypeInspection : AbstractKotlinInspection() { if (!function.hasBlockBody()) { val bodyExpression = function.bodyExpression if (bodyExpression != null) { -+ if (bodyExpression is KtThrowExpression) return","It's not the correct way. `KtThrowException` is not the only one that has `Nothing` type, e.g. `null!!` has the same type. You should check `bodyExpression` type directly.",Remove the `!hasBlockBody()` condi ++ if (bodyExpression is KtThrowExpression) return","It's not the correct way. `KtThrowException` is not the only one that has `Nothing` type, e.g. `null!!` has the same type. You should check `bodyExpression` type directly.",Remove the `!hasBlockBody()` condition below. 720,"@@ -43,6 +83,15 @@ public trait MutableListIterator : ListIterator, MutableIterator { // Modification Operations @@ -5453,7 +5453,7 @@ Btw, it would be interesting to test if Java compile avoidance works in this cas + } + catch (FileNotFoundException fileNotFoundException) { + // Display the absolute path of the file that wasn't loaded","You're explaining the code behavior in a comment. -The code you wrote is clear enough in that regard, no need for the comment :)",Do we really want to show the absolute path of the file that wasn't loaded +The code you wrote is clear enough in that regard, no need for the comment :)",Do we really want to show the absolute path of the file that wasn't loaded? Wouldn't it be better to just show the path of the file that was not found? 722,"@@ -433,25 +437,24 @@ internal abstract class AbstractKotlinPlugin( } @@ -5484,7 +5484,7 @@ fun test() { -+ ",Please remove changes in non-md files.,I'm not sure if it's a good idea to create a separate component for this. ++ ",Please remove changes in non-md files.,"I'm not sure if it's a good idea to create a separate component for this. It seems like the `NullNotNullManager` should be a component named `NonNullableManager`, but that's not the case here." 725,"@@ -44,7 +43,7 @@ abstract class IrModuleDeserializer( val logger: LoggingContext, val builtIns: IrBuiltIns, @@ -5498,7 +5498,7 @@ fun test() { - is KtValueArgument -> parent.parent.parent as? KtAnnotationEntry ?: return + is KtValueArgument -> { + if (parent.parent.parent !is KtAnnotationEntry) return -+ if (parent.getSpreadElement() != null && !parent.isNamed()) return","Why so? Quick-fix can still be applied, but `*` should be preserved!",@jon-wei if the parent is non-null and `parent.type != KtAnnotationEntry` ++ if (parent.getSpreadElement() != null && !parent.isNamed()) return","Why so? Quick-fix can still be applied, but `*` should be preserved!",@jon-wei if the parent is non-null and `parent.type != KtAnnotationEntry` then the nested `if` condition will never be true 727,"@@ -44,7 +44,7 @@ object MakeVisibleFactory : KotlinIntentionActionsFactory() { val module = DescriptorUtils.getContainingModule(descriptor) @@ -5516,14 +5516,14 @@ fun test() { - firstArgumentAsDispatchReceiver: Boolean = false + dispatchReceiverAsArgument: Boolean = false, + argumentAsDispatchReceiver: Boolean = false, -+ extensionReceiverAsArgument: Boolean = dispatchReceiverAsArgument,",These defaults look confusing. Any objections on changing it to `false` to make intent explicit?,I'm not sure if this should be called `extensionReceiverAsArgument` or `fi ++ extensionReceiverAsArgument: Boolean = dispatchReceiverAsArgument,",These defaults look confusing. Any objections on changing it to `false` to make intent explicit?,I'm not sure if this should be called `extensionReceiverAsArgument` or `firstArgumentAsArgument` but I'm not sure. 729,"@@ -441,4 +441,17 @@ object Mapping : TemplateGroupBase() { } } + val f_withEach = fn(""withEach(operation: T.() -> Unit)"") { + include(Iterables)","Consider providing it for the same receivers as `forEach`. -Also it makes sense to make it inline same as `forEach` (note that different overloads have different inline modifiers)",I don't think this is needed. f_withEach does not seem to be used anywhere +Also it makes sense to make it inline same as `forEach` (note that different overloads have different inline modifiers)",I don't think this is needed. f_withEach does not seem to be used anywhere. 730,"@@ -444,3 +444,6 @@ fun CallableId.isInvoke() = packageName == KotlinBuiltIns.BUILT_INS_PACKAGE_FQ_NAME && className?.asString()?.startsWith(""Function"") == true @@ -5535,10 +5535,10 @@ Also it makes sense to make it inline same as `forEach` (note that different ove fun Generator.getInferredTypeWithImplicitCasts(key: KtExpression): KotlinType? = - context.bindingContext.getType(key) -+ if (key.isUsedAsExpression(context.bindingContext))","Minor: I'd rather rename this function, because it no longer returns a type inferred by the front-end, but uses some extra logic to determine that implicit coercion to Unit should happen. Like, `getExpressionTypeWithCoercionToUnit` or something.",I think this throws the question question whether the key `isUsedAsExpress ++ if (key.isUsedAsExpression(context.bindingContext))","Minor: I'd rather rename this function, because it no longer returns a type inferred by the front-end, but uses some extra logic to determine that implicit coercion to Unit should happen. Like, `getExpressionTypeWithCoercionToUnit` or something.","I think this throws the question question whether the key `isUsedAsExpression` should be used as a key in the generated `bindingContext` or we should assume that the key is a `KotlinType`. If the latter, then we should check that the type is a `KotlinType`. Otherwise we should throw the question question question question question" 732,"@@ -45,15 +46,7 @@ class JoinBlockIntoSingleStatementHandler : JoinRawLinesHandlerDelegate { // if outer if has else-branch and inner does not have it, do not remove braces otherwise else-branch will belong to different if!","I can't unify the check because while the intention is simply not available, the join handler still has to go into this branch. -","This is not right. If an outer if has no else-branch, then the else-branch" +","This is not right. If an outer if has no else-branch, then the else-branch should be unchanged." 733,"@@ -45,4 +45,13 @@ class Regexps { // Because the search starts from the index 2, it finds the last ""to be"". assertPrints(regex3.find(inputString, 2)!!.range, ""13..17"") @@ -5601,7 +5601,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue +} + +private fun KtElement.getPrimaryConstructorParameterWithName(name: String): KtParameter? {","The name of this function doesn't describe what it does. A more descriptive name would be ""findContainingClassConstructorParameterWithName"" (which is long enough to hint that it may be better to split the logic: a function to generate a sequence of outer classes for a class and a single-line call to find the parameter of the class by name). -",These 2 methods are the same. Can we just reuse `getPrimaryConstructorParameterW +",These 2 methods are the same. Can we just reuse `getPrimaryConstructorParameterWithName` from `KtElement`? 740,"@@ -46,6 +46,7 @@ abstract class AbstractCheckLocalVariablesTableTest : CodegenTestCase() { assertNotNull(""Couldn't find class file for pattern $classFileRegex in: $pathsString"", outputFile) @@ -5613,7 +5613,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue var name = parameters.getOrNull(index - startParameterIndex)?.name - ?: localVariables.getOrNull(index + (if (isStatic) 0 else 1))?.name + ?: localVariables.getOrNull(index)?.name - ?: ""p${index - startParameterIndex}""","@yanex Removing this offset fixes https://youtrack.jetbrains.com/issue/KT-15024, but I'm not sure exactly what the offset was for in the first place. Could you let me know what it's for and I can update this PR if needed. I have included a static method in the test and it still works fine. ",This is the bug fix. The problem was that the start parameter index was out of t + ?: ""p${index - startParameterIndex}""","@yanex Removing this offset fixes https://youtrack.jetbrains.com/issue/KT-15024, but I'm not sure exactly what the offset was for in the first place. Could you let me know what it's for and I can update this PR if needed. I have included a static method in the test and it still works fine. ","This is the bug fix. The problem was that the start parameter index was out of the range of 0 to 1, and now the start parameter index is out of the range of 0 to 1, so the current code assumes that the start parameter index is out of the range of 0, so the current code assumes that the start parameter index is out of the range of 0, so the current code assumes that the current parameter index is out of the range of 0, so the current code assumes that the current parameter index is out of the range of 0, so the current" 742,"@@ -46,7 +53,13 @@ fun File.withZipFileSystem(mutable: Boolean = false, action: (FileSystem) -> return try { action(zipFileSystem) @@ -5631,7 +5631,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue + zipFileSystem.close() + counters.remove(zipFileSystem) + } else { -+ counters.computeIfPresent(zipFileSystem, { t, u -> u + 1})",`u - 1`,I think it would be more efficient to compute if `counters.contains(zipFileSyste ++ counters.computeIfPresent(zipFileSystem, { t, u -> u + 1})",`u - 1`,I think it would be more efficient to compute if `counters.contains(zipFileSystem)` and then remove it. This way there is no need to compute if there is nothing to remove. 744,"@@ -46,7 +53,13 @@ fun File.withZipFileSystem(mutable: Boolean = false, action: (FileSystem) -> return try { action(zipFileSystem) @@ -5656,7 +5656,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue +// No way to get script name if file is not present, should add another property (val fileName: String) interface ScriptContents { val file: File? - val annotations: Iterable",Maybe use `Collection` here?,Please also add a TODO to remove this way of getting the script name from the fi + val annotations: Iterable",Maybe use `Collection` here?,Please also add a TODO to remove this way of getting the script name from the file. 747,"@@ -47,18 +66,23 @@ interface DependenciesResolver : @Suppress(""DEPRECATION"") ScriptDependenciesReso } } @@ -5692,7 +5692,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue val descriptor = holder.manager.createProblemDescriptor( startElement, - list, -+ modifierElements.last(),","I think it would be better to highlight all modifiers, not only the last one (but annotations should not be included)",This is the only place where `modifierElements` is used. The only caller of this ++ modifierElements.last(),","I think it would be better to highlight all modifiers, not only the last one (but annotations should not be included)",This is the only place where `modifierElements` is used. The only caller of this method is `getModifierElements()`. The only caller of this method is passing a list of `Element`s instead of a `List`. 752,"@@ -47,7 +47,7 @@ public class AddBracesIntention : JetSelfTargetingIntention("" val jetBlockElement = element.findBlockInExpression(expressionKind)","[this is a dummy comment, please ignore it] ",Please revert the changes toPlease avoid unrelated changes in this PR. @@ -5718,7 +5718,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue ``` (outerClass?.isInterface ?: false) || tree.modifiers.isStatic -```",This makes the code much easier to read. The ternary operator should stay a +```",This makes the code much easier to read. The ternary operator should stay at the end of the line. 756,"@@ -48,7 +48,8 @@ class ReplaceContainsIntention : SelfTargetingRangeIntention): Method? { //Use only JDK 1.6 compatible api object TestProcessServer { @@ -5795,7 +5795,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue +public fun File.readBytes(): ByteArray = FileInputStream(this).use { input ->",Changed exception type to OutOfMemoryError,"unrelated to your change, but why is this needed?" 767,"@@ -49,7 +50,9 @@ class KotlinJvmTargetConfigurator(kotlinPluginVersion: String) : ): KotlinJvmTestRun = KotlinJvmTestRun(name, target).apply { - val testTaskOrProvider = target.project.registerTask(testTaskName) { testTask ->",`testTaskProvider`,Please don't make any changes here. This is unrelated to the rest of the PR + val testTaskOrProvider = target.project.registerTask(testTaskName) { testTask ->",`testTaskProvider`,Please don't make any changes here. This is unrelated to the rest of the PR. 768,"@@ -5,24 +5,21 @@ import org.testng.* import java.util.concurrent.* @@ -5823,7 +5823,7 @@ if (KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY.systemPropertyAsBooleanOrTrue + + + -+",I think this should be dropped,Maybe add a `TODO` to remove this once the property is remo ++",I think this should be dropped,Maybe add a `TODO` to remove this once the property is removed? 771,"@@ -5,6 +5,12 @@ import org.junit.Test as test class StringBuilderTest { @@ -5843,7 +5843,7 @@ To make this test JVM-only, move it to `StringBuilderJVMTest.kt` apply plugin: ""kotlin"" -configurePublishing(project) -+if (System.getProperty(""idl2k.deploy"", ""false"").toBoolean()) {","`system.idl2k.deploy` teamcity parameter translates to a project property `idl2k.deploy` in Gradle, rather than a JVM system property. So we need to use gradle's `Project.findProperty/getProperty/hasProperty`","I don't love this, but this feels like an anti-pattern. Why" ++if (System.getProperty(""idl2k.deploy"", ""false"").toBoolean()) {","`system.idl2k.deploy` teamcity parameter translates to a project property `idl2k.deploy` in Gradle, rather than a JVM system property. So we need to use gradle's `Project.findProperty/getProperty/hasProperty`","I don't love this, but this feels like an anti-pattern. Why not apply the same plugin twice?" 774,"@@ -5,8 +5,8 @@ import java.util.concurrent.locks.ReentrantReadWriteLock import java.util.concurrent.CountDownLatch @@ -5868,7 +5868,7 @@ To make this test JVM-only, move it to `StringBuilderJVMTest.kt` actual.source.containingFile != SourceFile.NO_SOURCE_FILE }.groupBy { actual -> - areCompatibleCallables(expected, actual) -+ areCompatibleCallables(expected, actual, platformModule)",Missed,I wonder if we should make `areCompatibleCallables` accept ++ areCompatibleCallables(expected, actual, platformModule)",Missed,I wonder if we should make `areCompatibleCallables` accept `PlatformModule` instead of `null`? 778,"@@ -501,6 +501,14 @@ class DefaultExpressionConverter : JavaElementVisitor(), ExpressionConverter { } } @@ -5876,13 +5876,13 @@ To make this test JVM-only, move it to `StringBuilderJVMTest.kt` + if (type?.canonicalText == CommonClassNames.JAVA_LANG_STRING) { + val argument = expression.argumentList?.expressions?.singleOrNull() + if (argument != null && argument.type?.canonicalText == CommonClassNames.JAVA_LANG_STRING) { -+ result = codeConverter.convertExpression(argument)",See my comment to https://youtrack.jetbrains.com/issue/KT-19555,"Rather than special-casing this, I think it would be better" ++ result = codeConverter.convertExpression(argument)",See my comment to https://youtrack.jetbrains.com/issue/KT-19555,"Rather than special-casing this, I think it would be better to add a new `ConvertExpression` method to `CommonClassNames` and then call `codeConverter.convertExpression` on it here." 779,"@@ -51,3 +52,7 @@ fun writeSyntheticClassMetadata(cb: ClassBuilder, state: GenerationState) { // Do nothing } } + -+fun markMethodAsGenerated(mv: MethodVisitor) {",Kotlin doc? Probably pretty simple. Might be useful information though.,Nit: Can we rename this to `visitMethodAsGenerated` for cla ++fun markMethodAsGenerated(mv: MethodVisitor) {",Kotlin doc? Probably pretty simple. Might be useful information though.,Nit: Can we rename this to `visitMethodAsGenerated` for clarity? 780,"@@ -51,4 +66,19 @@ class FriendPathsTest : TestCaseWithTmpdir() { ) ) @@ -5896,7 +5896,7 @@ To make this test JVM-only, move it to `StringBuilderJVMTest.kt` val factory = KtPsiFactory(project) val newKeyword = if (makeVar) factory.createVarKeyword() else factory.createValKeyword() element.valOrVarKeyword!!.replace(newKeyword) -+ if (deleteInitializer) (element as? KtProperty)?.initializer = null",Add braces,I don't think this is the right place for this. You should ++ if (deleteInitializer) (element as? KtProperty)?.initializer = null",Add braces,I don't think this is the right place for this. You should move the assignment before the if statement. 782,"@@ -51,6 +54,11 @@ open class ChangeVisibilityFix( } @@ -5912,7 +5912,7 @@ if (originalElement is KtDeclaration) { pointer?.element?.setVisibility(visibilityModifier) val propertyAccessor = pointer?.element as? KtPropertyAccessor -```",Is this necessary? It seems like the `visibilityModifier` i +```",Is this necessary? It seems like the `visibilityModifier` is only used to set `element.setVisibility(visibilityModifier);`. 783,"@@ -51,7 +51,7 @@ http://www.w3.org/1999/xhtml @@ -5937,7 +5937,7 @@ val propertyAccessor = pointer?.element as? KtPropertyAccessor ``` iv.invokevirtual((isInterface ? AsmTypes.OBJECT_TYPE : type).getInternalName(), ""hashCode"", ""()I"", false); ``` -",Shouldn't this remain `else if (type.getSort() == Type.OBJECT) +",Shouldn't this remain `else if (type.getSort() == Type.OBJECT)`? 786,"@@ -52,7 +52,10 @@ private class SingletonReferencesLowering(val context: JvmBackendContext) : File } @@ -5954,13 +5954,13 @@ interface Test { } } } -```","For clarity, remove the 'allScopes' arg and just do `if (allSc" +```","For clarity, remove the 'allScopes' arg and just do `if (allScopes.any { it.irElement == expression.symbol.owner })`." 787,"@@ -521,6 +521,7 @@ class CollectionTest { val coll = listOf(""foo"", ""bar"", ""abc"") assertEquals(listOf(""bar"", ""abc""), coll.drop(1)) assertEquals(listOf(""abc""), coll.drop(2)) + assertEquals(listOf(2147483647L, 2147483648), (0L..2147483648).drop(2147483647))","- I expect this test running for some noticeable time, so better to place it in stdlib/jvm/testLongRunning/collections/IndexOverflowJVMTest.kt -- I believe we can express these numbers with `Int.MAX_VALUE` constant",This is wrong. The `listOf` function does not accept any argum +- I believe we can express these numbers with `Int.MAX_VALUE` constant",This is wrong. The `listOf` function does not accept any arguments. 788,"@@ -529,15 +529,10 @@ private class AddContinuationLowering(private val context: JvmBackendContext) : it.putValueArgument(i++, irGet(irFunction.dispatchReceiverParameter!!)) } @@ -6013,12 +6013,12 @@ interface Test { } -@kotlin.internal.InlineOnly","@pyos Seems this removal causes next fail: https://teamcity.jetbrains.com/viewLog.html?buildId=2236246&tab=buildResultsDiv&buildTypeId=Kotlin_dev_Compiler -//cc @anton-bannykh ","I think this is safe to delete now, because the type is now `i" +//cc @anton-bannykh ","I think this is safe to delete now, because the type is now `internal.Iterator`." 794,"@@ -53,6 +54,9 @@ class K2MetadataCompiler : CLICompiler() { ): ExitCode { val collector = configuration.getNotNull(CLIConfigurationKeys.MESSAGE_COLLECTOR_KEY) -+ val plugLoadResult = PluginCliParser.loadPluginsSafe(arguments, configuration)",`val pluginLoadResult` please,This is the only place where `PluginCliParser.loadPluginsSafe` ++ val plugLoadResult = PluginCliParser.loadPluginsSafe(arguments, configuration)",`val pluginLoadResult` please,"This is the only place where `PluginCliParser.loadPluginsSafe` is used, the only caller of `PluginCliParser.loadPluginsSafe` now passes the arguments through." 795,"@@ -53,6 +54,9 @@ class K2MetadataCompiler : CLICompiler() { ): ExitCode { val collector = configuration.getNotNull(CLIConfigurationKeys.MESSAGE_COLLECTOR_KEY) @@ -6035,7 +6035,7 @@ cc @udalov - + -+",Not needed,I don't think you n ++",Not needed,I don't think you need the encoding here. 26,"@@ -0,0 +1,12 @@ +class A { + val a = """" @@ -189,7 +189,7 @@ and check how it works now? The code `element.else == null` is a bit suspicious. + return ""OK""","This test doesn't check that no classes were generated for the property reference. I don't think it's possible in a `box` test, please try a bytecode listing test instead (`compiler/testData/codegen/bytecodeListing`). Also please test constructors and functions -",Why is this a` +",Why is this a``This is not a`This is not a`This is not a`This is not a`This is not aThis is a`This is not aThis is a`This is not aThis is a`This is not aThis is not aIt is aThis is not aIt should be aThis is aThis is not aIt should be aIt should be aThis is not aIt should be aThis is not aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aIt should be aThis is not aIt should be aIt should be aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aThis is not aIt should be aIt should be aIt should be aIt should be aIt should be aIt should not be a function. It should not be a function.` 27,"@@ -0,0 +1,12 @@ +name: ""Validate Gradle Wrapper"" +on: [push]","Without `pull_request`, this won't actually run for external contributors", @@ -200,7 +200,7 @@ Also please test constructors and functions + var num = 0 + if (result == true) num = 1 + else num = 2","This is a nice test case, but is this code really needed to test your intention? I think it's just confusing... -",I think this should +",I think this should be named `test7` 29,"@@ -0,0 +1,13 @@","What is this file? Is it for manual testing? ",Why is this needed? 30,"@@ -0,0 +1,13 @@ @@ -211,7 +211,7 @@ Also please test constructors and functions + +package org.jetbrains.kotlin.utils + -+data class CompletionVariant(","I suspect that this class doesn't use the features provided by the data classes, while the data classes are not free from the overhead. So maybe it makes sense to drop ""data"" here.",I don't think we ne ++data class CompletionVariant(","I suspect that this class doesn't use the features provided by the data classes, while the data classes are not free from the overhead. So maybe it makes sense to drop ""data"" here.",I don't think we need this class. 31,"@@ -0,0 +1,13 @@ +// WITH_COROUTINES","This is really minor, but maybe these tests will succeed even with `WITH_RUNTIME`, now that `Result` in the standard library.", 32,"@@ -0,0 +1,13 @@ @@ -301,7 +301,7 @@ More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/4624 + +package org.jetbrains.kotlin.js.test.interop + -+interface InteropEngine {",JDK uses `ScriptEngine` for such things. I'd rather keep this name,nit: `InteropEngine` -> `Interpr ++interface InteropEngine {",JDK uses `ScriptEngine` for such things. I'd rather keep this name,nit: `InteropEngine` -> `InterpreterEngine` 41,"@@ -0,0 +1,14 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -312,7 +312,7 @@ More conversion/details are here: https://r8-review.googlesource.com/c/r8/+/4624 +import org.gradle.api.logging.Logger + +const val deprecatedBecauseNoConfigAvoidanceUseProvider = ""Using this brings performance issues. "" + -+ ""Use the provider instead to benefit from configuration avoidance.""",Use the equivalent provider method to benefit from gradle task configuration avoidance.,nit: `val` -> `deprecatedUseProv ++ ""Use the provider instead to benefit from configuration avoidance.""",Use the equivalent provider method to benefit from gradle task configuration avoidance.,nit: `val` -> `deprecatedUseProvider` 42,"@@ -0,0 +1,14 @@ +// KJS_WITH_FULL_RUNTIME +// WITH_RUNTIME @@ -354,7 +354,7 @@ fun box(): String { + false +} + -+fun main(args: Array) {","Idea: Similar to `LowLevelDebuggerTestBase`, maybe you can generate and write your own ""main class"" that simply calls `box()`? This way we can easily re-use existing box tests to create stepping tests.",I think this should be `if (cond ++fun main(args: Array) {","Idea: Similar to `LowLevelDebuggerTestBase`, maybe you can generate and write your own ""main class"" that simply calls `box()`? This way we can easily re-use existing box tests to create stepping tests.",I think this should be `if (cond())` 46,"@@ -0,0 +1,14 @@ +fun cond() = false + @@ -374,7 +374,7 @@ fun box(): String { // box(): 4 // cond(): 1 // box(): 4 7 8 -```",Can you add a few more examples +```",Can you add a few more examples here? 47,"@@ -0,0 +1,143 @@ +// WITH_RUNTIME + @@ -405,7 +405,7 @@ fun box(): String { + +fun A?.bar() { + foo() -+}","Why does this test have so strange name? I see no `invoke` here, either safe or unsafe",I don't think this is the right error message. It shou ++}","Why does this test have so strange name? I see no `invoke` here, either safe or unsafe","I don't think this is the right error message. It should be something like ""Only safe (?.) calls are allowed on a nullable receiver of type A?""." 50,"@@ -0,0 +1,15 @@ +// SKIP_TXT","Maybe we could skip txt generation for all these tests automatically by adding an abstract method into `AbstractDiagnosticsTest` and overriding it in this test, instead of adding `SKIP_TXT` to each file?", 51,"@@ -0,0 +1,15 @@ @@ -433,7 +433,7 @@ fun box(): String { + fun test2(sum: (a: Int, b: Int) -> Int, c: Int): Int {","Basically you've used the same test data file in all tests on `makeTypeExplicitInLambda`, altering the calling function in each case. This is not a good idea, because once one of such tests fails, the investigating person might think that the fact that there are several functions in this file is relevant to the test case, while in reality it's not. Please try to make your tests as minimal as possible, i.e. only include relevant data in each test. -",Why is this aI don't think it should be aI d +",Why is this aI don't think it should be aI don't think it should be aI don't think it should be aI don't think it should be aI think it should be a function. 53,"@@ -0,0 +1,15 @@ +data class Foo(val a: String) { + fun copy(i: Int) {} @@ -446,7 +446,7 @@ Please try to make your tests as minimal as possible, i.e. only include relevant ``` data class SomeName(a: Int, b: Int, c: String) f.copy(2, c = """") -```",I think this should be `````` +```",I think this should be ```````I don't think this is a``I don't think this is a`I don't think this is a`I don't think this is a`I don't think this is a``I don't think this is a`I don't think this is a`I don't think this is a`This is a`I don't know how I feel about this` 54,"@@ -0,0 +1,151 @@ +/* + * Copyright 2010-2019 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -499,7 +499,7 @@ f.copy(2, c = """") +// Outer\$Inner.f: +// 1 GETSTATIC Outer\$Inner.\$assertionsDisabled","Please also add: // 1 desiredAssertionStatus - // 1 public final static Z $assertionsDisabled",I don't think this is the right place to put this code + // 1 public final static Z $assertionsDisabled",I don't think this is the right place to put this code. It should be in `src/test/java/lang/Class.java`. 58,"@@ -0,0 +1,16 @@ +// TARGET_BACKEND: JVM +// IGNORE_BACKEND: JVM @@ -541,7 +541,7 @@ And add same test to box ones. ", +fun f() { + for (i in intArray()) {",...and here too,"I think this should be `longArrayOf(0, 0, 0, 0)`." 61,"@@ -0,0 +1,16 @@ -+fun testIsNullOrBlank(x: String?) {","This test has no annotation of some language feature on. Looks strange. I'd check all new tests, and most probably all of them should have some annotation. Otherwise backward compatibility can be broken",nit: space after `````````````````````````````````````` 62,"@@ -0,0 +1,16 @@ +interface A { + fun f(x: T): T @@ -596,7 +596,7 @@ And add same test to box ones. ", + return ""other"" +} + -+// 0 LOOKUPSWITCH",Is LOOKUPSWITCH is absent cause of hashAndSwitchLabels.size <= 2 condition?,I think this should be `kotlin.test.assertEquals( ++// 0 LOOKUPSWITCH",Is LOOKUPSWITCH is absent cause of hashAndSwitchLabels.size <= 2 condition?,"I think this should be `kotlin.test.assertEquals(x, ""abz"")`" 66,"@@ -0,0 +1,17 @@ +// TARGET_BACKEND: JVM","If I understood correctly, this test and the following one are just smoke tests to check, that there is no internal compiler error. Could you, please, turn them into assertion checks, like other ones from the same directory? ",// TARGET_BACKEND: JVM 67,"@@ -0,0 +1,17 @@ @@ -676,7 +676,7 @@ And add same test to box ones. ", + +fun foo(parent: Parent) = when(parent) { + is val child: Child1 -> parent.field1 + parent.field2 -+ !is val _ : Child2 -> 10",What's the difference between this syntax and just `!is Child2`?,Why is this aI don't think it should be a 10",What's the difference between this syntax and just `!is Child2`?,Why is this aI don't think it should be aI don't think it should be aI don't think it should be aI don't think it should be aI don't think it should be. 73,"@@ -0,0 +1,19 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. @@ -685,9 +685,9 @@ And add same test to box ones. ", + +package org.jetbrains.kotlin.utils; + -+data class KotlinReplError(val loc: Location?, val message: String = """", val severity: Severity) {",It seems that it could be substituted with ScriptDiagnostic. But the problem is that scripting-common is not accessible from compiler-cli. So it is one more reason to base completion on a new API and relocate it completely to a plugin similar to scripting plugin.,I think it would be better to use `Location` inst ++data class KotlinReplError(val loc: Location?, val message: String = """", val severity: Severity) {",It seems that it could be substituted with ScriptDiagnostic. But the problem is that scripting-common is not accessible from compiler-cli. So it is one more reason to base completion on a new API and relocate it completely to a plugin similar to scripting plugin.,I think it would be better to use `Location` instead of `val` here. 74,"@@ -0,0 +1,19 @@ -+// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","Why have you deleted the `kt-14258.kt` test from your earlier commits where a lot more cases were covered? If you think that test was getting too big to understand, you can always split it into several independent tests, e.g. `kt14258_1.kt`, `kt14258_2.kt`, ...","// Checks that methods 'access$getMy$p', 'access$" ++// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and","Why have you deleted the `kt-14258.kt` test from your earlier commits where a lot more cases were covered? If you think that test was getting too big to understand, you can always split it into several independent tests, e.g. `kt14258_1.kt`, `kt14258_2.kt`, ...","// Checks that methods 'access$getMy$p', 'access$getMy$cp', 'getMy' are not generated and" 75,"@@ -0,0 +1,19 @@ +// Checks that methods 'access$getMy$p', 'access$getMy$cp' and 'getMy' are not generated and +// that backed field 'my' is directly used through a 'getstatic' @@ -756,12 +756,12 @@ and test call from another package",This file is missing the license header. + +interface I { + companion object { -+ private val bar = ""Companion Field from I""","Let's make this a `var` in this test, call the setter and test that there are 2 `PUTSTATIC`s",I ++ private val bar = ""Companion Field from I""","Let's make this a `var` in this test, call the setter and test that there are 2 `PUTSTATIC`s",I'm not sure if this is the right way to test this. 82,"@@ -0,0 +1,202 @@ -+ /*",Nit: Formatting is off,W ++ /*",Nit: Formatting is off,Why is this needed? 83,"@@ -0,0 +1,203 @@ +/*","As far as I understand these functions are copies of functions at KotlinBuilder. Why not to reuse them at KotlinBuilder? I mean, these functions are in build common, so their originals from KotlinBuilder can be removed. -",W +",Why is this needed? 84,"@@ -0,0 +1,21 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -772,7 +772,7 @@ and test call from another package",This file is missing the license header. + +import kotlin.test.* + -+class StringsJsTest {",Better to move common tests into `libraries/stdlib/test/text/StringTest.kt` instead of duplicating them for each platform.,W ++class StringsJsTest {",Better to move common tests into `libraries/stdlib/test/text/StringTest.kt` instead of duplicating them for each platform.,Why do we need this file? 85,"@@ -0,0 +1,21 @@ +// Checks that methods 'access$getMy$p' and 'getMy' are not generated and +// that backed field 'my' is accessed through 'access$getMy$cp' @@ -791,14 +791,14 @@ and test call from another package",This file is missing the license header. +} + +// 1 GETSTATIC My.my -+// 1 PUTSTATIC My.my",Let's remove this line since it's irrelevant to what we're testing here,C ++// 1 PUTSTATIC My.my",Let's remove this line since it's irrelevant to what we're testing here,"Can you add a few more cases here, e.g. `// 1 GETSTATIC My.my`" 86,"@@ -0,0 +1,21 @@ +//method","You need to include the generated test class into your commit. ", 87,"@@ -0,0 +1,21 @@ +//method +void foo() {","Please try to ensure that testdata files are valid Java code. This file does not compile ('status' is undefined, the types do not match, etc.) -",P +",Please add a new line at the end of file. 88,"@@ -0,0 +1,21 @@ +//method +void foo() { @@ -817,10 +817,10 @@ and test call from another package",This file is missing the license header. + case ""fail"": + case ""busy"": + case ""error"":","Please add a test for the case when 'default' is in the middle of the list of other cases (`case ""error"": default: case ""busy:""`) and make sure that this test passes. -",I +",I don't think you need the ```````This is not a```This is not a``method` 89,"@@ -0,0 +1,22 @@ +// TARGET_BACKEND: JVM_IR","@ting-yuan Please add box tests for comparisonFalse/comparisonTrue -",/ +",// TARGET_BACKEND: JVM 90,"@@ -0,0 +1,22 @@ +package demo + @@ -838,7 +838,7 @@ and test call from another package",This file is missing the license header. + + fun getJoinedGreeting() : String? { + val joiner = Joiner.on("" and "").skipNulls();","why do we need guava in such simple example? we can replace ugly `Joiner.on("" and "").skipNulls().join(names)` with `names.filterNotNull().joinToString("" and "")`. We also don't need `com.google.common.primitives.Ints` at all... -",M +",Missing license header. 91,"@@ -0,0 +1,23 @@ +# When editing this file, update the following files as well: +# - META-INF/com.android.tools/r8-from-1.6.0/kotlin-reflect.pro @@ -848,15 +848,15 @@ and test call from another package",This file is missing the license header. +-keep class kotlin.Metadata { *; } + +# Keep implementations of service loaded interfaces -+# R8 will automatically handle these these in 1.6+",Nit: this file is not quite related to `R8`. :) (and below too),I ++# R8 will automatically handle these these in 1.6+",Nit: this file is not quite related to `R8`. :) (and below too),I don't think we need this file. 92,"@@ -0,0 +1,23 @@ -+// TARGET_BACKEND: JVM",Please name file with lower letter,/ ++// TARGET_BACKEND: JVM",Please name file with lower letter,// TARGET_BACKEND: JVM 93,"@@ -0,0 +1,24 @@ +// TARGET_BACKEND: JVM +// IGNORE_BACKEND: JVM +// JVM_TARGET: 1.8","`JVM_TARGET ` is redundant. We have special test suite that set target to 1.8 in codegenTarget8Jvm8Test. And run configuration for it in https://teamcity.jetbrains.com/buildConfiguration/Kotlin_KotlinPublic_CodegenTestsOnDifferentJDKs?branch= -`TARGET_BACKEND` could be also removed",I +`TARGET_BACKEND` could be also removed",I think this should be `1.8` 94,"@@ -0,0 +1,24 @@ +//FILE: test.kt +fun box() { @@ -888,10 +888,10 @@ I would prefer to have tests in next format: // TestKt.getB():15 // TestKt.box():4 // TestKt.box():5 -...",I +...",I don't think this file is needed. 95,"@@ -0,0 +1,25 @@ +// TARGET_BACKEND: JVM -+// The non-IR backend attempts to call a non-existent accessor in class Test.",@pyos Could you file an issue for this? With test name note,/ ++// The non-IR backend attempts to call a non-existent accessor in class Test.",@pyos Could you file an issue for this? With test name note,// The non-IR backend attempts to call a non-existent accessor in class Test 96,"@@ -0,0 +1,26 @@ +import kotlin.test.assertEquals + @@ -928,7 +928,7 @@ I would prefer to have tests in next format: + } + + fun getMyValue(): String { -+ // GETSTATIC for the companion object",What do you mean by this comment?,I'm not sure if this is the bes ++ // GETSTATIC for the companion object",What do you mean by this comment?,I'm not sure if this is the best way to test this. I think it would be better to have a test for this. 98,"@@ -0,0 +1,28 @@ +// TARGET_BACKEND: JVM","Looks like this test doesn't check what was tested previously in `testNoAssertionsForKotlinFromBinary`: that there are no assertions on calls to Kotlin declarations from dependencies. I'm afraid we'll have to keep that test until we have the necessary infrastructure to migrate it, namely multi-module bytecode text tests.",// TARGET_BACKEND: JVM 99,"@@ -0,0 +1,29 @@ @@ -944,7 +944,7 @@ I would prefer to have tests in next format: +import com.intellij.psi.PsiFile + +const val KOTLIN_WORKSHEET_EXTENSION: String = ""ws.kts"" -+const val KOTLIN_SCRIPT_EXTENSION = ""kts""",There is KotlinParserDefinition.STD_SCRIPT_SUFFIX property,I don't think we need this file ++const val KOTLIN_SCRIPT_EXTENSION = ""kts""",There is KotlinParserDefinition.STD_SCRIPT_SUFFIX property,I don't think we need this file. 100,"@@ -0,0 +1,29 @@ +package org.jetbrains.kotlin.library","@alexander-gorshenev Please add copyright to all new files (use ""Update Copyright"" IntelliJ action on the module)", 101,"@@ -0,0 +1,3 @@ @@ -992,7 +992,7 @@ I would prefer to have tests in next format: + +* Get rid of 23 hardwired physical function classes. One of the problems with them is that they should be effectively duplicated in reflection which means a lot of physical classes in the runtime.","""in the runtime"" -> ""at runtime""? Имеется в виду ""во время выполнения программы""? -",I'm not sure if this is the bes +",I'm not sure if this is the best way to do this. 112,"@@ -0,0 +1,304 @@ +# Function Types in Kotlin on JVM + @@ -1072,7 +1072,7 @@ I would prefer to have tests in next format: + return a[0]",This test can be removed, 122,"@@ -0,0 +1,4 @@ +// ERROR: Too many arguments for public final fun get(index: kotlin.Int): kotlin.Int defined in kotlin.Array","I can't seem to fix the source of this error, so I declared it. -",// ERROR: Too many arguments for public final fu +",// ERROR: Too many arguments for public final fun get(index: int): 123,"@@ -0,0 +1,4 @@ +// ERROR: Too many arguments for public final fun get(index: kotlin.Int): kotlin.Int defined in kotlin.Array +fun foo(a: Array, i: Int) { @@ -1092,7 +1092,7 @@ I would prefer to have tests in next format: +// DISABLE-ERRORS",Looks like switching to `RESTRICTED_RETENTION_FOR_EXPRESSION_ANNOTATION_WARNING` should fix this.,// PROBLEM: none 127,"@@ -0,0 +1,4 @@ +OUT: -+OK","This line is not printed in both CI when I run locally. The same happens in `helloAppSuspendMainInMultifileIr`. `helloAppSuspendMainIR`, however, runs OK when I run as a test, but does not print ""${args[0]$args[1]}"" when I compile the file and run it from CLI.",```````< ++OK","This line is not printed in both CI when I run locally. The same happens in `helloAppSuspendMainInMultifileIr`. `helloAppSuspendMainIR`, however, runs OK when I run as a test, but does not print ""${args[0]$args[1]}"" when I compile the file and run it from CLI.",`````````````Missing space before `:` 128,"@@ -0,0 +1,4 @@ +fun foo() {","This test does not test your change yet. You should add comment above (to both this file and after file): @@ -1195,11 +1195,11 @@ Array(1) { run { run { runReturning { return@Array result } }}}[0] 145,"@@ -0,0 +1,5 @@ + + -+This inspection reports explicit calls of Collection constructor calls which can be replaced by function calls from the stdlib.","It's an intention, not an inspection. ",I don't think this is n ++This inspection reports explicit calls of Collection constructor calls which can be replaced by function calls from the stdlib.","It's an intention, not an inspection. ",I don't think this is needed. 146,"@@ -0,0 +1,5 @@ + + -+This inspection reports suspicious collection reassignment by augmented assignment.",I think we need more detailed explanation here.,I think this should be ++This inspection reports suspicious collection reassignment by augmented assignment.",I think we need more detailed explanation here.,I think this should be `` instead of ``. 147,"@@ -0,0 +1,5 @@ + + @@ -1211,7 +1211,7 @@ Array(1) { run { run { runReturning { return@Array result } }}}[0] 149,"@@ -0,0 +1,5 @@ + + -+This inspection reports unused unary operators.",I'd prefer `This inspection reports unary operators that are not used.`,I think this should be ++This inspection reports unused unary operators.",I'd prefer `This inspection reports unary operators that are not used.`,I think this should be `` 150,"@@ -0,0 +1,5 @@ + + @@ -1220,22 +1220,22 @@ Can you add some more context, like `This intention adds a @Throws annota 151,"@@ -0,0 +1,5 @@ + + -+This intention converts an array parameter to a vararg parameter.","Here and below, please select `vararg` with bold.",I don't think this is n ++This intention converts an array parameter to a vararg parameter.","Here and below, please select `vararg` with bold.",I don't think this is necessary. 152,"@@ -0,0 +1,5 @@ + + +This intention detects 'Math.max' calls that can be safely replaced with 'coerceAtLeast'","inspection -",I think this should be +",I think this should be ``. 153,"@@ -0,0 +1,5 @@ + + +This intention detects 'Math.min' calls that can be safely replaced with 'coerceAtMost'","inspection -",I think this should be +",I think this should be `` instead of `` 154,"@@ -0,0 +1,5 @@ + + +This intention detects an empty class body.","removes -",I don't think we need t +",I don't think we need this. 155,"@@ -0,0 +1,5 @@ + + @@ -1249,16 +1249,16 @@ Can you add some more context, like `This intention adds a @Throws annota 157,"@@ -0,0 +1,5 @@ + + -+This intention removes labeled return to last expression in a lambda.",from last expression,I don't think this is n ++This intention removes labeled return to last expression in a lambda.",from last expression,I don't think this is needed. 158,"@@ -0,0 +1,5 @@ + + -+This intention replaces count() function calls with size.",It should be a description of the inspection. Example: [ReplaceCallWithBinaryOperator](https://github.com/JetBrains/kotlin/blob/master/idea/resources/inspectionDescriptions/ReplaceCallWithBinaryOperator.html) ,I'm not sure if this is ++This intention replaces count() function calls with size.",It should be a description of the inspection. Example: [ReplaceCallWithBinaryOperator](https://github.com/JetBrains/kotlin/blob/master/idea/resources/inspectionDescriptions/ReplaceCallWithBinaryOperator.html) ,I'm not sure if this is the best way to do this. 159,"@@ -0,0 +1,5 @@ + + +This intention take a qualified call to any function ""get"" which has some arguments and converts it to square brackets.","""takes"" -",I don't think this is n +",I don't think this is necessary. 160,"@@ -0,0 +1,5 @@ +fun box(): String { + val sub = Box(-1) @@ -1270,7 +1270,7 @@ Can you add some more context, like `This intention adds a @Throws annota +} else { + baz() +}","The branches aren't swapped in the after-template -",I don't think you need the ` +",I don't think you need the ````````This is not a``This is not a``This is not aThis is not aThis is aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is not aThis is aThis is not aThis is not aThis is aThis is not aThis is aThis is not aThis is aThis is not aThis is aThis is not aThis is aThis is not aThis is not aThis is aThis is not aThis is aThis is not aThis is a 162,"@@ -0,0 +1,5 @@ +package org.junit + @@ -1341,7 +1341,7 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa +// WITH_RUNTIME +fun test(list: List) { + list.forEach { item -> -+ /* aaa */ println(item); println(item) /* bbb */",Let's move every statement inside lambda to its own line in such cases as we already have multi-line lambda,I think you can use `forEach` her ++ /* aaa */ println(item); println(item) /* bbb */",Let's move every statement inside lambda to its own line in such cases as we already have multi-line lambda,I think you can use `forEach` here 174,"@@ -0,0 +1,6 @@ +//ERROR: Unresolved reference: SortedMap +fun a(b: SortedMap) { @@ -1361,20 +1361,20 @@ I don't see any $assertionsDisabled neither in companion nor in Bar. If understa 179,"@@ -0,0 +1,65 @@ +// WITH_RUNTIME",I'd name this test file `javaPrimitiveType.kt`, 180,"@@ -0,0 +1,66 @@ -+/*",Please fix inspections and reformat this file and `KClassJavaPrimitiveTypeProperty.kt`,W ++/*",Please fix inspections and reformat this file and `KClassJavaPrimitiveTypeProperty.kt`,Why is this needed? 181,"@@ -0,0 +1,7 @@ +// ""Replace initializer with getter"" ""true"" + +fun String.foo() = ""bar"" + +interface A { -+ val name = ""The quick brown fox jumps over the lazy dog"".foo()",Neither the existing intention action nor your suggested quickfix handle `var` properties correctly. They should create a stub for the setter with no implementation.,I ++ val name = ""The quick brown fox jumps over the lazy dog"".foo()",Neither the existing intention action nor your suggested quickfix handle `var` properties correctly. They should create a stub for the setter with no implementation.,I think this should be `` 182,"@@ -0,0 +1,7 @@ +// COMPILER_ARGUMENTS: -XXLanguage:+MixedNamedArgumentsInTheirOwnPosition + +fun foo(name1: Int, name2: Int, name3: Int) {}","There are also test `namedArgumentsBefore.kt` which might fail now, because by default `MixedNamedArgumentsInTheirOwnPosition` will be enabled in kotlin 1.4 -Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosition` is disabled in this test (by adding `// COMPILER_ARGUMENTS: -XXLanguage:-MixedNamedArgumentsInTheirOwnPosition`), and add additional test with `MixedNamedArgumentsInTheirOwnPosition` enabled (in which the intention should now work for this case)?",/ +Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosition` is disabled in this test (by adding `// COMPILER_ARGUMENTS: -XXLanguage:-MixedNamedArgumentsInTheirOwnPosition`), and add additional test with `MixedNamedArgumentsInTheirOwnPosition` enabled (in which the intention should now work for this case)?",// COMPILER_ARGUMENTS: 183,"@@ -0,0 +1,7 @@ +// COMPILER_ARGUMENTS: -XXLanguage:+MixedNamedArgumentsInTheirOwnPosition + @@ -1383,7 +1383,7 @@ Can you please explicitly specify there that `MixedNamedArgumentsInTheirOwnPosit +fun usage() {","It seems like there are missing test when `MixedNamedArgumentsInTheirOwnPosition` is enabled, but the arguments are not in their own positions, and the intention should be disabled With your code, this case will look like `foo(1, name3 = 3, name2 = 2)` -",I +",I think you can remove this line. 184,"@@ -0,0 +1,7 @@ +// ERROR: Unresolved reference: listOf +fun a() { @@ -1412,21 +1412,21 @@ If having all lowerings do the right thing on both lowered and unlowered inputs + println(""test2"") + } +}","Typo in file name. -",W +",Why is this needed? 187,"@@ -0,0 +1,7 @@ +// LANGUAGE_VERSION: 1.2 +// PROBLEM: none + +annotation class Some(vararg val strings: String) + -+@Some(*arrayOf(""alpha"", ""beta"", ""omega""))","This can be converted into `@Some(*[""alpha"", ""beta"", ""omega""])`",T ++@Some(*arrayOf(""alpha"", ""beta"", ""omega""))","This can be converted into `@Some(*[""alpha"", ""beta"", ""omega""])`","This should be `arrayOf(String, ...)`" 188,"@@ -0,0 +1,7 @@ +// PROBLEM: none +// ERROR: Assignment operators ambiguity:
public operator fun Collection.plus(element: Int): List defined in kotlin.collections
@InlineOnly public inline operator fun MutableCollection.plusAssign(element: Int): Unit defined in kotlin.collections +// WITH_RUNTIME +fun test() { + var list = mutableListOf(1) -+ list += 2",Just as an idea: why don't fix it by changing `var` to `val`?,I ++ list += 2",Just as an idea: why don't fix it by changing `var` to `val`?,I think it would be better to use `var list = mutableListOf(1)` 189,"@@ -0,0 +1,7 @@ +// WITH_RUNTIME + @@ -1434,7 +1434,7 @@ If having all lowerings do the right thing on both lowered and unlowered inputs +fun main(args: Array) { + val a1 = a + if (a1 != null) a1.length + 1 -+}","I'd add at least one test with regular (non-redundant) `let` and more tests with redundant `let`, e.g. `a?.let { foo(it) }` and may be something else.",I ++}","I'd add at least one test with regular (non-redundant) `let` and more tests with redundant `let`, e.g. `a?.let { foo(it) }` and may be something else.",I think this should be `val a1 = a.length + 1` 190,"@@ -0,0 +1,7 @@ +class A (val result: T)",...and test is redundant. there is more correct innerGenericConstuctor.kt with inners, 191,"@@ -0,0 +1,7 @@ @@ -1448,13 +1448,13 @@ If having all lowerings do the right thing on both lowered and unlowered inputs +enum class Foo(n: Int) { + A(1, 2), + B(3), -+ C(),","What if we write `C(3, 4)` here? Suppose this `C(3, 4)` will be changed to `C(3, 2)`, which is not correct.",I ++ C(),","What if we write `C(3, 4)` here? Suppose this `C(3, 4)` will be changed to `C(3, 2)`, which is not correct.",I think we can remove this file. 194,"@@ -0,0 +1,8 @@ +// ""Create method 'get' from usage"" ""true"" +import java.util.ArrayList + +class Foo {","Isn't it better to remove type parameter of class to avoid confusing when reading test? Or is it intentional? -",T +",This file is missing the license header. 195,"@@ -0,0 +1,8 @@ +// PROBLEM: none +class C { @@ -1466,13 +1466,13 @@ If having all lowerings do the right thing on both lowered and unlowered inputs +fun test() = C.Companion::foo","Please check also the following case: ``` val obj = C.Companion // PROBLEM: none -```",I +```",I don't think this is needed. 196,"@@ -0,0 +1,8 @@ +// PROBLEM: none +interface F + +val f = object : F { -+ fun equals(other: F?): Boolean {",Same as above.,W ++ fun equals(other: F?): Boolean {",Same as above.,Why is this needed? 197,"@@ -0,0 +1,8 @@ +// WITH_RUNTIME + @@ -1489,14 +1489,14 @@ val obj = C.Companion // PROBLEM: none c.mapNotNull label@{ return@label """" } -```",W +```",Why do we need this? 199,"@@ -0,0 +1,8 @@ +// WITH_RUNTIME +import java.io.File + +fun main(args: Array) { + File(""hello-world.txt"").bufferedReader().use { reader -> -+ reader.close()",I'm not sure this a valid fix in general case. This is not a refactoring and what if `close()` throws an exception on the second execution?,I ++ reader.close()",I'm not sure this a valid fix in general case. This is not a refactoring and what if `close()` throws an exception on the second execution?,I think you can use `File.createTempFile` here 200,"@@ -0,0 +1,8 @@ +enum class E { +FOO @@ -1507,7 +1507,7 @@ c.mapNotNull label@{ +public fun order() : Int { return 0 } +} \ No newline at end of file","The same: code formatting is wrong -",P +",Please add a newline at the end of file. 201,"@@ -0,0 +1,8 @@ +fun foo() { + if (true) { @@ -1522,7 +1522,7 @@ c.mapNotNull label@{ + */ +@Retention(AnnotationRetention.RUNTIME) +@Target(AnnotationTarget.FUNCTION) -+annotation class Generated","Why is this annotation is a part of `runtime.jvm`? And if it supposed to be Jvm only, according to existing naming convention it should be `JvmGenerated` instead",W ++annotation class Generated","Why is this annotation is a part of `runtime.jvm`? And if it supposed to be Jvm only, according to existing naming convention it should be `JvmGenerated` instead",Why is this needed? 204,"@@ -0,0 +1,8 @@ +var status: String = ""fail""","I beleive it would be good to add comment explaining why ""status"" here is top-level property instead of local variable. ", @@ -1533,7 +1533,7 @@ c.mapNotNull label@{ +package org.jetbrains.kotlin.idea.debugger.sequence.lib + +/** -+ * @author Vitaliy.Bibaev",We do not allow `@author` comments (`CodeConformanceTest#testNoBadSubstringsInProjectCode`).,W ++ * @author Vitaliy.Bibaev",We do not allow `@author` comments (`CodeConformanceTest#testNoBadSubstringsInProjectCode`).,Why do we need this file? 207,"@@ -0,0 +1,9 @@ +// Copyright 2000-2017 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package org.jetbrains.kotlin.idea.debugger.sequence.lib @@ -1542,7 +1542,7 @@ c.mapNotNull label@{ + * @author Vitaliy.Bibaev + */ +object LibraryUtil { -+ const val KOTLIN_LANGUAGE_ID = ""kotlin""",What's the purpose of this? You can use `KotlinLanguage.NAME` in Kotlin project.,I ++ const val KOTLIN_LANGUAGE_ID = ""kotlin""",What's the purpose of this? You can use `KotlinLanguage.NAME` in Kotlin project.,I don't think we need this file. 208,"@@ -0,0 +1,9 @@ +// FILE: test.kt +class AtomicInt(var value: Int) @@ -1598,7 +1598,7 @@ fun test() { } val a = 33 } -```",I think this should be ```````````` 216,"@@ -0,0 +1,9 @@ +// WITH_RUNTIME +// IS_APPLICABLE: FALSE @@ -1609,7 +1609,7 @@ fun test() { + + } +}","The test is fine, the naming is not: _overridden_ function is a function declaration in a subclass with the same signature as in the superclass. There are no overridden functions here, `withIndices` is just an extension function -",I think this should be `String.withIn +",I think this should be `String.withIndices()` 217,"@@ -0,0 +1,9 @@ +// WITH_RUNTIME +class FooException : Exception() @@ -1625,14 +1625,14 @@ fun test() { + items.forEach { item -> } + items.forEach { doSomething(it) } + items.forEach { item -> doSomething(item) } -+}","I'd add some tests with nested `forEach`, to make sure we handle correctly situations with multiple `it`.",This should be `items.forEach { doSom ++}","I'd add some tests with nested `forEach`, to make sure we handle correctly situations with multiple `it`.",This should be `items.forEach { doSomething(it) }` 219,"@@ -0,0 +1,9 @@ +interface B { + fun c() +} + + -+object A : B {",This should be `object a`,I think this should be ```< ++object A : B {",This should be `object a`,I think this should be `````````object A : B` 220,"@@ -0,0 +1,90 @@ +/* + * Copyright 2010-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license @@ -1707,7 +1707,7 @@ data class SourceSetImpl( - + ","Changes like this in meta-information should be carefully reviewed before committing, because they can easily break compilation or workflow for other developers. -",I think this should be `/artif +",I think this should be `/artifacts/Kotlin/kotlinc/kotlin-runtime.jar` 226,"@@ -1,11 +1,20 @@ -// IGNORE_BACKEND: JVM_IR -fun box() : String { @@ -1769,23 +1769,23 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i +fun anyany(a: kotlin.Any, ignore: java.util.List) = a",Unclear. Why should you have `kotlin.` here and how it works without it?,Why this change? 239,"@@ -1,4 +1,4 @@ -// ""Change type of overriden property 'A.x' to '(Int) -> Int'"" ""false"" -+// ""Change type of overridden property 'A.x' to '(Int) -> Int'"" ""false""",Isn't this a part of our testdata markup? @mglukhikh ,"// ""Change type of overridden " ++// ""Change type of overridden property 'A.x' to '(Int) -> Int'"" ""false""",Isn't this a part of our testdata markup? @mglukhikh ,"// ""Change type of overridden property 'A.x' to 'Int'""" 240,"@@ -1,4 +1,4 @@ -// WITH_RUNTIME +// RUNTIME_WITH_FULL_JDK",Don't understand why it's needed,// WITH_RUNTIME 241,"@@ -1,4 +1,5 @@ // !LANGUAGE: +InlineClasses -+// IGNORE_BACKEND: JVM_IR",What happens here?,// !LANGUAGE: + ++// IGNORE_BACKEND: JVM_IR",What happens here?,// !LANGUAGE: +InlineClasses 242,"@@ -1,4 +1,5 @@ // IGNORE_BACKEND_FIR: JVM_IR +// IGNORE_BACKEND: JVM_IR","This test should not have passed. It is checking that no delegate field is created for interface delegation. It is doing so based on the name of the field, and therefore it now correctly fails because we do not have that optimization.", 243,"@@ -1,4 +1,5 @@ package org.junit -+@Deprecated(""Use 'Test' from kotlin.test package"", replaceWith = ReplaceWith(""Test"", imports = ""kotlin.test.Test""))",👍 ,import org.juni ++@Deprecated(""Use 'Test' from kotlin.test package"", replaceWith = ReplaceWith(""Test"", imports = ""kotlin.test.Test""))",👍 ,import org.junit.Test; 244,"@@ -1,4 +1,7 @@ // KJS_WITH_FULL_RUNTIME -+// TODO: muted automatically, investigate should it be ran for JVM_IR or not","Do you understand why these two tests are now failing. If so, can you replace this auto mute comment with an actual comment explaining what is going on?",// KJS_WITH_FUL ++// TODO: muted automatically, investigate should it be ran for JVM_IR or not","Do you understand why these two tests are now failing. If so, can you replace this auto mute comment with an actual comment explaining what is going on?",// KJS_WITH_FULL_RUNTIME 245,"@@ -1,5 +1,3 @@ -// IGNORE_BACKEND: JVM_IR -// WITH_RUNTIME","Please return `WITH_RUNTIME` in these tests: @@ -1793,7 +1793,7 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i * `boxAgainstJava/sam/samConstructorGenericSignature.kt` * `boxAgainstJava/sam/adapters/inheritedOverriddenAdapter.kt` -(see cb7727d51a)",Why is this rem +(see cb7727d51a)",Why is this removed? 246,"@@ -1,5 +1,4 @@ // FILE: 1.kt -// IGNORE_BACKEND: JVM_IR @@ -1803,18 +1803,18 @@ It seems that it can affect different Kotlin builds, so I don't think, that it i - @Supress(""unused"") + @Suppress(""unused"")","I'm not sure what it can affect. I mean, initially it was a compile-time error; changing that may have some unintended consequences. -@semoro ",@SuppressWarnin +@semoro ","@SuppressWarnings(""unused"")" 248,"@@ -1,5 +1,6 @@ -// FILE: 1.kt // IGNORE_BACKEND: JVM_IR +// FILE: 1.kt -+",New line causes smap data shift: https://teamcity.jetbrains.com/viewLog.html?buildId=2450360&buildTypeId=Kotlin_dev_CompilerWithTests,Why did you rem ++",New line causes smap data shift: https://teamcity.jetbrains.com/viewLog.html?buildId=2450360&buildTypeId=Kotlin_dev_CompilerWithTests,Why did you remove this? 249,"@@ -1,6 +1,10 @@ // ""Change 'foo' function return type to '([ERROR : NoSuchType]) -> Int'"" ""false"" +// ACTION: Disable 'Make Types Implicit In Lambda' +// ACTION: Edit intention settings +// ACTION: Make types implicit in lambda","Note that this test is now failing again due to obvious reasons :) -",I don't think t +",I don't think this is the right place for this action. I think it should be in a separate file. 250,"@@ -1,6 +1,6 @@ // !LANGUAGE: +AllowContractsForCustomFunctions +UseCallsInPlaceEffect +ReadDeserializedContracts // !USE_EXPERIMENTAL: kotlin.contracts.ExperimentalContracts @@ -1828,19 +1828,19 @@ If so, the test should probably just be removed.",: companion object { - private var r: Int = 1; + private var r: Int = 1 -+ // Custom getter is needed, otherwise no need to generate getTest",I think this test is no longer necessary and can be safely deleted instead,Why is this nee ++ // Custom getter is needed, otherwise no need to generate getTest",I think this test is no longer necessary and can be safely deleted instead,Why is this needed? 253,"@@ -1,7 +1,6 @@ // Even before any IR lowerings, the type of `when` is determined to be // Unit even though the outer `if` still returns `Int?`. This results // in a ClassCastException when that Unit is converted into a Number. --// IGNORE_BACKEND: JVM_IR",The comment above is probably also not needed after this change?,Why is this rem +-// IGNORE_BACKEND: JVM_IR",The comment above is probably also not needed after this change?,Why is this removed? 254,"@@ -1,7 +1,6 @@ fun foo() { - val a: kotlin.test.Asserter? - if () { - a = null + val a = if () {","It's not OK that you're losing the variable type here. -",Why did you cha +",Why did you change this? 255,"@@ -1,7 +1,7 @@ fun foo() { - Loop@ while (true) { @@ -1856,7 +1856,7 @@ If so, the test should probably just be removed.",: - println(Integer.TYPE) - println(java.lang.Double.TYPE) + println(Unit::class.javaPrimitiveType)","This doesn't actually work: `Unit` has no `javaPrimitiveType`. For `void.class`, the previous variant of the code should be used. -",Why did you change thi +",Why did you change this? 258,"@@ -1,8 +1,9 @@ -// IGNORE_BACKEND: JVM_IR // IGNORE_BACKEND: JS_IR @@ -1877,12 +1877,12 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko /** + * Builds new string by populating newly created [StringBuilder] initialized with the given capacity using provided [builderAction] and then converting it to [String]. + */","Will fix formatting upon rebase. -",* Builds new [StringBu +",* Builds new [StringBuilder] initialized with the given capacity using provided [builderAction] and then converting it to [String]. 261,"@@ -10,6 +10,7 @@ import org.gradle.api.Named import org.gradle.api.file.SourceDirectorySet interface KotlinSourceSet : Named, HasKotlinDependencies { -+ val id: Long","In a single project, source set names are unique. Using the `name` as a key would be sufficient. Why a separate `id` is needed?",I don't think we need ++ val id: Long","In a single project, source set names are unique. Using the `name` as a key would be sufficient. Why a separate `id` is needed?",I don't think we need the `val` here 262,"@@ -10,6 +10,9 @@ package kotlin.system * Executes the given [block] and returns elapsed time in milliseconds. */ @@ -1901,20 +1901,20 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko } - for ((typeVariable, _) in allTypeParameterBounds) { -+ for (typeVariable in allTypeParameterBounds.keys) {",nit: `for (typeVariable in typeVariables)`. Probably combine with the above.,I don't think this cha ++ for (typeVariable in allTypeParameterBounds.keys) {",nit: `for (typeVariable in typeVariables)`. Probably combine with the above.,I don't think this change is correct. `allTypeParameterBounds` is a dict. 265,"@@ -101,7 +106,29 @@ class KotlinUFunctionCallExpression( } override val receiver: UExpression? - get() = (uastParent as? UQualifiedReferenceExpression)?.takeIf { it.selector == this }?.receiver + get() { -+ (uastParent as? UQualifiedReferenceExpression)?.let { return it.takeIf { it.selector == this }?.receiver }",Looks too complicated. Maybe replace a `let` with `if`?,Why is this change nee ++ (uastParent as? UQualifiedReferenceExpression)?.let { return it.takeIf { it.selector == this }?.receiver }",Looks too complicated. Maybe replace a `let` with `if`?,Why is this change needed? 266,"@@ -102,12 +102,12 @@ internal sealed class ForLoopHeader( // // (use `<` if last is exclusive) // (step > 0 && inductionVar <= last) || (step < 0 || last <= inductionVar) val stepKotlinType = progressionType.stepType(builtIns).toKotlinType() - val zero = if (progressionType == ProgressionType.LONG_PROGRESSION) irLong(0) else irInt(0) -+ val isLong = progressionType == ProgressionType.LONG_PROGRESSION;",Semicolon may be considered a nostalgic reminiscence here.,val isLong = progressi ++ val isLong = progressionType == ProgressionType.LONG_PROGRESSION;",Semicolon may be considered a nostalgic reminiscence here.,val isLong = progressionType.isLong(); 267,"@@ -102,6 +101,22 @@ class JvmBuiltinOptimizationLowering(val context: JvmBackendContext) : FileLower expression.branches.removeIf() { it.condition.isFalseConst() && isCompilerGenerated @@ -1931,17 +1931,17 @@ Could you tell me why there is doubled `kotlin` and `test` packages in path: `ko return when (right) { is KtNameReferenceExpression -> right.text != parameterName is KtDotQualifiedExpression -> !right.hasLambdaExpression() && !right.nameUsed(parameterName) -+ is KtBinaryExpression -> right.operationToken == KtTokens.RANGE",Don't quite understand why only `RANGE` is allowed here. Please consider other binary operators. It seems to me (may be I miss something) that it should be allowed for most of them.,is KtBinaryExpression ++ is KtBinaryExpression -> right.operationToken == KtTokens.RANGE",Don't quite understand why only `RANGE` is allowed here. Please consider other binary operators. It seems to me (may be I miss something) that it should be allowed for most of them.,is KtBinaryExpression -> right.operationToken == KtTokens.RANGE 270,"@@ -102,7 +103,7 @@ abstract class AbstractKotlinCompilation( // To configure a task that may have not yet been created at this point, use 'withType-matching-all`: .withType(AbstractKotlinCompile::class.java) - .matching { it.name == compileKotlinTaskName }","we can use `named(String, Class)` ",.withType(AbstractKotl + .matching { it.name == compileKotlinTaskName }","we can use `named(String, Class)` ",.withType(AbstractKotlinCompile::class) 271,"@@ -102,7 +103,7 @@ private class VarargLowering(val context: JvmBackendContext) : FileLoweringPass, context.createJvmIrBuilder(currentScope!!.scope.scopeOwnerSymbol, startOffset, endOffset) private val IrFunctionSymbol.isArrayOf: Boolean - get() = this == context.ir.symbols.arrayOf || owner.isPrimitiveArrayOf -+ get() = this == context.ir.symbols.arrayOf || owner.isArrayOf","Looks like it is already checking is this is `kotlin.Array`, so it is no need to check it by name",Why is this change nee ++ get() = this == context.ir.symbols.arrayOf || owner.isArrayOf","Looks like it is already checking is this is `kotlin.Array`, so it is no need to check it by name",Why is this change needed? 272,"@@ -102,7 +105,12 @@ class DefaultArgumentsConversion(context: NewJ2kConverterContext) : RecursiveApp return JKFieldAccessExpression(newSymbol) } @@ -1960,7 +1960,7 @@ val nextKeyword = when { keywordToken == SUSPEND_KEYWORD && position.getStrictParentOfType() != null -> null else -> COMPOUND_KEYWORDS[keywordToken] } -```",Why is this change needed +```",Why is this change needed? 274,"@@ -104,26 +100,33 @@ class BlockInfo private constructor(val parent: BlockInfo?) { class VariableInfo(val declaration: IrVariable, val index: Int, val type: Type, val startLabel: Label) @@ -2000,14 +2000,14 @@ private const val SPACES = "" "" ${SPACES + SPACES} """""" + SPACES + ).trimIndent() -```",computeStringTrimPhase th +```",computeStringTrimPhase then 277,"@@ -107,6 +107,7 @@ val jvmPhases = namedIrFilePhase( jvmTypeOperatorLoweringPhase then foldConstantLoweringPhase then flattenStringConcatenationPhase then + computeStringTrimPhase then","I wanted to add this for JS but I don't know how to test it. Would appreciate a pointer so I can follow-up. -And for native I assume I wait until they update to a version of compiler that contains this phase and then I can update their list?",computeStringTrimPhase th +And for native I assume I wait until they update to a version of compiler that contains this phase and then I can update their list?",computeStringTrimPhase then 278,"@@ -107,7 +108,9 @@ open class InsertImplicitCasts( override fun visitReturn(expression: IrReturn): IrExpression = @@ -2024,14 +2024,14 @@ class C() { ``` The artificial example above is considered well-formed, and contains smart cast to Unit. However. this smart cast will not be represented in IR. -Actually should cast constructor return value to `kotlin.Unit`.",I don't think this is the +Actually should cast constructor return value to `kotlin.Unit`.",I don't think this is the right place to do this. I think the right place to do this is in the `visitReturn` method. 279,"@@ -108,10 +109,11 @@ fun KtExpression.hasSuspendCalls(bindingContext: BindingContext = analyze(BodyRe } } else -> { - val resolvedCall = getResolvedCall(bindingContext) - if ((resolvedCall?.resultingDescriptor as? FunctionDescriptor)?.isSuspend == true) true -+ val target = (this as? KtParenthesizedExpression)?.getStrictParentOfType() ?: this","This is a very hacky way to solve the problem. The root of problem here is the fact that the considered call expression is not a valid candidate (see check at line 88), so I believe this place should be fixed instead.",`getStrictParentOfType() ?: this","This is a very hacky way to solve the problem. The root of problem here is the fact that the considered call expression is not a valid candidate (see check at line 88), so I believe this place should be fixed instead.",`getStrictParentOfType()` returns `null` if there is no parent. 280,"@@ -108,15 +115,18 @@ internal class InterfaceLowering(val context: JvmBackendContext) : IrElementTran Visibilities.isPrivate(function.visibility)","It's true for private non-JvmDefault, private JvmDefault should be keeped in interface", => 281,"@@ -109,11 +111,16 @@ class ErasedInlineClassBodyCodegen( @@ -2062,7 +2062,7 @@ Actually should cast constructor return value to `kotlin.Unit`.",I don't think t }.apply { - get().initialize(bindingTrace, get()) + if (useJavac) -+ get().initialize(bindingTrace, get())",I'd write here `JavacBasedClassFinder`. The current name is too close to just `JavaClassFinder`.,Why do we need this if st ++ get().initialize(bindingTrace, get())",I'd write here `JavacBasedClassFinder`. The current name is too close to just `JavaClassFinder`.,Why do we need this if statement? 285,"@@ -111,7 +114,9 @@ interface ReplEvalAction { sealed class ReplEvalResult : Serializable { @@ -2074,13 +2074,13 @@ Actually should cast constructor return value to `kotlin.Unit`.",I don't think t +irReturn( irCall(defaultImplFun.symbol, irFunction.returnType).apply { + interfaceImplementation.parentAsClass.typeParameters.forEachIndexed { index, irTypeParameter -> -+ //TODO: See Note [Interface Parameters]","Minor, but could you extract a simple method that would return `context.irBuiltIns.anyNType` and put the comment there instead, so that we have only one TODO instead of three?",This should be `interface ++ //TODO: See Note [Interface Parameters]","Minor, but could you extract a simple method that would return `context.irBuiltIns.anyNType` and put the comment there instead, so that we have only one TODO instead of three?","This should be `interfaceImplementation.parentAsClass.typeParameters.forEachIndexed(index, irTypeParameter ->`" 287,"@@ -112,7 +133,7 @@ private fun collectionsSort(list: MutableList, comparator: Comparator ""Property"" } @@ -2092,7 +2092,7 @@ Actually should cast constructor return value to `kotlin.Unit`.",I don't think t class Private : ChangeVisibilityModifierIntention(KtTokens.PRIVATE_KEYWORD), HighPriorityAction { override fun applicabilityRange(element: KtDeclaration): TextRange? { if (isAnnotationClassPrimaryConstructor(element)) return null -+ if (element is KtProperty && element.hasJvmFieldAnnotation()) return null","I'd say it's not the best way to fix it. It's more correct to have all necessary checks inside `canBePrivate` and `canBeInternal` (which does not yet exist). Take into account the fact that both `ChangeVisibilityFix` and `ChangeVisibilityModifierIntention` use `canBePrivate` (and should probably share the same logic). Also I'd recommend to consider content of `canBePrivate` inside `MemberVisibilityCanBePrivateInspection` (private fun). I suspect that some checks from there (not all) could be used inside global `canBePrivate` (that can also fix a bunch of minor bugs, not reported yet).",if (element.hasJvmFieldAnno ++ if (element is KtProperty && element.hasJvmFieldAnnotation()) return null","I'd say it's not the best way to fix it. It's more correct to have all necessary checks inside `canBePrivate` and `canBeInternal` (which does not yet exist). Take into account the fact that both `ChangeVisibilityFix` and `ChangeVisibilityModifierIntention` use `canBePrivate` (and should probably share the same logic). Also I'd recommend to consider content of `canBePrivate` inside `MemberVisibilityCanBePrivateInspection` (private fun). I suspect that some checks from there (not all) could be used inside global `canBePrivate` (that can also fix a bunch of minor bugs, not reported yet).",if (element.hasJvmFieldAnnotation()) return null; 290,"@@ -115,7 +115,21 @@ class CodeConverter( if (needConversion(actualType, expectedType)) { val expectedTypeStr = expectedType.canonicalText @@ -2113,7 +2113,7 @@ Because it looks like something similar required here #1200",Why is this needed? } } + -+ public static final class LineAndColumnRange {",I think that this change as well as the appropriate `getLineAndColumnRange*` helpers will fit into the separate commit with `CompilerMessageLocation` as well - they seem both have the same reason behind.,public static String toStri ++ public static final class LineAndColumnRange {",I think that this change as well as the appropriate `getLineAndColumnRange*` helpers will fit into the separate commit with `CompilerMessageLocation` as well - they seem both have the same reason behind.,"public static String toString(int line, int column) {" 293,"@@ -1168,3 +1122,96 @@ fun CompileTimeConstant<*>.isStandaloneOnlyConstant(): Boolean { else -> return false } @@ -2133,7 +2133,7 @@ Because it looks like something similar required here #1200",Why is this needed? + } +} + -+internal fun typeStrToCompileTimeType(str: String) = when (str) {",private,`isIntegerType(value) -> (v ++internal fun typeStrToCompileTimeType(str: String) = when (str) {",private,`isIntegerType(value) -> (value as Number).toLong() == 0L` 295,"@@ -118,6 +134,18 @@ internal class ProgressionHeaderInfo( } constLimitAsLong == lastValueAsLong @@ -2171,12 +2171,12 @@ fun box(): String { .replace(""\n"", LineSeparator.getSystemLineSeparator().getSeparatorString()); } -- private static final List WHITELISTED_FOR_JVM_IR_BACKEND = Collections.singletonList(""overflowZeroDownToMaxValue.kt"");",How did these changes break the test for JVM IR?,private static final String +- private static final List WHITELISTED_FOR_JVM_IR_BACKEND = Collections.singletonList(""overflowZeroDownToMaxValue.kt"");",How did these changes break the test for JVM IR?,"private static final String WHITELISTED_FOR_JVM_IR_BACKEND = ""overflowZeroDownToMaxValue.kt"";" 298,"@@ -1199,6 +1199,29 @@ class KotlinTypeMapper @JvmOverloads constructor( ?: computeInternalName(classDescriptor, typeMappingConfiguration, isIrBackend) } -+ private fun getDefaultDescriptor(",Could you keep old position of method and just pass neccesery parameters to it: actually there would be new TypeMapper for IR so all IR related logic would be removed from current one,private fun getDefaultDescr ++ private fun getDefaultDescriptor(",Could you keep old position of method and just pass neccesery parameters to it: actually there would be new TypeMapper for IR so all IR related logic would be removed from current one,private fun getDefaultDescriptor(ClassDescriptor classDescriptor) { 299,"@@ -12,3 +12,11 @@ class TestInitValInInitBlock { } } @@ -2184,7 +2184,7 @@ fun box(): String { +class TestInitValInLambdaCalledOnce { + val x: Int + init { -+ 1.run {",`init { 1.run { } }` causes fir to fail with an error at `ConeClassifierLookupTag.toSymbol` because the tag is of unexpected type `ConeTypeVariableTypeConstructor`. Is there a way to mute this test for fir?,I think this should be `Tes ++ 1.run {",`init { 1.run { } }` causes fir to fail with an error at `ConeClassifierLookupTag.toSymbol` because the tag is of unexpected type `ConeTypeVariableTypeConstructor`. Is there a way to mute this test for fir?,I think this should be `TestInitValInLambdaCalledOnce` 300,"@@ -12,6 +11,16 @@ fun test() { // 0 getFirst // 0 getLast @@ -2193,7 +2193,7 @@ fun box(): String { +// JVM IR uses if + do-while. + +// JVM_TEMPLATES - // 0 IF_ICMPGT",These two lines (`// 0 IF_...`) can be removed; they are made redundant by the `// 1 IF`,I don't think this is corre + // 0 IF_ICMPGT",These two lines (`// 0 IF_...`) can be removed; they are made redundant by the `// 1 IF`,I don't think this is correct. It should be `if + while`. 301,"@@ -12,6 +11,4 @@ fun test() { // 0 getFirst // 0 getLast @@ -2221,7 +2221,7 @@ Could be reverted, please also add : + /** + * Breadth-first search + */ -+ BFS,","I actually believe we should write these out as `BREADTH_FIRST` and `DEPTH_FIRST`, just like we have `TOP_DOWN` instead of `TD` below.",I think this should be name ++ BFS,","I actually believe we should write these out as `BREADTH_FIRST` and `DEPTH_FIRST`, just like we have `TOP_DOWN` instead of `TD` below.",I think this should be named `BreadthFirstSearchAlgorithm` 303,"@@ -12,6 +12,20 @@ import java.io.File import java.io.IOException import java.util.ArrayDeque @@ -2256,7 +2256,7 @@ Those could be utilized, but this is not a correct way to do that.",Why do we ne /** - * Returns the given iterator itself. This allows to use an instance of iterator in a ranged for-loop + * Returns the given iterator itself. This allows to use an instance of iterator in a ranged for-loop.","""ranged for-loop"" -> ""`for` loop"" -",Why did you change t +",Why did you change this? 306,"@@ -120,6 +120,7 @@ class DeclarationStubGenerator( constantValueGenerator.generateConstantValueAsExpression(UNDEFINED_OFFSET, UNDEFINED_OFFSET, it) ) @@ -2282,12 +2282,12 @@ Those could be utilized, but this is not a correct way to do that.",Why do we ne return when (identifier) { OperatorNameConventions.EQUALS -> { if (!dotQualified.isAnyEquals()) return null -+ if (dotQualified.isFloatingPointNumberEquals()) return null","Here we should not just drop a suggestion. It should be kept, but its highlight type should be changed to `INFORMATION`, and its fix text changed to `Replace total order equality with IEEE 754 equality`.",if (!dotQualified.is ++ if (dotQualified.isFloatingPointNumberEquals()) return null","Here we should not just drop a suggestion. It should be kept, but its highlight type should be changed to `INFORMATION`, and its fix text changed to `Replace total order equality with IEEE 754 equality`.",if (!dotQualified.isNumberEquals()) return null; 311,"@@ -124,21 +128,71 @@ class ConvertTwoComparisonsToRangeCheckIntention : SelfTargetingOffsetIndependen // To avoid possible side effects if (!min.isSimple() || !max.isSimple()) return null -+ val valContext = value.analyze()",You don't need to call `analyze()` multiple times; calling it once will return a `BindingContext` with information about all the expressions.,val valContext = min ++ val valContext = value.analyze()",You don't need to call `analyze()` multiple times; calling it once will return a `BindingContext` with information about all the expressions.,val valContext = min.analyze() 312,"@@ -124,6 +124,7 @@ private val jvmFilePhases = enumWhenPhase then @@ -2302,12 +2302,12 @@ Those could be utilized, but this is not a correct way to do that.",Why do we ne private val loweredEnumConstructors = HashMap() fun transform(): List { -- // Make sure InstanceInitializer exists",@skuzmich Please take a look for js/lower/EnumClassLowering.kt changes,private val loweredE +- // Make sure InstanceInitializer exists",@skuzmich Please take a look for js/lower/EnumClassLowering.kt changes,"private val loweredEnumConstructors = new HashMap();" 315,"@@ -126,6 +131,12 @@ fun IrFunctionBuilder.buildConstructor(): IrConstructor { } } -+inline fun buildFunWithDescriptor(originalDescriptor: FunctionDescriptor, b: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Please rename 'b' to something meaningful: builder?,inline IrFunctionImp ++inline fun buildFunWithDescriptor(originalDescriptor: FunctionDescriptor, b: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Please rename 'b' to something meaningful: builder?,"inline IrFunctionImpl buildFunWithDescriptor(FunctionDescriptor originalDescriptor: FunctionDescriptor, b: Unit): IrFunctionImpl {" 316,"@@ -127,7 +127,9 @@ open class ClassCodegen protected constructor( val shortName = File(fileEntry.name).name visitor.visitSource(shortName, null) @@ -2321,13 +2321,13 @@ Those could be utilized, but this is not a correct way to do that.",Why do we ne + * + * @sample samples.collections.Collections.Aggregates.reduceRightOrNull","This sample is for `reduceRightOrNull` and doesn't relate to `reduceRight`. Perhaps it was misplaced? -Note that the samples for `reduceRight` have been already submitted in the PR #2867.",This should be `redu +Note that the samples for `reduceRight` have been already submitted in the PR #2867.",This should be `reduceRightOrNull` 318,"@@ -128,6 +128,8 @@ extra[""versions.jsr305""] = ""1.3.9"" extra[""versions.jansi""] = ""1.16"" extra[""versions.jline""] = ""3.3.1"" extra[""versions.junit""] = ""4.12"" +extra[""versions.junit5""] = ""5.2.0"" -+extra[""versions.junit-platform""] = ""1.2.0""","Here we specify the versions that will be used throughout the entire project, e.g. in case if Kotlin project would be using JUnit 5 to run its own tests. That version should not necessary be the same as the version against which kotlin-test-junit5 is being built.",This should be `1.2. ++extra[""versions.junit-platform""] = ""1.2.0""","Here we specify the versions that will be used throughout the entire project, e.g. in case if Kotlin project would be using JUnit 5 to run its own tests. That version should not necessary be the same as the version against which kotlin-test-junit5 is being built.",This should be `1.2.0` 319,"@@ -129,19 +126,22 @@ class MovePropertyToConstructorIntention : return parameterDescriptor.source.getPsi() as? KtParameter } @@ -2335,7 +2335,7 @@ Note that the samples for `reduceRight` have been already submitted in the PR #2 - private fun KtAnnotationEntry.isApplicableToConstructorParameter(): Boolean { - val context = analyze(BodyResolveMode.PARTIAL) - val descriptor = context[BindingContext.ANNOTATION, this] ?: return false -+ private fun KtAnnotationEntry.getTextWithUseSite(context: BindingContext): String {","The priority of annotation without use-site application is value parameter (the highest), property, field (the lowest) for constructor parameter property. For regular property it is property (the highest), field (the lowest). So, if some annotation is not applicable to value parameter (VALUE_PARAMETER target is not included), then we should not add use-site while moving to constructor because it will be redundant (priority queue will be the same). And only in case VALUE_PARAMETER target is included we should do all this checks you have done here. ",`getTextWithUseSite` ++ private fun KtAnnotationEntry.getTextWithUseSite(context: BindingContext): String {","The priority of annotation without use-site application is value parameter (the highest), property, field (the lowest) for constructor parameter property. For regular property it is property (the highest), field (the lowest). So, if some annotation is not applicable to value parameter (VALUE_PARAMETER target is not included), then we should not add use-site while moving to constructor because it will be redundant (priority queue will be the same). And only in case VALUE_PARAMETER target is included we should do all this checks you have done here. ",`getTextWithUseSite` -> `getTextWithUseSite` 320,"@@ -129,19 +129,16 @@ public fun File.forEachBlock(action: (buffer: ByteArray, bytesRead: Int) -> Unit */ public fun File.forEachBlock(blockSize: Int, action: (buffer: ByteArray, bytesRead: Int) -> Unit): Unit { @@ -2360,7 +2360,7 @@ Maybe additional empty line is required to separate numbers from ignore directiv - * A base class to simplify implementing iterators so that implementations only have to implement [[computeNext()]] - * to implement the iterator, calling [[done()]] when the iteration is complete. + * A base class to simplify implementing iterators so that implementations only have to implement [computeNext]","Warning, using this class has performance penalty compared to plain raw implementation per case, measured with JMH when operation per element is relatively cheap. -",I don't think this change +",I don't think this change is correct. 323,"@@ -130,31 +127,11 @@ private class BodyTransformer( +irSetVar(parameterToVariable[parameter]!!.symbol, argument) } @@ -2371,7 +2371,7 @@ With tailrec optimization foo$default call from foo should be avoided", .dropWhile { !it.isTransformationOrTermination(context) } .takeWhile { it.isTransformationOrTermination(context) && !it.hasReturn() } .toList() -+ .dropLastWhile { it.calleeExpression?.text == ""groupingBy"" }","Why you did not want just to remove `groupingBy` from termination list, is there some reason I do not see?",.takeWhile { it.isTransfor ++ .dropLastWhile { it.calleeExpression?.text == ""groupingBy"" }","Why you did not want just to remove `groupingBy` from termination list, is there some reason I do not see?",.takeWhile { it.isTransformationOrTermination(context) && !it.hasReturn() } 325,"@@ -1314,4 +1314,19 @@ ${"" ""} assertEquals("" ABC\n \n 123"", ""ABC\n \n123"".prependIndent("" "")) assertEquals("" "", """".prependIndent("" "")) @@ -2397,20 +2397,20 @@ With tailrec optimization foo$default call from foo should be avoided", + def getExtraSourcesMethod = variantData.getMetaClass().getMetaMethod(""getExtraGeneratedSourceFolders"") + if (getExtraSourcesMethod.returnType.metaClass == List.metaClass) { + result.addAll(variantData.getExtraGeneratedSourceFolders())","variantData.getExtraGeneratedSourceFolders() is Nullable, so you should check for null, before calling addAll -",Shouldn't this be `if (get +",Shouldn't this be `if (getExtraSourcesMethod.returnType == List.metaClass)`? 328,"@@ -134,10 +135,24 @@ class ExpressionCodegen( mv.areturn(returnType) } } - writeLocalVariablesInTable(info) + val endLabel = writeLocalVariablesInTable(info) -+ createLocalVariablesForParameters(startLabel, endLabel)",It would be cleaner to create new label here,> createLocalVariablesForP ++ createLocalVariablesForParameters(startLabel, endLabel)",It would be cleaner to create new label here,"> createLocalVariablesForParameters [](start = 12, length = 33) Why do we need this?" 329,"@@ -134,7 +134,8 @@ open class KotlinCocoapodsPlugin: Plugin { if (requestedTargetName == KOTLIN_TARGET_FOR_DEVICE) { // We create a fat framework only for device platforms: iosArm64 and iosArm32. - val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32) -+ val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32,",The comment above also needs to be updated.,I don't think we need this ++ val devicePlatforms = listOf(KonanTarget.IOS_ARM64, KonanTarget.IOS_ARM32,",The comment above also needs to be updated.,I don't think we need this anymore. 330,"@@ -135,4 +136,23 @@ public static KotlinSingleIntentionActionFactory createFactory() { } }; @@ -2427,7 +2427,7 @@ With tailrec optimization foo$default call from foo should be avoided", public String[] scriptResolverEnvironment; + // Javac options -+ @Argument(value = ""-Xuse-javac"", description = ""Use Javac analysis"")","Consider something like `""Use javac for analysis of Java source and class files""`","@Argument(value = ""-Xuse-j" ++ @Argument(value = ""-Xuse-javac"", description = ""Use Javac analysis"")","Consider something like `""Use javac for analysis of Java source and class files""`","@Argument(value = ""-Xuse-javac"", description = ""Use Javac analysis"")" 333,"@@ -137,8 +137,8 @@ public inline fun String.Companion.format(format: String, vararg args: Any?): St public inline fun String.format(locale: Locale, vararg args : Any?) : String = java.lang.String.format(locale, this, *args) @@ -2435,20 +2435,20 @@ With tailrec optimization foo$default call from foo should be avoided", - * Uses this string as a format string and returns a string obtained by substituting the specified arguments, - * using the default locale. + * Uses this string as a format string and returns a string obtained by substituting the specified arguments, using","I've spotted another copy-paste related issue: it uses not this string, but the provided [format] string. -Could you improve these two overloads' docs as well?",I don't think this is corr +Could you improve these two overloads' docs as well?",I don't think this is correct. It should be using the default locale. 334,"@@ -138,7 +153,7 @@ internal class ProgressionLoopHeader( override fun buildLoop(builder: DeclarationIrBuilder, oldLoop: IrLoop, newBody: IrExpression?): LoopReplacement { with(builder) { - var (newLoop, replacementExpression) = if (headerInfo.canOverflow) { -+ return if (headerInfo.canOverflow) {","Nit, but could you use an expression body here?",I don't understand why thi ++ return if (headerInfo.canOverflow) {","Nit, but could you use an expression body here?",I don't understand why this change is needed. 335,"@@ -139,29 +140,34 @@ public fun File.readLines(charset: Charset = Charsets.UTF_8): List { return result } -/** Creates a buffered reader, or returns self if Reader is already buffered */ +/** Creates a buffered reader wrapping this Reader, or returns self if Reader is already buffered */","self -> this Reader -",/** Returns aI t +","/** Returns aI think this should be ""Creates a buffered reader, or returns self if Reader is already buffered""" 336,"@@ -139,6 +140,9 @@ class DefaultExpressionConverter : JavaElementVisitor(), ExpressionConverter { } } @@ -2466,12 +2466,12 @@ Please add such test anyway. ",if (!expression.isInSingleLine()) { val staticPrefix: String, val staticSuffix: String) { OSX (""kexe"", ""lib"", ""dylib"", ""lib"", ""a""), IOS (""kexe"", ""lib"", ""dylib"", ""lib"", ""a""), -+ TVOS (""kexe"", ""lib"", ""dylib"", ""lib"", ""a""),",Better align like others.,"TVOS (""tvos"", ""lib"", ""dylib"", """ ++ TVOS (""kexe"", ""lib"", ""dylib"", ""lib"", ""a""),",Better align like others.,"TVOS (""tvos"", ""lib"", ""dylib"", ""lib"", ""a"")," 339,"@@ -14,6 +14,9 @@ -+ { doc { f -> ""Returns `true` if [element] is found in the ${f.collection}."" } typeParam(""@kotlin.internal.OnlyInputTypes T"") @@ -2484,12 +2484,12 @@ Please add such test anyway. ",if (!expression.isInSingleLine()) { ""https://jcenter.bintray.com/"", - ""https://plugins.gradle.org/m2"") + ""https://plugins.gradle.org/m2"", -+ ""https://oss.sonatype.org/content/repositories/snapshots"")","Please, add the snapshots repo directly to the `kotlin-script-util` build script. I think we don't want (yet) to add it to all projects.",This should be `bootstrapKotlinRepo ++ ""https://oss.sonatype.org/content/repositories/snapshots"")","Please, add the snapshots repo directly to the `kotlin-script-util` build script. I think we don't want (yet) to add it to all projects.",This should be `bootstrapKotlinRepo`. 342,"@@ -140,6 +142,7 @@ class CommentSaver(originalElements: PsiChildRange, private val saveLineBreaks: private val lineBreaksToRestore = ArrayList() private var toNewPsiElementMap by Delegates.notNull>>() private var needAdjustIndentAfterRestore = false -+ private var isSingleExpressionWithCommentBeneath = false","Please convert it to a parameter of `restore`, with default value. Don't introduce additional class state when it's not required",private var isSingleExpressionWithC ++ private var isSingleExpressionWithCommentBeneath = false","Please convert it to a parameter of `restore`, with default value. Don't introduce additional class state when it's not required",private var isSingleExpressionWithCommentBeneath = false 343,"@@ -141,13 +141,28 @@ public static void checkAnnotationType( } } @@ -2522,7 +2522,7 @@ Please add such test anyway. ",if (!expression.isInSingleLine()) { + override fun createBlankArgs(): K2JSCompilerArguments = K2JSCompilerArguments() + + public fun addLibraryFiles(vararg fs: String) {","I think we should extract all libraries from dependencies instead. -",nit: `K2JSCompilerArguments` -> `K2 +",nit: `K2JSCompilerArguments` -> `K2JSCompiler` 347,"@@ -142,7 +144,7 @@ fun compile( val deserializedModuleFragments = sortedImmediateDependencies.map { @@ -2535,14 +2535,14 @@ Please add such test anyway. ",if (!expression.isInSingleLine()) { /** - * Puts all the entries into this [[MutableMap]] with the first value in the pair being the key and the second the value + * Puts all the entries into this [MutableMap] with the first value in the pair being the key and the second the value","""Puts given values"", and again value vs component for Pair. -",I don't think this should be change +",I don't think this should be changed. 349,"@@ -143,6 +143,9 @@ public IntrinsicMethods(JvmTarget jvmTarget, boolean shouldThrowNpeOnExplicitEqu declareIntrinsicFunction(FQ_NAMES.string, ""plus"", 1, new Concat()); declareIntrinsicFunction(FQ_NAMES.string, ""get"", 1, new StringGetChar()); + intrinsicsMap.registerIntrinsic(TEXT_PACKAGE_FQ_NAME, FQ_NAMES.string, ""trimMargin"", 1, new TrimMargin());","We've discussed internally the idea to apply the intrinsic unconditionally and figured that it might lead to compatibility problems in case we decide to change the behavior of `trimMargin`/`trimIndent` on some corner case in a future release. It seems that these kinds of problems will only arise if the compiler of version X is compiling code against standard library of version Y where Y > X: the user expects behavior of version Y at runtime, but will actually observe behavior of version X because that's what was executed in the compiler at compilation time. -Therefore we'd like to restrict these intrinsics to be applicable only if the version of the standard library in dependencies is _not greater_ than the version of the compiler itself. This can be achieved by checking if the API version (can be obtained via `languageVersionSettings.apiVersion` at the call site) is <= than the version of the standard library in the runtime of the compiler (can be obtained via `KotlinVersion.CURRENT`). (Sorry if this sounds confusing, I can do it myself if it's unclear, just wanted to notify you that we'd like it to work that way -- maybe you have some other related concerns.)",declareIntrinsicFunction(FQ_NAMES.s +Therefore we'd like to restrict these intrinsics to be applicable only if the version of the standard library in dependencies is _not greater_ than the version of the compiler itself. This can be achieved by checking if the API version (can be obtained via `languageVersionSettings.apiVersion` at the call site) is <= than the version of the standard library in the runtime of the compiler (can be obtained via `KotlinVersion.CURRENT`). (Sorry if this sounds confusing, I can do it myself if it's unclear, just wanted to notify you that we'd like it to work that way -- maybe you have some other related concerns.)","declareIntrinsicFunction(FQ_NAMES.string, ""trim"", 1, new Trim());" 350,"@@ -145,10 +145,22 @@ private static JetElement overrideFunction(Project project, JetFile file, Simple StringBuilder bodyBuilder = new StringBuilder(""override fun ""); bodyBuilder.append(descriptor.getName()); @@ -2587,7 +2587,7 @@ Therefore we'd like to restrict these intrinsics to be applicable only if the ve > If you don't every test -If you don't`,` every test",I don't think this is the right +If you don't`,` every test",I don't think this is the right way to do this. 356,"@@ -148,6 +148,10 @@ You can now run the various Run/Debug Configurations such as * All Compiler Tests * All IDEA Plugin Tests @@ -2596,7 +2596,7 @@ If you don't`,` every test",I don't think this is the right + +If you need to debug a specific generated test ensure that you have the `Working directory` your IntellJ run configuration set +to the root directory of this project. If you don't every test you try to run will fail with a `No such file or directory` exception.","Basically change the test run config from `$MODULE_DIR$` to `$PROJECT_DIR$`, right? -Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit default run config` to it?",I don't think this is the right +Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit default run config` to it?",I don't think this is the right place to put this. I think it should be at the top of the file 357,"@@ -148,7 +148,7 @@ abstract class AbstractKotlinCompile() : AbstractKo protected val additionalClasspath = arrayListOf() @@ -2608,13 +2608,13 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + override fun acceptsAnnotationTarget(target: AnnotationUseSiteTarget?): Boolean = + target == AnnotationUseSiteTarget.FIELD || -+ (sourcePsi is KtProperty) && (target == null || target == AnnotationUseSiteTarget.PROPERTY)",The `@delegate:` target also maps to fields.,I don't think you need the `is` ++ (sourcePsi is KtProperty) && (target == null || target == AnnotationUseSiteTarget.PROPERTY)",The `@delegate:` target also maps to fields.,I don't think you need the `is` here. 359,"@@ -149,7 +150,7 @@ class MemoizedInlineClassReplacements { } private fun buildReplacement(function: IrFunction, body: IrFunctionImpl.() -> Unit) = - buildFun { -+ buildFunWithDescriptor(function.descriptor) {",Could you clarify why descriptor is reuqired here?,nit: `function.descriptor` -> ` ++ buildFunWithDescriptor(function.descriptor) {",Could you clarify why descriptor is reuqired here?,nit: `function.descriptor` -> `function.descriptor` 360,"@@ -15,3 +17,20 @@ public static void runTest(doGenerateParamAssertions a) { throw new AssertionError(""Fail: IllegalArgumentException expected""); } @@ -2626,20 +2626,20 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau public open /*fake_override*/ fun clear(): kotlin.Unit public open /*fake_override*/ fun contains(/*0*/ T!): kotlin.Boolean public open /*fake_override*/ fun containsAll(/*0*/ kotlin.collections.Collection): kotlin.Boolean -+ public open /*fake_override*/ fun forEach(/*0*/ java.util.function.Consumer!): kotlin.Unit","You'd better not to change existing tests, but to create a new test group with the same test data and your flag enabled.",public open /*fake_override*/ f ++ public open /*fake_override*/ fun forEach(/*0*/ java.util.function.Consumer!): kotlin.Unit","You'd better not to change existing tests, but to create a new test group with the same test data and your flag enabled.",public open /*fake_override*/ fun forEach(/*0*/ java.util.function.Consumer!): kotlin.Unit 362,"@@ -151,15 +156,19 @@ abstract class AbstractWriteSignatureTest : CodegenTestCase() { } fun addClassExpectation(name: String, jvmSignature: String?, genericSignature: String) { - classExpectations.add(SignatureExpectation(""class: $name"", name, jvmSignature, genericSignature)) -+ classExpectations.add(SignatureExpectation(true,""class: $name"", name, jvmSignature, genericSignature))",Please reformat here and below,classExpectations.add(Signature ++ classExpectations.add(SignatureExpectation(true,""class: $name"", name, jvmSignature, genericSignature))",Please reformat here and below,"classExpectations.add(SignatureExpectation(""class: $name"", name, jvmSignature, genericSignature))" 363,"@@ -151,7 +185,7 @@ public fun MutableMap.putAll(vararg values: Pair): Unit { } /** - * Puts all the entries into this [[MutableMap]] with the first value in the pair being the key and the second the value + * Puts all the entries into this [MutableMap] with the first value in the pair being the key and the second the value","""Puts elements of the given collection"" -",I don't think this should be ch +",I don't think this should be changed. 364,"@@ -152,23 +147,14 @@ class IrExpressionLambdaImpl( } @@ -2667,13 +2667,13 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + enabledByDefault=""true"" + cleanupTool=""true"" + level=""WARNING""","This should be a WEAK_WARNING at most. There's nothing wrong with the original code - it's simply not as concise as it could be. -",I think it would be better to u +",I think it would be better to use `` instead of `` 367,"@@ -153,7 +153,7 @@ private class EnumClassLowering(val context: JvmBackendContext) : ClassLoweringP } }) - body = enumConstructor.body // will be transformed later -+ body = enumConstructor.body?.patchDeclarationParents(this)",Is this really needed? If so I think there is another issue with lost parent,I don't think this is correct. ++ body = enumConstructor.body?.patchDeclarationParents(this)",Is this really needed? If so I think there is another issue with lost parent,I don't think this is correct. The `enumConstructor` is a ```patchDeclarationParents` will return `undefined`. 368,"@@ -153,7 +154,13 @@ class IrSourceCompilerForInline( get() = callElement.descriptor as FunctionDescriptor @@ -2686,7 +2686,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau if (functions.size != 1) { val allMembers = members.joinToString(""\n"") { descriptor -> - DescriptorRenderer.DEBUG_TEXT.render(descriptor) + "" | "" + RuntimeTypeMapper.mapSignature(descriptor) -+ DescriptorRenderer.DEBUG_TEXT.render(descriptor) + "" | "" + RuntimeTypeMapper.mapSignature(descriptor).asString()",Thanks for fixing this. Please also fix the similar code above where `RuntimeTypeMapper.mapPropertySignature` is invoked,Why is this change ++ DescriptorRenderer.DEBUG_TEXT.render(descriptor) + "" | "" + RuntimeTypeMapper.mapSignature(descriptor).asString()",Thanks for fixing this. Please also fix the similar code above where `RuntimeTypeMapper.mapPropertySignature` is invoked,Why is this change needed? 370,"@@ -155,7 +161,31 @@ class KotlinAddImportAction internal constructor( return true } @@ -2704,7 +2704,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau } -inline fun IrDeclarationContainer.addFunction(builder: IrFunctionBuilder.() -> Unit): IrSimpleFunction = -+inline fun IrDeclarationContainer.addFunction(builder: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Is this change is neccessery for pr?,inline IrFunctionIm ++inline fun IrDeclarationContainer.addFunction(builder: IrFunctionBuilder.() -> Unit): IrFunctionImpl =",Is this change is neccessery for pr?,inline IrFunctionImpl IrFunctionImpl. 373,"@@ -158,11 +193,7 @@ private fun ModuleInfo.findNativeStdlib(project: Project): NativeLibraryInfo? = class NativeLibraryInfo(project: Project, library: Library, root: File) : LibraryInfo(project, library) { @@ -2714,7 +2714,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau - KOTLIN_NATIVE_CURRENT_ABI_VERSION, - metadataReader = CachingIdeMetadataReaderImpl - ) -+ private val nativeLibrary = createKotlinLibrary(root)",But here `CachingIdeKonanLibraryMetadataLoader` must be present in order to cache contents of any KLIB loaded in IDE.,Why do we need this ++ private val nativeLibrary = createKotlinLibrary(root)",But here `CachingIdeKonanLibraryMetadataLoader` must be present in order to cache contents of any KLIB loaded in IDE.,Why do we need this? 374,"@@ -158,6 +146,12 @@ internal class CallableReferenceLowering(val context: JvmBackendContext) : FileL }) } @@ -2725,7 +2725,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau String r = (String) method.invoke(null); assertEquals(""OK"", r); + System.out.println(generateToText());","I beleive this was commited erroneously. -",Please remove this +",Please remove this line. 376,"@@ -158,6 +162,23 @@ class IrExpressionLambdaImpl( override val invokeMethodDescriptor: FunctionDescriptor = function.descriptor @@ -2733,7 +2733,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + + override fun generateLambdaBody(sourceCompiler: SourceCompilerForInline, reifiedTypeInliner: ReifiedTypeInliner) { + require(sourceCompiler is IrSourceCompilerForInline) -+ lateinit var methodNode: MethodNode",Is it possible to move this logic and related one from ExpressionLambda to sourceCompiler? Seems all adapter related logic could be shared there,`lateinit` is not n ++ lateinit var methodNode: MethodNode",Is it possible to move this logic and related one from ExpressionLambda to sourceCompiler? Seems all adapter related logic could be shared there,`lateinit` is not needed here. 377,"@@ -1592,6 +1592,11 @@ Kotlin @@ -2742,7 +2742,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + org.jetbrains.kotlin.idea.intentions.IndentRawStringIntention + Kotlin + -+","For future PRs: we have now ""branch"" files `plugin.xml.172` etc. that also should be modified if you are modifying `plugin.xml`.",I don't think you n ++","For future PRs: we have now ""branch"" files `plugin.xml.172` etc. that also should be modified if you are modifying `plugin.xml`.",I don't think you need the `` here. 378,"@@ -16,10 +16,25 @@ package kotlin @@ -2752,7 +2752,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + * This class is intended to be used in 'for' loops, and the JVM backend suggests efficient + * bytecode generation for it. Progressions with a step of -1 can be created through the + * `downTo` method on classes representing primitive types.","Link to `downTo` method. -",I'm not sure if thi +","I'm not sure if this is the best name for this class. It's not a sequence of characters, it's a sequence of numbers." 379,"@@ -16,10 +16,25 @@ package kotlin @@ -2764,7 +2764,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau + /** + * Indicates whether some other object is ""equal to"" this one. Implementations must follow + * the same contract as the [Java equals() method](http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals%28java.lang.Object%29).","Referencing Java in such a way doesn't make it look like a standard library for a _language_, which is targeting different platforms. E.g. for JS it may be different contract. -","""Indicates whether " +","""Indicates whether this object is equal to this one.""" 380,"@@ -16,10 +16,30 @@ package kotlin @@ -2784,7 +2784,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau +/** + * Returns true if the receiver and the [other] object are ""equal"" to each other, or if they are","Link to Any.equals() on ""equal"" would be good, to refer to explanation of equality. -",I think this should +",I think this should be `public bool identityEquals(other: Any?);` 382,"@@ -16,18 +16,25 @@ package kotlin.script.templates.standard @@ -2795,7 +2795,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau +// Should we keep them here?","I think we should do the following with these: * `SimpleScriptTemplate` -> remove (script with args can be always used instead of it) * `kotlin.script.templates.standard.ScriptTemplateWithArgs` -> rename and move to `kotlin.script.templates.StandardScriptTemplate` -* `kotlin.script.templates.standard.ScriptTemplateWithBindings` -> move to `kotlin.script.templates.ScriptTemplateWithBindings`",I don't think we ne +* `kotlin.script.templates.standard.ScriptTemplateWithBindings` -> move to `kotlin.script.templates.ScriptTemplateWithBindings`",I don't think we need to keep them here. 383,"@@ -16,26 +16,66 @@ package kotlin @@ -2819,7 +2819,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau +// discuss + -+// Is this an appropriate place to put this class?","This doesn't look like an appropriate place. If I understand correctly, it has nothing to do with scripts, so putting it into package `kotlin.script.extensions` is risky. As soon as we decide to publish it as a supported compiler plugin, we'll have to move it to another package and artifact, causing incompatibilities, so maybe it'd be better to do this earlier.",I don't think this is the r ++// Is this an appropriate place to put this class?","This doesn't look like an appropriate place. If I understand correctly, it has nothing to do with scripts, so putting it into package `kotlin.script.extensions` is risky. As soon as we decide to publish it as a supported compiler plugin, we'll have to move it to another package and artifact, causing incompatibilities, so maybe it'd be better to do this earlier.",I don't think this is the right place to put this class. 386,"@@ -16,8 +16,18 @@ package kotlin @@ -2847,13 +2847,13 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau import org.jetbrains.kotlin.ir.declarations.* import org.jetbrains.kotlin.ir.expressions.* import org.jetbrains.kotlin.ir.util.* -+import org.jetbrains.kotlin.load.java.JavaVisibilities",These imports seem unused,I don't think we need this ++import org.jetbrains.kotlin.load.java.JavaVisibilities",These imports seem unused,I don't think we need this import. 389,"@@ -161,6 +162,13 @@ object KotlinToJVMBytecodeCompiler { ProgressIndicatorAndCompilationCanceledStatus.checkCanceled() writeOutput(state.configuration, state.factory, null) } + -+ if (chunk.size == 1 && projectConfiguration.getBoolean(JVMConfigurationKeys.USE_JAVAC)) {","If the chunk contains multiple modules and `-Xuse-javac` has been specified, maybe we should report a warning saying that we won't in fact use javac for compilation",I don't think this is the r ++ if (chunk.size == 1 && projectConfiguration.getBoolean(JVMConfigurationKeys.USE_JAVAC)) {","If the chunk contains multiple modules and `-Xuse-javac` has been specified, maybe we should report a warning saying that we won't in fact use javac for compilation",I don't think this is the right place to put this check. It should be in `writeOutput`. 390,"@@ -161,6 +162,14 @@ object KotlinToJVMBytecodeCompiler { ProgressIndicatorAndCompilationCanceledStatus.checkCanceled() writeOutput(state.configuration, state.factory, null) @@ -2870,13 +2870,13 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau \ No newline at end of file +remove.unnecessary.parentheses.family=Remove Unnecessary Parentheses +add.function.to.supertype.family=Add Function to Supertype","While I know it's technically not 100% correct and maybe ""Add Function to _Superclassifier_"" would be a more accurate name for this quickfix I still want to stick with the name ""Add Function to Supertype"". In my opinion the user would be confused by using word ""superclassifier"". I can change the name though if you disagree. -",This should be `Add Functio +",This should be `Add Function to Supertype` 392,"@@ -162,6 +169,22 @@ class FirExpressionsResolveTransformer(transformer: FirBodyResolveTransformer) : return completeInference.compose() } + // Use the generic invoke method in Function classes, to match bridges generated by the backend. -+ private fun checkIfInvoke(functionCall: FirFunctionCall) : FirFunctionCall {","I'd think frontend shouldn't rewrite declarations in sake of knowledge of backend behavior, so this logic should rather be in fir2ir or even in backend itself",nit: `checkIfInvoke` -> `ch ++ private fun checkIfInvoke(functionCall: FirFunctionCall) : FirFunctionCall {","I'd think frontend shouldn't rewrite declarations in sake of knowledge of backend behavior, so this logic should rather be in fir2ir or even in backend itself",nit: `checkIfInvoke` -> `checkInvoke` 393,"@@ -163,7 +164,10 @@ object KDocRenderer { // Avoid wrapping the entire converted contents in a

tag if it's just a single paragraph val maybeSingleParagraph = markdownNode.children.singleOrNull { it.type != MarkdownTokenTypes.EOL } @@ -2887,7 +2887,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau } if (expression.getReceiverExpression() == null) { -+ val targetCallable = (targetCallable as? FunctionImportedFromObject)?.callableFromObject ?: targetCallable",Should we use `ImportedFromObjectCallableDescriptor<*>` instead?,if (targetCallable == null) ++ val targetCallable = (targetCallable as? FunctionImportedFromObject)?.callableFromObject ?: targetCallable",Should we use `ImportedFromObjectCallableDescriptor<*>` instead?,if (targetCallable == null) { 395,"@@ -164,7 +164,8 @@ internal class CallableReferenceLowering(val context: JvmBackendContext) : FileL val irFunctionReference: IrFunctionReference ) { @@ -2900,7 +2900,7 @@ Could we maybe fix that, instead of explaining, i.e. by setting the `JUnit defau - +@PublishedApi -+@SinceKotlin(""1.3"")","Should this be `""1.4""`, or some other value?","This should be `@Since(""1.3" ++@SinceKotlin(""1.3"")","Should this be `""1.4""`, or some other value?","This should be `@Since(""1.3"")`" 397,"@@ -165,6 +165,9 @@ fun IrDeclarationWithVisibility.getVisibilityAccessFlag(kind: OwnerKind? = null) ?: visibilityToAccessFlag[visibility] ?: throw IllegalStateException(""$visibility is not a valid visibility in backend for ${ir2string(this)}"") @@ -2924,7 +2924,7 @@ when (visibility) { .filterTo(LinkedHashSet(), File::exists) - private val sourceFilesExtensionsSources: MutableList> = mutableListOf() -+ private val sourceFilesExtensionsSources = project.objects.listProperty(String::class.java)","`listProperty` is introduced in Gradle `4.3`, but we support all versions starting from `4.1`",.filterTo(LinkedHashSet::co ++ private val sourceFilesExtensionsSources = project.objects.listProperty(String::class.java)","`listProperty` is introduced in Gradle `4.3`, but we support all versions starting from `4.1`",.filterTo(LinkedHashSet::contains) 400,"@@ -169,6 +169,28 @@ class Maps { // map.containsValue(""string"") // cannot call extension when the argument type and the map value type are unrelated at all @@ -2973,7 +2973,7 @@ fun String.systemPropertyAsBooleanOrTrueOtherwise(negate: Boolean): Boolean { ``` See comments below. -",I'm not sure if this is the right plac +",I'm not sure if this is the right place to put this. I think it should be in the `kotlin.cli` package. 404,"@@ -17,6 +17,7 @@ package kotlin @@ -2992,7 +2992,7 @@ See comments below. // @TestKt.class // 2 NEW ``` -could be extracted in common part",public static void main(String[] args) +could be extracted in common part",public static void main(String[] args) throws Exception { 406,"@@ -17,8 +21,6 @@ fun test(u1: UInt, u2: UInt, us: Array) { // 2 INVOKESTATIC UInt\.box // 0 INVOKEVIRTUAL UInt.unbox @@ -3033,7 +3033,7 @@ could be extracted in common part",public static void main(String[] args) /** - * Returns last element matching the given *predicate*, or null if element was not found + * Returns the last element matching the given [predicate], or *null* if no such element was found.","_null_ -> `null` -",I don't think this is correct. It shou +",I don't think this is correct. It should be `this[length() - 1]`. 410,"@@ -175,6 +175,23 @@ class KotlinQuickDocumentationProvider : AbstractDocumentationProvider() { // element is not an KtReferenceExpression, but KtClass of enum return renderEnum(element, originalElement, quickNavigation) @@ -3058,7 +3058,7 @@ exp.accept(this, data).also { val result = ConstraintSystemBuilderImpl() - for ((typeParameter, typeBounds) in allTypeParameterBounds) { - result.allTypeParameterBounds.put(typeParameter, typeBounds.filter(filterConstraintPosition)) -+ for (typeParameter in allTypeParameterBounds.entries) {","Since `entries` returns a destructurable class, I suppose you can keep the `for ((typeParameter, typeBounds) ...` syntax here",`filterConstraintPosition` is no longe ++ for (typeParameter in allTypeParameterBounds.entries) {","Since `entries` returns a destructurable class, I suppose you can keep the `for ((typeParameter, typeBounds) ...` syntax here",`filterConstraintPosition` is no longer used. 413,"@@ -176,6 +174,18 @@ class FoldConstantLowering(private val context: JvmBackendContext) : IrElementTr else -> expression } @@ -3095,7 +3095,7 @@ expression.arguments.takeWhile {it is IrConst<*> && it.value is String } and . // 0 INVOKESTATIC KInterface2.access\$test2\$jd // = -// 1 INVOKESTATIC -+// 1 INVOKESTATIC KInterface",The test was over-specified: the IR backend produces a nullcheck on `this` in `KInterface$DefaultImpls$test2` which is a static method.,/ ++// 1 INVOKESTATIC KInterface",The test was over-specified: the IR backend produces a nullcheck on `this` in `KInterface$DefaultImpls$test2` which is a static method.,// 1 INVOKESTATIC 418,"@@ -18,7 +18,6 @@