Fix local confusion in generated implicit setters (#1102)

Author: dcodeIO

src/compiler.ts

@@ -1440,17 +1440,37 @@ export class Compiler extends DiagnosticEmitter {
     var nativeThisType = this.options.nativeSizeType;
     var nativeValueType = type.toNativeType();
     var module = this.module;
-    var valueExpr = module.local_get(1, nativeValueType);
+    var valueExpr: ExpressionRef;
+    var varTypes: NativeType[] | null = null;
     if (type.isManaged) {
-      valueExpr = this.makeReplace(
-        module.load(type.byteSize, false,
-          module.local_get(0, nativeThisType),
-          nativeValueType, instance.memoryOffset
+      // Can't use makeReplace here since there's no corresponding flow, so
+      // 0: this, 1: value, 2: oldValue (temp)
+      valueExpr = module.block(null, [
+        module.if(
+          module.binary(nativeValueType == NativeType.I64 ? BinaryOp.NeI64 : BinaryOp.NeI32,
+            // value != (oldValue = this.field)
+            module.local_get(1, nativeValueType),
+            module.local_tee(2,
+              module.load(type.byteSize, false,
+                module.local_get(0, nativeThisType),
+                nativeValueType, instance.memoryOffset
+              )
+            )
+          ),
+          module.block(null, [
+            module.drop(
+              this.makeRetain(module.local_get(1, nativeValueType))
+            ),
+            this.makeRelease(module.local_get(2, nativeValueType))
+          ])
         ),
-        valueExpr
-      );
+        module.local_get(1, nativeValueType)
+      ], nativeValueType);
+      varTypes = [ nativeValueType ];
+    } else {
+      valueExpr = module.local_get(1, nativeValueType);
     }
-    instance.setterRef = module.addFunction(instance.internalSetterName, createType([ nativeThisType, nativeValueType ]), NativeType.None, null,
+    instance.setterRef = module.addFunction(instance.internalSetterName, createType([ nativeThisType, nativeValueType ]), NativeType.None, varTypes,
       module.store(type.byteSize,
         module.local_get(0, nativeThisType),
         valueExpr,
@@ -6702,7 +6722,14 @@ export class Compiler extends DiagnosticEmitter {
   }
 
   /** Makes a replace, retaining the new expression's value and releasing the old expression's value, in this order. */
-  makeReplace(oldExpr: ExpressionRef, newExpr: ExpressionRef, alreadyRetained: bool = false): ExpressionRef {
+  makeReplace(
+    /** Old value being replaced. */
+    oldExpr: ExpressionRef,
+    /** New value being assigned. */
+    newExpr: ExpressionRef,
+    /** Whether the new value is already retained. */
+    alreadyRetained: bool = false,
+  ): ExpressionRef {
     var module = this.module;
     var flow = this.currentFlow;
     var nativeSizeType = this.options.nativeSizeType;

tests/compiler/extends-recursive.optimized.wat

@@ -12,13 +12,11 @@
   i32.load
  )
  (func $extends-recursive/Parent#set:child (; 1 ;) (param $0 i32) (param $1 i32)
-  (local $2 i32)
   local.get $0
-  local.get $1
-  local.tee $0
   i32.load
   drop
   local.get $0
+  local.get $1
   i32.store
  )
 )

tests/compiler/extends-recursive.untouched.wat

@@ -21,21 +21,21 @@
   nop
  )
  (func $extends-recursive/Parent#set:child (; 3 ;) (param $0 i32) (param $1 i32)
+  (local $2 i32)
   local.get $0
   local.get $1
-  local.tee $0
   local.get $0
   i32.load
-  local.tee $1
+  local.tee $2
   i32.ne
   if
-   local.get $0
-   call $~lib/rt/stub/__retain
-   local.set $0
    local.get $1
+   call $~lib/rt/stub/__retain
+   drop
+   local.get $2
    call $~lib/rt/stub/__release
   end
-  local.get $0
+  local.get $1
   i32.store
  )
 )

tests/compiler/implicit-getter-setter.js

@@ -0,0 +1,41 @@
+exports.postInstantiate = function(instance) {
+  const exports = instance.exports;
+  
+  // using an integer value
+  var basic = exports["Basic#constructor"](0, 123); // retain $0
+  (() => {
+    var val = exports["Basic#get:val"](basic);
+    if (val != 123) throw Error("invalid value");
+    exports["Basic#set:val"](basic, 42);
+    val = exports["Basic#get:val"](basic);
+    if (val != 42) throw Error("invalid value");
+  })();
+
+  // using a managed value
+  var managed = exports["Managed#constructor"](0, basic); // retain $1
+  (() => {
+    var foo = exports["Managed#get:foo"](managed); // retain $2
+    if (foo != basic) throw Error("invalid value");
+    exports.__release(foo); // release $2
+  })();
+  (() => {
+    var foo = exports["Basic#constructor"](0, 321); // retain $3
+    exports["Managed#set:foo"](managed, foo);
+    exports.__release(foo); // release $3
+    var expectedFoo = foo;
+    foo = exports["Managed#get:foo"](managed); // retain $4
+    if (foo != expectedFoo) throw Error("invalid value");
+    exports.__release(foo); // releae $4
+  })();
+
+  // combining both
+  (() => {
+    var foo = exports["Managed#get:foo"](managed); // retain $5
+    var val = exports["Basic#get:val"](foo);
+    if (val != 321) throw Error("invalid value");
+    exports.__release(foo); // release $5
+  })();
+
+  exports.__release(basic); // release $0
+  exports.__release(managed); // release $1
+};

tests/compiler/implicit-getter-setter.json

@@ -0,0 +1,6 @@
+{
+  "asc_flags": [
+    "--runtime full",
+    "--use ASC_RTRACE=1"
+  ]
+}