[WIP] compiler: fix handling of struct, array and interface values #669

myitcv · 2017-07-31T20:21:17Z

Fixes #661.

Two general cases that need to be solved (based on current analysis):

Case 1: assignment of struct/array val to interface

e.g.

type S struct {}
s := S{}
var i interface{} = s

Case 2: copy semantics of interface values.

This includes assigning interface values to another variable, interface method calls etc.

myitcv

Added some inline comments as well as a couple of basic tests.

myitcv · 2017-07-31T20:49:54Z

compiler/expressions.go

@@ -1134,7 +1139,7 @@ func (c *funcContext) translateImplicitConversion(expr ast.Expr, desiredType typ
 			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.formatExpr("new %1e.constructor.elem($clone(%1e, %s))", expr, c.typeName(exprType))


Address case 1

myitcv · 2017-07-31T20:50:01Z

compiler/expressions.go

@@ -1113,7 +1113,12 @@ func (c *funcContext) translateImplicitConversion(expr ast.Expr, desiredType typ

 	exprType := c.p.TypeOf(expr)
 	if types.Identical(exprType, desiredType) {
-		return c.translateExpr(expr)
+		switch exprType.Underlying().(type) {


Address case 2

myitcv · 2017-07-31T20:50:21Z

compiler/prelude/prelude.go

@@ -293,6 +293,14 @@ var $clone = function(src, type) {
  return clone;
 };

+var $copyIntf = function(src) {


Part of addressing case 2

I've never seen interfaces shortened to "intf". Instead of creating a novel way, let's reuse the same abbreviation this codebase (and Go codebase) already contains: "iface".

(https://github.com/golang/go/search?q=iface vs https://github.com/golang/go/search?q=intf)

Please update all places in this PR, not just this line.

Fair enough - I'd be using msgp as my pattern here.

Is there actually any reason to abbreviate here?

We could simply name this function the more precise $copyInterfaceVal

myitcv · 2017-07-31T20:55:47Z

Hmm seems I've broken the tests... will investigate tomorrow.

myitcv · 2017-07-31T21:14:45Z

Added support for array values.

myitcv · 2017-08-01T08:11:07Z

Ok, I think everything is fixed up now. CircleCI shows the test for text/template failed, but it passes locally. So not entirely clear what's going on there...

compiler/prelude/prelude.go

myitcv

Removed the --short flag to help debug the test failure on the CI server

myitcv · 2017-08-01T08:43:55Z

circle.yml

@@ -18,7 +18,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
+      gopherjs test --minify


Temp change to debug failure on CI server...

myitcv · 2017-08-01T09:30:52Z

Ok I've narrowed down the test failures in text/template to TestMaxExecDepth.

$ rm -rf /home/myitcv/gostuff/pkg/linux_js* && gopherjs test -v --short --minify text/template
...
=== RUN   TestMissingFieldOnNil
--- PASS: TestMissingFieldOnNil (0.00s)
=== RUN   TestMaxExecDepth
FAIL    text/template   1.528s

The test sometimes passes locally for me, sometimes not (node v8.2.1 or indeed node v6.2.2, the version running on CI). It seems to consistently fail on the CI server.

When it fails it always fails after the line:

=== RUN   TestMaxExecDepth

I also get a core dump when it fails.

@shurcooL has this been an issue before?

myitcv · 2017-08-01T09:49:23Z

Having just said it fails consistently on the CI server of course it just passed:

https://circleci.com/gh/gopherjs/gopherjs/1189

I then pushed up a commit to try with a later version of node:

https://circleci.com/gh/gopherjs/gopherjs/1190

... and that passed too.

So I'm at something of a loss 😢

myitcv · 2017-08-01T10:40:24Z

Ok, well on the assumption that it's some form of memory issue (unclear why it's so random) I've switched to using the latest Node version, as well as increasing the amount of memory available to the test run by passing in a V8 parameter via NODE_OPTIONS="--max-old-space-size=4096"

myitcv

Revised comments following simplification of the code

myitcv · 2017-08-01T11:37:59Z

circle.yml

@@ -1,6 +1,6 @@
 machine:
  node:
-    version: 6.2.2
+    version: 8.2.1


Upgrade Node version and pass in V8 option to increase available memory

Does this change need to be a part of this PR?

If not, can you factor it out? It's easier to deal with 2 smaller independent PRs.

myitcv · 2017-08-01T11:38:17Z

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 {


Address case 2.

myitcv · 2017-08-01T11:38:25Z

compiler/expressions.go

@@ -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) {


Address case 1.

Can this maybe be solved instead by making translateImplicitConversionWithCloning look at c.p.TypeOf(expr) instead of desiredType? This would also catch the case of "array/struct to interface".

As I understand it, we effectively need some combination of the logic from translateImplicitConversionWithCloning and translateImplicitConversion when it comes to array/struct values.

As this PR stands:

translateImplicitConversionWithCloning handles the case where the target is an array/struct type which by definition means the source must be exactly the same type, hence a $clone is required

translateImplicitConversion handles the conversion from an array/struct type to an interface type, hence the new X is required to wrap the $clone-ed value (part of Struct values not being cloned when assigned from/called with when variable type is interface #661 concerned the fact that we weren't cloning before wrapping)

I'm relatively relaxed on where the code lives because once your first comment is addressed I think this PR looks simpler as far as translateImplicitConversion is concerned in any case.

Let me know what you think

myitcv · 2017-08-01T12:12:33Z

Edit: updated link to issue I created

Raised nodejs/node#14567 to see if there is anything from the Node side they can help with.

myitcv

Added notes on ulimit -s and --stack_size

circle.yml

myitcv · 2017-08-01T14:05:01Z

Per the update to the linked node issue, I think the random failures have been resolved by the ulimit -s and --stack_size synchronisation changes I've also made part of this PR.

So we're back to a passing build and I now expect this built to pass reliably.

myitcv · 2017-08-01T16:50:54Z

@shurcooL per your question about splitting the Node upgrade into a separate PR (which I can't find for some reason).

Having just completed a test run using v6.2.2 (current master version), i.e. relying only on the harmonisation of ulimit -s and --stack_size, it seems we don't need to upgrade Node to v8.2.1. So I've reverted that change from this PR.

dmitshur · 2017-08-01T17:37:02Z

(The comment is likely gone because I left it on a line in the diff that has since been removed from the PR when you force pushed a single squashed commit; GitHub doesn't seem to handle that well.)

I meant that change to Node version and all the GOPHERJS_STACK_SIZE and ulimit stuff. Is it needed to fix #661?

myitcv · 2017-08-01T17:56:39Z

I meant that change to Node version and all the GOPHERJS_STACK_SIZE and ulimit stuff. Is it needed to fix #661?

The changes to harmonize ulimit -s and --stack_size are required for the changes (at least as currently implemented in this PR) to fix #661. For some reason the changes to fix #661 in this PR cause the TestMaxExecDepth test in text/template to randomly break the stack limit as discussed in nodejs/node#14567 which then fails the tests.

My guess is that @neelance introduced --stack_size in the first place precisely for this reason, i.e. to handle situations where we intentionally recurse (as the test does). However, as advised in nodejs/node#14567 (comment), we need to harmonise ulimit -s and --stack_size.

We can do that in a separate PR, just so long as that PR goes in before this one (assuming we're otherwise happy with the changes in this PR).

Would you like me to create a separate PR for the ulimit -s and --stack_size harmonisation changes?

dmitshur · 2017-08-01T20:53:49Z

No need, if it's needed to resolve the issue, then it belongs here. I wasn't sure that was the case.

dmitshur · 2017-08-03T17:29:44Z

I've read nodejs/node#14567. My question/suggestion, can we isolate that issue to CI only, with a comment to that issue?

I'm not happy with the complexity it adds, so the smaller surface area it has, the better IMO. I don't want all GopherJS users to have to understand the new GOPHERJS_STACK_SIZE variable, I don't want us to have to document it, I don't want us to maintain it, etc., if it's at all avoidable.

If I understand correctly, it's something we need to fix intermittent CI failures related to node?

If so, can we simplify it to just doing this hard-coded hack somewhere in the Circle CI script:

test:
   override:
    - ulimit -s 10000 # Fix for https://github.com/nodejs/node/issues/14567.
    ... the rest of commands, unmodified

Would that work, or is it not possible?

dmitshur · 2017-08-03T17:31:14Z

I see you tried something like that in f24a681, why didn't it work?

dmitshur · 2017-08-03T17:33:04Z

Can you please link me to some failures that result from not setting ulimit -s to 10000?

myitcv · 2017-08-27T06:50:01Z

@shurcooL #687 raised for the separate ulimit -s and --stack_size changes.

Hence the diff as part of this PR (which has also been rebased following the merge of the Go1.9 changes) should be viewed as if #687 has already been merged.

@neelance any other changes you want to see as part of this PR?

Thanks

myitcv · 2018-04-01T19:04:14Z

@neelance - can you take a final look at this please?

This issue came up again in the context of #783

...

neelance · 2018-04-02T23:24:34Z

I see that you addressed my comments where possible and I've looked at the current changes and they seem okay to me, since they only modify very specific cases now. However, since I haven't worked on GopherJS for a while, I don't have all the context in my head any more and it would take quite a while to properly assess the PR again. I've removed my review since I have no reason to block the merge any more, but it would be good if @shurcooL would give the final approval.

myitcv · 2018-04-03T07:37:37Z

Thanks @neelance

@shurcooL - please let me know if I can provide any background etc on this to help with the review.

myitcv · 2018-04-12T21:52:56Z

Gentle ping @shurcooL

dmitshur · 2018-04-13T05:26:23Z

Thanks for the ping. I'll try to find time to look at this and the other PR this weekend.

Sorry, I am unfortunately still in a more-overloaded-than-usual state and that's why I can't provide timely reviews of complex changes. :( I should be out of this state sometime within the next few weeks.

myitcv · 2018-04-16T12:21:45Z

myitcv/react#82 rebased to reference HEAD of this PR. Everything working well there so I have a high degree of confidence with this change.

cd $(go list -f "{{.Dir}}" github.com/gopherjs/gopherjs)
[ "$(git status --porcelain)" == "" ] && \
  git fetch origin refs/pull/669/head && \
  git checkout FETCH_HEAD || \
  echo "uncommitted working tree" && false

This is a rebase of #765. Whilst rebasing #669 I fell into a similar trap to #775 (#775 (comment)), by forgetting to commit the manually minified prelude. Then going through the manual process of manually minifying the prelude I wasted a whole load of time installing goexec, missing something the first time etc, issues that this PR addresses by automating the process, as previously discussed (#765 (comment)).

myitcv · 2018-04-20T11:22:21Z

Rebased post merge of #784

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. Fixes gopherjs#661.

vals

dmitshur · 2018-04-22T04:31:09Z

Thanks for working on this bug, and sorry it's taking so long. I've found some time and started reviewing this now. I will need more time, but I found something that I haven't seen mentioned so far. The fix so far works for a value assigned to an interface variable:

s1 := S{}
var s2 NameSet = s1
s1.Name = "Rob"
fmt.Printf("s1: %v, s2: %v\n", s1, s2)
s2.SetName("Pike")
fmt.Printf("s1: %v, s2: %v\n", s1, s2)

But it has no effect when a pointer is assigned to the interface variable:

s1 := S{}
var s2 NameSet = &s1
s1.Name = "Rob"
fmt.Printf("s1: %v, s2: %v\n", s1, s2)
s2.SetName("Pike")
fmt.Printf("s1: %v, s2: %v\n", s1, s2)

That code continues to produce incorrect output with GopherJS (unchanged on master vs this PR):

s1: {Rob}, s2: &{Rob}
s1: {Pike}, s2: &{Pike}

Instead of the expected:

s1: {Rob}, s2: &{Rob}
s1: {Rob}, s2: &{Rob}

First, I want to find out if you consider fixing that case in scope of this issue and/or PR, or if you'd rather leave the scope for fixing the existing interface value cases covered by the tests?

It might be that the fix for both may end up simpler, but maybe it's not as closely related as it might seem. Let me know how you'd prefer to proceed. Thanks again.

myitcv · 2018-04-22T17:10:19Z

@shurcooL - very good catch, thank you!

Digging in to this today, it's opened up a whole can of worms. I'm going to have to park looking at this further for now, but this example shows some of the worms involved (playground - open developer console): 😄

package main

import (
	"github.com/gopherjs/gopherjs/js"
)

type S struct {}

func main() {
	s := S{}
	js.Global.Get("console").Call("log", js.InternalObject(s).Get("constructor").Get("string"))
}

gives the output:

*main.S

That said, I think I see a path to properly solving #661 that also untangles some of those worms.

myitcv commented Jul 31, 2017

View reviewed changes

myitcv mentioned this pull request Jul 31, 2017

Struct values not being cloned when assigned from/called with when variable type is interface #661

Open

myitcv force-pushed the struct_val_bug branch from 24f7ea9 to 32e7130 Compare August 1, 2017 08:24

flimzy reviewed Aug 1, 2017

View reviewed changes

compiler/prelude/prelude.go Outdated Show resolved Hide resolved

myitcv force-pushed the struct_val_bug branch from 32e7130 to da2f549 Compare August 1, 2017 08:43

myitcv commented Aug 1, 2017

View reviewed changes

myitcv force-pushed the struct_val_bug branch from 07b76f2 to 926d94d Compare August 1, 2017 09:32

This was referenced Aug 1, 2017

[WIP] Test out GopherJS struct val fix myitcv/react#82

Open

[WIP] Test out GopherJS struct val fix myitcv/gopherize.me#12

Open

myitcv commented Aug 1, 2017

View reviewed changes

circle.yml Outdated Show resolved Hide resolved

myitcv force-pushed the struct_val_bug branch from 71ffac4 to a217ebf Compare August 1, 2017 16:48

myitcv force-pushed the struct_val_bug branch from a217ebf to b6ed656 Compare August 3, 2017 17:23

myitcv force-pushed the struct_val_bug branch from b6ed656 to bdd0387 Compare August 3, 2017 17:36

myitcv changed the title ~~Investigate/fix struct val bug~~ compiler: fix handling of struct, array and interface values Aug 3, 2017

myitcv force-pushed the struct_val_bug branch from 1935fa5 to eab2a7c Compare August 27, 2017 06:45

myitcv force-pushed the struct_val_bug branch from eab2a7c to 39dbcaf Compare August 27, 2017 08:50

myitcv force-pushed the struct_val_bug branch from 39dbcaf to 8bed5ea Compare April 1, 2018 17:52

This was referenced Apr 1, 2018

non-pointer receiver gets modified by method execution through interface #783

Closed

compiler: automate regeneration of prelude #784

Merged

myitcv force-pushed the struct_val_bug branch from 8bed5ea to 7a694bb Compare April 1, 2018 18:42

neelance previously approved these changes Apr 2, 2018

View reviewed changes

myitcv force-pushed the struct_val_bug branch 2 times, most recently from 4bc831c to 91be9d3 Compare April 16, 2018 09:37

myitcv mentioned this pull request Apr 16, 2018

Sequencing PRs to be merged #793

Open

myitcv force-pushed the struct_val_bug branch from 91be9d3 to 7dac6c0 Compare April 20, 2018 11:21

myitcv added 3 commits April 21, 2018 06:40

Fix externalizeFunction to also copyInterfaceVal for returned struct

a1b88e1

vals

Regen prelud_min

ab58715

myitcv force-pushed the struct_val_bug branch from 6638207 to ab58715 Compare April 21, 2018 05:41

myitcv mentioned this pull request Apr 21, 2018

Embed core GopherJS packages into build system; enable vendoring of GopherJS. #787

Merged

myitcv changed the title ~~compiler: fix handling of struct, array and interface values~~ [WIP] compiler: fix handling of struct, array and interface values Apr 22, 2018

flimzy added the NeedsInvestigation label Dec 28, 2021

[WIP] compiler: fix handling of struct, array and interface values #669

Are you sure you want to change the base?

[WIP] compiler: fix handling of struct, array and interface values #669

Conversation

myitcv commented Jul 31, 2017 • edited

Case 1: assignment of struct/array val to interface

Case 2: copy semantics of interface values.

myitcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myitcv commented Jul 31, 2017

myitcv commented Jul 31, 2017

myitcv commented Aug 1, 2017

myitcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myitcv commented Aug 1, 2017

myitcv commented Aug 1, 2017

myitcv commented Aug 1, 2017

myitcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myitcv commented Aug 1, 2017 • edited

myitcv left a comment

Choose a reason for hiding this comment

myitcv commented Aug 1, 2017

myitcv commented Aug 1, 2017

dmitshur commented Aug 1, 2017 • edited

myitcv commented Aug 1, 2017

dmitshur commented Aug 1, 2017

dmitshur commented Aug 3, 2017

dmitshur commented Aug 3, 2017

dmitshur commented Aug 3, 2017

myitcv commented Aug 27, 2017

myitcv commented Apr 1, 2018

neelance commented Apr 2, 2018

myitcv commented Apr 3, 2018

myitcv commented Apr 12, 2018

dmitshur commented Apr 13, 2018 • edited

myitcv commented Apr 16, 2018 • edited

myitcv commented Apr 20, 2018

dmitshur commented Apr 22, 2018

myitcv commented Apr 22, 2018

myitcv commented Jul 31, 2017 •

edited

myitcv commented Aug 1, 2017 •

edited

dmitshur commented Aug 1, 2017 •

edited

dmitshur commented Apr 13, 2018 •

edited

myitcv commented Apr 16, 2018 •

edited