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

myitcv · myitcv · commit bdd038715784 · 2017-08-03T18:23:35.000+01:00
As outlined in #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. #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 #661.
diff --git a/circle.yml b/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
diff --git a/compiler/expressions.go b/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)
diff --git a/compiler/prelude/prelude.go b/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 = {};
diff --git a/tests/interface_test.go b/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)
+	}
+}
diff --git a/tool.go b/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...)
