compiler: fix handling of struct, array and interface values.

As outlined in gopherjs#661, the
compiler fails to generate code to copy a struct/array value for an
assignment when the target's underlying type is an interface type,
whether for explicit variable assignments, implicit function/method
parameters etc. Instead, taking the example of explicit variable
assignment, the interface variable is assigned a value that contains the
same pointer to the source struct/array val (we're in Javascript world,
so everything is a pointer). This means that changes to the struct/array
value via the source variable are, incorrectly, visible via the target
variable. gopherjs#661 gives a simple
example. There is a further issue when interface values are assigned to
interface-typed variables: struct/array values are not copied when they
should be.

For some reason the changes that address the aforementioned issues start
to tip the std library tests over the V8 stack size limit, causing
text/template TestMaxExecDepth to fail randomly locally and on CI. This
had previously been addressed by @neelance in
1f89545; the --stack_size flag was
passed to NodeJS which in turn passed the value onto V8. But per
nodejs/node#14567 (comment) it
was pointed out that the value of ulimit -s must be >= the value of
--stack_size for the --stack_size to make any sort of sense. Hence this
commit also harmonises the setting of ulimit -s during the CI test run
with the value of --stack_size that is passed to NodeJS (which in turn
passes that value onto V8) when running either gopherjs test or gopherjs
run.

Fixes gopherjs#661.

circle.yml

@@ -3,6 +3,7 @@ machine:
     version: 6.2.2
   environment:
     SOURCE_MAP_SUPPORT: false
+    GOPHERJS_STACK_SIZE: 10000
 
 dependencies:
   pre:
@@ -18,7 +19,7 @@ test:
     - go tool vet *.go                                                   # Go package in root directory.
     - for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party".
     - >
-      gopherjs test --short --minify
+      ulimit -s $GOPHERJS_STACK_SIZE && ulimit -s && gopherjs test --short --minify
       github.com/gopherjs/gopherjs/tests
       github.com/gopherjs/gopherjs/tests/main
       github.com/gopherjs/gopherjs/js

compiler/expressions.go

@@ -1112,6 +1112,12 @@ func (c *funcContext) translateImplicitConversion(expr ast.Expr, desiredType typ
 	}
 
 	exprType := c.p.TypeOf(expr)
+
+	// whenever expr has an underlying *types.Interface type we need to $copyIntf
+	if _, isIntf := exprType.Underlying().(*types.Interface); isIntf {
+		return c.formatExpr("$copyIntf(%e)", expr)
+	}
+
 	if types.Identical(exprType, desiredType) {
 		return c.translateExpr(expr)
 	}
@@ -1130,12 +1136,17 @@ func (c *funcContext) translateImplicitConversion(expr ast.Expr, desiredType typ
 			// wrap JS object into js.Object struct when converting to interface
 			return c.formatExpr("new $jsObjectPtr(%e)", expr)
 		}
+
+		switch exprType.Underlying().(type) {
+		case *types.Array:
+			return c.formatExpr("new %1s($clone(%e, %1s))", c.typeName(exprType), expr)
+		case *types.Struct:
+			return c.formatExpr("new %1e.constructor.elem($clone(%1e, %s))", expr, c.typeName(exprType))
+		}
+
 		if isWrapped(exprType) {
 			return c.formatExpr("new %s(%e)", c.typeName(exprType), expr)
 		}
-		if _, isStruct := exprType.Underlying().(*types.Struct); isStruct {
-			return c.formatExpr("new %1e.constructor.elem(%1e)", expr)
-		}
 	}
 
 	return c.translateExpr(expr)

compiler/prelude/prelude.go

@@ -293,6 +293,14 @@ var $clone = function(src, type) {
   return clone;
 };
 
+var $copyIntf = function(src) {
+	if (src.constructor.copy) {
+		return new src.constructor($clone(src.$val, src.constructor));
+	}
+
+	return src;
+};
+
 var $pointerOfStructConversion = function(obj, type) {
   if(obj.$proxies === undefined) {
     obj.$proxies = {};

tests/interface_test.go

@@ -0,0 +1,88 @@
+package tests
+
+import (
+	"fmt"
+	"testing"
+)
+
+type Struct struct {
+	Name string
+}
+
+func (s Struct) SetName(n string) {
+	s.Name = n
+}
+
+type SetName interface {
+	SetName(n string)
+}
+
+func TestAssignStructValInterface(t *testing.T) {
+	s := Struct{
+		Name: "Rob",
+	}
+
+	var i1 interface{} = s
+	var i2 interface{} = i1
+
+	s.Name = "Pike"
+
+	ss := fmt.Sprintf("%#v", s)
+	i1s := fmt.Sprintf("%#v", i1)
+	i2s := fmt.Sprintf("%#v", i2)
+
+	if exp := "tests.Struct{Name:\"Pike\"}"; ss != exp {
+		t.Fatalf("ss should have been %q; got %q", exp, ss)
+	}
+
+	iexp := "tests.Struct{Name:\"Rob\"}"
+
+	if i1s != iexp {
+		t.Fatalf("is should have been %q; got %q", iexp, i1s)
+	}
+
+	if i2s != iexp {
+		t.Fatalf("is should have been %q; got %q", iexp, i2s)
+	}
+}
+
+func TestStructValIntfMethodCall(t *testing.T) {
+	var i SetName = Struct{
+		Name: "Rob",
+	}
+
+	i.SetName("Pike")
+
+	is := fmt.Sprintf("%#v", i)
+
+	if exp := "tests.Struct{Name:\"Rob\"}"; is != exp {
+		t.Fatalf("is should have been %q; got %q", exp, is)
+	}
+}
+
+func TestAssignArrayInterface(t *testing.T) {
+	a := [2]int{1, 2}
+
+	var i1 interface{} = a
+	var i2 interface{} = i1
+
+	a[0] = 0
+
+	as := fmt.Sprintf("%#v", a)
+	i1s := fmt.Sprintf("%#v", i1)
+	i2s := fmt.Sprintf("%#v", i2)
+
+	if exp := "[2]int{0, 2}"; as != exp {
+		t.Fatalf("ss should have been %q; got %q", exp, as)
+	}
+
+	iexp := "[2]int{1, 2}"
+
+	if i1s != iexp {
+		t.Fatalf("is should have been %q; got %q", iexp, i1s)
+	}
+
+	if i2s != iexp {
+		t.Fatalf("is should have been %q; got %q", iexp, i2s)
+	}
+}

tool.go

@@ -749,8 +749,24 @@ func runNode(script string, args []string, dir string, quiet bool) error {
 		}
 	}
 
+	// set a default stack size;
+	//
+	// Per https://github.com/nodejs/node/issues/14567#issuecomment-319367412
+	// ulimit -s needs to be at least this value
+	//
+	// because this probably isn't critical for everyday usage (it's only a real
+	// issue with massively recursive code, like the tests in text/template) we
+	// don't test to see if this is the case. Instead we ensure it's the case
+	// via GOPHERJS_STACK_SIZE which is set as part of the CI build
+	//
+	stackSize := "10000"
+
+	if ss, ok := os.LookupEnv("GOPHERJS_STACK_SIZE"); ok {
+		stackSize = ss
+	}
+
 	if runtime.GOOS != "windows" {
-		allArgs = append(allArgs, "--stack_size=10000", script)
+		allArgs = append(allArgs, "--stack_size="+stackSize, script)
 	}
 
 	allArgs = append(allArgs, args...)