From 0d89e5d01f86a177a86c20ff5b0a8aeb005f50b1 Mon Sep 17 00:00:00 2001 From: Pavel Talanov Date: Tue, 17 Jan 2012 17:37:40 +0400 Subject: [PATCH] KT-962: Function is called two times safecall expression now compute receiver only once. --- .../google/dart/compiler/util/AstUtil.java | 22 ++++++++++ .../translate/context/DynamicContext.java | 32 ++++++++------- .../declaration/NamespaceTranslator.java | 4 +- .../expression/ExpressionVisitor.java | 40 +++++++++---------- .../translate/expression/ForTranslator.java | 11 +++-- .../org/jetbrains/k2js/test/MiscTest.java | 12 ++++++ .../expression/function/cases/KT-921.kt | 2 +- .../testFiles/expression/misc/cases/KT-962.kt | 8 ++++ .../safecallComputesExpressionOnlyOnce.kt | 14 +++++++ 9 files changed, 101 insertions(+), 44 deletions(-) create mode 100644 translator/testFiles/expression/misc/cases/KT-962.kt create mode 100644 translator/testFiles/expression/misc/cases/safecallComputesExpressionOnlyOnce.kt diff --git a/js/src/com/google/dart/compiler/util/AstUtil.java b/js/src/com/google/dart/compiler/util/AstUtil.java index c47a04abcfd..69ad4c21241 100644 --- a/js/src/com/google/dart/compiler/util/AstUtil.java +++ b/js/src/com/google/dart/compiler/util/AstUtil.java @@ -309,6 +309,28 @@ public class AstUtil { throw new AssertionError("Set qualifier should be applied only to JsInvocation or JsNameRef instances"); } + public static JsExpression getQualifier(JsExpression expression) { + if (expression instanceof JsInvocation) { + return ((JsInvocation) expression).getQualifier(); + } + if (expression instanceof JsNameRef) { + return ((JsNameRef) expression).getQualifier(); + } + throw new AssertionError("Get qualifier should be applied only to JsInvocation or JsNameRef instances"); + } + + public static void substituteTopLevelQualifier(JsExpression expression, JsExpression substitutor) { + assert expression != null; + JsExpression previous = expression; + JsExpression current = getQualifier(expression); + assert current != null : "Cannot be applied to unqualified expression"; + while (getQualifier(current) != null) { + previous = current; + current = getQualifier(current); + } + setQualifier(previous, substitutor); + } + //TODO: look for should-be-usages public static JsNameRef qualified(JsName selector, JsExpression qualifier) { JsNameRef reference = selector.makeRef(); diff --git a/translator/src/org/jetbrains/k2js/translate/context/DynamicContext.java b/translator/src/org/jetbrains/k2js/translate/context/DynamicContext.java index 15ca0eb82ca..79a28f96a0c 100644 --- a/translator/src/org/jetbrains/k2js/translate/context/DynamicContext.java +++ b/translator/src/org/jetbrains/k2js/translate/context/DynamicContext.java @@ -5,54 +5,57 @@ import com.google.dart.compiler.util.AstUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.jet.lang.descriptors.DeclarationDescriptor; +//TODO: track that temporary declarations is in the scope of the block where they are declared + public class DynamicContext { @NotNull public static DynamicContext rootContext(@NotNull NamingScope rootScope, @NotNull JsBlock globalBlock) { - return new DynamicContext(rootScope, globalBlock); + return new DynamicContext(rootScope, rootScope, globalBlock); } @NotNull public static DynamicContext contextWithScope(@NotNull NamingScope scope) { - return new DynamicContext(scope, new JsBlock()); + return new DynamicContext(scope, scope, new JsBlock()); } @NotNull - private NamingScope namingScope; + private NamingScope currentScope; + @NotNull + private NamingScope blockScope; @NotNull private JsBlock currentBlock; - private DynamicContext(@NotNull NamingScope scope, @NotNull JsBlock block) { - this.namingScope = scope; + private DynamicContext(@NotNull NamingScope scope, @NotNull NamingScope blockScope, @NotNull JsBlock block) { + this.currentScope = scope; this.currentBlock = block; + this.blockScope = blockScope; } @NotNull public DynamicContext innerScope(@NotNull JsScope scope) { - return new DynamicContext(namingScope.innerScope(scope), currentBlock); + return new DynamicContext(currentScope.innerScope(scope), blockScope, currentBlock); } - //TODO: @NotNull public DynamicContext innerBlock(@NotNull JsBlock block) { - return new DynamicContext(namingScope, block); + return new DynamicContext(currentScope, currentScope, block); } @NotNull public JsName getLocalName(@NotNull DeclarationDescriptor descriptor) { - JsName name = namingScope.getName(descriptor); + JsName name = currentScope.getName(descriptor); assert name != null : descriptor.getName() + " is not declared. Use isDeclared to check."; return name; } public boolean isDeclared(@NotNull DeclarationDescriptor descriptor) { - return namingScope.isDeclared(descriptor); + return currentScope.isDeclared(descriptor); } @NotNull public TemporaryVariable declareTemporary(@NotNull JsExpression initExpression) { - - JsName temporaryName = namingScope.declareTemporary(); + JsName temporaryName = blockScope.declareTemporary(); JsVars temporaryDeclaration = AstUtil.newVar(temporaryName, null); jsBlock().addVarDeclaration(temporaryDeclaration); return new TemporaryVariable(temporaryName, initExpression); @@ -60,15 +63,14 @@ public class DynamicContext { @NotNull public JsName declareLocalVariable(@NotNull DeclarationDescriptor descriptor) { - return namingScope.declareVariable(descriptor, descriptor.getName()); + return currentScope.declareVariable(descriptor, descriptor.getName()); } @NotNull public JsScope jsScope() { - return namingScope.jsScope(); + return currentScope.jsScope(); } - //TODO: @NotNull public JsBlock jsBlock() { return currentBlock; diff --git a/translator/src/org/jetbrains/k2js/translate/declaration/NamespaceTranslator.java b/translator/src/org/jetbrains/k2js/translate/declaration/NamespaceTranslator.java index f119b4ab657..4ab75f07675 100644 --- a/translator/src/org/jetbrains/k2js/translate/declaration/NamespaceTranslator.java +++ b/translator/src/org/jetbrains/k2js/translate/declaration/NamespaceTranslator.java @@ -16,6 +16,7 @@ import java.util.List; * @author Pavel.Talanov */ //TODO: rework translator to translate everything in the namespace not only in one file +// TEST IT public final class NamespaceTranslator extends AbstractTranslator { @NotNull @@ -66,8 +67,7 @@ public final class NamespaceTranslator extends AbstractTranslator { JsInvocation namespaceDeclaration = namespaceCreateMethodInvocation(); addMemberDeclarations(namespaceDeclaration); addClassesDeclarations(namespaceDeclaration); - return AstUtil.newAssignmentStatement - (namespaceName.makeRef(), namespaceDeclaration); + return AstUtil.newVar(namespaceName, namespaceDeclaration); } private void addClassesDeclarations(@NotNull JsInvocation namespaceDeclaration) { diff --git a/translator/src/org/jetbrains/k2js/translate/expression/ExpressionVisitor.java b/translator/src/org/jetbrains/k2js/translate/expression/ExpressionVisitor.java index b1d6363c44a..5ff7a09b211 100644 --- a/translator/src/org/jetbrains/k2js/translate/expression/ExpressionVisitor.java +++ b/translator/src/org/jetbrains/k2js/translate/expression/ExpressionVisitor.java @@ -8,6 +8,7 @@ import org.jetbrains.jet.lang.psi.*; import org.jetbrains.jet.lang.resolve.BindingContext; import org.jetbrains.jet.lang.resolve.constants.CompileTimeConstant; import org.jetbrains.jet.lang.resolve.constants.NullValue; +import org.jetbrains.k2js.translate.context.TemporaryVariable; import org.jetbrains.k2js.translate.context.TranslationContext; import org.jetbrains.k2js.translate.general.Translation; import org.jetbrains.k2js.translate.general.TranslatorVisitor; @@ -16,7 +17,6 @@ import org.jetbrains.k2js.translate.operation.IncrementTranslator; import org.jetbrains.k2js.translate.operation.UnaryOperationTranslator; import org.jetbrains.k2js.translate.reference.AccessTranslator; import org.jetbrains.k2js.translate.reference.CallTranslator; -import org.jetbrains.k2js.translate.reference.PropertyAccessTranslator; import org.jetbrains.k2js.translate.reference.ReferenceTranslator; import org.jetbrains.k2js.translate.utils.BindingUtils; @@ -247,29 +247,23 @@ public final class ExpressionVisitor extends TranslatorVisitor { if (expression.getSelectorExpression() instanceof JetCallExpression) { return CallTranslator.translate(expression, context); } - return translateQualifiedExpression(expression, context); - } - - @NotNull - private JsExpression translateQualifiedExpression(@NotNull JetQualifiedExpression expression, - @NotNull TranslationContext context) { - if (PropertyAccessTranslator.canBePropertyGetterCall(expression, context)) { - return PropertyAccessTranslator.translateAsPropertyGetterCall(expression, context); - } - return translateAsQualifiedAccess(expression, context); - } - - @NotNull - private JsExpression translateAsQualifiedAccess(@NotNull JetQualifiedExpression expression, - @NotNull TranslationContext context) { JsExpression receiver = translateReceiver(expression, context); JsExpression selector = translateSelector(expression, context); - assert (selector instanceof JsNameRef || selector instanceof JsInvocation) + return composeQualifiedExpression(receiver, selector); + } + + @NotNull + private JsExpression composeQualifiedExpression(@NotNull JsExpression receiver, @NotNull JsExpression selector) { + //TODO: make sure that logic would not break for binary operation. check if there is a way to provide clearer logic + assert (selector instanceof JsNameRef || selector instanceof JsInvocation || selector instanceof JsBinaryOperation) : "Selector should be a name reference or a method invocation in dot qualified expression."; if (selector instanceof JsInvocation) { return translateAsQualifiedInvocation(receiver, (JsInvocation) selector); - } else { + } else if (selector instanceof JsNameRef) { return translateAsQualifiedNameReference(receiver, (JsNameRef) selector); + } else { + ((JsBinaryOperation) selector).setArg1(receiver); + return selector; } } @@ -327,11 +321,13 @@ public final class ExpressionVisitor extends TranslatorVisitor { @NotNull public JsNode visitSafeQualifiedExpression(@NotNull JetSafeQualifiedExpression expression, @NotNull TranslationContext context) { - JsExpression receiver = translateReceiver(expression, context); + TemporaryVariable receiver = context.declareTemporary(translateReceiver(expression, context)); JsNullLiteral nullLiteral = context.program().getNullLiteral(); - JsExpression thenExpression = translateQualifiedExpression(expression, context); - return new JsConditional(notNullCheck(context, receiver), - thenExpression, nullLiteral); + JsExpression selector = translateSelector(expression, context); + JsExpression thenExpression = composeQualifiedExpression(receiver.nameReference(), selector); + JsConditional callMethodIfNotNullConditional + = new JsConditional(notNullCheck(context, receiver.nameReference()), thenExpression, nullLiteral); + return AstUtil.newSequence(receiver.assignmentExpression(), callMethodIfNotNullConditional); } @Override diff --git a/translator/src/org/jetbrains/k2js/translate/expression/ForTranslator.java b/translator/src/org/jetbrains/k2js/translate/expression/ForTranslator.java index e1a56755c2d..503ccc159b1 100644 --- a/translator/src/org/jetbrains/k2js/translate/expression/ForTranslator.java +++ b/translator/src/org/jetbrains/k2js/translate/expression/ForTranslator.java @@ -37,7 +37,6 @@ public final class ForTranslator extends AbstractTranslator { this.expression = forExpression; } - @NotNull private JsBlock translate() { JsName parameterName = declareParameter(); @@ -55,9 +54,13 @@ public final class ForTranslator extends AbstractTranslator { @NotNull private JsBlock generateCycleBody(@NotNull JsName parameterName, @NotNull TemporaryVariable iterator) { - JsStatement parameterAssignment = AstUtil.newAssignmentStatement(parameterName.makeRef(), nextMethodInvocation(iterator)); - JsNode originalBody = Translation.translateExpression(getLoopBody(expression), context()); - return AstUtil.newBlock(parameterAssignment, AstUtil.convertToBlock(originalBody)); + JsBlock cycleBody = new JsBlock(); + JsStatement parameterAssignment = + AstUtil.newVar(parameterName, nextMethodInvocation(iterator)); + JsNode originalBody = Translation.translateExpression(getLoopBody(expression), context().innerBlock(cycleBody)); + cycleBody.addStatement(parameterAssignment); + cycleBody.addStatement(AstUtil.convertToBlock(originalBody)); + return cycleBody; } @NotNull diff --git a/translator/test/org/jetbrains/k2js/test/MiscTest.java b/translator/test/org/jetbrains/k2js/test/MiscTest.java index a398aef0319..3ab5c7b0ef8 100644 --- a/translator/test/org/jetbrains/k2js/test/MiscTest.java +++ b/translator/test/org/jetbrains/k2js/test/MiscTest.java @@ -26,4 +26,16 @@ public class MiscTest extends AbstractExpressionTest { // checkOutput("intRange.kt", " "); testFooBoxIsTrue("intRange.kt"); } + + + @Test + public void safecallComputesExpressionOnlyOnce() throws Exception { + testFooBoxIsTrue("safecallComputesExpressionOnlyOnce.kt"); + } + + @Test + public void KT962() throws Exception { + checkOutput("KT-962.kt", "stdout\n" + + "Hello, world!\n"); + } } diff --git a/translator/testFiles/expression/function/cases/KT-921.kt b/translator/testFiles/expression/function/cases/KT-921.kt index 873e37d2801..05b388369f7 100644 --- a/translator/testFiles/expression/function/cases/KT-921.kt +++ b/translator/testFiles/expression/function/cases/KT-921.kt @@ -61,7 +61,7 @@ fun main(args:Array) v.view(it, {(itemLifetime, item)-> x.add(item) Dump(x) - itemLifetime.attach { x.remove(item); Dump(x) } + itemLifetime.attach { x.remove(item as Any); Dump(x) } }) }) } \ No newline at end of file diff --git a/translator/testFiles/expression/misc/cases/KT-962.kt b/translator/testFiles/expression/misc/cases/KT-962.kt new file mode 100644 index 00000000000..9f435bdf9a4 --- /dev/null +++ b/translator/testFiles/expression/misc/cases/KT-962.kt @@ -0,0 +1,8 @@ +fun stdout(): java.io.PrintStream? { + System.out?.println("stdout") + return System.out +} + +fun main(args : Array) { + stdout()?.println("Hello, world!") +} \ No newline at end of file diff --git a/translator/testFiles/expression/misc/cases/safecallComputesExpressionOnlyOnce.kt b/translator/testFiles/expression/misc/cases/safecallComputesExpressionOnlyOnce.kt new file mode 100644 index 00000000000..e48f4d2da46 --- /dev/null +++ b/translator/testFiles/expression/misc/cases/safecallComputesExpressionOnlyOnce.kt @@ -0,0 +1,14 @@ +package foo + +var i = 0 + +fun test() : Int? = i++ + +fun box() : Boolean { + if (i != 0) return false + test()?.plus(1) + if (i != 1) return false + test()?.minus(2) + if (i != 2) return false + return true +} \ No newline at end of file