diff --git a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ConstructorConsistencyChecker.kt b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ConstructorConsistencyChecker.kt index 4fa9c204594..7a6d349c15e 100644 --- a/compiler/frontend/src/org/jetbrains/kotlin/cfg/ConstructorConsistencyChecker.kt +++ b/compiler/frontend/src/org/jetbrains/kotlin/cfg/ConstructorConsistencyChecker.kt @@ -16,6 +16,7 @@ package org.jetbrains.kotlin.cfg +import com.intellij.psi.PsiElement import org.jetbrains.kotlin.builtins.KotlinBuiltIns import org.jetbrains.kotlin.cfg.pseudocode.Pseudocode import org.jetbrains.kotlin.cfg.pseudocode.instructions.KtElementInstruction @@ -26,6 +27,8 @@ import org.jetbrains.kotlin.cfg.pseudocodeTraverser.TraversalOrder import org.jetbrains.kotlin.cfg.pseudocodeTraverser.traverse import org.jetbrains.kotlin.descriptors.* import org.jetbrains.kotlin.psi.* +import org.jetbrains.kotlin.lexer.KtTokens +import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType import org.jetbrains.kotlin.resolve.BindingContext import org.jetbrains.kotlin.resolve.BindingTrace import org.jetbrains.kotlin.types.expressions.OperatorConventions @@ -47,13 +50,30 @@ class ConstructorConsistencyChecker private constructor( ) { private val finalClass = classDescriptor.isFinalClass - private fun checkOpenPropertyAccess(reference: KtReferenceExpression) { - if (!finalClass) { - val descriptor = trace.get(BindingContext.REFERENCE_TARGET, reference) - if (descriptor is PropertyDescriptor && descriptor.isOverridable) { - trace.record(BindingContext.LEAKING_THIS, reference, LeakingThisDescriptor.NonFinalProperty(descriptor, classOrObject)) + private fun insideLValue(reference: KtReferenceExpression): Boolean { + val binary = reference.getStrictParentOfType() ?: return false + if (binary.operationToken in KtTokens.ALL_ASSIGNMENTS) { + val binaryLeft = binary.left + var current: PsiElement = reference + while (current !== binaryLeft && current !== binary) { + current = current.parent ?: return false } + return current === binaryLeft } + return false + } + + private fun safeReferenceUsage(reference: KtReferenceExpression): Boolean { + val descriptor = trace.get(BindingContext.REFERENCE_TARGET, reference) + if (descriptor is PropertyDescriptor) { + if (!finalClass && descriptor.isOverridable) { + trace.record(BindingContext.LEAKING_THIS, reference, LeakingThisDescriptor.NonFinalProperty(descriptor, classOrObject)) + return true + } + if (descriptor.containingDeclaration != classDescriptor) return true + if (insideLValue(reference)) return descriptor.setter?.isDefault != false else return descriptor.getter?.isDefault != false + } + return true } private fun safeThisUsage(expression: KtThisExpression): Boolean { @@ -61,12 +81,7 @@ class ConstructorConsistencyChecker private constructor( if (referenceDescriptor != classDescriptor) return true val parent = expression.parent return when (parent) { - is KtQualifiedExpression -> - if (parent.selectorExpression is KtSimpleNameExpression) { - checkOpenPropertyAccess(parent.selectorExpression as KtSimpleNameExpression) - true - } - else false + is KtQualifiedExpression -> (parent.selectorExpression as? KtSimpleNameExpression)?.let { safeReferenceUsage(it) } ?: false is KtBinaryExpression -> OperatorConventions.IDENTITY_EQUALS_OPERATIONS.contains(parent.operationToken) else -> false } @@ -132,7 +147,9 @@ class ConstructorConsistencyChecker private constructor( } } else if (element is KtReferenceExpression) { - checkOpenPropertyAccess(element) + if (!safeReferenceUsage(element)) { + handleLeakingThis(element) + } } } } diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/backing.kt b/compiler/testData/diagnostics/tests/constructorConsistency/backing.kt new file mode 100644 index 00000000000..2d91912fc52 --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/backing.kt @@ -0,0 +1,24 @@ +class My { + var x = 1 + set(value) { + field = value + } + + var y: Int = 1 + set(value) { + field = value + if (w == "") 0 else 1 + } + + var z: Int = 2 + set(value) { + field = value + if (w == "") 1 else 0 + } + + init { + // Writing properties using setters is dangerous + y = 4 + this.z = 5 + } + + val w = "6" +} \ No newline at end of file diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/backing.txt b/compiler/testData/diagnostics/tests/constructorConsistency/backing.txt new file mode 100644 index 00000000000..00e5bd66391 --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/backing.txt @@ -0,0 +1,12 @@ +package + +public final class My { + public constructor My() + public final val w: kotlin.String = "6" + public final var x: kotlin.Int + public final var y: kotlin.Int + public final var z: kotlin.Int + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String +} diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/getset.kt b/compiler/testData/diagnostics/tests/constructorConsistency/getset.kt new file mode 100644 index 00000000000..6586bd9b58e --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/getset.kt @@ -0,0 +1,17 @@ +class My(var x: String) { + + var y: String + get() = if (x != "") x else z + set(arg) { + if (arg != "") x = arg + } + + val z: String + + init { + // Dangerous: setter! + y = "x" + // Dangerous: getter! + if (y != "") z = this.y else z = y + } +} diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/getset.txt b/compiler/testData/diagnostics/tests/constructorConsistency/getset.txt new file mode 100644 index 00000000000..8632704efdf --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/getset.txt @@ -0,0 +1,11 @@ +package + +public final class My { + public constructor My(/*0*/ x: kotlin.String) + public final var x: kotlin.String + public final var y: kotlin.String + public final val z: kotlin.String + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String +} diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.kt b/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.kt new file mode 100644 index 00000000000..c8cf0307f2c --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.kt @@ -0,0 +1,19 @@ +class My { + val x: Int + get() = field + if (z != "") 1 else 0 + + val y: Int + get() = field - if (z == "") 0 else 1 + + val w: Int + + init { + // Safe, val never has a setter + x = 0 + this.y = 0 + // Unsafe + w = this.x + y + } + + val z = "1" +} diff --git a/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.txt b/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.txt new file mode 100644 index 00000000000..faf4cac666e --- /dev/null +++ b/compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.txt @@ -0,0 +1,12 @@ +package + +public final class My { + public constructor My() + public final val w: kotlin.Int + public final val x: kotlin.Int + public final val y: kotlin.Int + public final val z: kotlin.String = "1" + public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean + public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int + public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String +} diff --git a/compiler/testData/diagnostics/tests/controlFlowAnalysis/delegatedPropertyEarlyAccess.kt b/compiler/testData/diagnostics/tests/controlFlowAnalysis/delegatedPropertyEarlyAccess.kt index 23f9b0a3e01..e6f2a79edf0 100644 --- a/compiler/testData/diagnostics/tests/controlFlowAnalysis/delegatedPropertyEarlyAccess.kt +++ b/compiler/testData/diagnostics/tests/controlFlowAnalysis/delegatedPropertyEarlyAccess.kt @@ -9,16 +9,16 @@ class CustomDelegate { class Kaboom() { // Here and below we should have errors for simple AND delegated init { - delegated.hashCode() + delegated.hashCode() simple.hashCode() - withGetter.hashCode() + withGetter.hashCode() } - val other = delegated + val other = delegated val another = simple - val something = withGetter + val something = withGetter val delegated: String by CustomDelegate() @@ -28,5 +28,5 @@ class Kaboom() { get() = "abc" // No error should be here - val after = delegated + val after = delegated } diff --git a/compiler/testData/diagnostics/testsWithStdLib/PropertyDelegateWithPrivateSet.kt b/compiler/testData/diagnostics/testsWithStdLib/PropertyDelegateWithPrivateSet.kt index 864d8c11997..350f662c650 100644 --- a/compiler/testData/diagnostics/testsWithStdLib/PropertyDelegateWithPrivateSet.kt +++ b/compiler/testData/diagnostics/testsWithStdLib/PropertyDelegateWithPrivateSet.kt @@ -5,7 +5,7 @@ class My { private set // Error: Variable 'delegate' must be initialized - val another: String = delegate + val another: String = delegate var delegateWithBackingField: String by kotlin.properties.Delegates.notNull() private set(arg) { field = arg } diff --git a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java index 2054cdcf4a0..670b1309e94 100644 --- a/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java +++ b/compiler/tests/org/jetbrains/kotlin/checkers/DiagnosticsTestGenerated.java @@ -2913,6 +2913,12 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { doTest(fileName); } + @TestMetadata("backing.kt") + public void testBacking() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/backing.kt"); + doTest(fileName); + } + @TestMetadata("basic.kt") public void testBasic() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/basic.kt"); @@ -2949,12 +2955,24 @@ public class DiagnosticsTestGenerated extends AbstractDiagnosticsTest { doTest(fileName); } + @TestMetadata("getset.kt") + public void testGetset() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/getset.kt"); + doTest(fileName); + } + @TestMetadata("init.kt") public void testInit() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/init.kt"); doTest(fileName); } + @TestMetadata("initwithgetter.kt") + public void testInitwithgetter() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/initwithgetter.kt"); + doTest(fileName); + } + @TestMetadata("lambdaInObject.kt") public void testLambdaInObject() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/constructorConsistency/lambdaInObject.kt");