From 91e83c286004e57b9baa0f51fa877bad5c64719c Mon Sep 17 00:00:00 2001 From: "Natalia.Ukhorskaya" Date: Fri, 2 Nov 2012 11:49:10 +0400 Subject: [PATCH] Wrong instruction for invoke private function #KT-2202 Fixed --- .../jet/compiler/android/SpecialFiles.java | 2 - .../jetbrains/jet/codegen/CodegenUtil.java | 15 ++++ .../jet/codegen/ExpressionCodegen.java | 81 +++++++++++++------ .../jet/codegen/FunctionCodegen.java | 9 ++- .../codegen/ImplementationBodyCodegen.java | 9 ++- .../org/jetbrains/jet/codegen/StackValue.java | 26 +++--- .../jet/codegen/intrinsics/PsiMethodCall.java | 2 +- .../jet/codegen/state/JetTypeMapper.java | 22 ++++- compiler/testData/codegen/invokespecial.kt | 45 +++++++++++ .../testData/codegen/properties/kt2202.kt | 18 +++++ .../jet/codegen/NamespaceGenTest.java | 4 + .../jet/codegen/PropertyGenTest.java | 8 ++ 12 files changed, 191 insertions(+), 50 deletions(-) create mode 100644 compiler/testData/codegen/invokespecial.kt create mode 100644 compiler/testData/codegen/properties/kt2202.kt diff --git a/compiler/android-tests/tests/org/jetbrains/jet/compiler/android/SpecialFiles.java b/compiler/android-tests/tests/org/jetbrains/jet/compiler/android/SpecialFiles.java index 116d1df684f..b7aafbb7113 100644 --- a/compiler/android-tests/tests/org/jetbrains/jet/compiler/android/SpecialFiles.java +++ b/compiler/android-tests/tests/org/jetbrains/jet/compiler/android/SpecialFiles.java @@ -120,8 +120,6 @@ public class SpecialFiles { excludedFiles.add("kt684.jet"); // StackOverflow with StringBuilder (escape()) - excludedFiles.add("kt1199.kt"); // Bug KT-2202 - excludedFiles.add("kt1779.kt"); // Bug KT-2202 - private fun tryToComputeNext() in AbstractIterator.kt excludedFiles.add("kt344.jet"); // Bug KT-2251 excludedFiles.add("kt529.kt"); // Bug diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/CodegenUtil.java b/compiler/backend/src/org/jetbrains/jet/codegen/CodegenUtil.java index fa23b4898f6..7833393ae35 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/CodegenUtil.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/CodegenUtil.java @@ -22,6 +22,8 @@ import com.intellij.util.containers.Stack; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.jet.codegen.binding.CalculatedClosure; +import org.jetbrains.jet.codegen.context.CodegenContext; +import org.jetbrains.jet.codegen.context.NamespaceContext; import org.jetbrains.jet.codegen.signature.BothSignatureWriter; import org.jetbrains.jet.codegen.signature.JvmMethodParameterKind; import org.jetbrains.jet.codegen.signature.JvmMethodSignature; @@ -252,4 +254,17 @@ public class CodegenUtil { .addAll(innerClassesScope.getObjectDescriptors()) .build(); } + + public static boolean isCallInsideSameClassAsDeclared(CallableMemberDescriptor declarationDescriptor, CodegenContext context) { + boolean isFakeOverride = declarationDescriptor.getKind() == CallableMemberDescriptor.Kind.FAKE_OVERRIDE; + boolean isDelegate = declarationDescriptor.getKind() == CallableMemberDescriptor.Kind.DELEGATION; + + DeclarationDescriptor containingDeclaration = declarationDescriptor.getContainingDeclaration(); + containingDeclaration = containingDeclaration.getOriginal(); + + return !isFakeOverride && !isDelegate && + (((context.hasThisDescriptor() && containingDeclaration == context.getThisDescriptor()) || + (context.getParentContext() instanceof NamespaceContext && context.getParentContext().getContextDescriptor() == containingDeclaration)) + && context.getContextKind() != OwnerKind.TRAIT_IMPL); + } } diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java b/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java index 0d34f87cffc..90adc6bc381 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/ExpressionCodegen.java @@ -1521,16 +1521,9 @@ public class ExpressionCodegen extends JetVisitor implem boolean isStatic = containingDeclaration instanceof NamespaceDescriptor; boolean overridesTrait = isOverrideForTrait(propertyDescriptor); boolean isFakeOverride = propertyDescriptor.getKind() == CallableMemberDescriptor.Kind.FAKE_OVERRIDE; - boolean isDelegate = propertyDescriptor.getKind() == CallableMemberDescriptor.Kind.DELEGATION; PropertyDescriptor initialDescriptor = propertyDescriptor; propertyDescriptor = initialDescriptor.getOriginal(); - boolean isInsideClass = !isFakeOverride && - !isDelegate && - (((containingDeclaration == null && !context.hasThisDescriptor() || - context.hasThisDescriptor() && containingDeclaration == context.getThisDescriptor()) || - (context.getParentContext() instanceof NamespaceContext) && - context.getParentContext().getContextDescriptor() == containingDeclaration) - && contextKind() != OwnerKind.TRAIT_IMPL); + boolean isInsideClass = isCallInsideSameClassAsDeclared(propertyDescriptor, context); Method getter = null; Method setter = null; if (!forceField) { @@ -1594,32 +1587,60 @@ public class ExpressionCodegen extends JetVisitor implem } } - int invokeOpcode; + int getterInvokeOpcode; + int setterInvokeOpcode; JvmClassName owner; JvmClassName ownerParam; boolean isInterface; - if (isInsideClass || isStatic || propertyDescriptor.getGetter() == null) { - owner = ownerParam = typeMapper.getOwner(propertyDescriptor, contextKind()); + if (isStatic) { isInterface = overridesTrait; - invokeOpcode = isStatic ? INVOKESTATIC : - overridesTrait ? INVOKEINTERFACE - : INVOKEVIRTUAL; + owner = ownerParam = typeMapper.getOwner(propertyDescriptor, contextKind()); + + getterInvokeOpcode = INVOKESTATIC; + setterInvokeOpcode = INVOKESTATIC; } else { isInterface = isInterface(containingDeclaration) || overridesTrait; - // TODO ugly - CallableMethod callableMethod = typeMapper.mapToCallableMethod(propertyDescriptor.getGetter(), isSuper, contextKind()); - invokeOpcode = callableMethod.getInvokeOpcode(); - owner = isFakeOverride && !overridesTrait && !isInterface(initialDescriptor.getContainingDeclaration()) - ? JvmClassName.byType(typeMapper.mapType( - ((ClassDescriptor) initialDescriptor.getContainingDeclaration()).getDefaultType(), JetTypeMapperMode.IMPL)) - : callableMethod.getOwner(); - ownerParam = callableMethod.getDefaultImplParam(); + CallableMethod callableGetter = null; + CallableMethod callableSetter = null; + if (getter == null) { + getterInvokeOpcode = getOpcodeForPropertyDescriptorWithoutAccessor(propertyDescriptor); + } + else { + callableGetter = typeMapper.mapToCallableMethod(propertyDescriptor.getGetter(), isSuper, isInsideClass, contextKind()); + getterInvokeOpcode = callableGetter.getInvokeOpcode(); + } + + if (setter == null) { + setterInvokeOpcode = getOpcodeForPropertyDescriptorWithoutAccessor(propertyDescriptor); + } + else { + callableSetter = typeMapper.mapToCallableMethod(propertyDescriptor.getSetter(), isSuper, isInsideClass, contextKind()); + setterInvokeOpcode = callableSetter.getInvokeOpcode(); + } + + CallableMethod callableMethod = callableGetter != null ? callableGetter : callableSetter; + if (callableMethod == null) { + owner = ownerParam = typeMapper.getOwner(propertyDescriptor, contextKind()); + } + else { + owner = isFakeOverride && !overridesTrait && !isInterface(initialDescriptor.getContainingDeclaration()) + ? JvmClassName.byType(typeMapper.mapType( + ((ClassDescriptor) initialDescriptor.getContainingDeclaration()).getDefaultType(), JetTypeMapperMode.IMPL)) + : callableMethod.getOwner(); + ownerParam = callableMethod.getDefaultImplParam(); + } } return StackValue.property(propertyDescriptor, owner, ownerParam, asmType(propertyDescriptor.getType()), - isStatic, isInterface, isSuper, getter, setter, invokeOpcode, state); + isStatic, isInterface, isSuper, getter, setter, getterInvokeOpcode, setterInvokeOpcode, state); + } + + private static int getOpcodeForPropertyDescriptorWithoutAccessor(PropertyDescriptor descriptor) { + return isOverrideForTrait(descriptor) + ? INVOKEINTERFACE + : descriptor.getVisibility() == Visibilities.PRIVATE ? INVOKESPECIAL : INVOKEVIRTUAL; } private static boolean isOverrideForTrait(CallableMemberDescriptor propertyDescriptor) { @@ -1807,7 +1828,7 @@ public class ExpressionCodegen extends JetVisitor implem callableMethod = typeMapper.asCallableMethod(invoke); } else { - callableMethod = typeMapper.mapToCallableMethod(fd, superCall, OwnerKind.IMPLEMENTATION); + callableMethod = typeMapper.mapToCallableMethod(fd, superCall, isCallInsideSameClassAsDeclared(fd, context), OwnerKind.IMPLEMENTATION); } return callableMethod; } @@ -2899,7 +2920,11 @@ public class ExpressionCodegen extends JetVisitor implem Type type = asmType(javaClass.getDefaultType()); v.anew(type); v.dup(); - final CallableMethod callableMethod = typeMapper.mapToCallableMethod(descriptor, false, OwnerKind.IMPLEMENTATION); + final CallableMethod callableMethod = typeMapper.mapToCallableMethod( + descriptor, + false, + isCallInsideSameClassAsDeclared(descriptor, context), + OwnerKind.IMPLEMENTATION); invokeMethodWithArguments(callableMethod, expression, StackValue.none()); return type; } @@ -2997,7 +3022,11 @@ public class ExpressionCodegen extends JetVisitor implem } } else { - CallableMethod accessor = typeMapper.mapToCallableMethod(operationDescriptor, false, OwnerKind.IMPLEMENTATION); + CallableMethod accessor = typeMapper.mapToCallableMethod( + operationDescriptor, + false, + isCallInsideSameClassAsDeclared(operationDescriptor, context), + OwnerKind.IMPLEMENTATION); boolean isGetter = accessor.getSignature().getAsmMethod().getName().equals("get"); diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/FunctionCodegen.java b/compiler/backend/src/org/jetbrains/jet/codegen/FunctionCodegen.java index 1870deee473..6f684ee67d2 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/FunctionCodegen.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/FunctionCodegen.java @@ -77,8 +77,11 @@ public class FunctionCodegen extends GenerationStateAware { final SimpleFunctionDescriptor functionDescriptor = bindingContext.get(BindingContext.FUNCTION, f); assert functionDescriptor != null; JvmMethodSignature method = - typeMapper.mapToCallableMethod(functionDescriptor, false, owner.getContextKind()) - .getSignature(); + typeMapper.mapToCallableMethod( + functionDescriptor, + false, + isCallInsideSameClassAsDeclared(functionDescriptor, owner), + owner.getContextKind()).getSignature(); generateMethod(f, method, true, null, functionDescriptor); } @@ -391,7 +394,7 @@ public class FunctionCodegen extends GenerationStateAware { MethodVisitor mv ) { if (jvmSignature == null) { - jvmSignature = state.getTypeMapper().mapToCallableMethod(functionDescriptor, false, OwnerKind.IMPLEMENTATION).getSignature(); + jvmSignature = state.getTypeMapper().mapToCallableMethod(functionDescriptor, false, false, OwnerKind.IMPLEMENTATION).getSignature(); } List paramDescrs = functionDescriptor.getValueParameters(); diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/ImplementationBodyCodegen.java b/compiler/backend/src/org/jetbrains/jet/codegen/ImplementationBodyCodegen.java index fc60dae974f..edd8389d3b0 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/ImplementationBodyCodegen.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/ImplementationBodyCodegen.java @@ -1318,8 +1318,11 @@ public class ImplementationBodyCodegen extends ClassBodyCodegen { final MethodVisitor mv = v.newMethod(myClass, flags, function.getName(), function.getDescriptor(), null, null); AnnotationCodegen.forMethod(mv, state.getTypeMapper()).genAnnotations(fun); - JvmMethodSignature jvmSignature = - typeMapper.mapToCallableMethod(inheritedFun, false, OwnerKind.IMPLEMENTATION).getSignature(); + JvmMethodSignature jvmSignature = typeMapper.mapToCallableMethod( + inheritedFun, + false, + isCallInsideSameClassAsDeclared(inheritedFun, context), + OwnerKind.IMPLEMENTATION).getSignature(); JetMethodAnnotationWriter aw = JetMethodAnnotationWriter.visitAnnotation(mv); int kotlinFlags = getFlagsForVisibility(fun.getVisibility()); if (fun instanceof PropertyAccessorDescriptor) { @@ -1556,7 +1559,7 @@ public class ImplementationBodyCodegen extends ClassBodyCodegen { JvmClassName owner = typeMapper.getOwner(propertyDescriptor, OwnerKind.IMPLEMENTATION); Type propType = typeMapper.mapType(jetType); StackValue.property(propertyDescriptor, owner, owner, - propType, false, false, false, null, null, 0, state).store(propType, iv); + propType, false, false, false, null, null, 0, 0, state).store(propType, iv); } } } diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/StackValue.java b/compiler/backend/src/org/jetbrains/jet/codegen/StackValue.java index d333b71781d..9191692a23b 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/StackValue.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/StackValue.java @@ -161,10 +161,11 @@ public abstract class StackValue { boolean isSuper, @Nullable Method getter, @Nullable Method setter, - int invokeOpcode, + int getterInvokeOpcode, + int setterInvokeOpcode, GenerationState state ) { - return new Property(descriptor, methodOwner, methodOwnerParam, getter, setter, isStatic, isInterface, isSuper, type, invokeOpcode, + return new Property(descriptor, methodOwner, methodOwnerParam, getter, setter, isStatic, isInterface, isSuper, type, getterInvokeOpcode, setterInvokeOpcode, state); } @@ -940,7 +941,8 @@ public abstract class StackValue { private final boolean isStatic; private final boolean isInterface; private final boolean isSuper; - private final int invokeOpcode; + private final int getterInvokeOpcode; + private final int setterInvokeOpcode; @NotNull private final PropertyDescriptor descriptor; @NotNull @@ -949,7 +951,7 @@ public abstract class StackValue { public Property( @NotNull PropertyDescriptor descriptor, @NotNull JvmClassName methodOwner, @NotNull JvmClassName methodOwnerParam, @Nullable Method getter, @Nullable Method setter, boolean isStatic, boolean isInterface, boolean isSuper, - @NotNull Type type, int invokeOpcode, @NotNull GenerationState state + @NotNull Type type, int getterInvokeOpcode, int setterInvokeOpcode, @NotNull GenerationState state ) { super(type); this.methodOwner = methodOwner; @@ -959,13 +961,15 @@ public abstract class StackValue { this.isStatic = isStatic; this.isInterface = isInterface; this.isSuper = isSuper; - this.invokeOpcode = invokeOpcode; + this.getterInvokeOpcode = getterInvokeOpcode; + this.setterInvokeOpcode = setterInvokeOpcode; this.descriptor = descriptor; this.state = state; - if (invokeOpcode == 0) { - if (setter != null || getter != null) { - throw new IllegalArgumentException(); - } + if (getterInvokeOpcode == 0 && getter != null) { + throw new IllegalArgumentException(); + } + if (setterInvokeOpcode == 0 && setter != null) { + throw new IllegalArgumentException(); } } @@ -983,7 +987,7 @@ public abstract class StackValue { genNotNullAssertionForField(v, state, descriptor); } else { - v.visitMethodInsn(invokeOpcode, methodOwner.getInternalName(), getter.getName(), getter.getDescriptor()); + v.visitMethodInsn(getterInvokeOpcode, methodOwner.getInternalName(), getter.getName(), getter.getDescriptor()); } } coerceTo(type, v); @@ -1002,7 +1006,7 @@ public abstract class StackValue { this.type.getDescriptor()); } else { - v.visitMethodInsn(invokeOpcode, methodOwner.getInternalName(), setter.getName(), setter.getDescriptor()); + v.visitMethodInsn(setterInvokeOpcode, methodOwner.getInternalName(), setter.getName(), setter.getDescriptor()); } } diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/PsiMethodCall.java b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/PsiMethodCall.java index 84c006e610f..813b40ec570 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/PsiMethodCall.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/intrinsics/PsiMethodCall.java @@ -50,7 +50,7 @@ public class PsiMethodCall implements IntrinsicMethod { List arguments, StackValue receiver, @NotNull GenerationState state ) { final CallableMethod callableMethod = - state.getTypeMapper().mapToCallableMethod(method, false, OwnerKind.IMPLEMENTATION); + state.getTypeMapper().mapToCallableMethod(method, false, false, OwnerKind.IMPLEMENTATION); if(element instanceof JetBinaryExpression) { codegen.invokeMethodWithArguments(callableMethod, receiver, ((JetBinaryExpression)element).getOperationReference()); } else { diff --git a/compiler/backend/src/org/jetbrains/jet/codegen/state/JetTypeMapper.java b/compiler/backend/src/org/jetbrains/jet/codegen/state/JetTypeMapper.java index 23b871d0a69..81ace0e8fb6 100644 --- a/compiler/backend/src/org/jetbrains/jet/codegen/state/JetTypeMapper.java +++ b/compiler/backend/src/org/jetbrains/jet/codegen/state/JetTypeMapper.java @@ -385,7 +385,12 @@ public class JetTypeMapper extends BindingTraceAware { } } - public CallableMethod mapToCallableMethod(@NotNull FunctionDescriptor functionDescriptor, boolean superCall, OwnerKind kind) { + public CallableMethod mapToCallableMethod( + @NotNull FunctionDescriptor functionDescriptor, + boolean superCall, + boolean isInsideClass, + OwnerKind kind + ) { final DeclarationDescriptor functionParent = functionDescriptor.getOriginal().getContainingDeclaration(); functionDescriptor = unwrapFakeOverride(functionDescriptor); @@ -444,10 +449,19 @@ public class JetTypeMapper extends BindingTraceAware { ownerForDefaultParam = JvmClassName.byType(mapType(declarationOwner.getDefaultType(), JetTypeMapperMode.TYPE_PARAMETER)); ownerForDefaultImpl = JvmClassName.byInternalName( ownerForDefaultParam.getInternalName() + (originalIsInterface ? JvmAbi.TRAIT_IMPL_SUFFIX : "")); + if (isInterface) { + invokeOpcode = superCall ? INVOKESTATIC : INVOKEINTERFACE; + } + else { + if (isAccessor) { + invokeOpcode = INVOKESTATIC; + } + else { + boolean isPrivateFunInvocation = isInsideClass && functionDescriptor.getVisibility() == Visibilities.PRIVATE; + invokeOpcode = superCall || isPrivateFunInvocation ? INVOKESPECIAL : INVOKEVIRTUAL; + } + } - invokeOpcode = isInterface - ? (superCall ? INVOKESTATIC : INVOKEINTERFACE) - : (isAccessor ? INVOKESTATIC : (superCall ? INVOKESPECIAL : INVOKEVIRTUAL)); if (isInterface && superCall) { descriptor = mapSignature(functionDescriptor, false, OwnerKind.TRAIT_IMPL); owner = JvmClassName.byInternalName(owner.getInternalName() + JvmAbi.TRAIT_IMPL_SUFFIX); diff --git a/compiler/testData/codegen/invokespecial.kt b/compiler/testData/codegen/invokespecial.kt new file mode 100644 index 00000000000..f8c65f91ca6 --- /dev/null +++ b/compiler/testData/codegen/invokespecial.kt @@ -0,0 +1,45 @@ +// KT-2202 Wrong instruction for invoke private setter + +class A { + private fun f1() { } + fun foo() { + f1() + } +} + +class B { + private val foo = 1 + get + + fun foo() { + foo + } +} + +class C { + private var foo = 1 + get + set + + fun foo() { + foo = 2 + foo + } +} + +class D { + var foo = 1 + private set + + fun foo() { + foo = 2 + } +} + +fun box(): String { + A().foo() + B().foo() + C().foo() + D().foo() + return "OK" +} diff --git a/compiler/testData/codegen/properties/kt2202.kt b/compiler/testData/codegen/properties/kt2202.kt new file mode 100644 index 00000000000..c8c3801bead --- /dev/null +++ b/compiler/testData/codegen/properties/kt2202.kt @@ -0,0 +1,18 @@ +// KT-2202 Wrong instruction for invoke private setter + +class A { + private fun f1() {} + + fun foo() { + f1() + } +} + +class B { + var foo = 1 + private set + + fun foo() { + foo = 2 + } +} \ No newline at end of file diff --git a/compiler/tests/org/jetbrains/jet/codegen/NamespaceGenTest.java b/compiler/tests/org/jetbrains/jet/codegen/NamespaceGenTest.java index c7534408427..e545171255b 100644 --- a/compiler/tests/org/jetbrains/jet/codegen/NamespaceGenTest.java +++ b/compiler/tests/org/jetbrains/jet/codegen/NamespaceGenTest.java @@ -549,4 +549,8 @@ public class NamespaceGenTest extends CodegenTestCase { public void testPrivateTopLevelPropAndVarInInner() { blackBoxFile("privateTopLevelPropAndVarInInner.kt"); } + + public void testInvokeSpecial() { + blackBoxFile("invokespecial.kt"); + } } diff --git a/compiler/tests/org/jetbrains/jet/codegen/PropertyGenTest.java b/compiler/tests/org/jetbrains/jet/codegen/PropertyGenTest.java index 9f518a6498f..b596ae14e7d 100644 --- a/compiler/tests/org/jetbrains/jet/codegen/PropertyGenTest.java +++ b/compiler/tests/org/jetbrains/jet/codegen/PropertyGenTest.java @@ -376,4 +376,12 @@ public class PropertyGenTest extends CodegenTestCase { blackBoxFile("properties/accessToPrivateSetter.kt"); } + public void testKt2202() throws Exception { + createEnvironmentWithMockJdkAndIdeaAnnotations(ConfigurationKind.JDK_ONLY); + loadFile("properties/kt2202.kt"); + String text = generateToText(); + assertFalse(text.contains("INVOKEVIRTUAL")); + assertTrue(text.contains("INVOKESPECIAL")); + } + }