From 7a6f80606bd887523ee4e98d4105f32ed6f0f84d Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Mon, 4 Dec 2017 13:50:28 +0300 Subject: [PATCH] Warn on for-in-array range variable assignment in loop body According to KT-21354, this should be a warning in 1.2 and before, and no warning (with changed semantics) in 1.3 and later. NB there are some false positives in this check. #KT-21354 In Progress #KT-21321 In Progress --- ...JvmArrayVariableInLoopAssignmentChecker.kt | 84 ++++++++++++++ .../diagnostics/DefaultErrorMessagesJvm.java | 3 + .../resolve/jvm/diagnostics/ErrorsJvm.java | 2 + .../jvm/platform/JvmPlatformConfigurator.kt | 3 +- ...capturedRangeVariableAssignmentBefore13.kt | 103 ++++++++++++++++++ ...ocalDelegatedPropertyAssignmentBefore13.kt | 16 +++ .../rangeVariableAssignment13.kt | 19 ++++ .../rangeVariableAssignmentBefore13.kt | 39 +++++++ .../DiagnosticsTestWithStdLibGenerated.java | 33 ++++++ ...ticsTestWithStdLibUsingJavacGenerated.java | 33 ++++++ .../kotlin/config/LanguageVersionSettings.kt | 1 + 11 files changed, 335 insertions(+), 1 deletion(-) create mode 100644 compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmArrayVariableInLoopAssignmentChecker.kt create mode 100644 compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt create mode 100644 compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt create mode 100644 compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt create mode 100644 compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmArrayVariableInLoopAssignmentChecker.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmArrayVariableInLoopAssignmentChecker.kt new file mode 100644 index 00000000000..de22323ee70 --- /dev/null +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmArrayVariableInLoopAssignmentChecker.kt @@ -0,0 +1,84 @@ +/* + * Copyright 2010-2017 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.jetbrains.kotlin.resolve.jvm.checkers + +import org.jetbrains.kotlin.builtins.KotlinBuiltIns +import org.jetbrains.kotlin.config.LanguageFeature +import org.jetbrains.kotlin.descriptors.CallableDescriptor +import org.jetbrains.kotlin.descriptors.impl.LocalVariableDescriptor +import org.jetbrains.kotlin.descriptors.impl.SyntheticFieldDescriptor +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.KtBinaryExpression +import org.jetbrains.kotlin.psi.KtExpression +import org.jetbrains.kotlin.psi.KtForExpression +import org.jetbrains.kotlin.psi.KtSimpleNameExpression +import org.jetbrains.kotlin.psi.psiUtil.parents +import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall +import org.jetbrains.kotlin.resolve.calls.checkers.AdditionalTypeChecker +import org.jetbrains.kotlin.resolve.calls.context.ResolutionContext +import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowValue +import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowValueFactory +import org.jetbrains.kotlin.resolve.jvm.diagnostics.ErrorsJvm +import org.jetbrains.kotlin.types.KotlinType + +object JvmArrayVariableInLoopAssignmentChecker : AdditionalTypeChecker { + + override fun checkType( + expression: KtExpression, + expressionType: KotlinType, + expressionTypeWithSmartCast: KotlinType, + c: ResolutionContext<*> + ) { + if (c.languageVersionSettings.supportsFeature(LanguageFeature.ProperForInArrayLoopRangeVariableAssignmentSemantic)) return + + val binaryExpression = expression as? KtBinaryExpression ?: return + if (binaryExpression.operationToken != KtTokens.EQ) return + + val lhsExpression = binaryExpression.left as? KtSimpleNameExpression ?: return + val resolvedCall = lhsExpression.getResolvedCall(c.trace.bindingContext) ?: return + val variableDescriptor = resolvedCall.resultingDescriptor as? LocalVariableDescriptor ?: return + if (variableDescriptor is SyntheticFieldDescriptor) return + if (variableDescriptor.isDelegated) return + + val variableType = variableDescriptor.returnType + if (!KotlinBuiltIns.isArrayOrPrimitiveArray(variableType)) return + + if (!isOuterForLoopRangeVariable(expression, variableDescriptor, c)) return + + val dataFlowValueKind = DataFlowValueFactory.createDataFlowValue(lhsExpression, variableType, c).kind + if (dataFlowValueKind != DataFlowValue.Kind.STABLE_VARIABLE) return + + c.trace.report(ErrorsJvm.ASSIGNMENT_TO_ARRAY_LOOP_VARIABLE.on(lhsExpression)) + } + + private fun isOuterForLoopRangeVariable( + expression: KtExpression, + variableDescriptor: CallableDescriptor, + c: ResolutionContext<*> + ): Boolean { + for (parent in expression.parents) { + if (parent is KtForExpression) { + val rangeExpression = parent.loopRange as? KtSimpleNameExpression ?: continue + val rangeResolvedCall = rangeExpression.getResolvedCall(c.trace.bindingContext) ?: continue + if (rangeResolvedCall.resultingDescriptor == variableDescriptor) { + return true + } + } + } + return false + } +} \ No newline at end of file diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java index 81f54c6afc1..e58d35d2bcd 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java @@ -127,6 +127,9 @@ public class DefaultErrorMessagesJvm implements DefaultErrorMessages.Extension { MAP.put(JAVA_MODULE_DOES_NOT_EXPORT_PACKAGE, "Symbol is declared in module ''{0}'' which does not export package ''{1}''", STRING, STRING); MAP.put(API_VERSION_IS_AT_LEAST_ARGUMENT_SHOULD_BE_CONSTANT, "'apiVersionIsAtLeast' argument should be a constant expression"); + + MAP.put(ASSIGNMENT_TO_ARRAY_LOOP_VARIABLE, "Assignment to a for-in-array loop range variable. Behavior may change in Kotlin 1.3. " + + "See https://youtrack.jetbrains.com/issue/KT-21354 for more details"); } @NotNull diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java index 8f6a7cdf799..ac4ab6a4cc1 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java @@ -109,6 +109,8 @@ public interface ErrorsJvm { DiagnosticFactory0 API_VERSION_IS_AT_LEAST_ARGUMENT_SHOULD_BE_CONSTANT = DiagnosticFactory0.create(WARNING); + DiagnosticFactory0 ASSIGNMENT_TO_ARRAY_LOOP_VARIABLE = DiagnosticFactory0.create(WARNING); + enum NullabilityInformationSource { KOTLIN { @NotNull diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/platform/JvmPlatformConfigurator.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/platform/JvmPlatformConfigurator.kt index 1e90097c2e0..bdb8d7b59c8 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/platform/JvmPlatformConfigurator.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/platform/JvmPlatformConfigurator.kt @@ -62,7 +62,8 @@ object JvmPlatformConfigurator : PlatformConfigurator( JavaNullabilityChecker(), RuntimeAssertionsTypeChecker, JavaGenericVarianceViolationTypeChecker, - JavaTypeAccessibilityChecker() + JavaTypeAccessibilityChecker(), + JvmArrayVariableInLoopAssignmentChecker ), additionalClassifierUsageCheckers = listOf( diff --git a/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt new file mode 100644 index 00000000000..225b44a863e --- /dev/null +++ b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt @@ -0,0 +1,103 @@ +// !LANGUAGE: -ProperForInArrayLoopRangeVariableAssignmentSemantic +// !DIAGNOSTICS: -UNUSED_VALUE +// SKIP_TXT + +fun testArrayCapturedInLocalFun() { + var xs = arrayOf("a", "b", "c") + + fun updateXs() { + xs = arrayOf("d", "e", "f") + } + + for (x in xs) { + println(x) + updateXs() + } +} + +fun testArrayCapturedInLabmda() { + var xs = arrayOf("a", "b", "c") + + val updateXs = { xs = arrayOf("d", "e", "f") } + + for (x in xs) { + println(x) + updateXs() + } +} + +fun testArrayCapturedInInlineLambda() { + var xs = arrayOf("a", "b", "c") + + for (x in xs) { + println(x) + run { + xs = arrayOf("d", "e", "f") + } + } +} + +fun testArrayCapturedInLocalObject() { + var xs = arrayOf("a", "b", "c") + + val updateXs = object : () -> Unit { + override fun invoke() { + xs = arrayOf("d", "e", "f") + } + } + + for (x in xs) { + println(x) + updateXs() + } +} + +fun testArrayCapturedInLocalClass() { + var xs = arrayOf("a", "b", "c") + + class LocalClass { + fun updateXs() { + xs = arrayOf("d", "e", "f") + } + } + + val updater = LocalClass() + + for (x in xs) { + println(x) + updater.updateXs() + } +} + +fun testCapturedInLambdaAfterLoop() { + // NB false positive + var xs = intArrayOf(1, 2, 3) + for (x in xs) { + println(x) + xs = intArrayOf(4, 5, 6) + } + val lambda = { xs = intArrayOf() } + lambda() +} + +fun testCapturedInLambdaInLoopAfterAssignment() { + // NB false positive + var xs = intArrayOf(1, 2, 3) + for (x in xs) { + println(x) + xs = intArrayOf(4, 5, 6) + val lambda = { xs = intArrayOf() } + lambda() + } +} + +fun testCapturedInNonChangingClosure() { + // NB false positive + var xs = intArrayOf(1, 2, 3) + val lambda = { println(xs) } + for (x in xs) { + println(x) + xs = intArrayOf(4, 5, 6) + lambda() + } +} \ No newline at end of file diff --git a/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt new file mode 100644 index 00000000000..64edb6438df --- /dev/null +++ b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt @@ -0,0 +1,16 @@ +// !LANGUAGE: -ProperForInArrayLoopRangeVariableAssignmentSemantic +// !DIAGNOSTICS: -UNUSED_VALUE +// SKIP_TXT + +class Delegate(var v: T) { + operator fun getValue(thisRef: Any?, kProp: Any?) = v + operator fun setValue(thisRef: Any?, kProp: Any?, value: T) { v = value } +} + +fun testLocalDelegatedProperty() { + var xs by Delegate(arrayOf("a", "b", "c")) + for (x in xs) { + println(x) + xs = arrayOf("d", "e", "f") + } +} \ No newline at end of file diff --git a/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt new file mode 100644 index 00000000000..1689dd22563 --- /dev/null +++ b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt @@ -0,0 +1,19 @@ +// !LANGUAGE: +ProperForInArrayLoopRangeVariableAssignmentSemantic +// !DIAGNOSTICS: -UNUSED_VALUE +// SKIP_TXT + +fun testObjectArray() { + var xs = arrayOf("a", "b", "c") + for (x in xs) { + println(x) + xs = arrayOf("d", "e", "f") + } +} + +fun testPrimitiveArray() { + var xs = intArrayOf(1, 2, 3) + for (x in xs) { + println(x) + xs = intArrayOf(4, 5, 6) + } +} \ No newline at end of file diff --git a/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt new file mode 100644 index 00000000000..2a009c493ed --- /dev/null +++ b/compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt @@ -0,0 +1,39 @@ +// !LANGUAGE: -ProperForInArrayLoopRangeVariableAssignmentSemantic +// !DIAGNOSTICS: -UNUSED_VALUE +// SKIP_TXT + +fun testObjectArray() { + var xs = arrayOf("a", "b", "c") + for (x in xs) { + println(x) + xs = arrayOf("d", "e", "f") + } +} + +fun testPrimitiveArray() { + var xs = intArrayOf(1, 2, 3) + for (x in xs) { + println(x) + xs = intArrayOf(4, 5, 6) + } +} + +var global = arrayOf("a", "b", "c") + +fun testGlobalArray() { + for (x in global) { + println(x) + global = arrayOf("d", "e", "f") + } +} + +fun testAssignmentNotInLoop() { + var xs = intArrayOf(1, 2, 3) + println(xs) + xs = intArrayOf(7, 8, 9) + for (x in xs) { + println(x) + } + xs = intArrayOf(4, 5, 6) + println(xs) +} \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithStdLibGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithStdLibGenerated.java index 54fc14edead..c96f87ea5db 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithStdLibGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestWithStdLibGenerated.java @@ -1746,6 +1746,39 @@ public class DiagnosticsTestWithStdLibGenerated extends AbstractDiagnosticsTestW } } + @TestMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class ForInArrayLoop extends AbstractDiagnosticsTestWithStdLib { + public void testAllFilesPresentInForInArrayLoop() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.ANY, true); + } + + @TestMetadata("capturedRangeVariableAssignmentBefore13.kt") + public void testCapturedRangeVariableAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeLocalDelegatedPropertyAssignmentBefore13.kt") + public void testRangeLocalDelegatedPropertyAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeVariableAssignment13.kt") + public void testRangeVariableAssignment13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeVariableAssignmentBefore13.kt") + public void testRangeVariableAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt"); + doTest(fileName); + } + } + @TestMetadata("compiler/testData/diagnostics/testsWithStdLib/functionLiterals") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsTestWithStdLibUsingJavacGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsTestWithStdLibUsingJavacGenerated.java index db6fe67cae1..ef8682e7034 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsTestWithStdLibUsingJavacGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/javac/DiagnosticsTestWithStdLibUsingJavacGenerated.java @@ -1746,6 +1746,39 @@ public class DiagnosticsTestWithStdLibUsingJavacGenerated extends AbstractDiagno } } + @TestMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop") + @TestDataPath("$PROJECT_ROOT") + @RunWith(JUnit3RunnerWithInners.class) + public static class ForInArrayLoop extends AbstractDiagnosticsTestWithStdLibUsingJavac { + public void testAllFilesPresentInForInArrayLoop() throws Exception { + KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop"), Pattern.compile("^(.+)\\.kt$"), TargetBackend.ANY, true); + } + + @TestMetadata("capturedRangeVariableAssignmentBefore13.kt") + public void testCapturedRangeVariableAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/capturedRangeVariableAssignmentBefore13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeLocalDelegatedPropertyAssignmentBefore13.kt") + public void testRangeLocalDelegatedPropertyAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeLocalDelegatedPropertyAssignmentBefore13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeVariableAssignment13.kt") + public void testRangeVariableAssignment13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignment13.kt"); + doTest(fileName); + } + + @TestMetadata("rangeVariableAssignmentBefore13.kt") + public void testRangeVariableAssignmentBefore13() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/testsWithStdLib/forInArrayLoop/rangeVariableAssignmentBefore13.kt"); + doTest(fileName); + } + } + @TestMetadata("compiler/testData/diagnostics/testsWithStdLib/functionLiterals") @TestDataPath("$PROJECT_ROOT") @RunWith(JUnit3RunnerWithInners.class) diff --git a/compiler/util/src/org/jetbrains/kotlin/config/LanguageVersionSettings.kt b/compiler/util/src/org/jetbrains/kotlin/config/LanguageVersionSettings.kt index b953548a0f9..472c6bd6244 100644 --- a/compiler/util/src/org/jetbrains/kotlin/config/LanguageVersionSettings.kt +++ b/compiler/util/src/org/jetbrains/kotlin/config/LanguageVersionSettings.kt @@ -75,6 +75,7 @@ enum class LanguageFeature( RestrictionOfWrongAnnotationsWithUseSiteTargetsOnTypes(KOTLIN_1_3), ProhibitInnerClassesOfGenericClassExtendingThrowable(KOTLIN_1_3), ProperVisibilityForCompanionObjectInstanceField(KOTLIN_1_3), + ProperForInArrayLoopRangeVariableAssignmentSemantic(KOTLIN_1_3), StrictJavaNullabilityAssertions(sinceVersion = null, defaultState = State.DISABLED),