JS/Inlining: minor improvements and clarifications after code review. Test whether expression without side effect does not prevent to relocate another expressions

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

@@ -267,10 +267,9 @@ internal class ExpressionDecomposer private constructor(
             // Qualifier might be a reference to lambda property. See KT-7674
             // An exception here is `fn.call()`, which are marked as side effect free. Further recognition of such
             // case in inliner might be quite difficult, so never extract such call (and other calls marked this way).
-            if (qualifier in containsNodeWithSideEffect && (qualifier as? HasMetadata)?.sideEffects != SideEffectKind.PURE) {
-                qualifier = qualifier.extractToTemporary()
-            }
-            else if (callee != null && receiver != null && receiver in containsNodeWithSideEffect) {
+             if ((qualifier !in containsNodeWithSideEffect || (qualifier as? HasMetadata)?.sideEffects == SideEffectKind.PURE) &&
+                 (callee != null && receiver != null && receiver in containsNodeWithSideEffect)
+             ) {
                 val receiverTmp = receiver.extractToTemporary()
                 callee.qualifier = receiverTmp
             }

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

@@ -161,7 +161,7 @@ class IneffectiveStatementElimination(private val root: JsFunction) {
             }
 
             // Although it can be suspicious case, sometimes it really helps.
-            // Consider the following case: `Kotlin.modules['foo'].bar()`, where `foo` is inlineable. Expression decomposer produces
+            // Consider the following case: `Kotlin.modules['foo'].bar()`, where `bar` is inlineable. Expression decomposer produces
             //   var $tmp = Kotlin.modules['foo'];
             //   $tmp.bar();
             // Then, inlined body of `bar` never uses `$tmp`, therefore we can eliminate it, so `Kotlin.modules['foo']` remains.

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

@@ -24,8 +24,7 @@ import org.jetbrains.kotlin.js.inline.util.collectFreeVariables
 import org.jetbrains.kotlin.js.translate.utils.JsAstUtils
 
 internal class TemporaryAssignmentElimination(private val root: JsBlock) {
-    // We say "use" about any references to a temporary variable
-    private val useCount = mutableMapOf<JsName, Int>()
+    private val referenceCount = mutableMapOf<JsName, Int>()
 
     // We say "usage" about special kind of a reference to a temporary variable in the following cases:
     //   someVar = $tmp;
@@ -129,7 +128,7 @@ internal class TemporaryAssignmentElimination(private val root: JsBlock) {
 
     private fun getUsageSequence(name: JsName): UsageSequence? {
         return usageSequences.getOrPut(name) {
-            if (useCount[name] != 1) return null
+            if (referenceCount[name] != 1) return null
 
             val usage = usages[name]
             val mappedUsage: UsageSequence? = when (usage) {
@@ -255,7 +254,7 @@ internal class TemporaryAssignmentElimination(private val root: JsBlock) {
     }
 
     private fun use(name: JsName) {
-        useCount[name] = 1 + (useCount[name] ?: 0)
+        referenceCount[name] = 1 + (referenceCount[name] ?: 0)
     }
 
     private sealed class Usage(val statement: JsStatement) {

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

@@ -33,7 +33,7 @@ internal class TemporaryVariableElimination(function: JsFunction) {
     private val definedValues = mutableMapOf<JsName, JsExpression>()
     private val temporary = mutableSetOf<JsName>()
     private var hasChanges = false
-    private val namesToProcess = function.collectLocalVariables()
+    private val localVariables = function.collectLocalVariables()
     private val statementsToRemove = mutableSetOf<JsNode>()
     private val namesToSubstitute = mutableSetOf<JsName>()
     private val variablesToRemove = mutableSetOf<JsName>()
@@ -54,7 +54,7 @@ internal class TemporaryVariableElimination(function: JsFunction) {
                 val assignment = JsAstUtils.decomposeAssignmentToVariable(x.expression)
                 if (assignment != null) {
                     val (name, value) = assignment
-                    if (name in namesToProcess) {
+                    if (name in localVariables) {
                         assignVariable(name, value)
                         addVar(name)
                         accept(value)
@@ -71,7 +71,7 @@ internal class TemporaryVariableElimination(function: JsFunction) {
                 for (v in x.vars) {
                     val name = v.name
                     val value = v.initExpression
-                    if (value != null && name in namesToProcess) {
+                    if (value != null && name in localVariables) {
                         assignVariable(name, value)
                         addVar(name)
                         accept(value)
@@ -84,7 +84,7 @@ internal class TemporaryVariableElimination(function: JsFunction) {
 
             override fun visitNameRef(nameRef: JsNameRef) {
                 val name = nameRef.name
-                if (name != null && nameRef.qualifier == null && name in namesToProcess) {
+                if (name != null && nameRef.qualifier == null && name in localVariables) {
                     useVariable(name)
                     if (name !in currentScope) {
                         inconsistent += name
@@ -523,13 +523,16 @@ internal class TemporaryVariableElimination(function: JsFunction) {
         usages[name] = (usages[name] ?: 0) + 1
     }
 
-    private fun shouldConsiderUnused(name: JsName) = (definitions[name] ?: 0) == 1 && (usages[name] ?: 0) == 0 && name in temporary
+    private fun shouldConsiderUnused(name: JsName) = definitions[name] == 1 && (usages[name] ?: 0) == 0 && name in temporary
 
     private fun shouldConsiderTemporary(name: JsName): Boolean {
-        if (definitions[name] ?: 0 != 1 || name !in temporary) return false
+        if (definitions[name] != 1 || name !in temporary) return false
 
         val expr = definedValues[name]
-        return (expr != null && isTrivial(expr)) || usages[name] ?: 0 == 1
+        // It's useful to copy trivial expressions when they are used more than once. Example are temporary variables
+        // that receiver another (non-temporary) variables. To prevent code from bloating, we don't treat large value literals
+        // as trivial expressions.
+        return (expr != null && isTrivial(expr)) || usages[name] == 1
     }
 
     private fun isTrivial(expr: JsExpression): Boolean = when (expr) {
@@ -540,14 +543,15 @@ internal class TemporaryVariableElimination(function: JsFunction) {
             }
             else {
                 val name = expr.name
-                name in namesToProcess && when (definitions[name]) {
+                name in localVariables && when (definitions[name]) {
+                    // Local variables with zero definitions are function parameters. We can relocate and copy them.
                     null, 0 -> true
                     1 -> name !in namesToSubstitute || definedValues[name]?.let { isTrivial(it) } ?: false
                     else -> false
                 }
             }
         }
-        is JsLiteral.JsValueLiteral -> true
+        is JsLiteral.JsValueLiteral -> expr.toString().length < 10
         is JsInvocation -> expr.sideEffects == SideEffectKind.PURE && isTrivial(expr.qualifier) && expr.arguments.all { isTrivial(it) }
         is JsArrayAccess -> isTrivial(expr.arrayExpression) && isTrivial(expr.indexExpression)
         else -> false

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

@@ -65,6 +65,12 @@ public class InlineSizeReductionTestGenerated extends AbstractInlineSizeReductio
         doTest(fileName);
     }
 
+    @TestMetadata("propertyReferenceDoesNotProduceSideEffect.kt")
+    public void testPropertyReferenceDoesNotProduceSideEffect() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/inlineSizeReduction/cases/propertyReferenceDoesNotProduceSideEffect.kt");
+        doTest(fileName);
+    }
+
     @TestMetadata("returnInlineCall.kt")
     public void testReturnInlineCall() throws Exception {
         String fileName = KotlinTestUtils.navigationMetadata("js/js.translator/testData/inlineSizeReduction/cases/returnInlineCall.kt");

js/js.translator/src/org/jetbrains/kotlin/js/translate/callTranslator/CallInfoExtensions.kt

@@ -77,6 +77,9 @@ fun VariableAccessInfo.constructAccessExpression(ref: JsExpression): JsExpressio
         return ref
     } else {
         return if (value !is JsEmptyExpression) {
+            // This is useful when passing AST to TemporaryAssignmentElimination. It can bring
+            // property assignment like `obj.propertyName = $tmp` to places where `$tmp` gets its value,
+            // but only when it's sure that no side effects possible.
             (ref as? HasMetadata)?.let { it.sideEffects = SideEffectKind.PURE }
             JsAstUtils.assignment(ref, value!!)
         }

js/js.translator/src/org/jetbrains/kotlin/js/translate/callTranslator/VariableCallCases.kt

@@ -75,7 +75,7 @@ object DefaultVariableAccessCase : VariableAccessCase() {
     override fun VariableAccessInfo.dispatchReceiver(): JsExpression {
         val accessor = JsNameRef(variableName, dispatchReceiver!!)
         val descriptor = callableDescriptor
-        if (descriptor is PropertyDescriptor && !JsDescriptorUtils.sideEffectsPossible(descriptor)) {
+        if (descriptor is PropertyDescriptor && !JsDescriptorUtils.sideEffectsPossibleOnRead(descriptor)) {
             accessor.sideEffects = SideEffectKind.DEPENDS_ON_STATE
         }
         return constructAccessExpression(accessor)

js/js.translator/src/org/jetbrains/kotlin/js/translate/utils/JsDescriptorUtils.java

@@ -147,7 +147,7 @@ public final class JsDescriptorUtils {
         return accessorDescriptor == null || accessorDescriptor.isDefault();
     }
 
-    public static boolean sideEffectsPossible(@NotNull PropertyDescriptor property) {
+    public static boolean sideEffectsPossibleOnRead(@NotNull PropertyDescriptor property) {
         return !isDefaultAccessor(property.getGetter()) || ModalityKt.isOverridableOrOverrides(property) ||
                 isStaticInitializationPossible(property);
     }

js/js.translator/testData/inlineSizeReduction/cases/propertyReferenceDoesNotProduceSideEffect.kt

@@ -0,0 +1,29 @@
+// CHECK_VARS_COUNT: function=box count=1
+
+package foo
+
+var log = ""
+
+class A() {
+    var x = 23
+}
+
+fun sideEffect(): Int {
+    log += "sideEffect();"
+    return 42
+}
+
+fun bar(a: Int, b: Int) {
+    log += "bar($a, $b);"
+}
+
+inline fun test(a: Int, b: Int) {
+    bar(b, a)
+}
+
+fun box(): String {
+    val a = A()
+    test(sideEffect(), a.x)
+    assertEquals("sideEffect();bar(23, 42);", log)
+    return "OK"
+}

js/js.translator/testData/js-optimizer/temporary-assignment/ifWithoutElse.original.js

@@ -4,18 +4,18 @@ function f(x) {
 
 function box() {
     var result1, result2, $a, $b;
-    
+
     if (f(true)) {
         $a = "1";
     }
     result1 = $a;
     if (result1 !== "1") return "fail1: " + result1;
-    
+
     if (f(false)) {
         $b = "1";
     }
     result2 = $b;
     if (result2 !== void 0) return "fail2: " + result2;
-    
+
     return "OK";
 }