KT-12152 : constructor consistency: distinguish properties with and w/o backing fields, with default / custom accessors

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) {
+    private fun insideLValue(reference: KtReferenceExpression): Boolean {
-        if (!finalClass) {
+        val binary = reference.getStrictParentOfType<KtBinaryExpression>() ?: return false
-            val descriptor = trace.get(BindingContext.REFERENCE_TARGET, reference)
+        if (binary.operationToken in KtTokens.ALL_ASSIGNMENTS) {
-            if (descriptor is PropertyDescriptor && descriptor.isOverridable) {
+            val binaryLeft = binary.left
-                trace.record(BindingContext.LEAKING_THIS, reference, LeakingThisDescriptor.NonFinalProperty(descriptor, classOrObject))
+            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 ->
+            is KtQualifiedExpression -> (parent.selectorExpression as? KtSimpleNameExpression)?.let { safeReferenceUsage(it) } ?: false
-                if (parent.selectorExpression is KtSimpleNameExpression) {
-                    checkOpenPropertyAccess(parent.selectorExpression as KtSimpleNameExpression)
-                    true
-                }
-                else 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)
+                                }
                             }
                         }
                 }

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
+        <!DEBUG_INFO_LEAKING_THIS!>y<!> = 4
+        this.<!DEBUG_INFO_LEAKING_THIS!>z<!> = 5
+    }
+
+    val w = "6"
+}

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
+}

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!
+        <!DEBUG_INFO_LEAKING_THIS!>y<!> = "x"
+        // Dangerous: getter!
+        if (<!DEBUG_INFO_LEAKING_THIS!>y<!> != "") z = this.<!DEBUG_INFO_LEAKING_THIS!>y<!> else z = <!DEBUG_INFO_LEAKING_THIS!>y<!>
+    }
+}

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
+}

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.<!DEBUG_INFO_LEAKING_THIS!>x<!> + <!DEBUG_INFO_LEAKING_THIS!>y<!>
+    }
+
+    val z = "1"
+}

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
+}

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 {
-        <!UNINITIALIZED_VARIABLE!>delegated<!>.hashCode()
+        <!UNINITIALIZED_VARIABLE, DEBUG_INFO_LEAKING_THIS!>delegated<!>.hashCode()
         <!UNINITIALIZED_VARIABLE!>simple<!>.hashCode()
-        withGetter.hashCode()
+        <!DEBUG_INFO_LEAKING_THIS!>withGetter<!>.hashCode()
     }
 
-    val other = <!UNINITIALIZED_VARIABLE!>delegated<!>
+    val other = <!UNINITIALIZED_VARIABLE, DEBUG_INFO_LEAKING_THIS!>delegated<!>
 
     val another = <!UNINITIALIZED_VARIABLE!>simple<!>
 
-    val something = withGetter
+    val something = <!DEBUG_INFO_LEAKING_THIS!>withGetter<!>
     
     val delegated: String by CustomDelegate()
 
@@ -28,5 +28,5 @@ class Kaboom() {
         get() = "abc"
 
     // No error should be here
-    val after = delegated
+    val after = <!DEBUG_INFO_LEAKING_THIS!>delegated<!>
 }

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 = <!DEBUG_INFO_LEAKING_THIS!>delegate<!>
 
     var delegateWithBackingField: String by kotlin.properties.Delegates.notNull()
         <!ACCESSOR_FOR_DELEGATED_PROPERTY!>private set(arg) { field = arg }<!>

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");