KT-11996 Fix issue with referencing outer class in case of inner class constructors and members/properties. Fix issue with referencing outer classes from secondary constructors. Remove unnecessary tests.

2016-04-21 18:03:56 +03:00
parent 1764000bf4
commit 826cee58bd
13 changed files with 90 additions and 106 deletions
@@ -1120,10 +1120,6 @@ fun main(args: Array<String>) {
            model("codegen/box/secondaryConstructors", targetBackend = TargetBackend.JS)
        }

-        testClass<AbstractNestedTypesTest>() {
-            model("codegen/box/classes/inner", targetBackend = TargetBackend.JS)
-        }
-
        testClass<AbstractClassesTest>() {
            model("codegen/box/classes/", targetBackend = TargetBackend.JS)
        }
@@ -172,15 +172,13 @@ public final class ClosureTest extends SingleFileTranslationTest {
        checkFooBoxIsOk();
    }

-    // TODO: tests UsageTracker but encounters issue somewhere else. Uncomment when issue with `this` generation gets fixed
-    // Presumably, KT-11996
-    /*public void testDeepInnerClassInLocalClass() throws Exception {
+    public void testDeepInnerClassInLocalClass() throws Exception {
        checkFooBoxIsOk();
    }

    public void testDeepInnerClassInLocalClassFromExtension() throws Exception {
        checkFooBoxIsOk();
-    }*/
+    }

    public void testImplicitGenericReceiverInExtensionInLocalClass() throws Exception {
        checkFooBoxIsOk();
@@ -86,6 +86,14 @@ public class NestedTypesTest extends SingleFileTranslationTest {
        checkFooBoxIsOk();
    }

+    public void testPrivateFieldNotOverridenInNestedSubclass() throws Exception {
+        checkFooBoxIsOk();
+    }
+
+    public void testOuterClassReferenceFromSecondaryConstructor() throws Exception {
+        checkFooBoxIsOk();
+    }
+
    @NotNull
    @Override
    protected List<String> additionalJsFiles(@NotNull EcmaVersion ecmaVersion) {
@@ -1,73 +0,0 @@
-/*
- * Copyright 2010-2016 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.js.test.semantics;
-
-import com.intellij.testFramework.TestDataPath;
-import org.jetbrains.kotlin.test.JUnit3RunnerWithInners;
-import org.jetbrains.kotlin.test.KotlinTestUtils;
-import org.jetbrains.kotlin.test.TestMetadata;
-import org.junit.runner.RunWith;
-
-import java.io.File;
-import java.util.regex.Pattern;
-
-/** This class is generated by {@link org.jetbrains.kotlin.generators.tests.TestsPackage}. DO NOT MODIFY MANUALLY */
-@SuppressWarnings("all")
-@TestMetadata("compiler/testData/codegen/box/classes/inner")
-@TestDataPath("$PROJECT_ROOT")
-@RunWith(JUnit3RunnerWithInners.class)
-public class NestedTypesTestGenerated extends AbstractNestedTypesTest {
-    public void testAllFilesPresentInInner() throws Exception {
-        KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("compiler/testData/codegen/box/classes/inner"), Pattern.compile("^(.+)\\.kt$"), true);
-    }
-
-    @TestMetadata("instantiateInDerived.kt")
-    public void testInstantiateInDerived() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/instantiateInDerived.kt");
-        doTest(fileName);
-    }
-
-    @TestMetadata("instantiateInDerivedLabeled.kt")
-    public void testInstantiateInDerivedLabeled() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/instantiateInDerivedLabeled.kt");
-        doTest(fileName);
-    }
-
-    @TestMetadata("instantiateInSameClass.kt")
-    public void testInstantiateInSameClass() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/instantiateInSameClass.kt");
-        doTest(fileName);
-    }
-
-    @TestMetadata("kt6708.kt")
-    public void testKt6708() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/kt6708.kt");
-        doTest(fileName);
-    }
-
-    @TestMetadata("properOuter.kt")
-    public void testProperOuter() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/properOuter.kt");
-        doTest(fileName);
-    }
-
-    @TestMetadata("properSuperLinking.kt")
-    public void testProperSuperLinking() throws Exception {
-        String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/box/classes/inner/properSuperLinking.kt");
-        doTest(fileName);
-    }
-}
@@ -50,8 +50,6 @@ abstract class AbstractReservedWordTest : SingleFileTranslationTest("reservedWor

 abstract class AbstractSecondaryConstructorTest : AbstractBlackBoxTest("secondaryConstructors/")

-abstract class AbstractNestedTypesTest : AbstractBlackBoxTest("classes/inner/")
-
 abstract class AbstractInnerNestedTest : AbstractBlackBoxTest("innerNested/")

 abstract class AbstractClassesTest : AbstractBlackBoxTest("classes/")
@@ -21,6 +21,7 @@ import com.google.dart.compiler.backend.js.ast.JsConditional
 import com.google.dart.compiler.backend.js.ast.JsExpression
 import com.google.dart.compiler.backend.js.ast.JsLiteral
 import org.jetbrains.kotlin.descriptors.CallableDescriptor
+import org.jetbrains.kotlin.descriptors.ConstructorDescriptor
 import org.jetbrains.kotlin.descriptors.FunctionDescriptor
 import org.jetbrains.kotlin.diagnostics.DiagnosticUtils
 import org.jetbrains.kotlin.js.translate.context.TranslationContext
@@ -31,6 +32,7 @@ import org.jetbrains.kotlin.js.translate.utils.TranslationUtils
 import org.jetbrains.kotlin.resolve.calls.callUtil.isSafeCall
 import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall
 import org.jetbrains.kotlin.resolve.calls.tasks.ExplicitReceiverKind.*
+import org.jetbrains.kotlin.resolve.scopes.receivers.ImplicitClassReceiver
 import org.jetbrains.kotlin.resolve.scopes.receivers.ReceiverValue

 interface CallInfo {
@@ -99,18 +101,27 @@ fun TranslationContext.getCallInfo(resolvedCall: ResolvedCall<out FunctionDescri
    return FunctionCallInfo(callInfo, argumentsInfo)
 }

-private fun TranslationContext.getDispatchReceiver(receiverValue: ReceiverValue): JsExpression {
-    return getDispatchReceiver(getReceiverParameterForReceiver(receiverValue))
+private fun TranslationContext.getDispatchReceiver(receiverValue: ReceiverValue, allowSuperCall: Boolean): JsExpression {
+    return getDispatchReceiver(getReceiverParameterForReceiver(receiverValue), allowSuperCall)
 }

 private fun TranslationContext.createCallInfo(resolvedCall: ResolvedCall<out CallableDescriptor>, explicitReceivers: ExplicitReceivers): CallInfo {
    val receiverKind = resolvedCall.explicitReceiverKind

+    // I'm not sure if it's a proper code, and why it should work. Just copied similar logic from ExpressionCodegen.generateConstructorCall.
+    // See box/classes/inner/instantiateInDerived.kt
+    // TODO: revisit this code later, write more tests (or borrow them from JVM backend)
+    fun isConstructorCall(): Boolean {
+        val descriptor = resolvedCall.resultingDescriptor.original
+        return descriptor is ConstructorDescriptor && descriptor.containingDeclaration.isInner &&
+               resolvedCall.dispatchReceiver is ImplicitClassReceiver
+    }
+
    fun getDispatchReceiver(): JsExpression? {
        val receiverValue = resolvedCall.dispatchReceiver ?: return null
        return when (receiverKind) {
            DISPATCH_RECEIVER, BOTH_RECEIVERS -> explicitReceivers.extensionOrDispatchReceiver
-            else -> this.getDispatchReceiver(receiverValue)
+            else -> this.getDispatchReceiver(receiverValue, isConstructorCall())
        }
    }

@@ -119,7 +130,7 @@ private fun TranslationContext.createCallInfo(resolvedCall: ResolvedCall<out Cal
        return when (receiverKind) {
            EXTENSION_RECEIVER -> explicitReceivers.extensionOrDispatchReceiver
            BOTH_RECEIVERS -> explicitReceivers.extensionReceiver
-            else -> this.getDispatchReceiver(receiverValue)
+            else -> this.getDispatchReceiver(receiverValue, isConstructorCall())
        }
    }

@@ -339,7 +339,17 @@ public class TranslationContext {
    }

    @NotNull
-    public JsExpression getDispatchReceiver(@NotNull ReceiverParameterDescriptor descriptor) {
+    public JsExpression getDispatchReceiver(@NotNull ReceiverParameterDescriptor descriptor, boolean allowSuperCall) {
+        // I don't see any reason for descriptor being treated inconsistently for different cases of call.
+        // descriptor should always point on the exact class. I.e., in the code
+        //
+        // class A { inner class B : A { foo() } }
+        //
+        // implicit receiver for `foo` must always point to either `A` or `B` depending on where `foo()` is picked by resolver.
+        // However, it's not always true. According to ExpressionCodegen, this rule is violated in the case of constructor call.
+        // It's reasonable since there won't be any ambiguity in this case, but it's simply inconsistent.
+        // TODO: avoid `allowSuperCall` by convincing people to alter behaviour of frontend
+
        JsExpression alias = getAliasForDescriptor(descriptor);
        if (alias != null) {
            return alias;
@@ -362,16 +372,13 @@ public class TranslationContext {

        ClassDescriptor cls = (ClassDescriptor) classifier;

-        JsExpression receiver = getAliasForDescriptor(cls.getThisAsReceiverParameter());
-
-        // I could reproduce situation when receiver == null, although there's no test to reproduce it. Have no time for investigation
-        // Hint: try generating field with getter/setter, see deepInnerClassInLocalClass.kt
-        // TODO: revisit this code later
+        assert classDescriptor != null : "Can't get ReceiverParameterDescriptor in top level";
+        JsExpression receiver = getAliasForDescriptor(classDescriptor.getThisAsReceiverParameter());
        if (receiver == null) {
            receiver = JsLiteral.THIS;
        }

-        return getDispatchReceiverPath(cls, receiver);
+        return getDispatchReceiverPath(cls, receiver, allowSuperCall);
    }

    private boolean isConstructorOrDirectScope(DeclarationDescriptor descriptor) {
@@ -384,7 +391,7 @@ public class TranslationContext {
    }

    @NotNull
-    private JsExpression getDispatchReceiverPath(@Nullable ClassDescriptor cls, JsExpression thisExpression) {
+    private JsExpression getDispatchReceiverPath(@Nullable ClassDescriptor cls, JsExpression thisExpression, boolean allowSuperCall) {
        if (cls != null) {
            JsExpression alias = getAliasForDescriptor(cls);
            if (alias != null) {
@@ -392,17 +399,16 @@ public class TranslationContext {
            }
        }
        if (classDescriptor == cls ||
-            (classDescriptor != null &&
-             cls != null && DescriptorUtils.isSubclass(classDescriptor, cls)) ||
+            (allowSuperCall && classDescriptor != null && cls != null && DescriptorUtils.isSubclass(classDescriptor, cls)) ||
            parent == null) {
            return thisExpression;
        }
        ClassDescriptor parentDescriptor = parent.classDescriptor;
        if (classDescriptor != parentDescriptor) {
-            return new JsNameRef(Namer.OUTER_FIELD_NAME, parent.getDispatchReceiverPath(cls, thisExpression));
+            return new JsNameRef(Namer.OUTER_FIELD_NAME, parent.getDispatchReceiverPath(cls, thisExpression, allowSuperCall));
        }
        else {
-            return parent.getDispatchReceiverPath(cls, thisExpression);
+            return parent.getDispatchReceiverPath(cls, thisExpression, allowSuperCall);
        }
    }

@@ -496,7 +502,7 @@ public class TranslationContext {
        JsExpression alias = getAliasForDescriptor(descriptor);
        if (alias != null) return alias;
        if (descriptor instanceof ReceiverParameterDescriptor) {
-            return getDispatchReceiver((ReceiverParameterDescriptor) descriptor);
+            return getDispatchReceiver((ReceiverParameterDescriptor) descriptor, false);
        }
        return getNameForDescriptor(descriptor).makeRef();
    }
@@ -468,7 +468,8 @@ public final class ExpressionVisitor extends TranslatorVisitor<JsNode> {
                getDescriptorForReferenceExpression(context.bindingContext(), expression.getInstanceReference());
        assert thisExpression != null : "This expression must reference a descriptor: " + expression.getText();

-        return context.getDispatchReceiver(getReceiverParameterForDeclaration(thisExpression)).source(expression);
+        // TODO: not sure if `false` is a proper argument here, revisit this code later
+        return context.getDispatchReceiver(getReceiverParameterForDeclaration(thisExpression), false).source(expression);
    }

    @Override
@@ -483,7 +484,9 @@ public final class ExpressionVisitor extends TranslatorVisitor<JsNode> {
    public JsNode visitSuperExpression(@NotNull KtSuperExpression expression, @NotNull TranslationContext context) {
        DeclarationDescriptor superTarget = getSuperTarget(context, expression);
        ReceiverParameterDescriptor receiver = getReceiverParameterForDeclaration(superTarget);
-        return context.getDispatchReceiver(receiver);
+
+        // TODO: not sure if `true` is a proper argument here, revisit this code later
+        return context.getDispatchReceiver(receiver, true);
    }

    @Override
@@ -156,7 +156,7 @@ public final class TranslationUtils {
            receiver = JsLiteral.THIS;
        }
        else {
-            receiver = context.getDispatchReceiver(JsDescriptorUtils.getReceiverParameterForDeclaration(containingDescriptor));
+            receiver = context.getDispatchReceiver(JsDescriptorUtils.getReceiverParameterForDeclaration(containingDescriptor), false);
        }
        return new JsNameRef(backingFieldName, receiver);
    }
@@ -3,13 +3,13 @@ package foo
 class A() {
    fun test(): Int {
        open class B(open val x: Int) {
-            inner class C(override val x: Int) : B(x * 10) {
+            inner class C(x: Int) : B(x * 10) {
                inner class D() {
                    var baz: () -> Int = { 0 }
                    constructor(b: Boolean) : this() {
                        baz = { x + this@B.x }
                    }
-                    fun bar() = { x + this@B.x }
+                    fun bar() = { 100 * (x + this@B.x) }
                }
            }
        }
@@ -3,7 +3,7 @@ package foo
 class A() {
    fun test(): Int {
        open class B(open val x: Int) {
-            inner class C(override val x: Int) : B(x * 10) {
+            inner class C(x: Int) : B(x * 10) {
                inner class D() {
                    fun baz() = bar()
                }
@@ -0,0 +1,19 @@
+package foo
+
+class A(val x: Int) {
+    inner class B() {
+        inner class C() {
+            var result = 0
+
+            constructor(y: Boolean) : this() {
+                result = x
+            }
+        }
+    }
+}
+
+fun box(): String {
+    assertEquals(23, A(23).B().C(true).result)
+
+    return "OK"
+}
@@ -0,0 +1,18 @@
+package foo
+
+open class A(private val bar: String = "1") {
+    inner class B : A("2") {
+        fun foo(a: A): String {
+            return bar + a.bar
+        }
+    }
+}
+
+fun box(): String {
+    var r = ""
+
+    r = A().B().foo(A("3"))
+    if (r != "13") return "r = $r"
+
+    return "OK"
+}