Don't generate unnecessary accessors for private class properties

compiler/backend/src/org/jetbrains/jet/codegen/PropertyCodegen.java

@@ -22,7 +22,6 @@ import com.intellij.psi.PsiElement;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.jet.codegen.context.*;
-import org.jetbrains.jet.lang.resolve.java.jvmSignature.JvmMethodSignature;
 import org.jetbrains.jet.codegen.state.GenerationState;
 import org.jetbrains.jet.codegen.state.JetTypeMapper;
 import org.jetbrains.jet.descriptors.serialization.descriptors.DeserializedPropertyDescriptor;
@@ -33,6 +32,7 @@ import org.jetbrains.jet.lang.resolve.DescriptorFactory;
 import org.jetbrains.jet.lang.resolve.calls.model.ResolvedCall;
 import org.jetbrains.jet.lang.resolve.constants.CompileTimeConstant;
 import org.jetbrains.jet.lang.resolve.java.JvmAbi;
+import org.jetbrains.jet.lang.resolve.java.jvmSignature.JvmMethodSignature;
 import org.jetbrains.jet.lang.resolve.name.Name;
 import org.jetbrains.jet.lang.types.ErrorUtils;
 import org.jetbrains.jet.lang.types.JetType;
@@ -44,13 +44,14 @@ import org.jetbrains.org.objectweb.asm.commons.InstructionAdapter;
 import org.jetbrains.org.objectweb.asm.commons.Method;
 
 import static org.jetbrains.jet.codegen.AsmUtil.*;
-import static org.jetbrains.jet.lang.resolve.java.diagnostics.DiagnosticsPackage.OtherOrigin;
 import static org.jetbrains.jet.codegen.JvmCodegenUtil.getParentBodyCodegen;
 import static org.jetbrains.jet.codegen.JvmCodegenUtil.isInterface;
 import static org.jetbrains.jet.codegen.JvmSerializationBindings.*;
+import static org.jetbrains.jet.lang.resolve.DescriptorUtils.isClassObject;
 import static org.jetbrains.jet.lang.resolve.DescriptorUtils.isTrait;
 import static org.jetbrains.jet.lang.resolve.java.AsmTypeConstants.OBJECT_TYPE;
 import static org.jetbrains.jet.lang.resolve.java.AsmTypeConstants.PROPERTY_METADATA_TYPE;
+import static org.jetbrains.jet.lang.resolve.java.diagnostics.DiagnosticsPackage.OtherOrigin;
 import static org.jetbrains.org.objectweb.asm.Opcodes.*;
 
 public class PropertyCodegen {
@@ -112,17 +113,55 @@ public class PropertyCodegen {
             }
         }
 
-        generateGetter(declaration, descriptor, getter);
-        generateSetter(declaration, descriptor, setter);
+        if (isAccessorNeeded(declaration, descriptor, getter)) {
+            generateGetter(declaration, descriptor, getter);
+        }
+        if (isAccessorNeeded(declaration, descriptor, setter)) {
+            generateSetter(declaration, descriptor, setter);
+        }
 
         context.recordSyntheticAccessorIfNeeded(descriptor, bindingContext);
     }
 
+    /**
+     * Determines if it's necessary to generate an accessor to the property, i.e. if this property can be referenced via getter/setter
+     * for any reason
+     *
+     * @see JvmCodegenUtil#couldUseDirectAccessToProperty
+     */
+    private boolean isAccessorNeeded(
+            @Nullable JetProperty declaration,
+            @NotNull PropertyDescriptor descriptor,
+            @Nullable JetPropertyAccessor accessor
+    ) {
+        boolean isDefaultAccessor = accessor == null || !accessor.hasBody();
+
+        // Don't generate accessors for trait properties with default accessors in TRAIT_IMPL
+        if (kind == OwnerKind.TRAIT_IMPL && isDefaultAccessor) return false;
+
+        if (declaration == null) return true;
+
+        // Delegated or extension properties can only be referenced via accessors
+        if (declaration.hasDelegate() || declaration.getReceiverTypeRef() != null) return true;
+
+        // Class object properties always should have accessors, because their backing fields are moved/copied to the outer class
+        if (isClassObject(descriptor.getContainingDeclaration())) return true;
+
+        // Private class properties have accessors only in cases when those accessors are non-trivial
+        if (kind == OwnerKind.IMPLEMENTATION && descriptor.getVisibility() == Visibilities.PRIVATE) {
+            return !isDefaultAccessor;
+        }
+
+        return true;
+    }
+
     public void generatePrimaryConstructorProperty(JetParameter p, PropertyDescriptor descriptor) {
         generateBackingField(p, descriptor);
-        generateGetter(p, descriptor, null);
-        if (descriptor.isVar()) {
-            generateSetter(p, descriptor, null);
+        if (descriptor.getVisibility() != Visibilities.PRIVATE) {
+            generateGetter(p, descriptor, null);
+            if (descriptor.isVar()) {
+                generateSetter(p, descriptor, null);
+            }
         }
     }
 
@@ -285,12 +324,8 @@ public class PropertyCodegen {
             @Nullable JetPropertyAccessor accessor,
             @NotNull PropertyAccessorDescriptor accessorDescriptor
     ) {
-        boolean isDefaultAccessor = accessor == null || accessor.getBodyExpression() == null;
-
-        if (kind == OwnerKind.TRAIT_IMPL && isDefaultAccessor) return;
-
         FunctionGenerationStrategy strategy;
-        if (isDefaultAccessor) {
+        if (accessor == null || !accessor.hasBody()) {
             if (p instanceof JetProperty && ((JetProperty) p).hasDelegate()) {
                 strategy = new DelegatedPropertyAccessorStrategy(state, accessorDescriptor, indexOfDelegatedProperty((JetProperty) p));
             }

compiler/testData/asJava/lightClasses/nullabilityAnnotations/PrivateInClass.java

@@ -1,9 +1,8 @@
 public final class PrivateInClass implements kotlin.jvm.internal.KObject {
-    private final java.lang.String nn = "";
-    private final java.lang.String n = "";
-
     private final java.lang.String getNn() { /* compiled code */ }
 
+    private final void setNn(@jet.runtime.typeinfo.JetValueParameter(name = "value") java.lang.String value) { /* compiled code */ }
+
     private final java.lang.String getN() { /* compiled code */ }
 
     private final java.lang.String bar(@jet.runtime.typeinfo.JetValueParameter(name = "a") java.lang.String a, @jet.runtime.typeinfo.JetValueParameter(name = "b", type = "?") java.lang.String b) { /* compiled code */ }

compiler/testData/asJava/lightClasses/nullabilityAnnotations/PrivateInClass.kt

@@ -1,7 +1,10 @@
 // PrivateInClass
 
 class PrivateInClass private (s: String?) {
-    private val nn: String = ""
-    private val n: String? = ""
+    private var nn: String
+        get() = ""
+        set(value) {}
+    private val n: String?
+        get() = ""
     private fun bar(a: String, b: String?): String? = null
 }

compiler/testData/asJava/lightClasses/nullabilityAnnotations/PrivateInTrait.java

@@ -2,6 +2,8 @@ public interface PrivateInTrait extends kotlin.jvm.internal.KObject {
     @org.jetbrains.annotations.NotNull
     java.lang.String getNn();
 
+    void setNn(@jet.runtime.typeinfo.JetValueParameter(name = "value") @org.jetbrains.annotations.NotNull java.lang.String value);
+
     @org.jetbrains.annotations.Nullable
     java.lang.String getN();
 

compiler/testData/asJava/lightClasses/nullabilityAnnotations/PrivateInTrait.kt

@@ -1,7 +1,10 @@
 // PrivateInTrait
 
 trait PrivateInTrait {
-    private val nn: String
+    private var nn: String
+        get() = ""
+        set(value) {}
     private val n: String?
+        get() = ""
     private fun bar(a: String, b: String?): String?
 }

compiler/testData/codegen/box/properties/privatePropertyInConstructor.kt

@@ -0,0 +1,18 @@
+class A(
+        private val x: String,
+        private var y: Double
+) {
+    fun foo() {
+        val r = {
+            if (x != "abc") throw AssertionError("$x")
+            y = 0.0
+            if (y != 0.0) throw AssertionError("$y")
+        }
+        r()
+    }
+}
+
+fun box(): String {
+    A("abc", 3.14).foo()
+    return "OK"
+}

compiler/testData/codegen/properties/privateClassPropertyAccessors.kt

@@ -0,0 +1,31 @@
+class C {
+    // All these properties should have corresponding accessors
+    private val valWithGet: String
+        get() = ""
+
+    private var varWithGetSet: Int
+        get() = 0
+        set(value) {}
+
+    private var delegated: Int by Delegate
+
+    private var String.extension: String
+        get() = this
+        set(value) {}
+
+    class object {
+        private val classObjectVal: Long
+            get() = 1L
+    }
+
+    // This property should not have accessors
+    private var varNoAccessors = 0L
+        get set
+}
+
+
+object Delegate {
+    fun get(x: C, p: PropertyMetadata) = throw AssertionError()
+
+    fun set(x: C, p: PropertyMetadata, value: Int) = throw AssertionError()
+}

compiler/testData/diagnostics/tests/duplicateJvmSignature/functionAndProperty/privateClassPropertyNoClash.kt

@@ -0,0 +1,13 @@
+// FILE: A.java
+
+public class A {
+    public String getFoo() {
+        return "Foo";
+    }
+}
+
+// FILE: B.kt
+
+class B(private val foo: String) : A() {
+    override fun getFoo(): String = foo
+}

compiler/tests/org/jetbrains/jet/checkers/JetDiagnosticsTestGenerated.java

@@ -2936,6 +2936,11 @@ public class JetDiagnosticsTestGenerated extends AbstractJetDiagnosticsTest {
                     doTest("compiler/testData/diagnostics/tests/duplicateJvmSignature/functionAndProperty/objectExpressionInConstructor.kt");
                 }
                 
+                @TestMetadata("privateClassPropertyNoClash.kt")
+                public void testPrivateClassPropertyNoClash() throws Exception {
+                    doTest("compiler/testData/diagnostics/tests/duplicateJvmSignature/functionAndProperty/privateClassPropertyNoClash.kt");
+                }
+                
                 @TestMetadata("topLevel.kt")
                 public void testTopLevel() throws Exception {
                     doTest("compiler/testData/diagnostics/tests/duplicateJvmSignature/functionAndProperty/topLevel.kt");

compiler/tests/org/jetbrains/jet/codegen/CodegenTestUtil.java

@@ -96,12 +96,20 @@ public class CodegenTestUtil {
 
     @NotNull
     public static Method findDeclaredMethodByName(@NotNull Class<?> aClass, @NotNull String name) {
+        Method result = findDeclaredMethodByNameOrNull(aClass, name);
+        if (result == null) {
+            throw new AssertionError("Method " + name + " is not found in " + aClass);
+        }
+        return result;
+    }
+
+    public static Method findDeclaredMethodByNameOrNull(@NotNull Class<?> aClass, @NotNull String name) {
         for (Method method : aClass.getDeclaredMethods()) {
             if (method.getName().equals(name)) {
                 return method;
             }
         }
-        throw new AssertionError("Method " + name + " is not found in class " + aClass);
+        return null;
     }
 
     public static void assertIsCurrentTime(long returnValue) {

compiler/tests/org/jetbrains/jet/codegen/PropertyGenTest.java

@@ -18,12 +18,12 @@ package org.jetbrains.jet.codegen;
 
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.jet.ConfigurationKind;
+import org.jetbrains.jet.lang.resolve.java.JvmAbi;
 import org.jetbrains.org.objectweb.asm.Opcodes;
 
 import java.lang.reflect.*;
 
-import static org.jetbrains.jet.codegen.CodegenTestUtil.assertIsCurrentTime;
-import static org.jetbrains.jet.codegen.CodegenTestUtil.findDeclaredMethodByName;
+import static org.jetbrains.jet.codegen.CodegenTestUtil.*;
 
 public class PropertyGenTest extends CodegenTestCase {
     @Override
@@ -236,4 +236,21 @@ public class PropertyGenTest extends CodegenTestCase {
             assertEquals(String.class, parameterizedType.getActualTypeArguments()[0]);
         }
     }
+
+    public void testPrivateClassPropertyAccessors() throws Exception {
+        loadFile("properties/privateClassPropertyAccessors.kt");
+        Class<?> c = generateClass("C");
+        findDeclaredMethodByName(c, "getValWithGet");
+        findDeclaredMethodByName(c, "getVarWithGetSet");
+        findDeclaredMethodByName(c, "setVarWithGetSet");
+        findDeclaredMethodByName(c, "getDelegated");
+        findDeclaredMethodByName(c, "setDelegated");
+        findDeclaredMethodByName(c, "getExtension");
+        findDeclaredMethodByName(c, "setExtension");
+
+        findDeclaredMethodByName(initializedClassLoader.loadClass("C" + JvmAbi.CLASS_OBJECT_SUFFIX), "getClassObjectVal");
+
+        assertNull("Property should not have a getter", findDeclaredMethodByNameOrNull(c, "getVarNoAccessors"));
+        assertNull("Property should not have a setter", findDeclaredMethodByNameOrNull(c, "setVarNoAccessors"));
+    }
 }

compiler/tests/org/jetbrains/jet/codegen/generated/BlackBoxCodegenTestGenerated.java

@@ -4501,6 +4501,11 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest {
             doTest("compiler/testData/codegen/box/properties/primitiveOverrideDelegateAccessor.kt");
         }
         
+        @TestMetadata("privatePropertyInConstructor.kt")
+        public void testPrivatePropertyInConstructor() throws Exception {
+            doTest("compiler/testData/codegen/box/properties/privatePropertyInConstructor.kt");
+        }
+        
         @TestMetadata("privatePropertyWithoutBackingField.kt")
         public void testPrivatePropertyWithoutBackingField() throws Exception {
             doTest("compiler/testData/codegen/box/properties/privatePropertyWithoutBackingField.kt");

core/runtime.jvm/src/kotlin/reflect/jvm/internal/KMemberPropertyImpl.kt

@@ -25,25 +25,30 @@ open class KMemberPropertyImpl<T : Any, out R>(
         override val name: String,
         protected val owner: KClassImpl<T>
 ) : KMemberProperty<T, R>, KPropertyImpl<R> {
+    // TODO: extract, make lazy (weak?), use our descriptors knowledge
     override val field: Field?
-        get() = try {
-            owner.jClass.getDeclaredField(name)
+    override val getter: Method?
+
+    {
+        try {
+            field = owner.jClass.getDeclaredField(name)
         }
         catch (e: NoSuchFieldException) {
-            null
+            field = null
         }
 
-    // TODO: extract, make lazy (weak?), use our descriptors knowledge
-    override val getter: Method = try {
-        owner.jClass.getMaybeDeclaredMethod(getterName(name))
-    }
-    catch (e: NoSuchMethodException) {
-        throw NoSuchPropertyException(e)
+        try {
+            getter = owner.jClass.getMaybeDeclaredMethod(getterName(name))
+        }
+        catch (e: NoSuchMethodException) {
+            if (field == null) throw NoSuchPropertyException(e)
+            getter = null
+        }
     }
 
     override fun get(receiver: T): R {
         try {
-            return getter(receiver) as R
+            return (if (getter != null) getter!!(receiver) else field!!.get(receiver)) as R
         }
         catch (e: java.lang.IllegalAccessException) {
             throw kotlin.reflect.IllegalAccessException(e)
@@ -65,16 +70,22 @@ class KMutableMemberPropertyImpl<T : Any, R>(
         name: String,
         owner: KClassImpl<T>
 ) : KMutableMemberProperty<T, R>, KMutablePropertyImpl<R>, KMemberPropertyImpl<T, R>(name, owner) {
-    override val setter: Method = try {
-        owner.jClass.getMaybeDeclaredMethod(setterName(name), getter.getReturnType()!!)
-    }
-    catch (e: NoSuchMethodException) {
-        throw NoSuchPropertyException(e)
+    override val setter: Method?
+
+    {
+        try {
+            val returnType = if (getter != null) getter.getReturnType() else field!!.getType()
+            setter = owner.jClass.getMaybeDeclaredMethod(setterName(name), returnType!!)
+        }
+        catch (e: NoSuchMethodException) {
+            if (field == null) throw NoSuchPropertyException(e)
+            setter = null
+        }
     }
 
     override fun set(receiver: T, value: R) {
         try {
-            setter(receiver, value)
+            if (setter != null) setter!!(receiver, value) else field!!.set(receiver, value)
         }
         catch (e: java.lang.IllegalAccessException) {
             throw kotlin.reflect.IllegalAccessException(e)

core/runtime.jvm/src/kotlin/reflect/jvm/properties.kt

@@ -22,8 +22,13 @@ import kotlin.reflect.jvm.internal.*
 public var <R> KProperty<R>.accessible: Boolean
         get() {
             return when (this) {
-                is KMutableMemberPropertyImpl<*, R> -> getter.isAccessible() && setter.isAccessible()
-                is KMemberPropertyImpl<*, R> -> getter.isAccessible()
+                is KMutableMemberPropertyImpl<*, R> ->
+                        field?.isAccessible() ?: true &&
+                        getter?.isAccessible() ?: true &&
+                        setter?.isAccessible() ?: true
+                is KMemberPropertyImpl<*, R> ->
+                        field?.isAccessible() ?: true &&
+                        getter?.isAccessible() ?: true
                 is KForeignMemberProperty<*, R> -> field.isAccessible()
                 else -> {
                     // Non-member properties always have public visibility on JVM, thus accessible has no effect on them
@@ -34,11 +39,13 @@ public var <R> KProperty<R>.accessible: Boolean
         set(value) {
             when (this) {
                 is KMutableMemberPropertyImpl<*, R> -> {
-                    getter.setAccessible(value)
-                    setter.setAccessible(value)
+                    field?.setAccessible(value)
+                    getter?.setAccessible(value)
+                    setter?.setAccessible(value)
                 }
                 is KMemberPropertyImpl<*, R> -> {
-                    getter.setAccessible(value)
+                    field?.setAccessible(value)
+                    getter?.setAccessible(value)
                 }
                 is KForeignMemberProperty<*, R> -> {
                     field.setAccessible(value)