JS: fix translation of reassignment of inline properties. See KT-15590

compiler/testData/codegen/boxInline/property/augAssignmentAndIncInClass.kt

@@ -1,5 +1,4 @@
 // FILE: 1.kt
-// IGNORE_BACKEND: JS
 package test
 
 class A {

compiler/testData/codegen/boxInline/property/augAssignmentAndIncInClassViaConvention.kt

@@ -1,5 +1,4 @@
 // FILE: 1.kt
-// IGNORE_BACKEND: JS
 package test
 
 class Test(var result: Int)

js/js.inliner/src/org/jetbrains/kotlin/js/inline/DummyAccessorInvocationTransformer.kt

@@ -0,0 +1,68 @@
+/*
+ * Copyright 2010-2017 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.inline
+
+import org.jetbrains.kotlin.descriptors.PropertyGetterDescriptor
+import org.jetbrains.kotlin.descriptors.PropertySetterDescriptor
+import org.jetbrains.kotlin.js.backend.ast.*
+import org.jetbrains.kotlin.js.backend.ast.metadata.descriptor
+import org.jetbrains.kotlin.js.backend.ast.metadata.inlineStrategy
+import org.jetbrains.kotlin.js.backend.ast.metadata.psiElement
+
+class DummyAccessorInvocationTransformer : JsVisitorWithContextImpl() {
+    override fun endVisit(x: JsNameRef, ctx: JsContext<in JsNode>) {
+        super.endVisit(x, ctx)
+        val dummy = tryCreatePropertyGetterInvocation(x)
+        if (dummy != null) {
+            ctx.replaceMe(dummy)
+        }
+    }
+
+    override fun endVisit(x: JsBinaryOperation, ctx: JsContext<in JsNode>) {
+        super.endVisit(x, ctx)
+        val dummy = tryCreatePropertySetterInvocation(x)
+        if (dummy != null) {
+            ctx.replaceMe(dummy)
+        }
+    }
+
+    private fun tryCreatePropertyGetterInvocation(x: JsNameRef): JsInvocation? {
+        if (x.inlineStrategy != null && x.descriptor is PropertyGetterDescriptor) {
+            val dummyInvocation = JsInvocation(x)
+            copyInlineMetadata(x, dummyInvocation)
+            return dummyInvocation
+        }
+        return null
+    }
+
+    private fun tryCreatePropertySetterInvocation(x: JsBinaryOperation): JsInvocation? {
+        if (!x.operator.isAssignment || x.arg1 !is JsNameRef) return null
+        val name = x.arg1 as JsNameRef
+        if (name.inlineStrategy != null && name.descriptor is PropertySetterDescriptor) {
+            val dummyInvocation = JsInvocation(name, x.arg2)
+            copyInlineMetadata(name, dummyInvocation)
+            return dummyInvocation
+        }
+        return null
+    }
+
+    private fun copyInlineMetadata(from: JsNameRef, to: JsInvocation) {
+        to.inlineStrategy = from.inlineStrategy
+        to.descriptor = from.descriptor
+        to.psiElement = from.psiElement
+    }
+}

js/js.inliner/src/org/jetbrains/kotlin/js/inline/JsInliner.java

@@ -21,8 +21,6 @@ import kotlin.jvm.functions.Function1;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.kotlin.descriptors.CallableDescriptor;
-import org.jetbrains.kotlin.descriptors.PropertyGetterDescriptor;
-import org.jetbrains.kotlin.descriptors.PropertySetterDescriptor;
 import org.jetbrains.kotlin.diagnostics.DiagnosticSink;
 import org.jetbrains.kotlin.diagnostics.Errors;
 import org.jetbrains.kotlin.js.backend.ast.*;
@@ -71,6 +69,7 @@ public class JsInliner extends JsVisitorWithContextImpl {
         JsProgram program = context.program();
         Map<JsName, JsFunction> functions = CollectUtilsKt.collectNamedFunctions(program);
         Map<String, JsFunction> accessors = CollectUtilsKt.collectAccessors(program);
+        new DummyAccessorInvocationTransformer().accept(program);
         JsInliner inliner = new JsInliner(functions, accessors, new FunctionReader(context), context.bindingTrace());
         inliner.accept(program);
         RemoveUnusedFunctionDefinitionsKt.removeUnusedFunctionDefinitions(program, functions);
@@ -89,75 +88,6 @@ public class JsInliner extends JsVisitorWithContextImpl {
         this.trace = trace;
     }
 
-    @Override
-    public boolean visit(@NotNull JsNameRef x, @NotNull JsContext ctx) {
-        JsInvocation dummy = tryCreatePropertyGetterInvocation(x);
-        if (dummy != null) {
-            return visit(dummy, ctx);
-        }
-        return super.visit(x, ctx);
-    }
-
-    @Override
-    public void endVisit(@NotNull JsNameRef x, @NotNull JsContext ctx) {
-        JsInvocation dummy = tryCreatePropertyGetterInvocation(x);
-        if (dummy != null) {
-            endVisit(dummy, ctx);
-        }
-        super.visit(x, ctx);
-    }
-
-    @Override
-    public boolean visit(@NotNull JsBinaryOperation x, @NotNull JsContext ctx) {
-        JsInvocation dummy = tryCreatePropertySetterInvocation(x);
-        if (dummy != null) {
-            return visit(dummy, ctx);
-        }
-        return super.visit(x, ctx);
-    }
-
-    @Override
-    public void endVisit(@NotNull JsBinaryOperation x, @NotNull JsContext ctx) {
-        JsInvocation dummy = tryCreatePropertySetterInvocation(x);
-        if (dummy != null) {
-            // Prevent FunctionInlineMutator from creating a variable for the result (there is none, because assignment is a statement)
-            // TODO is there a better way to achieve this?
-            getInliningContext().getStatementContext().replaceMe(new JsExpressionStatement(dummy));
-            endVisit(dummy, ctx);
-        }
-        super.visit(x, ctx);
-    }
-
-    @Nullable
-    private static JsInvocation tryCreatePropertyGetterInvocation(@NotNull JsNameRef x) {
-        if (MetadataProperties.getInlineStrategy(x) != null && MetadataProperties.getDescriptor(x) instanceof PropertyGetterDescriptor) {
-            JsInvocation dummyInvocation = new JsInvocation(x);
-            copyInlineMetadata(x, dummyInvocation);
-            return dummyInvocation;
-        }
-        return null;
-    }
-
-    @Nullable
-    private static JsInvocation tryCreatePropertySetterInvocation(@NotNull JsBinaryOperation x) {
-        if (!x.getOperator().isAssignment() || !(x.getArg1() instanceof JsNameRef)) return null;
-        JsNameRef name = (JsNameRef) x.getArg1();
-        if (MetadataProperties.getInlineStrategy(name) != null &&
-            MetadataProperties.getDescriptor(name) instanceof PropertySetterDescriptor) {
-
-            JsInvocation dummyInvocation = new JsInvocation(name, x.getArg2());
-            copyInlineMetadata(name, dummyInvocation);
-            return dummyInvocation;
-        }
-        return null;
-    }
-
-    private static void copyInlineMetadata(@NotNull JsNameRef from, @NotNull JsInvocation to) {
-        MetadataProperties.setInlineStrategy(to, MetadataProperties.getInlineStrategy(from));
-        MetadataProperties.setDescriptor(to, MetadataProperties.getDescriptor(from));
-        MetadataProperties.setPsiElement(to, MetadataProperties.getPsiElement(from));
-    }
-
     @Override
     public boolean visit(@NotNull JsFunction function, @NotNull JsContext context) {
         inliningContexts.push(new JsInliningContext(function));
@@ -261,7 +191,7 @@ public class JsInliner extends JsVisitorWithContextImpl {
         assert inlineableBody == inlineableBodyWithLambdasInlined;
         statementContext.addPrevious(flattenStatement(inlineableBody));
 
-        /**
+        /*
          * Assumes, that resultExpression == null, when result is not needed.
          * @see FunctionInlineMutator.isResultNeeded()
          */

js/js.inliner/src/org/jetbrains/kotlin/js/inline/clean/RedundantStatementElimination.kt

@@ -45,6 +45,20 @@ class RedundantStatementElimination(private val root: JsFunction) {
                 }
                 return super.visit(x, ctx)
             }
+
+            override fun visit(x: JsBinaryOperation, ctx: JsContext<JsNode>): Boolean {
+                if (x.operator == JsBinaryOperator.COMMA) {
+                    val expressions = replace(x.arg1)
+                    val replacement = if (expressions.isEmpty()) {
+                        x.arg2
+                    }
+                    else {
+                        JsAstUtils.newSequence(expressions + x.arg2)
+                    }
+                    ctx.replaceMe(replacement)
+                }
+                return super.visit(x, ctx)
+            }
         }.accept(root.body)
     }
 

js/js.tests/test/org/jetbrains/kotlin/js/test/optimizer/RedundantStatementEliminationTest.kt

@@ -28,4 +28,6 @@ class RedundantStatementEliminationTest() : BasicOptimizerTest("redundant-statem
     @Test fun literal() = box()
 
     @Test fun parameters() = box()
+
+    @Test fun comma() = box()
 }

js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/BoxJsTestGenerated.java

@@ -4658,6 +4658,12 @@ public class BoxJsTestGenerated extends AbstractBoxJsTest {
             doTest(fileName);
         }
 
+        @TestMetadata("propertyReassignment.kt")
+        public void testPropertyReassignment() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/inlineSizeReduction/propertyReassignment.kt");
+            doTest(fileName);
+        }
+
         @TestMetadata("propertyReferenceDoesNotProduceSideEffect.kt")
         public void testPropertyReferenceDoesNotProduceSideEffect() throws Exception {
             String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/box/inlineSizeReduction/propertyReferenceDoesNotProduceSideEffect.kt");

js/js.tests/test/org/jetbrains/kotlin/js/test/semantics/PropertyAccessorsInlineTestsGenerated.java

@@ -45,25 +45,13 @@ public class PropertyAccessorsInlineTestsGenerated extends AbstractPropertyAcces
     @TestMetadata("augAssignmentAndIncInClass.kt")
     public void testAugAssignmentAndIncInClass() throws Exception {
         String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/boxInline/property/augAssignmentAndIncInClass.kt");
-        try {
+        doTest(fileName);
-            doTest(fileName);
-        }
-        catch (Throwable ignore) {
-            return;
-        }
-        throw new AssertionError("Looks like this test can be unmuted. Remove IGNORE_BACKEND directive for that.");
     }
 
     @TestMetadata("augAssignmentAndIncInClassViaConvention.kt")
     public void testAugAssignmentAndIncInClassViaConvention() throws Exception {
         String fileName = KotlinTestUtils.navigationMetadata("compiler/testData/codegen/boxInline/property/augAssignmentAndIncInClassViaConvention.kt");
-        try {
+        doTest(fileName);
-            doTest(fileName);
-        }
-        catch (Throwable ignore) {
-            return;
-        }
-        throw new AssertionError("Looks like this test can be unmuted. Remove IGNORE_BACKEND directive for that.");
     }
 
     @TestMetadata("augAssignmentAndIncOnExtension.kt")

js/js.translator/testData/box/inlineSizeReduction/propertyReassignment.kt

@@ -0,0 +1,42 @@
+// CHECK_VARS_COUNT: function=test1 count=0
+// CHECK_VARS_COUNT: function=test2 count=1
+// CHECK_VARS_COUNT: function=test3 count=0
+
+class A {
+    var result = 1
+
+    inline var z: Int
+        get() = result
+        set(value) {
+            result = value
+        }
+}
+
+val a = A()
+
+fun test1(): Int {
+    a.z += 1
+    return a.z
+}
+
+fun test2(): Int {
+    return a.z++
+}
+
+fun test3(): Int {
+    return ++a.z
+}
+
+fun box(): String {
+    if (test1() != 2) return "fail 1: ${a.z}"
+
+    var p = test2()
+    if (a.z != 3) return "fail 2: ${a.z}"
+    if (p != 2) return "fail 3: $p"
+
+    p = test3()
+    if (a.z != 4) return "fail 4: ${a.z}"
+    if (p != 4) return "fail 5: $p"
+
+    return "OK"
+}

js/js.translator/testData/js-optimizer/redundant-statement-elimination/comma.optimized.js

@@ -0,0 +1,12 @@
+var global = "";
+
+function log(x) {
+    global += x + ";"
+}
+
+function box() {
+    var $x = log(1);
+    var result = (log(2), log(3));
+    if (global != "1;2;3;") return "fail: " + global;
+    return "OK";
+}

js/js.translator/testData/js-optimizer/redundant-statement-elimination/comma.original.js

@@ -0,0 +1,12 @@
+var global = "";
+
+function log(x) {
+    global += x + ";"
+}
+
+function box() {
+    var $x = log(1);
+    var result = (log(2), $x, log(3));
+    if (global != "1;2;3;") return "fail: " + global;
+    return "OK";
+}